Clean up various things in the parser - #162269
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
|
r? @mati865 rustbot has assigned @mati865. Use Why was this reviewer chosen?The reviewer was selected based on:
|
|
r? @petrochenkov |
47c209d to
068a771
Compare
This comment has been minimized.
This comment has been minimized.
|
c31845c to
e0b59b1
Compare
This comment has been minimized.
This comment has been minimized.
|
r=me after rebase. |
The comment was added in PR rust-lang#115131. The feature was removed in PR rust-lang#131045.
The logic was added in commit ff61949 (2020) to suppress noisy follow-up syntax errors. For example, consider `src/test/ui/issues/issue-39616.rs` from said commit. However, removing it nowadays doesn't regress any diagnostics. On the contrary, since we no longer try to recover from such malformed array & slice types, the code in `parse_generic_arg` responsible for attempting to recover from complex const exprs that weren't wrapped in braces `{` `}` has an easier time to recover from e.g., `[10]` (as in `Wrap<[10]>`) and doesn't need to create "dummy" diagnostics to attach suggestions onto. In the next commit, we'll remove the special casing of recovered array & array types in `parse_generic_arg` that's no longer necessary.
See the previous commit for details.
e0b59b1 to
8a845e5
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
Instead of checking for `#[` before trying to parse outer attributes,
check for `#` or doc comments instead. This way, we automatically recover
from leading doc comments and inner attributes on types, too.
Previously, we would only recover from doc comments and inner attributes
on types if they were preceded by normal outer attributes.
Move the whole recovery into a separate more general function in
preparation for using it in various other places, too.
This means we drop the recovery from attributes in type contexts that
aren't ascribed to a valid type (at least until the next sync point).
For example, `type T = #[a];` and `fn f() -> #[a] {}` are now fatal
(meaning the parser will only continue parsing from the next sync point).
This could be considered unfortunate but honestly it didn't make much sense:
Why would we only defatalize syntax errors in type contexts if there are
preceding attributes (which aren't legal / expected on types in the first
place, mind you); there's no reason to recover from e.g., `type T = #[a] @;`
(which used to lead to bad follow-up diagnostics). Defatalizing syntax
errors isn't inherently bad, it just needs to be done carefully and not
made dependent on "random" conditions.
In commit 0564168, the check for doc comments was added to prevent us from trying to reparse the type as a const expression and suggesting to wrap it in `{` `}` if successful as that's unlikely what the user meant. However, now that we always try to parse outer attributes (including doc comments) on types to provide better diagnostics, this case in `parse_generic_arg` can't be reached anymore.
Given we now recover from outer attributes on any type, this specialized logic is no longer necessary. Given we now recover from outer attributes on any type, this specialized logic is no longer necessary.
Use our new routine. Again, this means we can now also recover from
leading doc comments and inner attributes.
Moreover, we now actually store type & const arguments that have outer
attributes in the AST instead of using `{type error}` & `{const error}`
for them, respectively.
Admittedly, this slightly regresses diagnostic quality in certain scenarios but this can be improved in the future *for all* users of `recover_from_outer_attributes`. For example, it's a bit contradictory to say "expected outer doc comment" followed by "doc comments cannot be applied to XY". We should probably only emit a single error saying "doc comments cannot be applied to XY" but that needs modifications to `parse_outer_attributes` or something similar.
1. Rename `parse_ty_tuple_or_parens` to `parse_paren_start_ty` since it
doesn't just parse parenthesized types and tuple types, it also parses
bare trait object types where the first bound is parenthesized.
Naming is inspired by `parse_path_start_ty`.
2. Don't use `parse_remaining_bounds_path` ("finish parsing bare trait
object type") when recovering from `for<…> impl …` / `for<…> impl …`
since we're in fact not parsing a bare trait object type but either an
impl-Trait type or a dyn-Trait type. This means we no longer need to
match on the `TyKind` to extract the `bounds`.
3. Rename `parse_remaining_bounds_path` to
`finish_parsing_bare_trait_object_ty` since the previous name really
didn't make it clear that it's specific to bare trait object types.
4. Add a bunch of comments explaining what we're doing in (now)
`parse_paren_start_ty` to successfully parse a bare trait object type
where the first bound is parenthesized.
5. Inline `parse_remaining_bounds` since it didn't really have any reason
to exist. The callsites look just fine or even better with the inlined
body.
|
@bors r=petrochenkov |
This comment has been minimized.
This comment has been minimized.
What is this?This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.Comparing 2e6e39f (parent) -> 6b410a8 (this PR) Test differencesShow 34 test diffsStage 1
Stage 2
Additionally, 28 doctest diffs were found. These are ignored, as they are noisy. Job group index
Test dashboardRun cargo run --manifest-path src/ci/citool/Cargo.toml -- \
test-dashboard 6b410a8387fa41607d50b4f7da844f4d79a565bc --output-dir test-dashboardAnd then open Job duration changes
How to interpret the job duration changes?Job durations can vary a lot, based on the actual runner instance |
|
Finished benchmarking commit (6b410a8): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis perf run didn't have relevant results for this metric. Max RSS (memory usage)Results (primary -0.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary -0.2%, secondary -2.3%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis perf run didn't have relevant results for this metric. Bootstrap: 482.327s -> 477.191s (-1.06%) |
View all comments
I urge you to review this PR commit by commit. Note that almost every single commit comes with a rather elaborate commit message. Well, excuse the probably questionable grammar & structure of some of these commit messages, I was a bit tired while writing them; I'll go over them again some other day to revise them.
What does this PR actually do?
(No LLM was or will be used by me during the entire creation process of this PR)
Footnotes
It doesn't consolidate all of them. I've left alone the one for
if&elsebranches and the one for use-trees. Those offer slightly more complex recovery. I might revisit this in the future. ↩