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.

Sign up for GitHub

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

Jump to bottom

Revert "resolve: Avoid "self-confirming" import resolutions in one more case" #77421

Merged
merged 1 commit into from
Oct 3, 2020
Merged

Revert "resolve: Avoid "self-confirming" import resolutions in one more case" #77421

merged 1 commit into from
Oct 3, 2020

Conversation

petrochenkov
Copy link
Contributor

And remove the assert that #70236 tried to avoid instead.

Closes #74556.

@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 1, 2020
@petrochenkov petrochenkov added beta-nominated Nominated for backporting to the compiler in the beta channel. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 1, 2020
@petrochenkov petrochenkov mentioned this pull request Oct 1, 2020
Closed
@Mark-Simulacrum
Copy link
Member

cc @rust-lang/compiler @pnkfelix @nikomatsakis for beta-backport approval, we'll need it by Monday if we want it to get in

I don't think I'm a good reviewer here. r? @Aaron1011 perhaps?

@Aaron1011
Copy link
Member

I've never touched any of the resolver code (all of my work has been in the parser/macro code). However, I can try my best to review this.

@Aaron1011
Copy link
Member

@petrochenkov: Can you add a test for issue #74556

@jyn514 jyn514 added the A-resolve Area: Name/path resolution done by `rustc_resolve` specifically label Oct 1, 2020
@petrochenkov
Copy link
Contributor Author

The case in #74556 is very similar to issue-62767.rs, but it actually triggers a different assert. I'll add it.

@petrochenkov
Copy link
Contributor Author

Updated.

@Aaron1011
Copy link
Member

I'm not really sure what any of this resolver code is doing - is there someone with more familiarity who can review this?

@nagisa
Copy link
Member

nagisa commented Oct 2, 2020

@bors r+, though I'm not comfortable with this being beta-nominated.

@oli-obk
Copy link
Contributor

oli-obk commented Oct 2, 2020

@bors r=nagisa

@bors
Copy link
Contributor

bors commented Oct 2, 2020

📌 Commit b20bce8 has been approved by nagisa

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 2, 2020
@petrochenkov
Copy link
Contributor Author

petrochenkov commented Oct 2, 2020 •
edited
Loading

Given that the release is very soon, I agree that it's risky to backport, since the PR can potentially turn code compiling on 1.44-1.46 into an error (it fixes a stable-to-stable regression).
If it's not backported, then the regression will affect four releases instead of three.

@vlad20012
Copy link
Member

vlad20012 commented Oct 2, 2020 •
edited
Loading

This re-opens #62767?

@petrochenkov
Copy link
Contributor Author

No, this PR removes the assert from #62767.

@jonas-schievink jonas-schievink mentioned this pull request Oct 2, 2020
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 2, 2020
@bors
Auto merge of rust-lang#77470 - jonas-schievink:rollup-9a2hulp, r=jon…
6ebad43 
…as-schievink

Rollup of 8 pull requests

Successful merges:

 - rust-lang#75377 (Fix Debug implementations of some of the HashMap and BTreeMap iterator types)
 - rust-lang#76107 (Write manifest for MAJOR.MINOR channel to enable rustup convenience)
 - rust-lang#76745 (Move Wrapping<T> ui tests into library)
 - rust-lang#77182 (Add missing examples for Fd traits)
 - rust-lang#77251 (Bypass const_item_mutation if const's type has Drop impl)
 - rust-lang#77264 (Only use LOCAL_{STDOUT,STDERR} when set_{print/panic} is used. )
 - rust-lang#77421 (Revert "resolve: Avoid "self-confirming" import resolutions in one more case")
 - rust-lang#77452 (Permit ty::Bool in const generics for v0 mangling)

Failed merges:

r? `@ghost`
@bors bors merged commit 23408de into rust-lang:master Oct 3, 2020
@rustbot rustbot added this to the 1.48.0 milestone Oct 3, 2020
@codyps codyps mentioned this pull request Oct 5, 2020
Closed
@petrochenkov
Copy link
Contributor Author

Removing beta-nomination since this causes breakage in practice - #77586.

@petrochenkov petrochenkov removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Oct 5, 2020
@Mark-Simulacrum Mark-Simulacrum mentioned this pull request Nov 5, 2020
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 7, 2020
@bors
Auto merge of rust-lang#78784 - Mark-Simulacrum:revert-77421, r=petro…
771cc7f 
…chenkov

Revert "Revert "resolve: Avoid "self-confirming" import resolutions in one more case""

Specifically, this reverts commit b20bce8 from rust-lang#77421 to fix rust-lang#77586.

The lang team has decided that for the time being we want to avoid the breakage here (perhaps for a future edition; though almost certainly not the upcoming one), though a future PR may want to add a lint around this case (and perhaps others) which are unlikely to be readable code.

r? `@petrochenkov` to confirm this is the right way to fix rust-lang#77586.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@Aaron1011 Aaron1011

Labels
A-resolve Area: Name/path resolution done by `rustc_resolve` specifically S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.48.0
Development

Successfully merging this pull request may close these issues.

Glob Time Travel
10 participants
@petrochenkov @rust-highfive @Mark-Simulacrum @Aaron1011 @nagisa @oli-obk @bors @vlad20012 @jyn514 @rustbot