-
Notifications
You must be signed in to change notification settings - Fork 13.3k
resolve: Simplify import resolution for mixed 2015/2018 edition mode #58349
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Conversation
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.
lgtm, assigning over to double check...
r? @pnkfelix -- sorry @petrochenkov for being slow. I've been pretty overloaded so I'm moving the review over to @pnkfelix who hopefully has a bit more time! |
ping from triage @pnkfelix waiting for your review on this |
☔ The latest upstream changes (presumably #59044) made this pull request unmergeable. Please resolve the merge conflicts. |
This is now blocking another PR - #58805. |
r=me assuming merge conflict is trivial to resolve. |
@bors r=pnkfelix |
📌 Commit 1d6f4d6 has been approved by |
resolve: Simplify import resolution for mixed 2015/2018 edition mode Non-controversial part of #57745. Before: | Local edition (per-span) | Global edition (--edition) | Imports (`use foo;`) | Absolute paths (`::foo`) | | ------------- |----------------|-----------------------------------------|------------------------------------------------| | 2018 | Any | Uniform | Extern prelude | | 2015 | 2015 | Crate-relative | Crate-relative | | 2015 | 2018 | Crate-relative with fallback to Uniform (future-proofed to error if the result is not Crate-relative or from Extern prelude) | Crate-relative with fallback to Extern prelude | After: | Local edition (per-span) | Global edition (--edition) | Imports (`use foo;`) | Absolute paths (`::foo`) | | ------------- |----------------|-----------------------------------------|------------------------------------------------| | 2018 | Any | Uniform | Extern prelude | | 2015 | 2015 | Crate-relative | Crate-relative | | 2015 | 2018 | Crate-relative with fallback to Extern prelude | Crate-relative with fallback to Extern prelude | I.e. only the behavior of the mixed local-2015-global-2018 mode is changed. This mixed mode has two goals: - Address regressions from #56053 (comment). Both "before" and "after" variants address those regressions. - Be retrofit-able to "full 2015" edition (#57745). Any more complex fallback scheme (with more candidates) than "Crate-relative with fallback to Extern prelude" will give more regressions than #57745 (comment) and is therefore less retrofit-able while also being, well, more complex. So, we can settle on "Crate-relative with fallback to Extern prelude". (I'll hopefully proceed with #57745 after mid-February.) r? @Centril
☀️ Test successful - checks-travis, status-appveyor |
Non-controversial part of #57745.
Before:
use foo;
)::foo
)After:
use foo;
)::foo
)I.e. only the behavior of the mixed local-2015-global-2018 mode is changed.
This mixed mode has two goals:
Both "before" and "after" variants address those regressions.
Any more complex fallback scheme (with more candidates) than "Crate-relative with fallback to Extern prelude" will give more regressions than [WIP] resolve: Fallback to extern crates in absolute paths on 2015 edition #57745 (comment) and is therefore less retrofit-able while also being, well, more complex.
So, we can settle on "Crate-relative with fallback to Extern prelude".
(I'll hopefully proceed with #57745 after mid-February.)
r? @Centril