Add lint for catch-all match arms

[schmee]

A step towards #3482. There is nothing fancy here (no special treatment of enums, no print of missing cases) because there seemed to be no clear consensus on what to do.

[alexcrichton]

I believe the intent of the original lint was to allow things like Some(_) as they're at least a little bit targeted. If it warns about all ignored fields via _ then the lint probably isn't so useful. I personally don't believe that the lint would be all that useful anyway (too many false positives), but that's why it's allow-by-default.

[schmee]

I'm inclined to agree with @alexcrichton. Warning for bindings like Some(_) would make it even harder to sift through the output.

[ghost]

@alexcrichton, @schmee Oh sorry, poorly chosen example. What I meant is the second arm, y, which is a catch-all pattern.

[alexcrichton]

Aha! I would expect that to not be linted about because if it's used it's legitimate and if it's not used then it'll get a lint anyway.

[schmee]

@alexcrichton Rebased on the new lint infrastructure.

[flaper87]

This looks good. Thanks for working on it.

[huonw]

Maybe this could be a little more intelligent about the type of thing it is warning about, e.g. it's not really possible to have an exhaustive match for numbers or for strings at all, and so warning about those instances seems like it decreases the signal to noise ratio of this lint (that is, it may mean people avoid activating it, even when it would assist with matches on enums etc.).

[ghost]

To me this is somewhat related to #5657.

@huonw Agreed. This should only trigger when the value being matched has a finite set of possible constructors. In fact, in the test cases that are of this PR, there is nothing the user could really do other than disabling the lint.

Even more so, I think this lint would be very useful for "open" enums like our AST node types but could get pretty annoying for "sealed" enums like Option where it's just more ergonomic to match on Some and have a catch-all pattern for None:

match Some(42u) { Some(x) => (), _ => () }

In which case maybe it should really work on an opt-in basis for each type. But then I fear it also falls out of scope of the compiler to come with that kind of lints...

[schmee]

@huonw It now only lints for enums.

@jakub- Agreed. This could be accomplished by attributes, but I don't think it's worth the effort. This could be worth revisiting if the "negative impls" RFC goes through.

[ghost]

@schmee I think this will definitely do for now but at the same time having the lint only trigger on ty_enum at the top level seems fairly arbitrary. What I find very common are match expressions on ty_rptr(ty_enum) (match x { &Some(_) => (), _ => () }) etc. But I think this is okay for now. Thank you!

What I think would be desired in the future is have this lint be produced by check_match.rs for all types, whenever there is a wildcard pattern in any column that captures more than one constructor. Tying it with check_match.rs would be somewhat unfortunate but very convenient as the message could be more specific about the constructors being captured. On top of that, then the lint could be enabled by default because it'd only produce warnings when those catch-all patterns are actually dangerous (i.e. when they capture more than one variant of an enum).

[schmee]

@jakub- Ahh, of course it should lint for that as well! Can I just add in ty_rptr(ty_enum) in the match in the same way as ty_enum, or is there more to it? Anything else I forgot?

That would definitely be a nicer solution. Hopefully I can get back to that when I'm more knowledgeable about rustc :)

[ghost]

@schmee There's also ty_box. And tuples. Nested enums. That's why I think we should perhaps go ahead and make this lint be part of the sanity checker in check_match.rs based on the general rules outlined in my previous comment.

My personal opinion on lints in general is that now that Rust has pluggable lints, the threshold for built-in should be much higher. In particular, I do not think lints disabled by default are particularly suited to be part of the compiler. In addition, if they're too specific that they hardly trigger at all when they should, or too general that they're too noisy, that's also bad. But this is my personal opinion and the Rust team may feel differently.

In this case, though, I think we could get away with making this way more general. And to battle test it, I recall @nikomatsakis has a strong preference for avoiding _ patterns in the borrow checker so maybe this should support something along the lines of #[must_use] of the unused result lint. An annotation you could put on a specific enum to turn the lint on for it.

I'm happy to help out with integrating this with check_match.rs. It would be nice to increase the bus factor of that part of the compiler!

[ghost]

I mean, I don't want to be derailing this. If others feel happy about it as-is, we could do the improved version separately.

[schmee]

@jakub- That's a lot of stuff I didn't think about! I'm not interested contributing stuff for the sake of contributing, and I agree that in its current state this PR won't be very useful. So I'll close this for now, but I very well might take you up on your offer to mentor on an improved version in the near future!

[ghost]

@schmee Sure, ping me on IRC any time I can be of help!