check_match: Dereference ref x before comparing it and some other type

[barosl]

The arity of ref x is always 1, so it needs to be dereferenced before being compared with some other type whose arity is not 1.

Fixes #23009.

[rust-highfive]

r? @nrc

(rust_highfive has picked a reviewer for you, use r? to override)

[barosl]

I'm really not sure if this is the right way to fix the issue, so I beg for forgiveness in advance.

[nrc]

Please add a comment to tests saying what is being tested - why it should fail, why it shouldn't ICE. This is helpful for future contributors who break your test and need to know if it is important.

[nrc]

Afraid I don't know this code well enough to review, sorry. Perhaps r? @pnkfelix

[pnkfelix]

so I'm not totally familiar with this code; I'm looking at what you've added here, and I do not immediately see why it suffices to add a special case for ConstantValue(..) but not need one for e.g. ConstantRange . Can you enlighten me?

[pnkfelix]

(i'm going to be out until wedneday; handing off to @nikomatsakis to find a reviewer to replace me.)

r? @nikomatsakis

[ghost]

I'll take this.

@barosl Thanks for tackling this. I don't think your fix is the appropriate one here. If you notice, the example will compile fine if you drop the ref from the binding.

It looks like we're getting bitten by our old problem of ref x having a type of '&T' as opposed to just 'T'. In this case, this is problematic for the check_match::constructor_arity function. For non-structural types such as usize in this case, the arity of ref x should be 0 but is instead 1 due to the misleading type information.

The place to hunt for a solution will be around https://github.com/rust-lang/rust/blob/ee7696383f3423cdd17373ff9e75c01acd8e3417/src/librustc/middle/check_match.rs#L649-658.

    let left_ty = if real_pat.id == DUMMY_NODE_ID {
        ty::mk_nil(cx.tcx)
    } else {
        ty::pat_ty(cx.tcx, &*real_pat)
    };

One way or another, we will need this to be the type of the value being destructured, free from the idiosyncracies of ref x.

[ghost]

Also, could you add the following assertion:

assert!(rows.iter().all(|row| row.len() == v.len()));

above this line:

rust/src/librustc/middle/check_match.rs

Line 649 in ee76963

let real_pat = match rows.iter().find(|r| (*r)[0].id != DUMMY_NODE_ID) {

[barosl]

If we sanitize left_ty before passing it to another function, as you said, the following code would work:

let left_ty = match (&real_pat.node, &left_ty.sty) {
    (&ast::PatIdent(..), &ty::ty_rptr(_, ty::mt { ty, .. })) => match ty.sty {
        ty_int(..) | ty_uint(..) | ty_bool | ty_char | ty_float(..) => ty,
        _ => left_ty,
    },
    _ => left_ty,
};

But I find this ugly, as it hard-codes the list of the non-structural types. It also has a nested match which is too deep, in my opinion. (Possibly due to my inexperiences)

How about modifying constructor_arity() directly? In that case, we can utilize ctor to guess the type of the previous row. I've updated the code in my pull request to reflect this approach. If it is not appropriate, I will change it again to apply the code above.

[ghost]

@barosl left_ty is used for various things across the board so I am afraid it needs to be correct.

I don't think we need the nested match at all. Any time you see the pattern ref x, the real type we're interested in is ty::deref(left_ty), no matter what the inner type is.

left_ty = match left_ty {
    ast::PatIdent(ByRef, _, _) => ty::deref(left_ty, true).unwrap().ty,
    _ => left_ty
};

[ghost]

Sorry, I realise I'd misled you by talking about non-structural and structural types. My only point was that non-structural types should always have arity of 0 but that for ref x it's always 1, hence this bug.

[barosl]

Oh, I see! So the arity itself wasn't wrong.

It seems that you coded the most part, though 😄. I'll update my pull request based on your comment, and run tests locally.

[barosl]

@jakub- Ok, I've done it. The tests pass locally.

Also, I changed the code to pass explicit=false to ty::deref(), in order to make sure the type is not ty_ptr. Would that be OK?

[ghost]

@bors: r+ edbc0e5

[ghost]

@barosl: Sure, that's fine. :) Thanks!

[bors]

⌛ Testing commit edbc0e5 with merge 8c85a9d...

[bors]

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-32-nopt-t, auto-win-32-opt, auto-win-64-nopt-t, auto-win-64-opt