Skip to content

Implement machine-applicable suggestions in diagnostics#392

Merged
mcy merged 7 commits intomainfrom
mcy/suggestions2
Jan 21, 2025
Merged

Implement machine-applicable suggestions in diagnostics#392
mcy merged 7 commits intomainfrom
mcy/suggestions2

Conversation

@mcy
Copy link
Member

@mcy mcy commented Dec 13, 2024

This PR adds report.SuggestEdits, which allows diagnostics to suggest changes to an annotated span. This will be used for surfacing quick-fixes in the LSP (for example, for suggesting deleting unused imports, adding a syntax declaration, adding missing field numbers, and so on).

The compiler is also able to render these as a simple diff, similar to some of rustc's diagnostics. I've tried to keep the data model simple: no diffing actually happens, the diagnostics are expected to generate the diffs manually. All the renderer does is convert the edits into hunks to show to the user. This PR contains examples in the lexer that shows how it is meant to be used.

@mcy mcy requested a review from jhump December 13, 2024 22:06
Comment on lines 13 to 14
2 | 0b1_000_0000x40
| -----------++++
Copy link
Member

Choose a reason for hiding this comment

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

This notation is confusing. It would be much nicer to render this more like a diff

2 |- 0b1_000_000
  |+ 0x40

It looks like the logic already does this for multi-line suggestions. I think it would be more clear to render the same way, even for single-line suggestions.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the best compromise is to only use multi-line for when the suggestion only adds something. This allows things like the suggestion to add a field number to look like

// Single line.
LL |   repeated int32 numbers = 7;
   |                         ++++

// Multiline.
LL | -  repeated int32 numbers;
   | +  repeated int32 numbers = 7;
   | 

We could also do this for pure deletions but I think it's not as

// to mark the snippet with the same color as the overall diagnostic.
primary bool

edits []Edit
Copy link
Member

Choose a reason for hiding this comment

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

nit: the struct's Go doc should likely be updated to describe the alternate rendering behavior when this is non-empty.

Also, does this really need to be a slice? I think only the thousands-separator diagnostic uses multiple edits, but it could just as easily suggest a single edit that replaces the entire numeric literal with a value that has no underscores.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, for errors where we specify missing brackets. It's not strictly necessary, but I don't really want to give up the flexibility.

Comment on lines 926 to 928
// When the suggestion spans multiple lines, we don't bother doing a by-the-rune
// diff, because the result can be hard for users to understand how to apply
// to their code.
Copy link
Member

Choose a reason for hiding this comment

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

Per other comment above: I think it would be more clear to always use the multi-line rendering strategy.

Copy link
Member Author

Choose a reason for hiding this comment

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

As mentioned above, I've made it so that the single-line strategy is only used for pure additions.

@mcy mcy requested a review from jhump January 9, 2025 20:07
@mcy mcy merged commit bdba7cb into main Jan 21, 2025
7 checks passed
@mcy mcy deleted the mcy/suggestions2 branch January 21, 2025 18:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants