Work around const item TypeOwnerId mismatch - #23174
Open
jix wants to merge 1 commit into
Open
Conversation
Const items do not introduce their own generic scope and when completing in the definition of a const, resolved completion candidates end up with a `TypeOwnerId` of the const item's container, the `HasResolver` implementation just skips the item scope, whereas expected types (from argument positions, the const type itself isn't used as expected type) end up with the const item itself as TypeOwnerId. This can make the `Type`s non-unifiable triggering panics during completion. This works around this by making `Type::unify` and `Type::can_rebase_into` normalize the `TypeOwnerId` of a `Type` to the innermost scope that actually can have generics for const and static items (or to `NoParams` if there is none).
Contributor
|
No that's not a correct fix. I'll create a PR for the correct fix. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
For some time rust-analyzer started panicking regularly on the kind of code I tend to write, so today I decided to debug it and to try fixing it. I minimized it the following reproducer, which this PR also adds as a test:
Since I wasn't familiar with this codebase before I started debugging this, I was working backwards from the panic's backtrace. My current understanding of the issue is that const items do not introduce their own generic scope and when completing in the definition of a const, resolved completion candidates end up with a
TypeOwnerIdof the const item's container, theHasResolverimplementation just skips the item scope, whereas expected types (from argument positions, the const type itself isn't used as expected type) end up with the const item itself as TypeOwnerId. This can make theTypes non-unifiable triggering panics during completion.This PR works around this by making
Type::unifyandType::can_rebase_intonormalize theTypeOwnerIdof aTypeto the innermost scope that actually can have generics for const and static items (or toNoParamsif there is none).I'm not at all confident that this is the best fix, it seems like it would be better to make sure that
TypeOwnerIds are assigned consistently, but that would require a) ensuring no types end up with the const item as owner, and I don't have on overview of which places that would be, or b) changing the resolver, which I didn't really look into at all. Also without changing the representation or at least the exposed constructors to ensure consistent assignment, it would be easy to accidentally re-introduce new places that create inconsistentTypeOwnerIds. I thus opted for a work-around that is relatively localized and more robust, by forcing theTypeOwnerIds to be consistent around the parts that triggered the panic.PS: The panic I got was the same as in #22659, but the regression test from #22858 still fails with this, so I believe what I fixed is not the same issue.