Do not leak type variables from opaque type relation #99928
Conversation
rustbot
added
the
T-compiler
|
r? @oli-obk (rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
@oli-obk, do you mind taking a look at this? There are quite a repeated reports of this issue.. |
|
🙈 thanks for bumping it, it was too far back in the notification list and I haven't caught up yet |
| // Don't leak any generalized type variables out of this | ||
| // subtyping relation in the case of a type error. | ||
| .map_err(|err| { | ||
| let (ga, gb) = self.fields.infcx.resolve_vars_if_possible((ga, gb)); |
There was a problem hiding this comment.
This is probably overkill, and we maybe should just do something like .map_err(|_| TypeError::Sorts(ExpectedFound { expected: a, found: b })) instead.
There was a problem hiding this comment.
yea, I was just checking out all error sources in handle_opaque_types, and they all boil down to calling infcx.at(...).eq(x, y)? in some manner. The issue is that sometimes it's not a and b as passed, but another previously registered hidden type. Then you want to report the previous hidden type mismatching the new hidden type.
So I guess I'm saying there is a version of this issue where your change will actually not catch it. Trying to produce an example now
There was a problem hiding this comment.
I was unable to reproduce the issue in the defining scope (that doesn't mean it's not possible, just that I gave up for now ^^). I found this absolutely useless diagnostic though: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=b80473c86172959763c139535154c3f9
Did some more thinking and I'm wondering now if the did.is_local() check should actually be infcx.opaque_type_origin(did).is_some() so we only generate those generalized inference vars if we're actually able to handle the opaque type. I was kinda hoping to keep that isolated in handle_opaque_type, but that appears to have been a misplaced hope.
One more thing is that we're using eq inside handle_opaque_types when it should probably be using the relation that is calling handle_opaque_types. So maybe we could scrap the entire generalization logic and just always invoke handle_opaque_types by passing the relation as an argument and forwarding to that instead of using eq.
so many possibilities to try out. Let's merge this PR for now and I'm moving this comment to an issue so we can continue there.
|
@bors r+ |
bors
added
S-waiting-on-bors
Rollup of 5 pull requests Successful merges: - rust-lang#99517 (Display raw pointer as *{mut,const} T instead of *-ptr in errors) - rust-lang#99928 (Do not leak type variables from opaque type relation) - rust-lang#100473 (Attempt to normalize `FnDef` signature in `InferCtxt::cmp`) - rust-lang#100653 (Move the cast_float_to_int fallback code to GCC) - rust-lang#100941 (Point at the string inside literal and mention if we need string inte…) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
The "root cause" is that we call
InferCtxt::resolve_vars_if_possible(3d9dd68) on the types we get back inTypeError::Sortssince I added a call to it inInferCtxt::same_type_modulo_infer. However if thisTypeErrorcomes from aInferCtxt::commit_if_ok, then it may reference type variables that do not exist anymore, which is problematic.We avoid this by substituting the
TypeErrorwith the types we had before being generalized while handling opaques.This is kinda gross, and I feel like we can get the same issue from other places where we generalize type/const inference variables. Maybe not? I don't know.
Fixes #99914
Fixes #99970
Fixes #100463