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

upstreaming suggestions from the spade fork #372

Open
sornas opened this issue Feb 7, 2025 · 1 comment
Open

upstreaming suggestions from the spade fork #372

sornas opened this issue Feb 7, 2025 · 1 comment
Labels
enhancement New feature or request

Comments

@sornas
Copy link

sornas commented Feb 7, 2025

hi all! happy to see some things happening here again :) for some background: we've used codespan in the spade compiler and forked codespan so we could implement a suggestion system. i think it would be great if we could finish the implementation (since it is still limited to replacements on one line) and upstream our changes. i've started to go over our changes and figure out what it would take to upstream everything. i would like to take the opportunity to implement this "properly".

to start with, here's what a suggestion diagnostic looks like in spade currently:

error: cannot instantiate entities and pipelines in functions  <-- Diagnostic
  ┌─ testinput:4:5
  │
3 │ fn X() -> bool {
  │ -- this is a function                <-- label
4 │     inst Y()
  │     ^^^^ inst not allowed here       <-- label
  │
  = note: functions can only contain combinatorial logic <-- note
  = consider making the function an entity               <-- Subdiagnostic::Suggestion
  │
3 │ entity X() -> bool {
  │ ~~~~~~    <-- SuggestionPart

it turns out we modelled suggestions as "subdiagnostics", meaning a separate diagnostic that is rendered just below the initial ("main") diagnostic. a suggestion is built up of these:

pub struct Suggestion<FileId> {
    pub file_id: FileId,
    pub message: String,
    pub parts: Vec<SuggestionPart>,
}

pub struct SuggestionPart {
    pub range: Range<usize>,  // what to remove from the source
    pub replacement: String,  // what to add instead
}

(side note: the SuggestionPart works great, imo. it can describe replacements, removals (empty replacement) and additions (range with start == end) with just these two fields!)

we also added Subdiagnostic::SpannedNote:

error: Use of consumed resource   <-- Diagnostic
  ┌─ testinput:2:19
  │
2 │     let many_p = [p; 3];
  │                   ^
  │                   │
  │                   Use of consumed resource  <-- label
  │                   Previously used here      <-- label
  │
note: The resource is used in this array initialization  <-- Subdiagnostic::SpannedNote
  ┌─ testinput:2:18
  │
2 │     let many_p = [p; 3];
  │                  ^^^^^^         <-- SpannedNote label (no message on the label but it can have one, of course)
  │

defined as

pub struct SpannedNote<FileId> {
    pub severity: Severity,
    pub message: String,
    pub labels: Vec<Label<FileId>>,
}

now, the suggestions are definitely not ready for merging. but i'm not sure what the best way forward is. currently our suggestions can only handle replacing/adding/removing things that span a single line. (the parts themselves can be placed on different lines, i think.)

if you want suggestions down the road, maybe we can start with just subdiagnostics and spanned notes? if that's the case, let me throw out some thoughts and ideas here and we can figure out what needs to be changed/discussed.

  • should the current notes be a Subdiagnostic::Note each without a span? then the programmer can interleave them however they want.
  • how should subdiagnostics be rendered? as you can see above, spanned notes include a locus but the suggestion does not. they should probably work the same.
  • there are no limitations on the subdiagnostic severity. you could have a main diagnostic with severity info and a subdiagnostic with severity error. bonus points for flexibility, or just weird?
  • is it clear what is a subdiagnostic and what is a new diagnostic when rendering? if the user can't separate them it gets really confusing.
@kaleidawave
Copy link
Collaborator

Awesome! Yes I am thinking a "suggestion" system could be good in the future. I want to do more with LSPs, so it might be good if it also can print to the terminal.

As it is a feature, it will not make the next release. But I can see this being something it could have in the future.

@kaleidawave kaleidawave added the enhancement New feature or request label Feb 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants