Add expected closing delimiter to diagnostic

[Crazycolorz5]

When looking through for a closing bracket in the loop condition, adds them to expecteds.

Fix #38777.

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @eddyb (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[Crazycolorz5]

I'm working on making/testing some code that might provide an even better descriptor of where the closing bracket should go (basically concluding that it should in fact be a closing bracket and not something else). Not sure if anything will come of that though.

[Crazycolorz5]

Two orders of business.

1. The previous test error is not in fact an error but rather demonstrates that this change works. I've changed the expected output of the test to be the new expectation.

2. Regarding the potential code I was considering earlier, when I tried adding it it gives me

error: expected one of `)`, `-`, `.`, `<`, `?`, `break`, `continue`, `false`, `for`, `if`, `loop`, `match`, `move`, `return`, `static`, `true`, `unsafe`, `while`, `yield`, or an operator, found `;`
  --> token-error-correct-3.rs:24:35
   |
24 |             callback(path.as_ref(); //~ ERROR expected one of
   |                                   ^ expected one of 20 possible tokens here

In other words, way too many options. What I'm trying to add is something like:

match f(self) {
                            Ok(t) => {
                                v.push(t);
                                continue;
                            },
                            Err(mut e) => {
                                //If f errored, it should be because we're missing a bracket (and thus shouldn't have parsed f).
                                e.cancel();
                                self.expect_one_of(&[], unref_kets)?;
                                //The double cloned() is ugly but is necessary due to kets being a slice of references.
                                break;
                            }
                        }

At line 1133 in parser.rs, as well as something similar at the other invocation of f a few lines below it:

let before_span = self.span.clone();
            let maybe_err = f(self);
            match maybe_err {
                Ok(t) => {
                    v.push(t);
                }
                Err(e) => {
                    if before_span == self.span {
                        //If no input was consumed, assume we wanted a sequence ender.
                        self.expect_one_of(&[], unref_kets)?;
                        //The double cloned() is ugly but is necessary due to kets being a slice of references.
                        //If there was in fact a sequence ender, return the error the function gave us.
                    }
                    else {
                        debug!("parse_seq_to_before_tokens: No equality. Propogating error.");
                    }
                    return Err(e.into());
                }
            }

I know the error is caused by self.expected_tokens being populated by the invocation of f, but the only clears to the expected_tokens is in bump(), and this isn't an instance where I want to call it (meaning, bump or bump_with). I'm just a bit wary of adding more calls, because then it seems like it'd be hard to keep track of where it's cleared. Considering we already suppress errors from f, it doesn't seem too illogical though. Opinions from an actual maintainer of the code? (Furthermore, we could defer the other error -- that arises from missing a separator, to here, or as the author of the issue points out, suppress the previous error only if we encounter a ;? Just thoughts/rambling here, but I feel like error handling after a failed call to f could be more informative.)

[eddyb]

Please use slice::from_ref here.

[Crazycolorz5]

Changing this currently breaks compilation as it is still an unstable feature on my rustc -- should I go through with the change anyway, despite not being able to build it locally?

[eddyb]

You need to add a feature-gate to lib.rs I guess - actually, wait, cc @Manishearth @dtolnay - what does syntex need from libsyntax? Is this one of those things where it must work on stable? If so, can we at least leave a comment above this unsafe code saying it should use slice::from_ref but syntex must work on stable Rust?

[dtolnay]

To my knowledge, syntex no longer has any bearing on what we can write in libsyntax. Use slice::from_ref.

[Crazycolorz5]

Interestingly, this isn't directly relevant to the issue being fixed, but I thought it'd be worth addressing an unsafe block.

[eddyb]

I'm confused, why is this the opposite of the error message above?

[Crazycolorz5]

Opposite? I changed the error message because that's the purpose of the issue being solved.

Edit: OH! I see what you mean. Yes, that's just my mistake. I'll swap it.

[eddyb]

r? @nikomatsakis

[Crazycolorz5]

The error is

[00:44:34] ---- [ui] ui/similar-tokens.rs stdout ----
[00:44:34] diff of stderr:
[00:44:34] 
[00:44:34] -	error: expected one of `,`, `::`, or `as`, found `.`
[00:44:34] +	error: expected one of `,`, `::`, `as`, or `}`, found `.`
[00:44:34] 2	  --> $DIR/similar-tokens.rs:17:10
[00:44:34] 3	   |
[00:44:34] 4	LL | use x::{A. B}; //~ ERROR expected one of `,`, `::`, or `as`, found `.`
[00:44:34] 
[00:44:34] -	   |          ^ expected one of `,`, `::`, or `as` here
[00:44:34] +	   |          ^ expected one of `,`, `::`, `as`, or `}` here
[00:44:34] 6	
[00:44:34] 7	error: aborting due to previous error
[00:44:34] 8	

Now, I think that this also falls under an expected error message as per the issue? Though I'm not so familiar with this syntax so I could be wrong, but the compiler says that . isn't permissible inside the {} expression so it suggests just ending it before the ., which seems logical to me.

[Crazycolorz5]

The merge commit actually resulted from me trying to squash two commits! That's my mistake; I haven't had to worry about things like that before. I'll fix it when I'm able. What I did was make and push commit 07620b3, but then made the next commit locally, then squashed the two together. But I couldn't push that before pulling and, yeah... Anyway, not able to fix it at the moment but I should soon.

[eddyb]

But I couldn't push that before pulling ...

Yeah, I think that's where you need to use git push -f to force it to accept the changed history.

[nikomatsakis]

My suggestion for cleaning up the history in this kind of thing is just to git reset and recreate the commit:

> git reset $(git merge-base rust-lang/master HEAD)
> git commit -a -m 'blah blah blah'
> git push -f 

Anyway, please do fix the history. Code seems fine to me. That said, I'm going to delegate to someone else because I'm drowning in reviews and I don't actually know this part of the parser particularly well (so e.g. I'm not sure what's on with that match exactly).

r? @petrochenkov (cc @estebank — you might be another good choice here)

[Crazycolorz5]

All right, I've fixed the history, as well as modified the other failing test cases. I'm still a bit wary about the token-error-correct-3 test case, as that's meant to continue parsing to the end due to a similar token, so I'd like to hear someone more experienced's opinion on what the error should be like.

Interestingly, but somewhat unrelatedly, if the token is NOT similar we end up with two error messages. (This is using rustc before this change in this PR but it shouldn't change the number of error messages)

error: expected one of `,`, `::`, or `as`, found `-`
  --> test.rs:17:10
   |
17 | use x::{A- B}; //~ ERROR expected one of `,`, `::`, or `as`, found `.`
   |          ^ expected one of `,`, `::`, or `as` here

error: expected one of `*`, `::`, `;`, `as`, or `{`, found `-`
  --> test.rs:17:10
   |
17 | use x::{A- B}; //~ ERROR expected one of `,`, `::`, or `as`, found `.`
   |          ^ expected one of `*`, `::`, `;`, `as`, or `{` here

error: aborting due to 2 previous errors```

[Crazycolorz5]

Okay addressing these discrepancies one-by-one:

[01:05:31] error: /checkout/src/test/compile-fail/issue-39616.rs:11: unexpected error: '11:16: 11:17: expected one of `)`, `,`, `->`, `where`, or `{`, found `]`'
[01:05:31] 
[01:05:31] error: /checkout/src/test/compile-fail/issue-39616.rs:11: expected error not found: expected one of `->`, `where`, or `{`, found `]`

Offending code:

fn foo(a: [0; 1]) {} //~ ERROR expected type, found `0`
//~| ERROR expected one of `->`, `where`, or `{`, found `]`
// FIXME(jseyfried): avoid emitting the second error (preexisting)

I believe this to be caused by attempted recovery from the parsing of the [0; 1]. That is, it fails upon 0, so it discards the current tree, then errors upon reaching ]. (Though by this explanation why doesn't it error on the ;?)

[01:05:31] error: /checkout/src/test/compile-fail/privacy/restricted/tuple-struct-fields/test.rs:14: unexpected error: '14:26: 14:27: expected one of `)` or `,`, found `(`'
[01:05:31] 
[01:05:31] error: /checkout/src/test/compile-fail/privacy/restricted/tuple-struct-fields/test.rs:14: expected error not found: expected `,`, found `(`

Offending code:

    struct S2(pub((foo)) ());
    //~^ ERROR expected `,`, found `(`

The second () is invalid, so it suggests either adding a comma for another parameter, or immediately closing the expression with a ). In this case, the new behavior seems correct to me?

Tests 2 and 3 are basically the same as the above scenario.

Should I change the expected error messages? I think an argument for the first one could be made. Also, it seems more useful, but context sensitive error messages (as suggested in the issue) would be difficult due to the need to parse the callback (and thus generate the error) BEFORE looking ahead to see if the next token is a ;.

[estebank]

@Crazycolorz5 change the tests, and if you can, move them to ui. I feel that the changes are reasonable but probably should have tickets associated to them to make them better.

[Crazycolorz5]

I've changed the tests and their folders as suggested above. I'm not familiar with the process of opening tickets so, ...

[Crazycolorz5]

My bad, I didn't quite previously understand the testing infrastructure. I'll add stderr files when I'm able to run the built compiler locally.

[estebank]

You can use the --bless flag for x.py to create the stderr files for you.

[estebank]

@bors r+ rollup

[bors]

📌 Commit df0c6a9 has been approved by estebank