New lint: Trait method `#[must_use]` added

[SmolSir]

This is a part of solving https://github.com/obi1kenobi/cargo-semver-checks/issues/159, as well as splitting https://github.com/obi1kenobi/cargo-semver-checks/pull/268 into more manageable, smaller PRs.

Implements the check against adding #[must_use] attribute to a public Trait's method.

[obi1kenobi]

Looks good so far!

[SmolSir]

Unfortunately I'm in a spot of bother with the query. Say we have the following code:

pub trait Foo {

    #[must_use]
    fn bar();
}

The thing is - how do we access the method bar() inside Foo to get its attributes? I looked through the schema, but could not figure out a way to get there. Even if there was an impl Foo for ... block (unlike above) in which we can find the method and its implemented_trait, we don't need its attributes there (because of (1) from this comment) and it pretty much comes down to the code above - given a trait, access its method and attribute. Maybe you have a quick answer to this, just like with __typename that I missed?

[obi1kenobi]

I think it requires a bit of new schema. The method bar() is kind of like the existing Method type, but its body is optional (it could be fn bar(); or fn bar() {}) unlike Method which currently must have a body.

The body presence or absence information comes from the has_body JSON field here: https://docs.rs/rustdoc-types/latest/rustdoc_types/struct.Function.html

We have a few different options here:

• We could add a property (e.g. has_body) to the existing Method type, and then guarantee that has_body is always true on everything except traits.
• We could add a separate FunctionLike & Item type analogous to Method but with the extra has_body field, and only use it for trait methods.
• We could turn Method into an interface, and derive the new type from Method while also adding the new has_body field there.

Perhaps there are other alternatives as well.

What do you think would be the best approach?

[SmolSir]

The has_body field surely would be handy here! Thanks to your ideas, I think I came up with a possible solution.

We could add a property (e.g. has_body) to the existing Method type, and then guarantee that has_body is always true on everything except traits.

This is almost perfect, but I see one point of contention - the fn bar () {} does have a body, but would be marked with has_body = false when inside a trait? Even if so, how would we check that the method is inside the same trait in both baseline and current version using the Method schema? (we sadly can't use ImplOwner here)

We could add a separate FunctionLike & Item type analogous to Method but with the extra has_body field, and only use it for trait methods.

This is the sweet spot in my opinion - if we could solve the problem of "what trait am I inside?" by adding a field for that as well, we would be able to query for trait methods and even distinguish declared and provided methods. I would add the has_body to the regular Method as well for potential future use.

We could turn Method into an interface, and derive the new type from Method while also adding the new has_body field there.

This could be more wise thinking about future schema additions than the previous one if we could know what trait the method is in with the derived type. I'm only not sure if there would be anything else deriving from it in the future - if not, would it be worth it to have an Interface with just two objects deriving it? I'm not experienced enough with project structuring to tell, though.

Overall, I would probably go with the middle option since it appears to me as the one that is not just sufficient, but also reliable and beneficial for the whole schema. What do you think about this, knowing the adapter best and having a lot more experience with project design?

[obi1kenobi]

This is almost perfect, but I see one point of contention - the fn bar () {} does have a body, but would be marked with has_body = false when inside a trait?

Ah sorry for the confusion, on traits has_body would only be true if the method has a body, like that bar method for example. It could be true or false on trait methods, we only guarantee it's true outside of traits.

Even if so, how would we check that the method is inside the same trait in both baseline and current version using the Method schema? (we sadly can't use ImplOwner here)

Would our current way of ensuring types are "the same" across baseline and current not work for some reason? Right now Method is only reachable after crossing the impl or inherent_impl edges, and I think we'd add one more edge on Trait for accessing its methods. Then we'd make sure it's the same trait in the usual way.

This is the sweet spot in my opinion - if we could solve the problem of "what trait am I inside?" by adding a field for that as well, we would be able to query for trait methods and even distinguish declared and provided methods. I would add the has_body to the regular Method as well for potential future use.

If we're adding has_body to the regular Method, then I'm not sure I see the advantage to making a separate type for trait methods. It seems like they'd have the same fields and would share the same underlying rustdoc representation, and at that point may as well also share the same type in the schema.

I'm guessing my confusing wording of the first option probably disqualified it immediately from your consideration. Has any of this changed your mind?

[SmolSir]

I'm guessing my confusing wording of the first option probably disqualified it immediately from your consideration. Has any of this changed your mind?

Yes, the first option is now definitely the best one! And combined with

Right now Method is only reachable after crossing the impl or inherent_impl edges, and I think we'd add one more edge on Trait for accessing its methods. Then we'd make sure it's the same trait in the usual way.

will allow for successfully accessing the trait methods. If this is the final choice, I will go for these additions to the schema, as well as the Union item one.

One more question, related to the lint but not the schema - turns out we don't have a check against turning the method provided in baseline into a declared method in current version, which obviously should not be allowed. Making the change the opposite way is fine I believe. Should we consider it when writing this lint, or just ignore the has_body field and focus on the attribute? Should I open an issue for this case with request for a new lint once the schema additions will be complete?

[obi1kenobi]

If this is the final choice, I will go for these additions to the schema, as well as the Union item one.

Sounds great! Go for it!

One more question, related to the lint but not the schema - turns out we don't have a check against turning the method provided in baseline into a declared method in current version, which obviously should not be allowed. Making the change the opposite way is fine I believe. Should we consider it when writing this lint, or just ignore the has_body field and focus on the attribute? Should I open an issue for this case with request for a new lint once the schema additions will be complete?

I think that should be a new lint, and not reported here — it would be great to have a test case for it. If you're interested in writing that new lint as well, go for it! And yes, feel free to open an issue for it. I believe this new lint is mentioned in #5 so just make sure to link it to the appropriate entry (or make one if I'm misremembering and it isn't actually in the list there).

[SmolSir]

Sounds great! Go for it!

I think that should be a new lint, and not reported here — it would be great to have a test case for it. If you're interested in writing that new lint as well, go for it! And yes, feel free to open an issue for it. I believe this new lint is mentioned in https://github.com/obi1kenobi/cargo-semver-checks/issues/5 so just make sure to link it to the appropriate entry (or make one if I'm misremembering and it isn't actually in the list there).

Will do all of these! Thank you for answering with great suggestions and details, I believe there are no more uncertainties as far as this lint goes for now, great to have it all be clear :)

[obi1kenobi]

Awesome, I'm happy to help and excited to help you keep building!

[obi1kenobi]

@SmolSir just checking in since it's been a couple of months since the last activity on this PR — are you still interested in working on this?

[SmolSir]

Yes, I just shifted the focus to writing our thesis after the winter break. I planned to code again after writing one more chapter in about a week from now if that's not a problem!

[obi1kenobi]

Not a problem, I just wanted to figure out whether you're still interested in this PR, or else close it to let someone else take over the task.

[obi1kenobi]

@SmolSir checking in again, are you still planning on finishing up this PR or should I reassign it to someone else?

[SmolSir]

I'm sorry for blocking this for so long, last time when I said that I will sit down and finish it we got overwhelmed by homeworks 😞 Currently my plan is to study hard for our bachelor's exam at the end of June, so I can get back to this no sooner than July. If you have somebody to assign this to sooner than that, feel free to do so (I will still keep my eye on this in case there were any questions about what has been done so far).

[obi1kenobi]

No worries, good luck with exams!