New lint: Inherent method #[must_use] added

[SmolSir]

After some testing it turned out it is not possible to have a single lint for both Struct and Trait methods. Here is the reason:

• assume we want to support both scenarios, Struct inherent method becomes #[must_use] and Trait method becomes #[must_use] at the same time. The former method has public visibility, the latter has default, while both Struct and Trait must have public visibility.
• in a single query, we have to give exact parameters to both baseline and current parts of the query. So we opt for method visibility to be either public or default, because otherwise we will miss some Struct or Trait methods.
• now imagine that we have one pub Struct Foo and one pub Trait Bar, that both have a method that becomes #[must_use]. If we run the query that looks for public or default methods in both baseline and current crates, the query will find matches for all 4 possible pairings, despite there being only 2 of them sensible to the user. To avoid this, the query would have to "on Struct look for a matching Struct, then for the matching method, on Trait look for a matching Trait, then for the matching method", which even if was possible would be begging to get split into 2 separate queries (at least I see it this way).
• because of this we have to precise our query to a single visibility value for either baseline or current version.

And since we decided in #268 that there is no need for a lint that detects addition of #[must_use] for a method that is also moved to Trait, we eliminated the only cross-type case from the 3 possibilities I introduced in that PR and are left with:

• Struct method -> Struct #[must_use] method, with the lint inherent_method_must_use_added implemented in this PR,
• Trait method -> Trait #[must_use] method, with the lint trait_method_must_use_added, on which I am still working in New lint: Trait method #[must_use] added #284.

[obi1kenobi]

I think this implementation has an interesting bug.

The PR description describes this lint as Struct method -> Struct #[must_use] method, but that is actually too narrow. Enums and unions can also have inherent impl blocks whose methods may be marked with #[must_use]. The lint correctly anticipates this and uses the coercion ... on ImplOwner instead of ... on Struct. This means it will correctly flag enum methods (and, when implemented, union methods as well).

Except, the current and baseline branches of the query never ensure that the kind of item matches between the baseline and the current code. So I predict that the following code will cause a false-positive here:

baseline:

pub struct Foo {};

impl Foo {
    pub fn bar() {}
}

current:

pub enum Foo {
    A,
}

impl Foo {
    #[must_use]
    pub fn bar() {}
}

I predict that the current code will flag Foo::bar as having gained a #[must_use] attribute, when in fact that's a completely new method on a completely new type.

It would be great to add "true-positive" test cases for enum and union methods just like the existing ones for structs, even though the union ones won't be caught (opne an issue about this and link it in a comment, so we remember that it's okay to for that test output to change when we implement unions in the adapter).

It would also be good to have "true-negative" test cases where structs/enums/unions get each other's names like this.

With respect to fixing the lint query, there are two options:

• Each vertex has a special implicit field __typename: String! that contains the exact type of the vertex: on ImplOwner it would be Struct or Enum or Union (when implemented). You could tag and filter on that value across the baseline / current branches of the query to make sure the item type matches up.
• Alternatively, if you think there's an advantage to doing so, you could split the lint into separate lints for Struct / Enum / Union types and replace ... on ImplOwner with ... on Struct etc. This doesn't seem to offer any obvious advantages over the alternative, and is more complex, but I wanted to bring it up for your consideration in case I missed something.

[SmolSir]

That's a very interesting bug, I wasn't aware of the inherent_impl existing for types other than Struct. At first glance I also think it will be best to just broaden the query to handle both Struct and Enum, Union cases, but I will look deeper into this first before proceeding with changes.

Alternatively, if you think there's an advantage to doing so, you could split the lint into separate lints for Struct / Enum / Union types and replace ... on ImplOwner with ... on Struct etc. This doesn't seem to offer any obvious advantages over the alternative, and is more complex, but I wanted to bring it up for your consideration in case I missed something.

The only "advantage" I can point out for now is that this would result in multiple smaller test crates, one per lint, instead of one big test crate handling all of the cases for one, common lint. But if the test crate turns out too large, we will be able to easily split it into smaller test cases and leave the common lint unchanged (I will keep it in mind to write the tests as easily splittable because of this).

[SmolSir]

I will keep it in mind to write the tests as easily splittable because of this.

The best option was to use the hybrid approach mentioned here #282 (comment), so I already split the tests into relevant-named files. I also added a "test file pair" against the changed type false-positive scenario which was shown above.

This is currently blocked on #149 & #292.

If the lint was split into more specialized ones for Enum, Struct& Union, this would not require implementing #149 and would allow for adding Enum & Struct lints quickly, while the Union lint would wait until #292 is complete (and could be mentioned in the issue itself to be added once Union is in the schema, among some other lints that it will enable).

@obi1kenobi considering the above, despite it requiring a bit more work with splitting the lint, I think it could in return make the workflow a lot smoother and faster (with #149 being rated as the hardest addition in #241, I am not sure if this is a good idea to block this PR on that issue). If you have nothing against it, I would proceed with this approach to thin out the #[must_use] issue as much as possible.

[obi1kenobi]

I don't think I see why this is blocked on #149. That issue is required for representing the type of fields, function arguments, generics, etc. I'm confused why those are involved here.

Are we not able to use the ImplOwner type's inherent_impl edge? The __typename special field on ImplOwner will be one of "Struct" / "Enum" / "Union" so it seems like we should be able to use the standard @tag + @filter combination across current and baseline to determine when the types are of the same kind (struct / enum / union).

[SmolSir]

It is indeed not blocked, my reading has just fooled me on this bit

Each vertex has a special implicit field __typename: String! that contains the exact type of the vertex: on ImplOwner it would be Struct or Enum or Union (when implemented).

I applied the "when implemented" part to the whole sentence, that is __typename and assumed it was a part of #149, while it of course was just about the Union...
With that problem out of the way, this should be working now! I also updated the issue for adding Union #292 to remind that once it is released, the output file will become invalid.

[obi1kenobi]

Looks great! I'll apply my two tiny suggestions and merge.