Fix early exiting when ExprUseVisitor meets unresolved type var

[ldm0]

Closes #87814

fn main() {
    let mut schema_all = vec![];
    (0..42).for_each(|_x| match Err(()) as Result<(), _> {
        Ok(()) => schema_all.push(()),
        Err(_) => (),
    });
}

ExprUseVisitor::walk_expr ======> hir::ExprKind::Match => MemCategorizationContext::cat_pattern ======> self.cat_pattern_ ======> PatKind::TupleStruct => self.pat_ty_adjusted ======> self.pat_ty_unadjusted ======> self.node_ty ======> self.resolve_type_vars_or_error

With as Result<(), _> presents, the argument ty of resolve_type_vars_or_error when checking Err(_) => .. arm is a type var rather than a unit. And the error bubbles through all the functions listed above. Which means the walk_arm() call in the ExprUseVisitor::walk_expr is skipped.

I think walk_arm is a process not mangled with the cat_pattern. So working around this problem by swallowing the type var resolving error.

r? @roxelo @nikomatsakis

[roxelo]

I think we should add a test where the match discriminant also needs to be borrowed mutably so we can confirm that this fix actually resolves the root issue. For instance, something like this also causes an ICE on nightly:

#![feature(try_reserve)]
#![feature(capture_disjoint_fields)]

fn main() {
    let mut schema_all : (Vec<String>, Vec<String>) = (vec![], vec![]);
    
    let _c = || {
        match schema_all.0.try_reserve(1) as Result<(), _> {
            Ok(()) => (),
            Err(_) => (),
        }
    };
    
}

thread 'rustc' panicked at 'called `Result::unwrap()` on an `Err` value: PlaceBuilder { base: Upvar { var_hir_id: HirId { owner: DefId(0:3 ~ playground[4e8d]::main), local_id: 26 }, closure_def_id: DefId(0:4 ~ playground[4e8d]::main::{closure#0}), closure_kind: Fn }, projection: [Field(field[0], std::vec::Vec<std::string::String>)] }', compiler/rustc_mir_build/src/build/expr/as_place.rs:306:69

[ldm0]

I think we should add a test where the match discriminant also needs to be borrowed mutably so we can confirm that this fix actually resolves the root issue. For instance, something like this also causes an ICE on nightly:

#![feature(try_reserve)]
#![feature(capture_disjoint_fields)]

fn main() {
    let mut schema_all : (Vec<String>, Vec<String>) = (vec![], vec![]);
    
    let _c = || {
        match schema_all.0.try_reserve(1) as Result<(), _> {
            Ok(()) => (),
            Err(_) => (),
        }
    };
    
}

thread 'rustc' panicked at 'called `Result::unwrap()` on an `Err` value: PlaceBuilder { base: Upvar { var_hir_id: HirId { owner: DefId(0:3 ~ playground[4e8d]::main), local_id: 26 }, closure_def_id: DefId(0:4 ~ playground[4e8d]::main::{closure#0}), closure_kind: Fn }, projection: [Field(field[0], std::vec::Vec<std::string::String>)] }', compiler/rustc_mir_build/src/build/expr/as_place.rs:306:69

The original use case of cat_pattern ignores the error it returned, I think we can do the same thing here.

[ldm0]

r? @nikomatsakis @roxelo

[ldm0]

@bors try @rust-timer +queue

[rust-timer]

Insufficient permissions to issue commands to rust-timer.

[bors]

@ldm0: 🔑 Insufficient privileges: not in try users

[roxelo]

I believe we ignore the error returned by cat_pattern in the rest of the code because if an error is found, we expect an appropriate error message to be provided later on. Maybe some states have been or not been applied yet, which is why it returns an error here? (The PR that introduces the regression added cat_pattern for the first time in that part of the code...)

I agree that we can definitely just not return early for now, although I am not sure if we should close the issue; I think we should still figure out why cat_pattern returns an error when we don't expect it to. The good thing for us is the error seems to be returned after the op(&place_with_id, pat); is run in cat_pattern, so we should still be getting the correct information on whether the match discriminant should be read or not.

I think there is a simpler way to do this than your current change; we should just need to replace the return_if_err! with a let _ = ..., so something like this
let _ = mc.cat_pattern(discr_place.clone(), &arm.pat, |place, pat| {

[ldm0]

@roxelo Thanks for reviewing!

The good thing for us is the error seems to be returned after the op(&place_with_id, pat); is run in cat_pattern, so we should still be getting the correct information on whether the match discriminant should be read or not.

Hm.... the field checking is skipped I think. Why it's still "correct" in this situation? 🤔

I think there is a simpler way to do this than your current change; we should just need to replace the return_if_err! with a let _ = ...

When a typeck error is uncovered, rustc should pessimistically assume borrow is needed(correct me if I'm wrong). BTW, I changed the signature of the closure given to cat_pattern, so emitting an error in the closure can terminates the whole walking process. With the more complex change, rustc will ends the walking process when it finds an evidence that borrow is needed, then performance should be improved.

[roxelo]

Hm.... the field checking is skipped I think. Why it's still "correct" in this situation? 🤔

Oh right, you can ignore my earlier comment.

When a typeck error is uncovered, rustc should pessimistically assume borrow is needed(correct me if I'm wrong)

As a workaround for this issue I guess yes, but we really want it to be deterministic since it wasn't expected that cat_pattern would fail with correct code.
EDIT: This is also why I was suggesting using the let _ = ... as, this way, we can ensure that reading the match discriminant is deterministic on concrete rules set in the op(...)

BTW, I changed the signature of the closure given to cat_pattern, so emitting an error in the closure can terminates the whole walking process.

I am personally not a fan of using an error in that part of the code as it's not an error for the discriminant to need to be read/borrowed...

[nikomatsakis]

Closing in favor of #88266