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

Don't normalize xform_ret_ty during method candidate assembly #86506

Merged
merged 2 commits into from
Oct 9, 2021

Conversation

b-naber
Copy link
Contributor

@b-naber b-naber commented Jun 21, 2021

Fixes #85671

Normalizing the return type of a method candidate together with the expected receiver type of the method can lead to valid method candidates being rejected during probing. Specifically in the example of the fixed issue we have a self_ty of the form &A<&[Coef]> whereas the impl_ty of the method would be &A<_>, if we normalize the projection in the return type we unify the inference variable with Cont, which will lead us to reject the candidate in the sup type check in consider_probe. Since we don't actually need the normalized return type during candidate assembly, we postpone the normalization until we consider candidates in consider_probe.

@rust-highfive
Copy link
Collaborator

r? @matthewjasper

(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 Jun 21, 2021
@b-naber b-naber changed the title Don't normalize xform_ret_ty during method probing Don't normalize xform_ret_ty during method candidate assembly Jun 21, 2021
@jackh726
Copy link
Member

So, I haven't looked into this issue or this exact fix, but I'm not sure if we should be moving to "normalize less" instead of "normalize more"

@b-naber
Copy link
Contributor Author

b-naber commented Jun 22, 2021

Hm well maybe you should at least look at the PR :)

As I wrote above we do normalize the return type, just not during candidate assembly.

@jackh726
Copy link
Member

jackh726 commented Jun 22, 2021

I mean, I've looked at the PR, but don't know exactly why this particular change fixes things.

Does this issue get fixed by #85499 as-is.

@b-naber
Copy link
Contributor Author

b-naber commented Jun 22, 2021

Does this issue get fixed by #85499 as-is.

If that is a question, I don't know. You have a build, can you test whether this issue is fixed by that PR?

@crlf0710 crlf0710 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 9, 2021
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 25, 2021
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 15, 2021
@inquisitivecrystal inquisitivecrystal added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Aug 24, 2021
@jackh726
Copy link
Member

jackh726 commented Sep 9, 2021

r? @jackh726

@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 27, 2021
@bors
Copy link
Contributor

bors commented Oct 2, 2021

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

@jackh726
Copy link
Member

jackh726 commented Oct 5, 2021

Okay, so finally getting a chance to look at this. Sorry this has sat so long @b-naber

Now that I'm looking at this, I actually understand the problem here. It's a bit weird. Because of the Self: AsSlice<Element = Coef> bound, we end up with a predicate <A<Cont> as AsSlice>::Element = Coef in the param_env when type checking failing. Then, later, we try to normalize <A<_#3t> as AsSlice>::Element. We then use that predicate in the param_env as a candidate, unifying _#3t with Cont and getting a result of Coef.

That also causes our self_ty to gets inferred to be &ReErased A<Cont>. This doesn't match &ReErased A<&'_#1r [Coef]>.

Delaying normalization of the return type does work, but I think that it hides a deeper bug. Ideally, I think the problem is the param_env itself. The clauses here probably shouldn't have Params, but instead either fresh vars or maybe just be canonicalized.

For this, I suppose I'm okay with merging as-is. But with a more explicit comment that this problem stems from the param_env having Params instead of inference/canonical vars. I'd also like an issue filed and a FIXME linking to that.

r=me with that

@jackh726 jackh726 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 Oct 5, 2021
@b-naber b-naber force-pushed the gen_trait_impl_inconsistent branch from 4e8bb79 to fcf55a0 Compare October 7, 2021 21:21
@rust-log-analyzer

This comment has been minimized.

@b-naber
Copy link
Contributor Author

b-naber commented Oct 7, 2021

@jackh726 Thanks for the review. Opened the issue and added a FIXME comment.

Kind of stumped by the CI fail though.

@jackh726
Copy link
Member

jackh726 commented Oct 7, 2021

You checked in submodule changes. That's probably it.

@b-naber b-naber force-pushed the gen_trait_impl_inconsistent branch from fcf55a0 to 2d7ed7a Compare October 7, 2021 23:39
@b-naber
Copy link
Contributor Author

b-naber commented Oct 7, 2021

You checked in submodule changes. That's probably it.

Thanks, hope this is fixed now.

@jackh726
Copy link
Member

jackh726 commented Oct 8, 2021

Looks better. Can you edit the FIXME to include issue (#89650)? r=me with that

@bors delegate+

@bors
Copy link
Contributor

bors commented Oct 8, 2021

✌️ @b-naber can now approve this pull request

@b-naber b-naber force-pushed the gen_trait_impl_inconsistent branch from 2d7ed7a to 3215403 Compare October 8, 2021 08:56
@b-naber
Copy link
Contributor Author

b-naber commented Oct 8, 2021

@bors r=jackh726 rollup

@bors
Copy link
Contributor

bors commented Oct 8, 2021

📌 Commit 3215403 has been approved by jackh726

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 8, 2021
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Oct 8, 2021
…, r=jackh726

Don't normalize xform_ret_ty during method candidate assembly

Fixes rust-lang#85671

Normalizing the return type of a method candidate together with the expected receiver type of the method can lead to valid method candidates being rejected during probing. Specifically in the example of the fixed issue we have a `self_ty` of the form `&A<&[Coef]>` whereas the `impl_ty` of the method would be `&A<_>`, if we normalize the projection in the return type we unify the inference variable with `Cont`, which will lead us to reject the candidate in the sup type check in `consider_probe`. Since we don't actually need the normalized return type during candidate assembly, we postpone the normalization until we consider candidates in `consider_probe`.
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 8, 2021
…laumeGomez

Rollup of 6 pull requests

Successful merges:

 - rust-lang#86506 (Don't normalize xform_ret_ty during method candidate assembly )
 - rust-lang#89538 (Make rustdoc not highlight `->` and `=>` as operators)
 - rust-lang#89649 (clippy::complexity fixes)
 - rust-lang#89668 (Cfg hide more conditions for core and alloc)
 - rust-lang#89669 (Remove special-casing of never primitive in rustdoc-json-types)
 - rust-lang#89672 (Remove unwrap_or! macro)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 3250240 into rust-lang:master Oct 9, 2021
@rustbot rustbot added this to the 1.57.0 milestone Oct 9, 2021
@b-naber b-naber deleted the gen_trait_impl_inconsistent branch October 6, 2022 15:19
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. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Generic trait implementation not consistently recognized by the compiler
10 participants