mbe: Rework diagnostics for metavariable expressions. #142950
Conversation
These tests have expanded beyond the RFC, so rename the directory `rfc-3086-metavar-expr` to `metavar-expressions`. `concat` (which wasn't part of the RFC) now fits in this group, so merge its tests into the `metavar-expressions` directory. Additionally rename some related `issue-*` tests.
`syntax-errors` currently contains both actual syntax error tests and tests (which don't need expansion) and tests for incorrect usage of the `count` metavariable expression (which dos need expansion). Split the expansion-dependent tests to a separate file, remove unneeded invocations from `syntax-errors` to ensure we are catching these before expansion.
Add tests showing the current state to make it more clear when output gets updated later in refactoring.
More diagnostic structs related to metavariable expressions will be introduced. Introduce the abbreviation "mve" which is reasonably unambiguous (`rg Mve` and `rg '(\b|_|-)mve(\b|_|-)'` return no results outside of a Thumb target feature) and use it for these diagnostic types. A new module is also created.
Move the `concat` implementation to a separate function so it is easier to work on. Other metavariable expressions are already split this way. This is a non-functional change.
Move this structure directly above the `parse_<expr>` functions that return it to keep top-down flow. This is a non-functional change.
The current messages have the potential to be more accurate; "expected identifier or string literal" is printed in a few cases where only an identifier should be expected, and it suggests removing string literals when that might not solve the problem. Add a new diagnostic for these kind of errors that gives some more context. For `count` and `ignore` this should likely be combined with the diagnositcs for `eat_dollar` to produce a helpful error if they get anything other than a metavariable first argument. I am planning to do this in a followup.
Give a more user-friendly diagnostic about the following:
* Invalid syntax within the `${...}` braces, including missing
parentheses or trailing tokens.
* Incorrect number of arguments passed to specific metavariable
expressions.
rustbot
added
A-tidy
|
There are changes to the cc @jieyouxu |
| let Some(tt) = iter.next() else { | ||
| let err = errors::MveExpectedIdent { span: fallback_span, not_ident_label: None, context }; | ||
| return Err(psess.dcx().create_err(err)); | ||
| }; |
There was a problem hiding this comment.
I have mixed feelings about this, in general.
Now we take some typical parser errors, like "expected X, found Y", including "expected EOF, found Y" aka trailing tokens, and over-specialize them for the specific case of metavariable expressions.
Like for every "unexpected token" we create a separate "unexpected token ... in space!".
This is not scalable, parser code cannot be shared well if the errors are over-specialized.
For example, this specific change adds some specialized logic here, but leaves previously used parse_token and parse_ident_from_token unchanged.
If some improvements are made, they should go into parse_token and be general enough to improve all users of that function, not some single specific case.
There was a problem hiding this comment.
There is a bit more context; I have continued work on top of these changes that eliminates parse_ident_from_token and parse_token for a more specific error 847b9f7 and then reuses the new logic for expansion diagnostics b576c49 (those two patches are a bit messy yet). The total diff was just getting a bit large so I put this PR up as a portion that can stand on its own without touching concat too much. Created a draft with the (incomplete) rest just now #142975.
I'm happy to adjust the diagnostics further or break the PRs up differently if something else makes more sense to you. What would you prefer?
petrochenkov
added
S-waiting-on-author
Make the diagnostics for metavariable expressions more user-friendly. This mostly addresses syntactic errors; I will be following up with improvements to
concat(..).This is probably easiest to review by-commit, more details are in the messages.
r? @petrochenkov