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

[WIP] resolve: Fallback to extern crates in absolute paths on 2015 edition #57745

Open
wants to merge 1 commit into
base: master
from

Conversation

@petrochenkov
Copy link
Contributor

petrochenkov commented Jan 18, 2019

TODO: Run crater, fix diagnostics, cleanup implementation.

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 was first suggested in #56053 (comment), but wasn't done in that PR due to release schedule. Now there's enough time to do that experiment.

This way we

  • Get rid of the special case "2015 macro used on 2018 edition" (and eliminate one more use of "global 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

@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

petrochenkov commented Jan 18, 2019

@bors try

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 18, 2019

⌛️ Trying commit bd4a554 with merge 29640c5...

bors added a commit that referenced this pull request Jan 18, 2019

Auto merge of #57745 - petrochenkov:uni2015, r=<try>
[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

This comment has been minimized.

Copy link
Contributor

bors commented Jan 18, 2019

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

@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

petrochenkov commented Jan 19, 2019

@craterbot run start=master#f613dc138b4012cf3d2eb40718fbcc7cf0a34039 end=try#29640c57b5f92febba0e40c50cb863c9a7ede51d mode=check-only

@craterbot

This comment has been minimized.

Copy link
Collaborator

craterbot commented Jan 19, 2019

👌 Experiment pr-57745 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 has been minimized.

Copy link
Collaborator

craterbot commented Jan 19, 2019

🚧 Experiment pr-57745 is now running on agent aws-2.

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

@craterbot

This comment has been minimized.

Copy link
Collaborator

craterbot commented Jan 20, 2019

🎉 Experiment pr-57745 is completed!
📊 9 regressed and 1 fixed (50551 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

petrochenkov commented Jan 20, 2019

Root regressions:

  • actix-web v0.7.15 (tests) - extern crate http shadowed by a glob import
  • spaces v4.1.0 - extern crate core shadowed by a macro-expanded name
  • shared_memory v0.8.1 - extern crate nix shadowed by a macro-expanded name
  • static-filez v0.1.0 - extern crate structopt shadowed by a glob import
  • skeptic v0.5.0 - extern crate structopt shadowed by a glob import
  • feeder v0.1.0 - extern crate structopt shadowed by a glob import

In all cases the error is fixable with one of suggestions provided by the compiler.

That's not much given that the change affects all 2015 imports and almost every crate in existence uses imports.
So, if we want to make this change then it's quite realistic to do it through deprecation lint.

@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

petrochenkov commented Jan 20, 2019

@Centril
Could this go through lang team now?

@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

petrochenkov commented Jan 20, 2019

If you remember early stages of the import reform discussion, this is exactly the "fallback from crate::foo to extern::foo in use foo to eliminate extern crate without edition breakage" variant.

I had concerns about its implementability back then and wanted to make a proof of concept implementation before choosing it, but now all the infrastructure is ready thanks to uniform paths, so the implementation can be done easily.

@Centril
Copy link
Contributor

Centril left a comment

Code & tests look good from what I can tell.

@@ -765,6 +765,9 @@ impl<'a, 'b:'a> ImportResolver<'a, 'b> {
} else {
// For better failure detection, pretend that the import will
// not define any names while resolving its module path.
let orig_root_glob_import = mem::replace(
&mut self.root_glob_import, directive.is_glob() && directive.module_path.len() == 1

This comment has been minimized.

@Centril

Centril Jan 21, 2019

Contributor

Would be nice to extract this out to a new method (+ comment on that) since it's repeated below.

@Centril

This comment has been minimized.

Copy link
Contributor

Centril commented Jan 21, 2019

Assigning over to r? @nikomatsakis to double check.

I believe these changes are sorely needed to make the module system and resolution more scrutable and reduce the number of rules since even language team members have a hard time following them.

At the same time, this is not exactly a bug-fix but rather a choice to make to simplify things and make the edition story smoother. In particular, the breakage to skeptic makes me uneasy. (actix only has a test breakage which makes it less problematic).

I am torn about this... but the amount of regressions is sufficiently small to make filing PRs to each of them possible.. and I think the ecosystem as a whole will benefit more than it suffers; so with that said, let's see what the rest of the team thinks...

@rfcbot merge

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Jan 21, 2019

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

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 Jan 22, 2019

That this has any breakage at all concerns me greatly. I understand that this simplifies the story, but it does so at the expense of a little bit of 2015 compatibility.

@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

petrochenkov commented Jan 22, 2019

@joshtriplett
I've just confirmed that all the regressed crates (#57745 (comment)) build successfully if the ambiguity errors are demoted to deprecation lints.
So the "breakage process" can be stretched for an arbitrary period of time.

@joshtriplett

This comment has been minimized.

Copy link
Member

joshtriplett commented Jan 22, 2019

@petrochenkov I understand that the crates can be fixed, but why do we consider it reasonable to break backward compatibility here?

Ambiguity errors are supposed to be a 2018 thing, not a 2015 thing.

@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

petrochenkov commented Jan 22, 2019

@joshtriplett

Ambiguity errors are supposed to be a 2018 thing, not a 2015 thing.

Technically the ambiguity errors are an "early-phase resolution + fallback" thing, so they happen on 2015 edition as well, with macro paths.

why do we consider it reasonable to break backward compatibility here?

Because it makes everyone's life easier.
Starting from users confused by edition differences and the "half there" model currently working on 2015 edition with regards to requiring extern crate, ending with people supporting these edition differences and interactions between them in the compiler / language spec.
A lint firing in corner cases seems like an ok price to pay in this case.

@aturon

This comment has been minimized.

Copy link
Member

aturon commented Jan 23, 2019

Nominating for Lang Team discussion.

@Centril

This comment has been minimized.

Copy link
Contributor

Centril commented Jan 23, 2019

@petrochenkov Can you attend the meeting tomorrow (at 21:00 CET)?

@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

petrochenkov commented Jan 23, 2019

@Centril
Yes, most likely.

@Centril Centril removed the I-nominated label Jan 24, 2019

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Jan 24, 2019

We discussed this in the @rust-lang/lang meeting today, with @petrochenkov present.

What is being proposed here is a backwards incompatible change made in the name of overall language simplicity and consistency. This kind of things is always a balancing act: on the one hand, we aim not to break existing code. On the other hand, simplifying things helps everyone in the end, and definitely may help with long-term compiler maintenance. Not to put words in their mouth, but @joshtriplett seemed to feel that this change was not worth it overall, but I think that @Centril disagreed, and others seemed a bit uncertain (these were my impressions).

One compromise that was proposed was to try and put in some form of warning and specifically request feedback from those affected -- basically defer the final decision until we've had warnings for some time and possibly heard from affected crates. @aturon called it an "extended crater run".

Ultimately, no firm conclusion was reached.

@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

petrochenkov commented Jan 24, 2019

One compromise that was proposed was to try and put in some form of warning and specifically request feedback from those affected -- basically defer the final decision until we've had warnings for some time and possibly heard from affected crates.

I suggest to specifically:

  • Report the ambiguity errors as warnings.
  • Fall back to extern crates, but emit a feature gate error if the fallback actually happens.

The final (positive) decision in that case would be removal of the feature gate, and the final negative decision would be removal of the fallback perhaps ambiguity warnings.

@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

petrochenkov commented Jan 24, 2019

I also think I haven't properly answered the question "would this change make sense in isolation, regardless of breakage and cross-edition concerns (?)".

The answer is that if getting rid of extern crate items is a worthwhile goal (and it seems it is, because this is the direction in which 2018 edition moved), then it's certainly a worthwhile goal because this fallback gives exactly the "no extern crate needed" effect on 2015 edition.

@joshtriplett

This comment has been minimized.

Copy link
Member

joshtriplett commented Jan 24, 2019

@Centril

This comment has been minimized.

Copy link
Contributor

Centril commented Jan 24, 2019

but I think that @Centril disagreed, and others seemed a bit uncertain (these were my impressions).

To clarify, I am uncertain / torn as well. It's not an easy decision to make.. ;)

bors bot added a commit to rust-analyzer/rust-analyzer that referenced this pull request Feb 13, 2019

Merge #816
816: Prelude & Edition 2015 import resolution r=matklad a=flodiebold

I implemented the prelude import, but it turned out to be useless without being able to resolve any of the imports in the prelude 😅 So I had to add some edition handling and handle 2015-style imports (at least the simplified scheme proposed in rust-lang/rust#57745). So now finally `Option` resolves 😄 

One remaining problem is that we don't actually know the edition for sysroot crates. They're currently hardcoded to 2015, but there's already a bunch of PRs upgrading the editions of various rustc crates, so we'll have to detect the edition somehow, or just change the hardcoding to 2018 later, I guess...

~Also currently missing is completion for prelude names, though that shouldn't be hard to add. And `Vec` still doesn't resolve, so I need to look into that.~

Co-authored-by: Florian Diebold <flodiebold@gmail.com>

bors bot added a commit to rust-analyzer/rust-analyzer that referenced this pull request Feb 13, 2019

Merge #816
816: Prelude & Edition 2015 import resolution r=matklad a=flodiebold

I implemented the prelude import, but it turned out to be useless without being able to resolve any of the imports in the prelude 😅 So I had to add some edition handling and handle 2015-style imports (at least the simplified scheme proposed in rust-lang/rust#57745). So now finally `Option` resolves 😄 

One remaining problem is that we don't actually know the edition for sysroot crates. They're currently hardcoded to 2015, but there's already a bunch of PRs upgrading the editions of various rustc crates, so we'll have to detect the edition somehow, or just change the hardcoding to 2018 later, I guess...

~Also currently missing is completion for prelude names, though that shouldn't be hard to add. And `Vec` still doesn't resolve, so I need to look into that.~

Co-authored-by: Florian Diebold <flodiebold@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment