Skip to content

Fix some LSP bugs causing code actions to not show up correctly#1805

Merged
dzhou121 merged 5 commits intolapce:masterfrom
TudbuT:master
Dec 12, 2022
Merged

Fix some LSP bugs causing code actions to not show up correctly#1805
dzhou121 merged 5 commits intolapce:masterfrom
TudbuT:master

Conversation

@TudbuT
Copy link
Copy Markdown
Contributor

@TudbuT TudbuT commented Dec 12, 2022

  • Added an entry to CHANGELOG.md if this change could be valuable to users

This will fix the following LSP-related issues:

  • Java LS does not show "Surround with try/catch" and similar issue-related actions
  • All language servers don't always update code actions when the document was changed
  • Rust LS sometimes highlights a line with a fix suggestion, but is unable to actually provide it in code actions

This PR updates the capabilities and code action requests to fully match LSP spec.

@TudbuT
Copy link
Copy Markdown
Contributor Author

TudbuT commented Dec 12, 2022

Wait what, why'd clippy fail here but not on my local PC?

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Dec 12, 2022

Codecov Report

Merging #1805 (b06623b) into master (4bdb16d) will increase coverage by 0.00%.
The diff coverage is 0.00%.

@@          Coverage Diff           @@
##           master   #1805   +/-   ##
======================================
  Coverage    6.39%   6.40%           
======================================
  Files         128     128           
  Lines       55350   55330   -20     
======================================
  Hits         3542    3542           
+ Misses      51808   51788   -20     
Impacted Files Coverage Δ
lapce-data/src/editor.rs 0.00% <0.00%> (ø)
lapce-proxy/src/dispatch.rs 0.00% <0.00%> (ø)
lapce-proxy/src/plugin/lsp.rs 0.00% <0.00%> (ø)
lapce-proxy/src/plugin/mod.rs 0.00% <0.00%> (ø)
lapce-rpc/src/proxy.rs 0.00% <0.00%> (ø)
lapce-ui/src/app.rs 0.00% <0.00%> (ø)
lapce-data/src/proxy.rs 0.00% <0.00%> (ø)
lapce-data/src/update.rs 0.00% <0.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@TudbuT TudbuT requested a review from dzhou121 December 12, 2022 17:35
);
}
// REMOVED(if) this prevents some code actions from showing up correctly, and there is almost no performance penalty for this
//if self.doc.code_actions.get(&prev_offset).is_none() {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Does this make it not work properly?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The if-statement causes some delayed code actions to be left out, causing some quick fixes to not appear. REMOVED(if) is just the reason why it was removed and what was removed, in this case an if statement.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Is there any way I can test the delayed code actions bit?

Copy link
Copy Markdown
Contributor Author

@TudbuT TudbuT Dec 12, 2022

Choose a reason for hiding this comment

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

Take this rust code:

fn main() {
    let x = 0;
    x = 1;
    println!(x);
}

There are two errors here. Open quick fixes on the variable name, there will be no fix. Now you fix the println one, which has a higher internal priority in RA. Save the file. The bug happens when you open quick fixes on the variable name again, even after you have now fixed the println one and it shows a new error.

There is a "consider making this binding mutable" in the error dialog, but not in the quick fixes.

Copy link
Copy Markdown
Contributor Author

@TudbuT TudbuT Dec 12, 2022

Choose a reason for hiding this comment

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

The same goes for code actions on code modified after caching, which is even easier to encounter.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks

@dzhou121 dzhou121 merged commit 94ebdd1 into lapce:master Dec 12, 2022
@panekj panekj added this to the Next release milestone Dec 12, 2022
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.

4 participants