Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make parse error suggestions verbose and fix spans #127407

Merged
merged 7 commits into from
Jul 15, 2024

Conversation

estebank
Copy link
Contributor

@estebank estebank commented Jul 6, 2024

Go over all structured parser suggestions and make them verbose style.

When suggesting to add or remove delimiters, turn them into multiple suggestion parts.

@rustbot
Copy link
Collaborator

rustbot commented Jul 6, 2024

r? @oli-obk

rustbot has assigned @oli-obk.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Jul 6, 2024
@oli-obk
Copy link
Contributor

oli-obk commented Jul 8, 2024

Go over all structured parser suggestions and make them verbose style.

why? Most of these seemed fine to me

When suggesting to add or remove delimiters, turn them into multiple suggestion parts.

For these It makes sense

@bors
Copy link
Contributor

bors commented Jul 10, 2024

☔ The latest upstream changes (presumably #127419) made this pull request unmergeable. Please resolve the merge conflicts.

@estebank estebank force-pushed the parser-suggestions branch 2 times, most recently from 44e85d3 to e4df175 Compare July 12, 2024 00:51
@rust-log-analyzer

This comment has been minimized.

@oli-obk oli-obk added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 12, 2024
@oli-obk
Copy link
Contributor

oli-obk commented Jul 12, 2024

As discussed in our 1:1, this needs a motivation and some links to annotate snippets discussions

@estebank
Copy link
Contributor Author

The initial impetus to start this work is to facilitate the migration to annotate-snippets, which doesn't yet support suggestions, but @Muscraft is working on. Sadly the conversations were 1:1, but the gist of them is:

  • a precondition for migrating is minimal changes to the output, we'll accept small divergences as long as they are reasonable, but we're aiming for 100% mimicry
  • annotate-snippets is unlikely to ever support "inline suggestions"
  • rustc is the one dealing with json output where these are relevant, so rustc can continue transforming suggestions into labels for annotate-snippets' sake
  • Minimizing how many of the inline suggestions exist will help with the migration
  • The inline suggestions were the first ones to be implemented; over time we supported multiple suggestions, suggestions with multiple parts, and rendering these suggestions with underlines for addition and replacements and a diff format for removals. I am of the opinion that the "verbose" renderings are easier to understand for people for any case beyond "add an x here" or remove this x`", particularly because suggestions where the code is similar in both cases it is easier to see the difference when presented side by side (people are already used to the patch format that diff tools use), color further aids with this. For TTS users, none of the available outputs are good.
  • Making suggestions verbose is dredging up several suggestions that were incorrectly written (bad spans, using them as span_help, bad code being suggested) that were harder to spot with the inline suggestions; there are also many cases where the suggestion message is something non-descriptive like "try:", which also needs to be changed (editors can show the message without the code, so the message should be enough for someone that understands what's going on to fix their own code)

The additional commits after the one you initially reviewed are addressing your review comments.

@oli-obk
Copy link
Contributor

oli-obk commented Jul 15, 2024

@bors r+

@bors
Copy link
Contributor

bors commented Jul 15, 2024

📌 Commit 71f16bd has been approved by oli-obk

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 15, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jul 15, 2024
…-obk

Make parse error suggestions verbose and fix spans

Go over all structured parser suggestions and make them verbose style.

When suggesting to add or remove delimiters, turn them into multiple suggestion parts.
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 15, 2024
…iaskrgr

Rollup of 5 pull requests

Successful merges:

 - rust-lang#124921 (offset_from: always allow pointers to point to the same address)
 - rust-lang#127407 (Make parse error suggestions verbose and fix spans)
 - rust-lang#127675 (Remove invalid help diagnostics for const pointer)
 - rust-lang#127684 (consolidate miri-unleashed tests for mutable refs into one file)
 - rust-lang#127758 (coverage: Restrict `ExpressionUsed` simplification to `Code` mappings)

r? `@ghost`
`@rustbot` modify labels: rollup
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jul 15, 2024
…-obk

Make parse error suggestions verbose and fix spans

Go over all structured parser suggestions and make them verbose style.

When suggesting to add or remove delimiters, turn them into multiple suggestion parts.
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 15, 2024
…iaskrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#124921 (offset_from: always allow pointers to point to the same address)
 - rust-lang#127407 (Make parse error suggestions verbose and fix spans)
 - rust-lang#127684 (consolidate miri-unleashed tests for mutable refs into one file)
 - rust-lang#127729 (Stop using the `gen` identifier in the compiler)
 - rust-lang#127736 (Add myself to the review rotation)
 - rust-lang#127758 (coverage: Restrict `ExpressionUsed` simplification to `Code` mappings)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 2b82729 into rust-lang:master Jul 15, 2024
6 checks passed
@rustbot rustbot added this to the 1.81.0 milestone Jul 15, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jul 15, 2024
Rollup merge of rust-lang#127407 - estebank:parser-suggestions, r=oli-obk

Make parse error suggestions verbose and fix spans

Go over all structured parser suggestions and make them verbose style.

When suggesting to add or remove delimiters, turn them into multiple suggestion parts.
Comment on lines +31 to +34
help: escape the character
|
LL | '\r';
| ++
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm noticing that the renderer considers any whitespace as "not there" for the purposes of deciding whether this is a replacement or addition (this is normally done to have better output), but it has the side effect of rendering not quite correctly in the face of suggestions to replace zero-width whitespace characters.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants