Rust: Error reporting for missing `match` comma could be improved

[sleeparrow]

UPDATE: Mentoring instructions below.

---

Given this code:

enum Foo { A, B, }

fn main() {
    match &Foo::A {
        &Foo::A => "art"
        &Foo::B => "bart"
    };
}

rustc reports this error:

error: expected one of `!`, `,`, `.`, `::`, `?`, `{`, `}`, or an operator, found `=>`
 --> src/main.rs:6:17
  |
6 |         &Foo::B => "bart"
  |                 ^^ expected one of 8 possible tokens here

As @mbrubeck pointed out in IRC, the cause is that a bitwise-and operation, namely "art" & Foo::B is being parsed.

[clarfonthey]

This is still occurring in the latest nightly. Would be nice to have this improvement because I've stumbled upon it several times.

[estebank]

The parser should use self.expect(&token::OpenDelim(token::Brace)) to verify whether, after the "Fat Arrow" (=>) has been parsed, there is an opening brace. If there isn't, it probably means that the match arm should be relatively short, a one liner even. From this, we can infer that _any_ error that occurs when doing parse_expr_res (parsing the match arm) with a primary span in a line different to the span of the Fat Arrow is suspect. In this case the following span label could be added to make it clearer what is happening:

error: expected one of `!`, `,`, `.`, `::`, `?`, `{`, `}`, or an operator, found `=>`
 --> src/main.rs:6:17
  |
5 |         &Foo::A => "art"
  |                    - in the match arm starting here...
6 |         &Foo::B => "bart"
  |                 ^^ expected one of 8 possible tokens here
  = note: is a comma missing here?
  |
5 |         &Foo::A => "art",
  |                         ^

The suggestion (the note above) could be provided when checking wether the span between => and \n could be parsed as an expr and doesn't end with a comma.

[fnune]

Hello there, I'd like to try tackling this issue. It would be my first work on the compiler so I'll be slow, but I understand the issue and the mentoring instructions.

[fnune]

Could anybody point me to a place where I can write tests for this, and maybe any tests that are similar to what I would need to write?

Thanks in advance.

[estebank]

@brainlessdeveloper the tests are all in src/test, you'll want to add new ui tests file. Once you've written a test file that will be compiled, you run ./x.py test src/test/ui --stage 1. This will fail as there's no .stderr file for your test to compare against. After this is done, you can run ./src/test/ui/update_all_references.sh build/x86_64-apple-darwin/test/ui (if you're in macOS). This will create a new file src/test/ui/token/<your test filename>.stderr that will hold the text output of the compiler. Verify that it looks like what you expect and add it to your commit. If you run the test again, it should pass.

[fnune]

So, I'm trying to make progress with this. I wrote the test and have its stderr file.

Correct me if I'm wrong, but I can't use self.expect to check if we have an opening brace because that give me no opportunity to recover if there's no brace? I'm doing this instead:

let has_braces = self.check(&token::OpenDelim(token::Brace));

The rest of the work should be checking for errors during self.parse_expr_res and adding a note if there's an error and it's on the same line as the fat arrow (I'm having dificulties figuring out how to check if it's on the same line).

Afterwards, there's this logic:

        let require_comma = classify::expr_requires_semi_to_be_stmt(&expr)
            && self.token != token::CloseDelim(token::Brace);

        if require_comma {
            self.expect_one_of(&[token::Comma], &[token::CloseDelim(token::Brace)])?;
        } else {
            self.eat(&token::Comma);
        }

I'm not sure if I can reuse this or if it should be removed and handled differently depending on whether the arm has braces or not.

[estebank]

Correct me if I'm wrong, but I can't use self.expect to check if we have an opening brace because that give me no opportunity to recover if there's no brace? I'm doing this instead:

@brainlessdeveloper, that is correct and the appropriate way of doing looking for something that might not be there.

I'm having dificulties figuring out how to check if it's on the same line.

You can use CodeMap::span_to_lines() (self.sess.codemap().span_to_lines(sp)) and look at the FileLinesResult.lines[0].line_index for the line number. You can compare the line number for two spans. You can look at self.prev_span for the last consumed token and self.span for the currently being consumed token.

Afterwards, there's this logic: (require_comma)

I don't know if you can use it as I think that the failure happens on the previous line and that in that case the method returns. If that is not the case, then require_comma would be appropriate to be used.

[fnune]

With your help I managed to show a note in the appropriate cases - now I have to figure out which method is the one I need to use to show the note properly because it should also point to where the missing comma should be added. I should be able to work on this further during the coming weekend :).

[estebank]

@brainlessdeveloper I'm glad I could be of help! If you feel comfortable with the code you have so far, feel free to post a PR for the new labels, referencing this issue (but not closing it), and posting a follow up PR adding the suggestion when you have the time.

[fnune]

The code is terrible - I increased the size of the method by 80% and the suggestion note does not come up where it should - I'll take some time to go over it first in the next days :)