-
Notifications
You must be signed in to change notification settings - Fork 1.8k
fix: check uncertain subst when unsized coerce #20608
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
base: master
Are you sure you want to change the base?
Conversation
@@ -716,7 +717,7 @@ impl<'db> InferenceTable<'db> { | |||
let goal: Goal = coerce_unsized_tref.cast(Interner); | |||
|
|||
self.commit_if_ok(|table| match table.solve_obligation(goal) { | |||
Ok(Certainty::Yes) => Ok(()), | |||
Ok((_, Certainty::Yes) | (HasChanged::Yes, Certainty::Maybe(_))) => Ok(()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the late reply. Has been quite busy on migrating more things to next-solver 😅
I think we shouldn't accept all ambiguous solutions here. If we success here, we skip for the remaining structural coercion attempts tried after this, so this might regress other type inferences or be false negative on type mismatches.
IMO we have to check extra conditions for the trait solve result like in rustc
or try this even better WIP implementation rust-lang/rust#141926
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added this simply because what r-a used to do in chalk based trait solver, which accpets incomplete but not identical subst. It's far from perfect though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But it seems that rustc current impl isn't too hard to follow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the previous rust-analyzer had been doing so but I hope we could do better 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But r-a doesn't have any proof tree related infra at the moment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we have to implement it, too.
Actually, that's why I worked on #20578 before solving this issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I setup the proof tree infra in my (to be published) coercion PR.
related: #20422