missing match arms diagnostic

[JoshMcguigan]

Following up on #3689 (comment), this PR creates a missing match arms diagnostic.

At the moment this is a very early draft, but I wanted to open it just to get some initial feedback.

Initial questions:

• Have I roughly created the correct boilerplate?
• Inside the new validate_match function:
  • Am I correct in thinking I want to do validation by comparing the match arms against match_expr? And when analyzing match_expr I should be looking at it as a hir_def::expr::Expr?
  • I mostly copied the chained if-let statements from the struct validation. Shouldn't there be a non-failable way to get an AstPtr from the hir data structures?

Thanks for all the guidance.

[flodiebold]

Seems fine so far, except for using the AST; this analysis should only use the HIR.

[flodiebold]

yes, these kinds of analyses shouldn't touch the AST at all. Actually, you shouldn't even be using the body_with_source_map query, but just body (probably just pass the body as an argument).

[JoshMcguigan]

Thanks for the feedback. My usage of body_with_source_map was inspired by the validate_record_literal method, which uses it to build up the MissingField diagnostic (which contains an AstPtr). Just to clarify, do you mean the analysis shouldn't use the AST, or even the diagnostic shouldn't use the AST?

[flodiebold]

Well, this is about two concerns mostly: 1. Using the abstractions of the HIR for analysis because that's what it's for, and 2. incrementality -- calling the body_with_source_map means this has to be recalculated whenever the AST changes in any way (i.e. if I type a space in one function, this would need to be rerun for all following functions in the file). validate_record_literal is probably fine because it only calls body_with_source_map if there actually is a diagnostic, which should not be the case for most functions. So using the source map in the end to provide the AST pointer is probably fine, but it should only be done if there are diagnostics.

[flodiebold]

Using the abstractions of the HIR for analysis because that's what it's for

For example, consider macros in patterns. I'm actually not sure whether we expand them yet, but if we do, the HIR body will already contain the expanded form, so this code doesn't need to be concerned with them at all.

[JoshMcguigan]

I believe I am only using the AST in creating the diagnostic, to report the error. This is in line with other diagnostics as far as I can tell.

Has your concern here been resolved?

[flodiebold]

Am I correct in thinking I want to do validation by comparing the match arms against match_expr? And when analyzing match_expr I should be looking at it as a hir_def::expr::Expr?

Yes, basically you can get the type of the expr (which is the thing being matched), and then figure out what match arms should be there and compare them with the ones that exist.

[JoshMcguigan]

Should validate_match be tested only in an integration type test by adding tests to crates/ra_ide/src/diagnostics.rs as I've started to do? This seems to fit the pattern of the missing field diagnostic for structs, but I can see this needing a lot more testing than the missing field validation needed.

[flodiebold]

We have a test for the missing field diagnostic here in hir_ty. Tests for the diagnostic should be in hir_ty; the quick fix will be calculated in ide, so the tests for that will need to be there.

[matklad]

Also, I am super unhappy about current code organisation for diagnostics in hir, it is kindof bad that diagnostics.rs, expr.rs and the file with the test are three unrelated files. On of the side-effects of this is that some diagnostics are indeed tested on the ide level, and not on the hir level.

So, another related task here is to refactor all this to a single self-contained module (which shouldn't be done in the same PR that adds a new diagnostics).

[JoshMcguigan]

So, another related task here is to refactor all this to a single self-contained module (which shouldn't be done in the same PR that adds a new diagnostics).

It seems to me perhaps this refactor would be best done before adding another diagnostic. Thoughts on this? If so, I can pivot to looking at that and I'd probably have another round of questions once I get a chance to dive in.

On a different note, I think I found the place where rustc does exhastiveness checking of patterns.

https://github.com/rust-lang/rust/blob/master/src/librustc_mir_build/hair/pattern/_match.rs

edit - On second thought, after looking briefy at the refactoring task and realizing diagnostics cross several crates, I think that would require a broader scope of understanding than I currently have. I'm going to continue reviewing the rustc pattern matching source as a step toward implementing the missing match arm diagnostic.

[matklad]

It seems to me perhaps this refactor would be best done before adding another diagnostic. Thoughts on this?
edit - On second thought, after looking briefy at the refactoring task and realizing diagnostics cross several crates, I think that would require a broader scope of understanding than I currently have.

My thoughts exactly! Generally, I prefer to add things to the current infra, and then apply refactoring ontop immediately after: that way, I make sure I have all the necessary context in the cache before changing code.

[JoshMcguigan]

I've spent a little bit of time reading the documentation in rustc around pattern matching, and I think I'm starting to understand.

The entry point for the interesting pattern match exhaustiveness checking work seems to be here.

https://github.com/rust-lang/rust/blob/master/src/librustc_mir_build/hair/pattern/_match.rs#L1626

But it seems that method is able to assume that all of the patterns in all of the arms of the match statement are valid. By that I mean they are patterns which match the type of the match expression. So I assume there is some earlier pass of the compiler which checks for this? But I don't think that earlier pass exists (yet) in rust analyzer?

Does this all sound correct? If so, I wonder if implementing a match arm validation diagnostic is a reasonable stepping stone on the way to an implementation of exhaustiveness checking?

[flodiebold]

Hm. First of all, we don't have 'passes', i.e. steps that are executed one after the other and when one fails, analysis stops. If we don't want to do this analysis if there are type mismatches in the expression, we need to implement a check for that.

We do type-check match expressions; that's what infer does. We don't report errors for it to the user since there are still too many false positives in type inference in general, but the inference result contains the type_mismatches. So you can check whether there are type mismatches on the match arms and act accordingly.

The last question is, how do we actually want to act if the match doesn't fully type-check. Just skipping the exhaustiveness check is an option, but I actually imagine there are lots of cases where we could still correctly do the analysis even if there are type errors. I think it might be a valid approach to start by always doing the analysis, and then see in which situations we absolutely cannot do it. It'd also be a possibility to skip it for now though.

[flodiebold]

Sorry, wrong button 😬

[JoshMcguigan]

This is now ready for review. On documentation, I tried to find a balance between referencing the relevant rustc documentation and copying it over, leaning toward referencing it. I'm not sure what the standard practice is here for this project, so let me know if you'd like to see something different.

I also tested this a small amount by running it against this repository. I also ran the CLI rust-analyzer analysis-stats, but I'm not sure that actually checks the diagnostics. In any event, it at least didn't crash, and seems to do the right things based on what I can see.

One thing I did notice while testing it though it that I will get duplicate warnings about missing match arms, since I have cargo watch configured in rust-analyzer, so I get the warning from rustc and rust-analyzer. Are we dealing with that somehow in other diagnostics? I also noticed here that rustc puts that diagnostic on the match expression, while I had somewhat arbitrarily chosen to put it on the match arms. Any opinions on where this should go?

[matklad]

Are we dealing with that somehow in other diagnostics?

No, not at the moment.

I also noticed here that rustc puts that diagnostic on the match expression, while I had somewhat arbitrarily chosen to put it on the match arms. Any opinions on where this should go?

Generally, the smaller target element the better.

@flodiebold do you want to do a final review pass here (not necessary before today's relase, mind you). If not, I can take over the reviewing

[flodiebold]

I was planning to review it this evening.

[flodiebold]

Regarding the rustc diagnostic and target element, being able to deduplicate the diagnostic is probably also a good reason to choose the same target element as rustc 🤔

[flodiebold]

I also ran the CLI rust-analyzer analysis-stats, but I'm not sure that actually checks the diagnostics.

It doesn't yet, but it should!

[JoshMcguigan]

I've moved the diagnostic to the match expression (from the match arms) to align with rustc. This makes the duplication much less of a UX problem imo after a small amount of testing.

[flodiebold]

I think it looks pretty good, I did have a bunch of comments, but it's a large PR 😉

[JoshMcguigan]

@flodiebold thanks for the review! I think I've addressed your comments, let me know if there is anything else you'd like to see before this merges.

[flodiebold]

bors r+

[flodiebold]

Yes, let's leave it as is for now.

I think it might make sense to separate the 'comparing the constructors' and 'expanding the pattern' parts. Comparing the constructors can then be done on Constructor alone, and expanding the pattern just needs to match on the pattern.

[flodiebold]

btw, "enums with no associated data" are usually called "unit enum variants"

[bors]

Build succeeded

• Rust (macos-latest)
• Rust (ubuntu-latest)
• TypeScript
• Rust (windows-latest)

[cynecx]

Somehow I am hitting a panic:

thread '<unnamed>' panicked at 'index out of bounds: the len is 0 but the index is 0', crates/ra_hir_ty/src/_match.rs:278:9
stack backtrace:
   0: backtrace::backtrace::libunwind::trace
             at /cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.3.46/src/backtrace/libunwind.rs:86
   1: backtrace::backtrace::trace_unsynchronized
             at /cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.3.46/src/backtrace/mod.rs:66
   2: std::sys_common::backtrace::_print_fmt
             at src/libstd/sys_common/backtrace.rs:78
   3: <std::sys_common::backtrace::_print::DisplayBacktrace as core::fmt::Display>::fmt
             at src/libstd/sys_common/backtrace.rs:59
   4: core::fmt::write
             at src/libcore/fmt/mod.rs:1069
   5: std::io::Write::write_fmt
             at src/libstd/io/mod.rs:1439
   6: std::sys_common::backtrace::_print
             at src/libstd/sys_common/backtrace.rs:62
   7: std::sys_common::backtrace::print
             at src/libstd/sys_common/backtrace.rs:49
   8: std::panicking::default_hook::{{closure}}
             at src/libstd/panicking.rs:198
   9: std::panicking::default_hook
             at src/libstd/panicking.rs:218
  10: std::panicking::rust_panic_with_hook
             at src/libstd/panicking.rs:511
  11: rust_begin_unwind
             at src/libstd/panicking.rs:419
  12: core::panicking::panic_fmt
             at src/libcore/panicking.rs:111
  13: core::panicking::panic_bounds_check
             at src/libcore/panicking.rs:69
  14: <alloc::vec::Vec<T> as alloc::vec::SpecExtend<T,I>>::from_iter
  15: ra_hir_ty::_match::is_useful
  16: ra_hir_ty::_match::is_useful
  17: ra_hir_ty::expr::ExprValidator::validate_body
  18: ra_hir::code_model::Function::diagnostics
  19: ra_hir::code_model::Module::diagnostics
  20: ra_ide::diagnostics::diagnostics
  21: std::panicking::try
  22: ra_ide::Analysis::diagnostics
  23: rust_analyzer::main_loop::handlers::handle_code_action
  24: <F as threadpool::FnBox>::call_box
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

This might be caused by this PR.

use std::borrow::Cow;
use std::str::Utf8Error;

fn result() -> Result<Cow<'static, str>, Utf8Error> {
    unimplemented!()
}

fn main() {
    let location = match result() {
        Ok(loc) => loc,
        Err()
    };
}

[JoshMcguigan]

Thanks @cynecx for the report.

A minimal example of this is

    #[test]
    fn malformed_match() {
        let content = r"
            fn test_fn() {
                enum Either {
                    A,
                    B(u32),
                }
                match Either::A {
                    Either::A => (),
                    Either::B()
                }
            }
        ";

        check_no_diagnostic(content);
    }

I'll submit a fix tonight.