Make InferCtxtExt::could_impl_trait more precise, less ICEy
#119934
Conversation
|
r? @jackh726 (rustbot has picked a reviewer for you, use r? to override) |
rustbot
added
S-waiting-on-review
| // | ||
| // A correct suggestion here would take into account the fact | ||
| // that inference may be affected by missing types on bindings, | ||
| // etc., to improve "tests/ui/borrowck/issue-91206.stderr", for |
There was a problem hiding this comment.
91206 is the only test that i consider to have regressed with this PR. the problem is that fixing this code makes it much harder to only tailor the suggestion for when it should apply. I don't think that it's worth it.
| _ => false, | ||
| }) | ||
| { | ||
| let mut fulfill_cx = FulfillmentCtxt::new(self); |
There was a problem hiding this comment.
Why were we using the new trait solver!?
| let obligations = self | ||
| .tcx | ||
| .predicates_of(impl_def_id) | ||
| .instantiate(self.tcx, args) |
There was a problem hiding this comment.
Here is the cause of the ICE -- we were trying to substitute an ADT's args into an impl's predicates. There's no guarantee that these generics line up.
| &self, | ||
| trait_def_id: DefId, | ||
| ty: Ty<'tcx>, | ||
| param_env: ty::ParamEnv<'tcx>, | ||
| ) -> Option<Vec<traits::FulfillmentError<'tcx>>> { | ||
| self.probe(|_snapshot| { | ||
| if let ty::Adt(def, args) = ty.kind() |
There was a problem hiding this comment.
Why is this name "could_impl_trait" if it was specific to ADTs? No comment too! :(
compiler-errors
changed the title
InferCtxtExt::could_impl_trait less messed upInferCtxtExt::could_impl_trait more precise, less ICEy
rust-cloud-vms
bot
force-pushed
the
could-impl
branch
from
8fc6a06
to
6c5e432
Compare
|
Seems like this was added in 210a672#diff-622a7d238c727ee83660b2a7fe4dbd2c0fd0ac25a432fc531dfdb17c3e516bf1 Using the new trait solver was probably an accident. I'm wondering if it's worth linting against it. |
|
I don't think it's worth linting, but I can add assertions against it. |
|
Some changes occurred to the core trait solver cc @rust-lang/initiative-trait-system-refactor |
rustbot
added
the
WG-trait-system-refactor
rust-cloud-vms
bot
force-pushed
the
could-impl
branch
from
ce5cf3d
to
6ca55e3
Compare
|
r=me with CI green cc @estebank |
rust-cloud-vms
bot
force-pushed
the
could-impl
branch
from
6ca55e3
to
7724ba7
Compare
|
(elaborated the comment a bit more, fixed a typo) |
|
@bors r=jackh726 |
bors
added
S-waiting-on-bors
Make `InferCtxtExt::could_impl_trait` more precise, less ICEy The implementation for `InferCtxtExt::could_impl_trait` was very wrong. Along with being pretty poorly named, way too specific to ADTs, it was also doing impl substitution wrong -- this caused an ICE (rust-lang#119915). This PR generalizes that code, gives it a clearer name, makes it stop using the new trait solver (lol), and fixes some fallout bad suggestions that are made worse with the code fix. Fixes rust-lang#119915
|
💥 Test timed out |
bors
added
S-waiting-on-review
|
@bors retry |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
|
Finished benchmarking commit (0dab65b): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)ResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 666.086s -> 668.181s (0.31%) |
The implementation for
InferCtxtExt::could_impl_traitwas very wrong. Along with being pretty poorly named, way too specific to ADTs, it was also doing impl substitution wrong -- this caused an ICE (#119915).This PR generalizes that code, gives it a clearer name, makes it stop using the new trait solver (lol), and fixes some fallout bad suggestions that are made worse with the code fix.
Fixes #119915