-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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 future_prelude_collision
lint
#85707
Add future_prelude_collision
lint
#85707
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @estebank (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
I believe all of the issues preventing a review have been resolved if you'd like to take a look or tag someone in @estebank! |
I can try to review this! |
/// method is called via dot-call syntax or a `try_from`/`from_iter` associated function | ||
/// is called directly on a type. | ||
/// | ||
/// [prelude changes]: https://blog.rust-lang.org/inside-rust/2021/03/04/planning-rust-2021.html#prelude-changes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: We should link to the edition guide once the 2021 stuff is online.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code looks very nice!
For some more test cases, try running
The rest of the output
|
It fails in another way on https://github.com/droundy/clapme:
( |
(Sorry! Don't want to demotivate you! ^^' I'm just going over the crater results from a test where we added these traits to all the preludes, to see if this lint fixes the crates that broke.) |
No worries, I appreciate the thorough review actually, these are very much good finds, there's a reason we have the tooling to check the things that are error-prone :) |
Another interesting case is https://github.com/mechiru/json-ext The lint does not trigger on that code. But it should, because compiling that crate on edition 2021 (with the prelude changes) results in:
Not sure why the new prelude affects this code though. |
And another one: https://github.com/mqnfred/ffishim The lint does also not trigger on this code. But compiling it in the new edition results in:
Here, the receiver is a |
I think that's all I can find. Everything else seem to be the same issues as in the cases I already commented about. |
|
This comment has been minimized.
This comment has been minimized.
It looks like this is because the way I was resolving the path via |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some nits and thoughts.
This comment has been minimized.
This comment has been minimized.
@m-ou-se when you have some time could you check if the warnings from the previous @rust-log-analyzer message are accurate? I took a look at them and they might be right? But I also wouldn't even know where to start with trying to compile rustc itself using the 2021 prelude, so I can't actually confirm if those are correct. And if they're correct, I can resolve them. |
This comment has been minimized.
This comment has been minimized.
Ok current status:
Point being: As far as I know, ignoring things that I don't think are my place to resolve (which group to add the lint to, linking to the edition guide), I believe these two issues are the last outstanding bits from what has been identified so far |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Remaining issues:
I have gone back through all the crater issues @m-ou-se pointed out and rustfix seems to handle them all. |
We need to do that for all the 2021 migraiton lints. We can just do that for all of them in a separate PR. |
This comment has been minimized.
This comment has been minimized.
257fe7a
to
3dc47e2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
left some nits, I may just fix them
@bors r+ |
📌 Commit aa3580b has been approved by |
…_lint, r=nikomatsakis Add `future_prelude_collision` lint Implements rust-lang#84594. (RFC rust-lang/rfcs#3114 ([rendered](https://github.com/rust-lang/rfcs/blob/master/text/3114-prelude-2021.md))) Not entirely complete but wanted to have my progress decently available while I finish off the last little bits. Things left to implement: * [x] UI tests for lints * [x] Only emit lint for 2015 and 2018 editions * [ ] Lint name/message bikeshedding * [x] Implement for `FromIterator` (from best I can tell, the current approach as mentioned from [this comment](rust-lang#84594 (comment)) won't work due to `FromIterator` instances not using dot-call syntax, but if I'm correct about this then that would also need to be fixed for `TryFrom`/`TryInto`)* * [x] Add to `rust-2021-migration` group? (See rust-lang#85512) (added to `rust-2021-compatibility` group) * [ ] Link to edition guide in lint docs *edit: looked into it, `lookup_method` will also not be hit for `TryFrom`/`TryInto` for non-dotcall syntax. If anyone who is more familiar with typecheck knows the equivalent for looking up associated functions, feel free to chime in.
☀️ Test successful - checks-actions |
Implements #84594. (RFC rust-lang/rfcs#3114 (rendered))
Not entirely complete but wanted to have my progress decently available while I finish off the last little bits.Things left to implement:
FromIterator
(from best I can tell, the current approach as mentioned from this comment won't work due toFromIterator
instances not using dot-call syntax, but if I'm correct about this then that would also need to be fixed forTryFrom
/TryInto
)*rust-2021-migration
group? (See force-warn for edition lints #85512) (added torust-2021-compatibility
group)*edit: looked into it,
lookup_method
will also not be hit forTryFrom
/TryInto
for non-dotcall syntax. If anyone who is more familiar with typecheck knows the equivalent for looking up associated functions, feel free to chime in.