Hardened contributions

[suprohub]

This pr tries make contributions more hardened, but highly improve code quality and future extensibility.
This pr also tries improve current quality of all project code.

TODO:

• Try fix all warnings
• Add contribution guide

[mat-1]

Thanks!

I'm not opposed to this but there's a lot of highly subjective changes here.

I think I'm going to keep this PR open for a bit until I can spend some time with these lints so I can decide which ones are worth keeping. From skimming through the diff, I'll note that I'm not a fan of replacing struct references with Self everywhere (since imo it makes it less obvious what the code is doing in certain cases) but the other ones are fine.

[suprohub]

Can you give more concrete examples for bad Self usage? I want see they, because i not see diff too much.

[suprohub]

Why i changed cargo.toml for using major.minor versioning? So, my opinion is use latest patches, because they often include emergerency changes or maybe security bug fixes. Also, patch versions very often dont break code.
Minor update can often break code, and better specify it.
If library long time dont break code in minor updates, it can this in near future, because minor updates by definition can break code.

[mat-1]

Can you give more concrete examples for bad Self usage? I want see they, because i not see diff too much.

Sure, for example:

- pub const AIR: BlockState = BlockState { id: 0 };
+ pub const AIR: Self = Self { id: 0 };

In my opinion, the code that doesn't include BlockState is marginally more effort to read because now you need to check what Self is referencing to properly understand it. Obviously this is extremely subjective but I don't think this is a change that's worth making.

Regarding dependency versions, I don't have strong opinions on this. I run cargo upgrade --incompatible occasionally to ensure that all dependencies in the Cargo.toml are new versions anyways.

[suprohub]

Can you give more concrete examples for bad Self usage? I want see they, because i not see diff too much.

Sure, for example:

- pub const AIR: BlockState = BlockState { id: 0 };
+ pub const AIR: Self = Self { id: 0 };

In my opinion, the code that doesn't include BlockState is marginally more effort to read because now you need to check what Self is referencing to properly understand it. Obviously this is extremely subjective but I don't think this is a change that's worth making.

Regarding dependency versions, I don't have strong opinions on this. I run cargo upgrade --incompatible occasionally to ensure that all dependencies in the Cargo.toml are new versions anyways.

Hm, ok. So you want disable lint for replacibg types with Self? Btw, this is nursery (but anyway good lint)

[suprohub]

I tried my best, removed weird lints and added new, and also get preview of all this, so now you can see final ver of lints what i selected

[suprohub]

If you confirm what i made, i can try fix all remain warnings (1k+)

[mat-1]

Tbh, this is a very big change that I think will have to be merged selectively. I think the Rust developers had good reasons for deciding which lints to enable/disable by default, so adding new lints should be done carefully.

To illustrate, I notice that there's at least one correctness bug introduced by switching add-multiply float operations to mul_add (this matters for anticheat compatibility, because fused-multiply-add has better precision and is thus incorrect for Azalea's purposes).

Another transformation that's (imo, at least) incorrect:

-    /// An override for the OAuth2 scope to authenticate with.
+    /// An override for the `OAuth2` scope to authenticate with.

(OAuth2 was capitalized because it's a proper noun; it's not a Rust type)

Another example: I don't think adding Eq to only some packets is a good idea, because it makes the implemented traits inconsistent across packets and thus makes them slightly more confusing to use.

Of course there's good changes here, and I definitely appreciate you putting the time into writing this. What I'm trying to say is that a lot of these changes should be done with extra care. I don't really have time to read the whole diff here and provide feedback on every set of changes, so I think the way I might go about merging this is by making a new commit (or series of commits) that contain a subset of your patch.

[suprohub]

Tbh, this is a very big change that I think will have to be merged selectively. I think the Rust developers had good reasons for deciding which lints to enable/disable by default, so adding new lints should be done carefully.

Default rust lints often used for default projects, which often dont have 10+ crates and ton of code. Default lints also donr include some lints from pedantic or restrictions, because its too annoying i think, but for ver big projects this are good lints. Default lints also never include experimental lints (which can be experimental forever), which are veeery good (like nightly rust).

To illustrate, I notice that there's at least one correctness bug introduced by switching add-multiply float operations to mul_add (this matters for anticheat compatibility, because fused-multiply-add has better precision and is thus incorrect for Azalea's purposes).

Another transformation that's (imo, at least) incorrect:

-    /// An override for the OAuth2 scope to authenticate with.
+    /// An override for the `OAuth2` scope to authenticate with.

(OAuth2 was capitalized because it's a proper noun; it's not a Rust type)

Yeah, therr are still some false positives, but not much. Better just use expect for this, because tbese lints like mul add often can improve performance or readability.

Another example: I don't think adding Eq to only some packets is a good idea, because it makes the implemented traits inconsistent across packets and thus makes them slightly more confusing to use.

Yeah, but this also add more features, but yes, only fir some packets. But this lint often good too, because sometimesi ver hate that some types from traits can implement eq but not implemented, but i very need this impl for some usage.

Of course there's good changes here, and I definitely appreciate you putting the time into writing this.

Thanka

What I'm trying to say is that a lot of these changes should be done with extra care. I don't really have time to read the whole diff here and provide feedback on every set of changes, so I think the way I might go about merging this is by making a new commit (or series of commits) that contain a subset of your patch.

Maybe just merge all lints and other good no-code changes and then try fix all warnings in separate prs?

[mat-1]

Closing this since I've started slowly enabling some of the lints and fixing related issues. I don't think I'll enable lints that have too many false positives since they add too much development friction, but most are good.

Regarding a CONTRIBUTING.md file, I would like to add one soon as well :). I might take inspiration from the one you wrote (thanks <3), though I do want to add a focus on certain things more related to Azalea like testing with anticheats, benchmarking, getting the decompiled Minecraft code, style things like using the word "kind" instead of "type", updating the changelog, etc.