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 for "ambiguous associated type" issue with ATBs #61919

Merged
merged 8 commits into from Aug 7, 2019

Conversation

@alexreg
Copy link
Contributor

commented Jun 18, 2019

@alexreg

This comment has been minimized.

Copy link
Contributor Author

commented Jun 18, 2019

I'm not sure of the underlying motivation for the actual fix (2nd commit)... having diagnosed the problem, I addressed it very much "head on". If this is the right solution, then great. Either way, I should probably add a little explainer comment to the predicates_from_bound function. Let me know your thoughts, @nikomatsakis.

src/test/ui/issues/issue-61752.rs Outdated Show resolved Hide resolved
src/librustc_typeck/collect.rs Outdated Show resolved Hide resolved
src/librustc_privacy/lib.rs Outdated Show resolved Hide resolved
src/librustc_typeck/astconv.rs Outdated Show resolved Hide resolved

@alexreg alexreg force-pushed the alexreg:fix-atb-1 branch from 4b8b60c to 42181e8 Jun 18, 2019

@bors

This comment has been minimized.

Copy link
Contributor

commented Jun 20, 2019

☔️ The latest upstream changes (presumably #61983) made this pull request unmergeable. Please resolve the merge conflicts.

@nikomatsakis
Copy link
Contributor

left a comment

So I spent some more time looking into this PR this morning. I actually think this is the wrong approach. The problem seems to be this:

The functon type_param_predicates in collect.rs is meant to return only the predicates that apply to the type parameter with the given def-id. i.e., if you have the defid for T, it should return predicates like T: Foo and T: Bar. But today, if you have T: Foo<Bar: Baz>, it is also returning <T as Foo>::Bar: Baz. This then confuses the code in find_bound_for_assoc_item in astconv.rs down the line, which assumes that the self-type is always just T. Threading this boolean flag works to suppress that confusion, but it's not my preferred approach.

I think what I would rather do is just add a filtering step right after this line, similar to what you find in the FnCtxt code.

Also, I think we should add a comment to type_param_predicates and get_type_parameter_bounds clarifying that they take a type parameter and return only the predicates that apply to that. I might push a comment or two to that effect to your branch.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Jul 12, 2019

An alternative might be to add filtering around this line -- i.e., at the place where get_type_parameter_bounds is called. In that case, the contract would not be to return just bounds like X: Foo, but rather some superset including those bounds.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Jul 12, 2019

and hence you would want to adjust the comments accordingly

@Alexendoo

This comment has been minimized.

Copy link
Member

commented Jul 24, 2019

Ping from triage, any updates? @alexreg

@alexreg

This comment has been minimized.

Copy link
Contributor Author

commented Jul 24, 2019

@Alexendoo Yeah sorry, I meant to get around to this, but I will finally in the next day or two.

@JohnCSimon

This comment has been minimized.

Copy link

commented Aug 3, 2019

Ping from triage.
@alexreg Hi! Any more progress on this?

@alexreg

This comment has been minimized.

Copy link
Contributor Author

commented Aug 4, 2019

@JohnCSimon Ah sorry. Later today, I'm pretty confident!

@alexreg alexreg force-pushed the alexreg:fix-atb-1 branch from 9bc0ff1 to eb8eef3 Aug 5, 2019

@alexreg

This comment has been minimized.

Copy link
Contributor Author

commented Aug 5, 2019

@nikomatsakis Tried your first approach, since that made perfect sense to me, while I didn't quite get your alternative one. Hope that's alright.

@varkor Pending a rebase and adding a test for #61738, would you mind kindly reviewing this in Niko's absence? I think you know this part of the compiler reasonably well.

@alexreg alexreg force-pushed the alexreg:fix-atb-1 branch from eb8eef3 to 9da5a00 Aug 5, 2019

@alexreg alexreg force-pushed the alexreg:fix-atb-1 branch from 83de06f to 0410e32 Aug 5, 2019

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Aug 7, 2019

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

commented Aug 7, 2019

📌 Commit 0410e32 has been approved by nikomatsakis

@bors

This comment has been minimized.

Copy link
Contributor

commented Aug 7, 2019

⌛️ Testing commit 0410e32 with merge d4abb08...

bors added a commit that referenced this pull request Aug 7, 2019
Auto merge of #61919 - alexreg:fix-atb-1, r=nikomatsakis
Fix for "ambiguous associated type" issue with ATBs

Fixes #61752.

r? @nikomatsakis

CC @Centril
@alexreg

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2019

Thanks @nikomatsakis!

@bors

This comment has been minimized.

Copy link
Contributor

commented Aug 7, 2019

☀️ Test successful - checks-azure
Approved by: nikomatsakis
Pushing d4abb08 to master...

@bors bors added the merged-by-bors label Aug 7, 2019

@bors bors merged commit 0410e32 into rust-lang:master Aug 7, 2019

5 checks passed

homu Test successful
Details
pr Build #20190805.54 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-6.0) Linux x86_64-gnu-llvm-6.0 succeeded
Details
pr (LinuxTools) LinuxTools succeeded
Details
@Centril Centril referenced this pull request Aug 8, 2019
2 of 8 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.