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

More through normalization, Feb/Mar 2017 edition #40163

Merged
merged 4 commits into from
Mar 4, 2017

Conversation

arielb1
Copy link
Contributor

@arielb1 arielb1 commented Feb 28, 2017

Fix a few normalization bugs.

Fixes #27901.
Fixes #28828.
Fixes #38135.
Fixes #39363.
Fixes #39367.

@rust-highfive
Copy link
Collaborator

r? @pnkfelix

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

@arielb1
Copy link
Contributor Author

arielb1 commented Mar 1, 2017

r? @eddyb

@rust-highfive rust-highfive assigned eddyb and unassigned pnkfelix Mar 1, 2017
@arielb1
Copy link
Contributor Author

arielb1 commented Mar 1, 2017

Don't merge - there's still a test (default_ty_param_default_dependent_associated_type) that causes broken MIR - silently until now - which is now an ICE.

EDIT: found a fix to that.

Copy link
Member

@eddyb eddyb left a comment

Choose a reason for hiding this comment

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

LGTM, but the Lvalue::Static change makes me nervous. cc @rust-lang/compiler Any opinions?

@arielb1
Copy link
Contributor Author

arielb1 commented Mar 1, 2017

Can @nikomatsakis confirm that I didn't break new-style default inference in 4dee3c45080b5f9b917f8d4696adb424ff67a28e ?

@nikomatsakis
Copy link
Contributor

r? @nikomatsakis -- I will review today.

@rust-highfive rust-highfive assigned nikomatsakis and unassigned eddyb Mar 1, 2017
@aravind-pg
Copy link
Contributor

The third issue linked in the PR description (#38185) must have a typo, since currently it's an unrelated PR and not an issue.

The types of statics, like all other items, are stored in the tcx
unnormalized. This is necessarily so, because
    a) Item types other than statics have generics, which can't be
normalized.
    b) Eager normalization causes undesirable on-demand dependencies.

Keeping with the principle that MIR lvalues require no normalization in
order to interpret, this patch stores the normalized type of the statics
in the Lvalue and reads it to get the lvalue type.

Fixes rust-lang#39367.
I'll like @nikomatsakis or someone to look at the unsolved variable
case.
We ought to do that sometime, and this PR fixes all broken MIR errors I
could find.
@arielb1
Copy link
Contributor Author

arielb1 commented Mar 1, 2017

@aravind-pg

Fixed

@nikomatsakis
Copy link
Contributor

@arielb1 this looks good, I think I will do a crater run perhaps though? I see you changed the MIR stuff from warn to bug and I'm curious what may be affected...

@nikomatsakis
Copy link
Contributor

@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Mar 3, 2017

📌 Commit 4aede75 has been approved by nikomatsakis

@bors
Copy link
Contributor

bors commented Mar 4, 2017

⌛ Testing commit 4aede75 with merge 8c6c0f8...

bors added a commit that referenced this pull request Mar 4, 2017
More through normalization, Feb/Mar 2017 edition

Fix a few normalization bugs.

Fixes #27901.
Fixes #28828.
Fixes #38135.
Fixes #39363.
Fixes #39367.
@bors
Copy link
Contributor

bors commented Mar 4, 2017

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

@bors bors merged commit 4aede75 into rust-lang:master Mar 4, 2017
arielb1 pushed a commit to arielb1/rust that referenced this pull request Mar 8, 2017
…wup, r=arielb1

Fix normalization error

Follows rust-lang#40163. I don't know whether this is good, but seems logical.

[This block of code](https://github.com/rust-lang/rust/blob/ba07bd5d23aced6d4baa5696213b11ca832c1a5d/src/librustc_typeck/check/mod.rs#L2110-L2138) doesn't contain a call to `normalize_associated_types_in`, while [this](https://github.com/rust-lang/rust/blob/master/src/librustc_typeck/check/mod.rs#L2027-L2028) block does, and is nearly identical.

Ideally these two blocks should be unified into one, but since the change doesn't seem trivial and I'm unsure if this patch will be accepted it hasn't been done yet.

r? @arielb1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants