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

[NLL] Improve DefiningTy::Const #47957

Merged
merged 2 commits into from
Feb 7, 2018
Merged

Conversation

sapphire-arches
Copy link
Contributor

Fixes #47590 by fixing the way DefiningTy represents constants. Previously, constants were represented using just the type of the variable. However, this will fail to capture early-bound regions as NLL inference vars, resulting in an ICE when we try to compute region VIDs a little bit later in the universal
region resolution process. (ref #47590)

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @michaelwoerister (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@sapphire-arches
Copy link
Contributor Author

r? @nikomatsakis

Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work =)

@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Feb 2, 2018

📌 Commit 2ed3969 has been approved by nikomatsakis

@shepmaster shepmaster added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Feb 3, 2018
@kennytm
Copy link
Member

kennytm commented Feb 5, 2018

@bors p=1

Increasing priority of PRs with higher complexity.

@bors
Copy link
Contributor

bors commented Feb 5, 2018

⌛ Testing commit 2ed39698ca15d9abe947491a56d17d0f2286aed2 with merge 2435c2ced0cb34b039b04033a3419c200d24f406...

@bors
Copy link
Contributor

bors commented Feb 5, 2018

💔 Test failed - status-travis

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 5, 2018
@kennytm
Copy link
Member

kennytm commented Feb 6, 2018

The ui/nll/trait-associated-constant.rs output doesn't match what is shown in this PR on arm-unknown-linux-gnueabihf. The error span ends at the struct name, instead of the }.

This might be due to recent diagnostic changes on master. Try to rebase on latest master and regenerate the *.stderr file?

[00:55:05] ---- [ui] ui/nll/trait-associated-constant.rs stdout ----
[00:55:05] 	diff of stderr:
[00:55:05] 
[00:55:05] 9	note: the lifetime 'c as defined on the impl at 30:1...
[00:55:05] 10	  --> $DIR/trait-associated-constant.rs:30:1
[00:55:05] 11	   |
[00:55:05] -	30 | / impl<'a: 'b, 'b, 'c> Anything<'a, 'b> for FailStruct1 {
[00:55:05] -	31 | |     const AC: Option<&'c str> = None;
[00:55:05] -	32 | |     //~^ ERROR: mismatched types
[00:55:05] -	33 | | }
[00:55:05] -	   | |_^
[00:55:05] +	30 | impl<'a: 'b, 'b, 'c> Anything<'a, 'b> for FailStruct1 {
[00:55:05] +	   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[00:55:05] 17	note: ...does not necessarily outlive the lifetime 'b as defined on the impl at 30:1
[00:55:05] 18	  --> $DIR/trait-associated-constant.rs:30:1
[00:55:05] 19	   |
[00:55:05] 
[00:55:05] -	30 | / impl<'a: 'b, 'b, 'c> Anything<'a, 'b> for FailStruct1 {
[00:55:05] -	31 | |     const AC: Option<&'c str> = None;
[00:55:05] -	32 | |     //~^ ERROR: mismatched types
[00:55:05] -	33 | | }
[00:55:05] -	   | |_^
[00:55:05] +	30 | impl<'a: 'b, 'b, 'c> Anything<'a, 'b> for FailStruct1 {
[00:55:05] +	   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[00:55:05] 25	
[00:55:05] 26	error[E0308]: mismatched types
[00:55:05] 27	  --> $DIR/trait-associated-constant.rs:38:5
[00:55:05] 
[00:55:05] 34	note: the lifetime 'a as defined on the impl at 37:1...
[00:55:05] 35	  --> $DIR/trait-associated-constant.rs:37:1
[00:55:05] 36	   |
[00:55:05] -	37 | / impl<'a: 'b, 'b> Anything<'a, 'b> for FailStruct2 {
[00:55:05] -	38 | |     const AC: Option<&'a str> = None;
[00:55:05] -	39 | |     //~^ ERROR: mismatched types
[00:55:05] -	40 | | }
[00:55:05] -	   | |_^
[00:55:05] +	37 | impl<'a: 'b, 'b> Anything<'a, 'b> for FailStruct2 {
[00:55:05] +	   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[00:55:05] 42	note: ...does not necessarily outlive the lifetime 'b as defined on the impl at 37:1
[00:55:05] 43	  --> $DIR/trait-associated-constant.rs:37:1
[00:55:05] 44	   |
[00:55:05] 
[00:55:05] -	37 | / impl<'a: 'b, 'b> Anything<'a, 'b> for FailStruct2 {
[00:55:05] -	38 | |     const AC: Option<&'a str> = None;
[00:55:05] -	39 | |     //~^ ERROR: mismatched types
[00:55:05] -	40 | | }
[00:55:05] -	   | |_^
[00:55:05] +	37 | impl<'a: 'b, 'b> Anything<'a, 'b> for FailStruct2 {
[00:55:05] +	   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[00:55:05] 50	
[00:55:05] 51	error: aborting due to 2 previous errors
[00:55:05] 52	

@kennytm kennytm 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 Feb 6, 2018
sapphire-arches and others added 2 commits February 6, 2018 23:42
Fixes rust-lang#47590 by fixing the way DefiningTy represents constants. Previously,
constants were represented using just the type of the variable. However, this
will fail to capture early-bound regions as NLL inference vars, resulting in an
ICE when we try to compute region VIDs a little bit later in the universal
region resolution process.
@sapphire-arches
Copy link
Contributor Author

I have updated the stderr file, tests should pass now.

@kennytm

@sapphire-arches sapphire-arches added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 7, 2018
@kennytm
Copy link
Member

kennytm commented Feb 7, 2018

@bors r=nikomatsakis

@bors
Copy link
Contributor

bors commented Feb 7, 2018

📌 Commit e99f8fc has been approved by nikomatsakis

@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 Feb 7, 2018
@bors
Copy link
Contributor

bors commented Feb 7, 2018

⌛ Testing commit e99f8fc with merge fee39ba...

bors added a commit that referenced this pull request Feb 7, 2018
[NLL] Improve DefiningTy::Const

Fixes #47590 by fixing the way DefiningTy represents constants. Previously, constants were represented using just the type of the variable. However, this will fail to capture early-bound regions as NLL inference vars, resulting in an ICE when we try to compute region VIDs a little bit later in the universal
region resolution process. (ref #47590)
@bors
Copy link
Contributor

bors commented Feb 7, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing fee39ba to master...

@bors bors merged commit e99f8fc into rust-lang:master Feb 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[NLL] issue-34780.rs test ICEs
7 participants