Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 3 commits into from

Conversation

SmolSir
Copy link
Collaborator

@SmolSir SmolSir commented Jan 10, 2023

This is a part of solving #159, as well as splitting #268 into more manageable, smaller PRs.

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

@SmolSir SmolSir linked an issue Jan 10, 2023 that may be closed by this pull request
@obi1kenobi
Copy link
Owner

Looks good so far!

@SmolSir
Copy link
Collaborator Author

SmolSir commented Jan 15, 2023

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
Copy link
Owner

obi1kenobi commented Jan 15, 2023

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
Copy link
Collaborator Author

SmolSir commented Jan 15, 2023

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
Copy link
Owner

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
Copy link
Collaborator Author

SmolSir commented Jan 16, 2023

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
Copy link
Owner

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
Copy link
Collaborator Author

SmolSir commented Jan 16, 2023

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 #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
Copy link
Owner

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

@obi1kenobi
Copy link
Owner

@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
Copy link
Collaborator Author

SmolSir commented Mar 23, 2023

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
Copy link
Owner

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
Copy link
Owner

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

@SmolSir
Copy link
Collaborator Author

SmolSir commented Jun 2, 2023

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
Copy link
Owner

No worries, good luck with exams!

@obi1kenobi
Copy link
Owner

Closing for staleness (no worries!), and I'm sure another contributor can pick this up soon.

@obi1kenobi obi1kenobi closed this Aug 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adding #[must_use] on an item
2 participants