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

Lsp/quickfix unknowns #753

Merged
merged 10 commits into from
Oct 21, 2023
Merged

Lsp/quickfix unknowns #753

merged 10 commits into from
Oct 21, 2023

Conversation

KtorZ
Copy link
Member

@KtorZ KtorZ commented Oct 21, 2023

2023-10-21 14 23 45

  • 📍 Define 'ExtraData' trait for errors
    This should allow passing some extra information to LSP diagnostic in order to provide quickfix actions, such as auto-imports.

  • 📍 Implement simple code action quickfix for unknown variable.

  • 📍 Use (untyped) AST to find the right insert location for imports.
    This removes the need to rely on the formatter to clear things up
    after insert a new import. While this is not so useful for imports, I
    wanted to experiment with the approach for future similar edits (for
    example, when suggesting an inline rewrite).

  • 📍 Refactor and relocate document edits function for imports.
    It's a bit 'off-topic' to keep these in aiken-lang as those functions are really just about lsp. Plus, it removes a bit some of the boilerplate and make the entire edition more readable and re-usable. Now we can tackle other similar errors with the same quickfix.

  • 📍 Relocate and refactor quickfix code into its own module
    We're going to have more quickfixes, to it's best not to overload the
    'server' module. Plus, there's a lot of boilerplate around the
    quickfixes so we might want to factor it out.

  • 📍 Implement quickfix for 'UnknownModule'.

  • 📍 Fix insertion of unqualified import when first
    I previously missed a case and it causes qualified imports to be added at the end if they are lexicographically smaller than ALL other qualified imports. No big deal, but this is now fixed.

  • 📍 Add quickfix for unknown alias & data types.

  • 📍 Fix incoherent 'UnknownVariable' being returned in type-check
    I initially removed the 'UnkownTypeConstructor' since it wasn't used anywhere and was in fact dead-code. On second thoughts however, it is nicer to provide a slightly better error message when a constructor is missing as well as some valid suggestion. Prior to that commit, we would simply return a 'UnknownVariable' and the hint might suggest lowercase identifiers; which is wrong.

  • 📍 Add quickfix for unknown constructors.

KtorZ added 10 commits October 20, 2023 18:00
  This should allow passing some extra information to LSP diagnostic in order to provide quickfix actions, such as auto-imports.
  This removes the need to rely on the formatter to clear things up
  after insert a new import. While this is not so useful for imports, I
  wanted to experiment with the approach for future similar edits (for
  example, when suggesting an inline rewrite).
  It's a bit 'off-topic' to keep these in aiken-lang as those functions are really just about lsp. Plus, it removes a bit some of the boilerplate and make the entire edition more readable and re-usable. Now we can tackle other similar errors with the same quickfix.
  We're going to have more quickfixes, to it's best not to overload the
  'server' module. Plus, there's a lot of boilerplate around the
  quickfixes so we might want to factor it out.
  I previously missed a case and it causes qualified imports to be added at the end if they are lexicographically smaller than ALL other qualified imports. No big deal, but this is now fixed.
  I initially removed the 'UnkownTypeConstructor' since it wasn't used anywhere and was in fact dead-code. On second thoughts however, it is nicer to provide a slightly better error message when a constructor is missing as well as some valid suggestion. Prior to that commit, we would simply return a 'UnknownVariable' and the hint might suggest lowercase identifiers; which is wrong.
@KtorZ KtorZ self-assigned this Oct 21, 2023
@KtorZ KtorZ requested a review from a team as a code owner October 21, 2023 12:28
@@ -0,0 +1,3 @@
pub trait ExtraData {
Copy link
Member

Choose a reason for hiding this comment

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

fancy

Copy link
Member Author

Choose a reason for hiding this comment

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

Kind of hacky for now but sufficient for the few quick fixes I needed to implement. Ideally, I'd like the data to be some serialisable thing or even just a serde_json::Value to make it more flexible. But then that means introducing serde as a dependency of Aiken-lang and I didn't feel quite like doing it now.

/// whether the import is a newline or not. It is set to 'false' when adding a qualified import
/// to an existing list.
impl ParsedDocument {
pub fn import(
Copy link
Member

Choose a reason for hiding this comment

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

wow, nice

@@ -61,6 +59,8 @@ impl LspProject {
self.modules.insert(module.name.to_string(), module);
}

result?;
Copy link
Member

Choose a reason for hiding this comment

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

ah nice

Copy link
Member Author

Choose a reason for hiding this comment

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

It puzzled me for a while this one. Debugging the lsp server isn't quite easy and it took me some time to figure out why the server wasn't aware of any module.

Copy link
Member

@rvcas rvcas left a comment

Choose a reason for hiding this comment

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

amazing, code too clean 🧼

@KtorZ KtorZ merged commit d6f74b5 into main Oct 21, 2023
@KtorZ KtorZ deleted the lsp/quickfix-unknowns branch October 21, 2023 19:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 🚀 Released
Development

Successfully merging this pull request may close these issues.

2 participants