[beta] resolve: Implement uniform paths 2.0 #55884
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
petrochenkov
added
S-waiting-on-crater
|
r? @eddyb |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Does that mean |
eddyb
approved these changes
Nov 12, 2018
|
I'll check this later today. :) |
|
I just read through all the commits. I'm not a compiler internals expert, so don't take this as a review on every aspect of the approach, but this makes sense to me. Could you explain the last commit, the "acceptable regressions", a bit more please? Is it difficult to give suggestions to tell people to delete Thank you for your incredible and timely work on the edition. |
No,
I haven't looked how exactly it's reported yet, if it's easily fixable, then I'll fix it. |
petrochenkov
force-pushed
the
uni
branch
2 times, most recently
from
3e417d0
to
c062c70
Compare
petrochenkov
removed
the
S-waiting-on-author
|
@bors try |
|
⌛ Trying commit c062c70f18bb3417fd5eb2537ca28215c4f4a666 with merge 85be765f536ab3c0a9f44f64f87ef72027503423... |
Centril
approved these changes
Nov 13, 2018
src/test/ui/rust-2018/uniform-paths-forward-compat/ambiguity-macros-nested.stderr
Show resolved
Hide resolved
|
|
||
| mod exported { | ||
| // Exported macros are treated as private as well, | ||
| // some better rules need to be figured out later. |
There was a problem hiding this comment.
Later is before stabilizing uniform_paths, or more into the future?
There was a problem hiding this comment.
Before.
Uniform paths can be stabilized in stages though - e.g. names in modules (named or unnamed) first, then everything else.
|
💔 Test failed - status-travis |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
bors
added
S-waiting-on-author
|
I'll look why clippy build fails. But why clippy prevents bors try from succeeding though? |
|
@petrochenkov I think a tool breaking is fatal on beta/stable and only ok on the nightly channel. |
|
Yes, beta and stable require all tools to build. |
petrochenkov
force-pushed
the
uni
branch
2 times, most recently
from
a558a55
to
2302ae6
Compare
|
@bors try |
bors
added a commit
that referenced
this pull request
Nov 18, 2018
[beta] resolve: Implement uniform paths 2.0 With this PR paths in imports on 2018 edition are resolved as relative in the scope in which they are written, similarly to any other paths. The previous implementation worked differently - it tried to resolve the import in the current module (`self::import`) and in the "crate universe" (`::import`), and then merge these two resolutions if possible. The difference is that the new scheme can refer to strictly larger set of names in scope - names from unnamed blocks, names from all kinds of preludes, including macros imported with `#[macro_use] extern crate`, built-in types/macros, macros introduced with `macro_rules`. This means strictly more potential ambiguities and therefore ambiguity errors, since we keep the rule that any two different candidate names in scope conflict with each other during import resolution. So this is a breaking change for 2018 edition, but it should be relatively minor. All paths that don't start with an extern crate are also gated with the `uniform_paths` feature, paths that refer to extern crates are not gated (so we effectively get something like "future-proofed anchored paths" on stable). Another difference is treatment of paths in visibilities (`pub(in path)`). Few people remember about paths in visibilities, so before this PR they still used the old 2015 rules even on 2018 edition. Namely, paths in visibilities were crate-relative, analogous to 2015 edition imports. This PR resolves paths in visibilities as uniform as well, or rather future proofs them in this direction. Paths in visibilities are restricted to parent modules, so relative paths almost never make sense there, and `pub(in a)` needs to be rewritten as `pub(in crate::a)` in the uniform scheme, de-facto cementing the discouraged status of non-`pub(crate)` and non-`pub(super)` fine-grained visibilities. This is clearly a breaking change for 2018 edition as well, but also a minor one. The in-scope resolution strategy for import paths mirrors what is currently done for macro paths on stable (on both editions), so it will continue working even if the "ambiguity always means error" restriction is relaxed in the future. This PR also significantly improves diagnostics for all kinds of resolution ambiguities, from the newly introduced import ones to pretty old "glob vs glob" conflicts. (That's probably what I've spent most of the time on.) Why beta: - This is a breaking change on 2018 edition. - This is a large PR, it's less risky to forward-port it to nightly, than back-port to beta. cc #55618 cc #53130 cc rust-lang/rfcs#1289 Closes #18084 Closes #54525 Fixes #54390 Fixes #55668 r? @ghost
|
☀️ Test successful - status-appveyor, status-travis |
This was referenced Nov 18, 2018
nagisa
added
the
beta-accepted
|
Marked as beta-accepted and removed beta-nominated (as it has landed) |
nagisa
removed
the
beta-nominated
bors
added a commit
that referenced
this pull request
Nov 25, 2018
[beta] resolve: Implement edition hygiene for imports and absolute paths The changes in behavior of imports and absolute paths are the most significant breaking changes of 2018 edition. However, these changes are not covered by edition hygiene, so macros defined by 2015 edition crates expanded in 2018 edition crates are still interpreted in the 2018 edition way when they contain imports or absolute paths. This means the promise of seamless integration of crates built with different editions, including use of macros, doesn't hold fully. This PR fixes that and implements edition hygiene for imports and absolute paths, so they behave according to the edition in which they were written, even in macros. ### Detailed rules (mostly taken from #50376) #### Preface We keep edition data per-span in the compiler. This means each span knows its edition. Each identifier has a span, so each identifier knows its edition. #### Absolute paths Explicitly written absolute paths `::ident::...` are desugared into something like `{{root}}::ident::...` in the compiler, where `{{root}}` is also treated as an identifier. `{{root}}` inherits its span/hygienic-context from the token `::`. If the span is from 2015 edition, then `{{root}}` is interpreted as the current crate root (`crate::...` with same hygienic context). If the span is from 2018 edition, then `{{root}}` is interpreted as "crate universe" (`extern::...`). #### Imports To resolve an import `use ident::...` we need to resolve `ident` first. The idea in this PR is that `ident` fully knows how to resolve itself. If `ident`'s span is from 2015 edition, then the identifier is resolved in the current crate root (effectively `crate::ident::...` where `crate` has the same hygienic context as `ident`). If `ident`'s span is from 2018 edition, then the identifier is resolved in the current scope, without prepending anything (well, at least with uniform paths). There's one corner case in which there's no identifier - prefix-less glob import `use *;`. In this case the span is inherited from the token `*`. `use *;` && `is_2015(span(*))` -> `use crate::*;` && `span(crate) == span(*)`. `use *;` && `is_2018(span(*))` -> error. --- Why beta: - Compatibility of 2015 edition crates with 2018 edition crates, including macros, is one of the main promises of the edition initiative. - This is technically a breaking change for 2018 edition crates or crates depending on 2018 edition crates. - ~This PR is based on #55884 which hasn't landed on nightly yet :)~ No longer true (#56042). Previous attempt #50999 Closes #50376 Closes #52848 Closes #53007 r? @ghost
pietroalbini
added a commit
to pietroalbini/rust
that referenced
this pull request
Nov 25, 2018
[master] Forward-ports from beta rust-lang#56206 + one commit from rust-lang#55884 that was accidentally missing in rust-lang#56042 due to an off-by-one mistake in commit ranges r? @ghost
This was referenced Nov 28, 2018
This was referenced Dec 27, 2018
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
With this PR paths in imports on 2018 edition are resolved as relative in the scope in which they are written, similarly to any other paths.
The previous implementation worked differently - it tried to resolve the import in the current module (
self::import) and in the "crate universe" (::import), and then merge these two resolutions if possible.The difference is that the new scheme can refer to strictly larger set of names in scope - names from unnamed blocks, names from all kinds of preludes, including macros imported with
#[macro_use] extern crate, built-in types/macros, macros introduced withmacro_rules.This means strictly more potential ambiguities and therefore ambiguity errors, since we keep the rule that any two different candidate names in scope conflict with each other during import resolution. So this is a breaking change for 2018 edition, but it should be relatively minor.
All paths that don't start with an extern crate are also gated with the
uniform_pathsfeature, paths that refer to extern crates are not gated (so we effectively get something like "future-proofed anchored paths" on stable).Another difference is treatment of paths in visibilities (
pub(in path)). Few people remember about paths in visibilities, so before this PR they still used the old 2015 rules even on 2018 edition. Namely, paths in visibilities were crate-relative, analogous to 2015 edition imports.This PR resolves paths in visibilities as uniform as well, or rather future proofs them in this direction.
Paths in visibilities are restricted to parent modules, so relative paths almost never make sense there, and
pub(in a)needs to be rewritten aspub(in crate::a)in the uniform scheme, de-facto cementing the discouraged status of non-pub(crate)and non-pub(super)fine-grained visibilities.This is clearly a breaking change for 2018 edition as well, but also a minor one.
The in-scope resolution strategy for import paths mirrors what is currently done for macro paths on stable (on both editions), so it will continue working even if the "ambiguity always means error" restriction is relaxed in the future.
This PR also significantly improves diagnostics for all kinds of resolution ambiguities, from the newly introduced import ones to pretty old "glob vs glob" conflicts. (That's probably what I've spent most of the time on.)
Why beta:
cc #55618
cc #53130
cc rust-lang/rfcs#1289
Closes #18084
Closes #54525
Fixes #54390
Fixes #55668
r? @ghost