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

Fix normalization error #40268

Merged
merged 3 commits into from
Mar 9, 2017
Merged

Conversation

Mark-Simulacrum
Copy link
Member

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

This block of code doesn't contain a call to normalize_associated_types_in, while this 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

@Mark-Simulacrum Mark-Simulacrum changed the title Fix normalization error. Fix potential normalization error Mar 5, 2017
@arielb1
Copy link
Contributor

arielb1 commented Mar 5, 2017

Nice catch! This is indeed an oversight I have made.

If you can merge the 2 blocks, that would be excellent (and I'll r+ it).

@Mark-Simulacrum
Copy link
Member Author

Pushed a unification and another commit which removes references to Ty as suggested by eddyb.

@Mark-Simulacrum Mark-Simulacrum changed the title Fix potential normalization error Fix normalization error Mar 5, 2017
@arielb1
Copy link
Contributor

arielb1 commented Mar 6, 2017

nit: I would rather have a name like apply_defaults_and_return_conflicts or something. Having a find_conflicting_defaults function called on a non-error path is confusing.

r+ modulo nit.

@Mark-Simulacrum
Copy link
Member Author

Changed name to suggested apply_defaults_and_return_conflicts.

@arielb1
Copy link
Contributor

arielb1 commented Mar 6, 2017

@bors r+

@bors
Copy link
Contributor

bors commented Mar 6, 2017

📌 Commit 403ae37 has been approved by arielb1

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
bors added a commit that referenced this pull request Mar 9, 2017
@bors bors merged commit 403ae37 into rust-lang:master Mar 9, 2017
@bors
Copy link
Contributor

bors commented Mar 9, 2017

⌛ Testing commit 403ae37 with merge 3087a1f...

@Mark-Simulacrum Mark-Simulacrum deleted the normalization-followup branch March 25, 2017 17:26
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.

3 participants