Avoid ICE: Account for for<'a> types when checking for non-structural type in constant as pattern
#134776
Conversation
rustbot
added
S-waiting-on-review
|
Some changes occurred in match checking cc @Nadrieril |
| @@ -468,6 +479,10 @@ fn type_has_partial_eq_impl<'tcx>( | |||
| let partial_eq_trait_id = tcx.require_lang_item(hir::LangItem::PartialEq, None); | |||
| let structural_partial_eq_trait_id = tcx.require_lang_item(hir::LangItem::StructuralPeq, None); | |||
|
|
|||
| if ty.has_escaping_bound_vars() { | |||
| return (false, false, false, None); | |||
| } | |||
There was a problem hiding this comment.
is this necessary given that you bail in the UsedParamsNeedInstantiationVisitor?
I'd expect all other callers to not provide a type with escaping bound vars. MAybe just add debug_assert!(!ty.has_escaping_bound_vars()) to the start of this function
There was a problem hiding this comment.
while you're at it, can you change the return type to be a struct with named fields? A tuple with 3 bools is very xx
| if let ty::Adt(def, _args) = ty.kind() { | ||
| if ty.has_escaping_bound_vars() { | ||
| self.has_escaping_bound_vars = true; | ||
| } else if let ty::Adt(def, _args) = ty.kind() { |
There was a problem hiding this comment.
I also believe that this visitor should not recur into fn-ptrs instead of bailing on bound vars.
Whether an adt implements PartialEq does not matter if it is inside of a trait object which does not implement PartialEq regardless.
There was a problem hiding this comment.
why do you set has_escaping_bound_vars = true when encountering trait objects?
also, please make it an exhaustive match on ty.kind() and skip fn ptrs (they always impl eq) and bail on trait objects (they never do, except for dyn PartialEq 💀 🤔) and generators. this means has_escaping_bound_vars should be unnecessary
There was a problem hiding this comment.
I was using that as a signal of "don't provide the suggestion". Can rename the field name or add another field.
estebank
force-pushed
the
vanilla-ice
branch
2 times, most recently
from
1ab7b2e to
58ae641
Compare
| without: FxHashSet<Ty<'tcx>>, | ||
| /// If any of the subtypes has escaping bounds (`for<'a>`) or involves a trait object, we | ||
| /// won't emit a note. | ||
| should_note: bool, |
There was a problem hiding this comment.
sorry, one final nit, instead of setting should_note and then ignoring everything, wouldn't it be easier to return ControlFlow::Break(()) and change the caller to
if v.visit_ty(ty).is_break() { return }
…constant as pattern When we encounter a constant in a pattern, we check if it is non-structural. If so, we check if the type implements `PartialEq`, but for types with escaping bound vars the check would be incorrect as is, so we break early. This is ok because these types would be filtered anyways. Fix rust-lang#134764.
Replace tuple with struct and remove unnecessary early return.
|
thanks 👍 @bors r+ rollup |
bors
added
S-waiting-on-bors
…iaskrgr Rollup of 4 pull requests Successful merges: - rust-lang#134030 (add `-Zmin-function-alignment`) - rust-lang#134776 (Avoid ICE: Account for `for<'a>` types when checking for non-structural type in constant as pattern) - rust-lang#135205 (Rename `BitSet` to `DenseBitSet`) - rust-lang#135314 (Eagerly collect mono items for non-generic closures) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#134776 - estebank:vanilla-ice, r=lcnr Avoid ICE: Account for `for<'a>` types when checking for non-structural type in constant as pattern When we encounter a constant in a pattern, we check if it is non-structural. If so, we check if the type implements `PartialEq`, but for types with escaping bound vars the check would be incorrect as is, so we break early. This is ok because these types would be filtered anyways. Slight tweak to output to remove unnecessary context as a drive-by. Fix rust-lang#134764.
When we encounter a constant in a pattern, we check if it is non-structural. If so, we check if the type implements
PartialEq, but for types with escaping bound vars the check would be incorrect as is, so we break early. This is ok because these types would be filtered anyways.Slight tweak to output to remove unnecessary context as a drive-by.
Fix #134764.