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

Add a lint for closure trait impls #346

Merged
merged 5 commits into from
Jul 7, 2023
Merged

Conversation

thomcc
Copy link
Contributor

@thomcc thomcc commented Jun 30, 2023

This sucked to write, but should be good enough. It will catch a lot of external crates incorrectly though, without the refinement.

@thomcc thomcc changed the base branch from main to develop June 30, 2023 17:03
//! trait GivesAssoc<A> { type Assoc; }
//! impl<A> GivesAssoc<A> for A { type Assoc = A; }
//!
//! trait Child<T> where Self: GivesAssoc<T> {}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not the best example because trait Foo where Self: Blah is shorthand for trait Foo: Blah, and is actually quite easy to check for in HIR.

However, there are other examples where other traits could be indirectly implemented via bounds on the impl, and this is the simplest such example.

Copy link
Contributor

@workingjubilee workingjubilee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either closure_trait_impl.rs or config-lints.md should explain the specific problem more exactly: We want to ban projecting a return type that is incorrectly considered 'static despite in actuality being lifetime-bound.

plrustc/plrustc/src/lints/fn_ptr.rs Show resolved Hide resolved
plrustc/plrustc/src/lints/closure_trait_impl.rs Outdated Show resolved Hide resolved
plrustc/plrustc/src/lints/closure_trait_impl.rs Outdated Show resolved Hide resolved
plrustc/plrustc/src/lints/closure_trait_impl.rs Outdated Show resolved Hide resolved
@workingjubilee
Copy link
Contributor

It would be nice to have an example of a crate that this too-conservative lint would nail that we want to allow but I consider that purely extra. This will be good with the doc change I mentioned.

Copy link
Contributor

@workingjubilee workingjubilee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this diff I mentioned wanting, or some variation of this:

thomcc and others added 5 commits July 7, 2023 09:45
Co-authored-by: Jubilee <46493976+workingjubilee@users.noreply.github.com>
Co-authored-by: Jubilee <46493976+workingjubilee@users.noreply.github.com>
@thomcc thomcc force-pushed the thomcc/closure_trait_impl branch from 4f43e5b to 170342e Compare July 7, 2023 16:45
@thomcc
Copy link
Contributor Author

thomcc commented Jul 7, 2023

It would be nice to have an example of a crate that this too-conservative lint would nail that we want to allow but I consider that purely extra

I don't think this is what you mean, but for clarity: if you are asking for this as a uitest, I do not want to start adding known-bug uitests yet, I think it could cause confusion, and I would eventually like to add some rules that we should be careful about reviewing changes to the stderr uitest files, since they currently should be very rare.

If you just want an example, well, it's still tricky but only because the pattern doesnt lend itself to github code search. These impls are semi-common, because the function traits are #[fundamental] -- the blanket impl doesn't forbid other things. As mentioned it's hard to search for, but https://github.com/rust-lang/rust/blob/fd68a6ded951bd7b852ab8107007f7145e3ad6ec/library/core/src/str/pattern.rs#L602-L610 is an example from the stdlib (and I'm sure there are more).

Such impls are fine, because they don't give access to the return type as an associated type. This is true if they show up in dependencies, this isn't a case where we are weakened if a dep is added where this falsely triggers (although if a dep did provide access to the return type, it would be a problem).

@workingjubilee
Copy link
Contributor

definitely not as a uitest, just wondering if we can find examples to use to help motivate our own discussions/research later. I expect we'll encounter them face-first tho'.

@thomcc thomcc merged commit dbe7e88 into develop Jul 7, 2023
12 checks passed
@thomcc thomcc deleted the thomcc/closure_trait_impl branch July 7, 2023 18:54
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.

None yet

2 participants