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

Avoid syntax error notification for source code actions #12148

Merged
merged 1 commit into from
Jul 4, 2024

Conversation

dhruvmanila
Copy link
Member

Summary

This PR avoids the error notification if a user selects the source code actions and there's a syntax error in the source.

Before #12134, the change would've been different. But that PR disables generating fixes if there's a syntax error. This means that we can return an empty map instead as there won't be any fixes in the diagnostics returned by the lint_fix function.

For reference, following are the screenshot as on main with the error:

VS Code:

Screenshot 2024-07-02 at 16 39 59

Neovim:

Screenshot 2024-07-02 at 16 38 50

fixes: #11931

Test Plan

Considering the following code snippet where there are two diagnostics (syntax error and useless semicolon E703):

x;

y =

VS Code

Screen.Recording.2024-07-02.at.16.42.29.mov

Neovim

Screen.Recording.2024-07-02.at.16.41.24.mov

@dhruvmanila dhruvmanila added bug Something isn't working server Related to the LSP server labels Jul 2, 2024
@dhruvmanila dhruvmanila requested a review from MichaReiser July 2, 2024 11:13
Copy link
Contributor

github-actions bot commented Jul 2, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

ℹ️ ecosystem check encountered linter errors. (no lint changes; 1 project error)

demisto/content (error)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview

warning: The top-level linter settings are deprecated in favour of their counterparts in the `lint` section. Please update the following options in `pyproject.toml`:
  - 'ignore' -> 'lint.ignore'
  - 'select' -> 'lint.select'
  - 'unfixable' -> 'lint.unfixable'
  - 'per-file-ignores' -> 'lint.per-file-ignores'
warning: `PGH001` has been remapped to `S307`.
warning: `PGH002` has been remapped to `G010`.
warning: `PLR1701` has been remapped to `SIM101`.
ruff failed
  Cause: Selection of deprecated rule `E999` is not allowed when preview is enabled.

Copy link
Member

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

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

The code changes look good, but I'm not sure if we want to keep some indicator to why the action failed?

Let's say you have a very long document and the syntax error is outside the visible area. It might now be unclear to users why the manually triggered action does nothing.

RustRover does show an error when running rustfmt failed because of a syntax error and I find this useful information.

@dhruvmanila
Copy link
Member Author

Let's say you have a very long document and the syntax error is outside the visible area. It might now be unclear to users why the manually triggered action does nothing.

RustRover does show an error when running rustfmt failed because of a syntax error and I find this useful information.

That is interesting. Does it show on every save? I don't mind providing a notification.

@MichaReiser
Copy link
Member

That is interesting. Does it show on every save? I don't mind providing a notification.

Only when I issue the Reformat code manually. It doesn't show a notification when the formatting on save fails.

@dhruvmanila
Copy link
Member Author

Only when I issue the Reformat code manually. It doesn't show a notification when the formatting on save fails.

I think it's difficult for a language server to know whether something was invoked manually or automatically by the client except for code actions for which there's CodeActionTriggerKind but that's also optional. I'll make this change in a follow-up PR.

I guess RustRover is able to do this reliably because the entire stack is in their control.

@dhruvmanila dhruvmanila merged commit e6e09ea into main Jul 4, 2024
20 checks passed
@dhruvmanila dhruvmanila deleted the dhruv/syntax-error-code-action branch July 4, 2024 04:07
@dhruvmanila
Copy link
Member Author

I've opened #12176 for now and I can tackle it later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working server Related to the LSP server
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Different behavior between servers for "fixAll" with syntax errors
2 participants