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

[beta] resolve: Implement edition hygiene for imports and absolute paths #56053

Merged
merged 6 commits into from Nov 26, 2018

Conversation

@petrochenkov
Copy link
Contributor

petrochenkov commented Nov 19, 2018

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.

UPDATE

After crater run the rules were updated to include fallback from 2015 edition behavior to 2018 edition behavior, see #56053 (comment) for more details.


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

@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

petrochenkov commented Nov 19, 2018

@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

petrochenkov commented Nov 19, 2018

It would be good to run crater on this if we still have time.
I wonder how many macro crates already rely on imports and absolute paths being edition-unhygienic.
cc @dtolnay @alexcrichton

@bors try

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Nov 19, 2018

⌛️ Trying commit 59d1c85 with merge 4f24eba...

bors added a commit that referenced this pull request Nov 19, 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.
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.

TODO: Write detailed rules.

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 :)

Previous attempt #50999
Closes #50376
Closes #52848
Closes #53007

r? @ghost
@eddyb

This comment has been minimized.

Copy link
Member

eddyb commented Nov 19, 2018

@petrochenkov How many other parts of the compiler check the "global" edition?
Would it be feasible to move all of them over?

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Nov 19, 2018

☀️ Test successful - status-travis
State: approved= try=True

@Centril

This comment has been minimized.

Copy link
Member

Centril commented Nov 19, 2018

The changes make a lot of sense. To get the ball rolling, pending crater run and code review (I mostly checked tests), I propose that we stabilize the behavior described in #56053 (comment).

@Centril Centril added the T-lang label Nov 19, 2018
@Centril

This comment has been minimized.

Copy link
Member

Centril commented Nov 19, 2018

@rfcbot merge

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Nov 19, 2018

Team member @Centril has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@joshtriplett

This comment has been minimized.

Copy link
Member

joshtriplett commented Nov 19, 2018

Assuming crater passes: wow. Incredible work, @petrochenkov!

@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

petrochenkov commented Nov 19, 2018

@eddyb

How many other parts of the compiler check the "global" edition?

I'm not sure, will check today.
Edition-specific keywords are certainly covered, everything lint-related is probably non-critical, and NLL must be enabled globally.
Other changes between editions should be minor, IIRC.

@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

petrochenkov commented Nov 19, 2018

Assuming crater passes

What I'm concerned most is macro crates using imports and absolute paths to refer solely to extern crates and assuming extern crate items in the calling crate's root.

// Macro crate 2015 (procedural or declarative)
macro mac2015() {
    use my_crate::foo;
    fn do_things() {
        ::my_crate::bar();
    }
}

---

// User crate 2015

extern crate my_crate;

mac2015!();

---

// User crate 2018 (without hygiene)

mac2015!();

In this case despite the change in resolution behavior the macro still works on 2018 edition, and even in more idiomatic way - extern crate item in the root is not required.

  • ::my_crate still refers to an extern crate with 2018 rules (but now in extern prelude rather than in the root)
  • use my_crate refers to an extern crate with 2018 rules (but now in extern prelude rather than in the root), but now it's unreliable - it may be shadowed.

With edition hygiene as implemented in this PR, 2018 crates will still need an item referring to the extern crate in the root module when using 2015 macro crates using absolute paths and imports in this way.

// User crate 2018 (with hygiene)

use my_crate;

mac2015!();

(However, imports use my_crate::foo; become reliable again.)

@petrochenkov

This comment was marked as outdated.

Copy link
Contributor Author

petrochenkov commented Nov 19, 2018

@craterbot run start=master#d08a0a89c6ed1d7c66cfbee2f85721a2d1696885 end=try#4f24eba9a9b426a262d22d2ce72a4139c3860eb6 mode=check-only

@craterbot

This comment was marked as outdated.

Copy link
Collaborator

craterbot commented Nov 19, 2018

👌 Experiment pr-56053 created and queued.
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot

This comment was marked as outdated.

Copy link
Collaborator

craterbot commented Nov 19, 2018

🚧 Experiment pr-56053 is now running on agent aws-1.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@nikomatsakis nikomatsakis added this to the Rust 2018 Release milestone Nov 19, 2018
@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

petrochenkov commented Nov 19, 2018

Argh, there's much worse issue - 2015 macros using use std::foo; or ::std::foo.
With hygiene this desugars into use crate::std::foo/crate::std::foo.
The problem is that std no longer exists in the crate root on 2018 edition, it was removed by @eddyb in cd47831.

Possible solutions:

  • Reintroduce std into the crate root on 2018 edition, but discourage its use through a lint or something.
    I think I'd prefer this solution because it also minimizes the behavior differences between editions and number of "global" edition checks.
    I'll prepare this variant for crater run.
  • Make {{root}} edition-unhygienic, whether it's added explicitly or not.
    This way use foo::bar still desugars into use {{root}}::foo::bar if span(foo).is_2015(), but the {{root}} is interpreted differently on 2015 and 2018.
  • Keep both absolute paths and imports unhygienic and close this PR.

@craterbot abort

@craterbot

This comment has been minimized.

Copy link
Collaborator

craterbot commented Nov 19, 2018

🗑 Experiment pr-56053 deleted!

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@Centril

This comment has been minimized.

Copy link
Member

Centril commented Nov 19, 2018

@petrochenkov in solution 1, would we also introduce crate::core:: for consistency then?

@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

petrochenkov commented Nov 19, 2018

@Centril
Yes, all automatically injected crates - std, core (depending on no-std options), and also compiler_builtins (but it's unstable to use, so it doesn't matter much).

@Centril

This comment has been minimized.

Copy link
Member

Centril commented Nov 19, 2018

@petrochenkov Seems like the cleanest solution to me also; and making it work with #![no_std] is great.

bors added a commit that referenced this pull request Nov 26, 2018
…chton

[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.

#### UPDATE

After crater run the rules were updated to include fallback from 2015 edition behavior to 2018 edition behavior, see #56053 (comment) for more details.

---
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

This comment has been minimized.

Copy link
Member

pietroalbini commented Nov 26, 2018

@bors retry r-

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Nov 26, 2018

Your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
Resolving deltas: 100% (7850/7850), done.
travis_time:end:008ac2ee:start=1543253255439045556,finish=1543253261293093851,duration=5854048295
$ cd rust-lang/rust
$ git checkout -qf d33bb3779c94a313e9d07bccc386258cc2d19ebb
fatal: reference is not a tree: d33bb3779c94a313e9d07bccc386258cc2d19ebb
The command "git checkout -qf d33bb3779c94a313e9d07bccc386258cc2d19ebb" failed and exited with 128 during .
Your build has been stopped.

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 @TimNN. (Feature Requests)

pietroalbini added a commit to nikomatsakis/rust that referenced this pull request Nov 26, 2018
…akis,alexcrichton
@nikomatsakis nikomatsakis mentioned this pull request Nov 26, 2018
2 of 2 tasks complete
@pietroalbini

This comment has been minimized.

Copy link
Member

pietroalbini commented Nov 26, 2018

Rolled up in #56240.

bors added a commit that referenced this pull request Nov 26, 2018
beta backports

Backported PRs:

- [x]  Avoid panic when matching function call #55742
- [x]  [beta] resolve: Implement edition hygiene for imports and absolute paths #56053

r? @ghost
cc @rust-lang/release
@bors bors merged commit 581b683 into rust-lang:beta Nov 26, 2018
1 of 2 checks passed
1 of 2 checks passed
homu Testing commit 581b68380f6740124100d6c434e7f3e3da03d764 with merge d33bb3779c94a313e9d07bccc386258cc2d19ebb...
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Nov 26, 2018

☔️ The latest upstream changes (presumably #56240) made this pull request unmergeable. Please resolve the merge conflicts.

bors added a commit that referenced this pull request Nov 27, 2018
[master] resolve: Implement edition hygiene for imports and absolute paths

Forward-port of #56053 to master.

r? @ghost
bors added a commit that referenced this pull request Jan 12, 2019
Stabilize `uniform_paths`

Address all the things described in #56417.

Assign visibilities to `macro_rules` items - `pub` to `macro_export`-ed macros and `pub(crate)` to non-exported macros, these visibilities determine how far these macros can be reexported with `use`.

Prohibit use of reexported inert attributes and "tool modules", after renaming (e.g. `use inline as imported_inline`) their respective tools and even compiler passes won't be able to recognize and properly check them.

Also turn use of uniform paths in 2015 macros into an error, I'd prefer to neither remove nor stabilize them right away because I still want to make some experiments in this area (uniform path fallback was added to 2015 macros used on 2018 edition in #56053 (comment)).

UPDATE: The last commit also stabilizes the feature `uniform_paths`.

Closes #53130
Closes #55618
Closes #56326
Closes #56398
Closes #56417
Closes #56821
Closes #57252
Closes #57422
bors added a commit that referenced this pull request Jan 12, 2019
Stabilize `uniform_paths`

Address all the things described in #56417.

Assign visibilities to `macro_rules` items - `pub` to `macro_export`-ed macros and `pub(crate)` to non-exported macros, these visibilities determine how far these macros can be reexported with `use`.

Prohibit use of reexported inert attributes and "tool modules", after renaming (e.g. `use inline as imported_inline`) their respective tools and even compiler passes won't be able to recognize and properly check them.

Also turn use of uniform paths in 2015 macros into an error, I'd prefer to neither remove nor stabilize them right away because I still want to make some experiments in this area (uniform path fallback was added to 2015 macros used on 2018 edition in #56053 (comment)).

UPDATE: The last commit also stabilizes the feature `uniform_paths`.

Closes #53130
Closes #55618
Closes #56326
Closes #56398
Closes #56417
Closes #56821
Closes #57252
Closes #57422
bors added a commit that referenced this pull request Jan 18, 2019
[WIP] resolve: Fallback to extern crates in absolute paths on 2015 edition

TODO: Run crater, fix diagnostics

This PR changes the resolution scheme for imports and absolute paths from

| Local edition | Global 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 | Crate-relative with fallback to Extern prelude |

(which was introduced in #56053) to

| Local edition | Global edition | Imports (`use foo;`)                                 | Absolute paths (`::foo`)                                 |
| ------------- |----------------|-----------------------------------------|------------------------------------------------|
| 2018          | Any            | Uniform                                 | Extern prelude                                 |
| 2015          | Any            | Crate-relative with fallback to Extern prelude                          | Crate-relative with fallback to Extern prelude                                 |

(with `use foo;` still desugaring into `use ::foo;` on 2015 edition).

This way we
- Get rid of the special case "2015 macro used on 2018 edition".
- Resolve the issue discussed in #55478, i.e. "on 2015 edition you don't need `extern crate` until you need `use`, then you need `extern crate`". With this change `use my_crate::foo` and `let x = ::my_crate::foo` work without needing `extern crate` consistently with `let x = my_crate::foo`.

r? @Centril
bors added a commit that referenced this pull request Mar 13, 2019
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
@petrochenkov petrochenkov deleted the petrochenkov:absedihyg branch Jun 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.