Adding #[must_use] on an item

[SmolSir]

#159 Adding #[must_use] on an item

According to Rust Reference about #[must_use] attribute, the following lints have to be added:

• enum_must_use_added
• struct_must_use_added
• function_must_use_added
• trait_must_use_added
• inherent_method_must_use_added
• trait_method_must_use_added

Having already done the enum lint I thought it would be a good idea to ask for a review before adding the rest of the lints, especially because they would be very similar to one another (besides tests, only single-word changes in generated messages).

Also a question @obi1kenobi - if I'm not mistaken, we currently do not have a Union item in the adapter. If there are plans to eventually include it in the schema, maybe this is a good moment to do so?

[obi1kenobi]

This looks good! If you'd like, we can merge it right now. If you'd prefer to merge multiple must_use lints at the same time, that's fine too — I don't have a strong preference either way.

[obi1kenobi]

I think we'll also need method_must_use_added, since methods are conceptually "functions inside an impl" rather than "functions inside a module" and therefore require a slightly different query.

Also a question @obi1kenobi - if I'm not mistaken, we currently do not have a Union item in the adapter. If there are plans to eventually include it in the schema, maybe this is a good moment to do so?

Yes, of course. Please feel free to add it.

The schema also lacks constants and statics, which would be great to have as well — we don't have a way to represent their type, so we can just skip adding it to the schema for now and add it later.

[SmolSir]

This looks good! If you'd like, we can merge it right now. If you'd prefer to merge multiple must_use lints at the same time, that's fine too — I don't have a strong preference either way.

I would definitely like to add the struct, function and trait lints before merging, as well as the method lint. After that we can either merge it and specify the #159 issue to the Union case, blocking it on the schema update temporarily, or leave it until fully complete - I think merging quickly would make the whole thing easier to manage, so I'll remove the Union from the check-list and specify it in the issue. I will also add the Union to schema.

[SmolSir]

First of all, sorry for making this large push - I was not expecting it myself to end up this way... After pushing the enum_must_use_added lint, I thought that since it was such a quick task, then the rest would be too. I implemented the lints for struct, trait and function, didn't push these changes and went on to the method lint. And surprise - it turned out to be larger than the rest combined...

What's changed?

• I have implemented 3 additional #[must_use] lints: struct_must_use_added, trait_must_use_added, function_must_use_added,
• Having noticed that the method_must_use_added lint was not going as easy as expected, I started by creating a large test file with reasonable test cases. During the process, I tested some things and was able to figure out that:
  • there are 3 possible places for a method to add the attribute and compile successfully:
    1. inside an impl Struct (inherent methods, both struct and method have to be public to cause a breaking change),
    2. inside a trait (the trait has to be public to cause a breaking change, methods are public by rule),
    3. inside an impl Trait for Struct (to my surprise, this compiles but does not produce the unused value warning - seems like attributes are "copied" from the trait itself, I did not know this behavior but it makes sense as their methods should be consistent everywhere)
  • knowing the above, I decided to split this lint into 3 separate lints, for easier navigation and better messages for the user (as for the -> notation, on the left is where the method is placed in the old crate, on the right is where it is in the new crate, #[must_use] added):
    1. inherent_method_must_use_added - this handles impl Struct -> impl Struct, probably the easiest case.
       Both lint and tests exist.
    2. trait_method_must_use_added - this handles pub Trait -> pub Trait, inside the trait the method can be either declared (fn Foo(&self);) or provided (fn Foo(&self) = {...}). Turning a provided method into a declared one requires additional impl blocks, but this is handled by other lints - here the focus is on the method being there in either form (should I change this approach?).
       Only tests exist (I run into an issue of implemented_trait being not implemented in the Impl schema, thus the lint could not distinguish a pub Struct with private methods from a pub Trait whose public methods written without pub and thus queries consider them private as well (is this intended?).
    3. inherent_method_moved_must_use_added - this handles impl Struct -> pub Trait, this is not a breaking change as far as the method changing it's position in the code is considered. The same as above applies to what the method inside the trait looks like. Additionally this time there has to be impl Trait for Struct implementing (or overriding!) the pub Trait methods.
       Only tests exist (same issue as with trait_method_must_use_added.

What's next?

From my point of view, the best solution would be to merge this PR with currently added lints, add the missing implemented_trait schema edge notice to the issue #159 this PR comes from (just like with the Union type addition to the schema), modify the adapter and finally finish the issue (I'm willing to do the implemented_trait just like I am with the Union).

If possible, it would be great to merge the 2 test crates without lints (trait_method_must_use_added and inherent_method_moved_must_use_added) to later on just add their respective lints, meanwhile allowing other lints to run on the crates (I did not have to modify any output files because of their addition).

These are just my ideas though, and I will fully agree with whatever works best in your opinion!

[obi1kenobi]

I'm excited to get all these new lints merged!

Unfortunately, this PR is way too large to be realistically reviewable as-is. There's quite a bit of tricky stuff going on (especially in the method lints), and I really want to avoid having to re-review 1600+ lines of code through multiple rounds of comments.

Please split it up into multiple PRs, at least 3 of them if not more:

• One PR for the "straightforward" lints: structs, enums, functions, together with their test cases. It could even be nice to have these separated out into separate PRs, but you don't have to do that if it's too much hassle.
• One PR for the testcase-only crates, which should be pretty easy to merge since none of the existing lints should trigger on them.
• One or more PRs for the remaining lints and their test cases.

Turning a provided method into a declared one requires additional impl blocks, but this is handled by other lints - here the focus is on the method being there in either form (should I change this approach?).

This sounds fine.

pub Trait whose public methods written without pub

I believe these have a visibility modifier called default, since trait methods, just like enum variants, cannot have explicit visibility modifiers applied to them. I believe it should be possible to write a filter that allows either public or default visibility.

impl Struct -> pub Trait

I think it's not worth implementing this lint for this case, since this is already by itself semver-minor even withour #[must_use]. In fact, I think we should explicitly not have a lint for this, since we don't want to report more than one lint for the same change (and eventually we'll implement the "method moved to trait" semver-minor lint, very much not a priority right now).

It would be great to have a test case for this case, to make sure we aren't reporting a lint here.

[obi1kenobi]

Also by all means please implement the missing schema part you ran into!

[SmolSir]

Please split it up into multiple PRs, at least 3 of them if not more

I will create a separate PR for each one of the lints (smaller is a lot better and easier, I regret not doing it from the start). I also was not aware what the default visibility meant - with that, I believe all of the lints mentioned above are possible without the implemented_trait edge - but regardless of whether it's needed or not, I will still do the schema addition here!

I believe these have a visibility modifier called default, since trait methods, just like enum variants, cannot have explicit visibility modifiers applied to them. I believe it should be possible to write a filter that allows either public or default visibility.

Knowing how the default works now, I also believe it is possible to merge both inherent_method_must_use_added and trait_method_must_use_added into a single lint. This might lead to less informative messages and larger test crate,
but if you would like to have them merged into method_must_use_added I can do that too.

impl Struct -> pub Trait

I think it's not worth implementing this lint for this case, since this is already by itself semver-minor even withour #[must_use]. In fact, I think we should explicitly not have a lint for this, since we don't want to report more than one lint for the same change (and eventually we'll implement the "method moved to trait" semver-minor lint, very much not a priority right now).

Agreed, the lint was not implemented yet anyways.

It would be great to have a test case for this case, to make sure we aren't reporting a lint here.

Sure, the test crate stay under the name inherent_method_moved_must_use_added will handle it perfectly as it was prepared for the lint we decided to skip, I will add a comment about it's new role. Should I change it's name to something else?

[obi1kenobi]

I also believe it is possible to merge both inherent_method_must_use_added and trait_method_must_use_added into a single lint.

I currently don't understand the distinction between these two lints. What are the arguments for and against merging them? Is there a risk of getting duplicate lints if they stay separate?

If you'd like, just use your best judgment to move forward and we can discuss more on the smaller PR.

Should I change it's name to something else?

It's hard for me to say without digging really deep into this PR, which wouldn't be an efficient use of time: you've shown you have good judgment, so I trust you to make a good decision as part of splitting up the PR and if needed, we can chat about it more on the new, smaller PRs.

[SmolSir]

This PR was successfully split into smaller PRs and is now closed.