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

petrochenkov · 2019-01-18T21:01:11Z

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 Rust stable 1.30 allows use of extern crates without declaration #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 · 2019-01-18T21:01:37Z

@bors try

bors · 2019-01-18T21:01:50Z

⌛ Trying commit bd4a554 with merge 29640c5...

@Centril

[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 · 2019-01-18T23:17:55Z

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

petrochenkov · 2019-01-19T00:00:58Z

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

craterbot · 2019-01-19T00:01:00Z

👌 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 · 2019-01-19T00:01:01Z

🚧 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 · 2019-01-20T07:39:57Z

🎉 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 · 2019-01-20T10:41:02Z

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 · 2019-01-20T10:45:04Z

@Centril
Could this go through lang team now?

petrochenkov · 2019-01-20T10:52:55Z

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

Code & tests look good from what I can tell.

Centril · 2019-01-21T03:23:20Z

src/librustc_resolve/resolve_imports.rs

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


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

Centril · 2019-01-21T22:47:03Z

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 · 2019-01-21T22:47:04Z

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 at most 2 approvals are outstanding), 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.

nikomatsakis · 2019-01-24T21:08:08Z

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 · 2019-01-24T21:17:53Z

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 · 2019-01-24T21:22:56Z

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 · 2019-01-24T21:29:36Z

On Thu, Jan 24, 2019 at 01:23:27PM -0800, Vadim Petrochenkov wrote: 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.

I don't think the goal should be "get rid of extern crate", it should be "get rid of extern crate in the 2018 edition".

Centril · 2019-01-24T23:14:18Z

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.. ;)

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>

Dylan-DPC-zz · 2019-03-11T09:33:20Z

ping from triage @petrochenkov any updates on this?

petrochenkov · 2019-03-11T09:57:12Z

Blocked on #58349 at least.

@Centril

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

Centril · 2019-03-16T03:31:45Z

Blocked on #58349 at least.

(landed)

petrochenkov · 2019-03-21T19:28:43Z

Another example of confusion caused by #55478 - https://www.reddit.com/r/rust/comments/b2uf9x/use_sometimes_cannot_be_exchanged_for_extern_crate/

petrochenkov · 2019-03-21T20:10:51Z

Ok, I think I'm going to close this anyway.
With #58349 merged there's not much reason left doing this for the compiler cleanup purpose.
The use of 2015 edition will decrease, so there's less initiative to fix 2015-only issues like #55478.

In any case, given that years of use of Rust 2015 with use items being used in every project produced only few regressions (#57745 (comment)), we'll be able to make this change at any time if necessary and there's no urgency to do it right now.

resolve: Fallback to extern crates in absolute paths on 2015 edition

bd4a554

rust-highfive assigned Centril Jan 18, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 18, 2019

petrochenkov added T-lang Relevant to the language team, which will review and decide on the PR/issue. S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 18, 2019

petrochenkov mentioned this pull request Jan 18, 2019

Rust stable 1.30 allows use of extern crates without declaration #55478

Closed

craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Jan 20, 2019

petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 20, 2019

petrochenkov added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 20, 2019

Centril approved these changes Jan 21, 2019

View reviewed changes

rust-highfive assigned nikomatsakis and unassigned Centril Jan 21, 2019

rfcbot added the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Jan 21, 2019

Centril removed the I-nominated label Jan 24, 2019

petrochenkov added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jan 24, 2019

Dylan-DPC-zz removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Feb 4, 2019

petrochenkov added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Feb 4, 2019

petrochenkov mentioned this pull request Feb 10, 2019

resolve: Simplify import resolution for mixed 2015/2018 edition mode #58349

Merged

flodiebold mentioned this pull request Feb 12, 2019

Prelude & Edition 2015 import resolution rust-lang/rust-analyzer#816

Merged

petrochenkov added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 11, 2019

petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. labels Mar 16, 2019

petrochenkov closed this Mar 21, 2019

rfcbot removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Mar 21, 2019

Dylan-DPC-zz removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Mar 21, 2019

petrochenkov deleted the uni2015 branch June 5, 2019 16:28

petrochenkov mentioned this pull request Jun 22, 2023

core sometimes is resolved when not defined #112924

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

petrochenkov commented Jan 18, 2019 •

edited

Loading

petrochenkov commented Jan 18, 2019

bors commented Jan 18, 2019

bors commented Jan 18, 2019

petrochenkov commented Jan 19, 2019

craterbot commented Jan 19, 2019

craterbot commented Jan 19, 2019

craterbot commented Jan 20, 2019

petrochenkov commented Jan 20, 2019

petrochenkov commented Jan 20, 2019

petrochenkov commented Jan 20, 2019 •

edited

Loading

Centril left a comment

Centril Jan 21, 2019

Centril commented Jan 21, 2019

rfcbot commented Jan 21, 2019 •

edited

Loading

nikomatsakis commented Jan 24, 2019

petrochenkov commented Jan 24, 2019

petrochenkov commented Jan 24, 2019

joshtriplett commented Jan 24, 2019 via email

Centril commented Jan 24, 2019

Dylan-DPC-zz commented Mar 11, 2019

petrochenkov commented Mar 11, 2019

Centril commented Mar 16, 2019

petrochenkov commented Mar 21, 2019

petrochenkov commented Mar 21, 2019 •

edited

Loading

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

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

Conversation

petrochenkov commented Jan 18, 2019 • edited Loading

petrochenkov commented Jan 18, 2019

bors commented Jan 18, 2019

bors commented Jan 18, 2019

petrochenkov commented Jan 19, 2019

craterbot commented Jan 19, 2019

craterbot commented Jan 19, 2019

craterbot commented Jan 20, 2019

petrochenkov commented Jan 20, 2019

petrochenkov commented Jan 20, 2019

petrochenkov commented Jan 20, 2019 • edited Loading

Centril left a comment

Choose a reason for hiding this comment

Centril Jan 21, 2019

Choose a reason for hiding this comment

Centril commented Jan 21, 2019

rfcbot commented Jan 21, 2019 • edited Loading

nikomatsakis commented Jan 24, 2019

petrochenkov commented Jan 24, 2019

petrochenkov commented Jan 24, 2019

joshtriplett commented Jan 24, 2019 via email

Centril commented Jan 24, 2019

Dylan-DPC-zz commented Mar 11, 2019

petrochenkov commented Mar 11, 2019

Centril commented Mar 16, 2019

petrochenkov commented Mar 21, 2019

petrochenkov commented Mar 21, 2019 • edited Loading

petrochenkov commented Jan 18, 2019 •

edited

Loading

petrochenkov commented Jan 20, 2019 •

edited

Loading

rfcbot commented Jan 21, 2019 •

edited

Loading

petrochenkov commented Mar 21, 2019 •

edited

Loading