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 Self type retrieval from ProjectionTy #778

Merged
merged 2 commits into from
Oct 11, 2022

Conversation

lowr
Copy link
Contributor

@lowr lowr commented Oct 11, 2022

As I understand it, a ProjectionTy's substitution is in the order of the args for the associated type, and then the args for its parent (i.e. impl or trait). ProjectionTy::self_type_parameter() has been returning the first type argument in the substitution, which is not the Self type its callers would expect when the associated type has its own type parameters.

The added test shows the problem: it fails in current master as the recursive solver reports ambiguity after it gets wrong Self type and flounders in here. It hasn't been uncovered because all the traits in tests for GATs are not #[non_enumerable] (and rust-analyzer hasn't implemented GATs yet).

@lowr
Copy link
Contributor Author

lowr commented Oct 11, 2022

The failure for Book lint check seems spurious (network failure). As for Test (nightly), it's failing on this compile_fail as GATs now compile without feature gate in nightly I assume. Is there a way to apply compile_fail conditionally?

@jackh726
Copy link
Member

For that one doc test, can you just ignore it with comment to unignore when GATs stable?

@lowr
Copy link
Contributor Author

lowr commented Oct 11, 2022

Replaced compile_fail with ignore and added comment about it.

@jackh726
Copy link
Member

@bors r+

Thanks!

@bors
Copy link
Contributor

bors commented Oct 11, 2022

📌 Commit 7bc0c59 has been approved by jackh726

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Oct 11, 2022

⌛ Testing commit 7bc0c59 with merge 1b32e5d...

@bors
Copy link
Contributor

bors commented Oct 11, 2022

☀️ Test successful - checks-actions
Approved by: jackh726
Pushing 1b32e5d to master...

@bors bors merged commit 1b32e5d into rust-lang:master Oct 11, 2022
lowr added a commit to lowr/rust-analyzer that referenced this pull request Oct 13, 2022
Two things:
- `TypeFolder` has been split into `TypeFolder` and `FallibleTypeFolder` (rust-lang/chalk#772)
- `ProjectionTy::self_type_parameter()` has been removed (rust-lang/chalk#778)
bors added a commit to rust-lang/rust-analyzer that referenced this pull request Oct 16, 2022
Bump chalk

There's a bug in current chalk that prevents us from properly supporting GATs, which is supposed to be fixed in v0.86. Note the following:
- v0.86 is only going to be released next Sunday so I'll keep this PR as draft until then.
- This doesn't compile without rust-lang/chalk#779, which I hope will be included in v0.86. I confirmed this compiles with it locally.

Two breaking changes from v0.84:
- `TypeFolder` has been split into `TypeFolder` and `FallibleTypeFolder` (rust-lang/chalk#772)
- `ProjectionTy::self_type_parameter()` has been removed (rust-lang/chalk#778)
bors added a commit that referenced this pull request Mar 12, 2023
Fix projection substitution order considering GATs

When an `AliasEq` goal contains another alias as its self type, we generate the following clause: `<<X as Y>::A as Z>::B == U :- <T as Z>::B == U, <X as Y>::A == T`, with `T` being a new variable. We've been building `<T as Z>::B` by swapping the first argument in the original projection's substitution with `T`, but it's not the self type when the associated type `B` has generic parameters, leading to wrong subgoals.

The added test would yield "No possible solution" in current master.

Also removes `ignore` attribute on a doctest that was added in #778 as GATs hit stable.

Spotted in rust-lang/rust-analyzer#14164.
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