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

Enable ruff-specific source actions #10916

Merged
merged 7 commits into from
Apr 16, 2024
Merged

Conversation

snowsignal
Copy link
Contributor

@snowsignal snowsignal commented Apr 13, 2024

Summary

Fixes #10780.

The server now send code actions to the client with a Ruff-specific kind, source.*.ruff. The kind filtering logic has also been reworked to support this.

Test Plan

Add this to your settings.json in VS Code:

{
  "[python]": {
    "editor.codeActionsOnSave": {
      "source.organizeImports.ruff": "explicit",
    },
  }
}

Imports should be automatically organized when you manually save with Ctrl/Cmd+S.

@snowsignal snowsignal added bug Something isn't working server Related to the LSP server labels Apr 13, 2024
Copy link
Contributor

github-actions bot commented Apr 13, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

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.

It looks good. I would have found it useful if the PR summary contained a small description of how this PR fixes the issue.

What happens if you put the following in your configuration

{
  "[python]": {
    "editor.codeActionsOnSave": {
      "source.organizeImports": "explicit",
      "source.organizeImports.ruff": "explicit",
    },
  }
}

Is this the same way we promote both actions today? Could the LSP know which code actions the client enables to avoid generating unnecessary actions?

I looked at ESLint, and it seems they only generate the source.fixAll.eslint action (and not source.fixAll). I tried to test this locally and it seems to work (There's the chance that I messed up my testing setup but I'm somewhat confident):

VS code configurations:

  "editor.codeActionsOnSave": {
    "source.fixAll": "always"
  }

fix_all

    dbg!("Source fix all ruff only");
    Ok([SOURCE_FIX_ALL_RUFF]
        .into_iter()
        .map(move |kind| types::CodeAction {
            title: format!("{DIAGNOSTIC_NAME}: Fix all auto-fixable problems"),
            kind: Some(kind),
            edit: edit.clone(),
            data: data.clone(),
            ..Default::default()
        })
        .map(CodeActionOrCommand::CodeAction)
        .collect())

And VS code automatically applied the fixes. So maybe generating the fixes twice isn't necessary after all.

crates/ruff_server/src/server/api/requests/code_action.rs Outdated Show resolved Hide resolved
@dhruvmanila
Copy link
Member

(I was in the middle of writing the review comment when I pressed the back button which erased everything, I'll try my best to write everything again.)

I don’t think the server should return two code actions if the user has provided both source.organizeImports and source.organizeImports.ruff. The main reason for having a .ruff specific code action kinds is to allow users to explicitly mention that the client should only apply Ruff specific organizeImports action. For example, a user might have multiple language servers enabled for a specific language but only wants Ruff to apply the organize imports action.

Think of it more as a hierarchy where a more specific code action kind wins. Here’s a snippet from the LSP spec:

/**
 * The kind of a code action.
 *
 * Kinds are a hierarchical list of identifiers separated by `.`,
 * e.g. `"refactor.extract.function"`.
 *
 * The set of kinds is open and client needs to announce the kinds it supports
 * to the server during initialization.
 */
export type CodeActionKind = string;

I’d also recommend testing this out with different values of codeActionOnKind which I believe are explicit, always , and off (confirm the values). What happens in a scenario when source.organizeImports is explicit while source.organizeImports.ruff is always. This might probably be handled by the editor but it would be useful to know the outcome of such scenarios.

I did a small test locally and saw a difference in the response between ruff and ruff-lsp. I’m not sure if it matters but something to check. If I ask only for source.organizeImports.ruff to both the server, the code action kind in the response is different: ruff gives source.organizeImports while ruff-lsp gives (possibly correct?) source.organizeImports.ruff.

@snowsignal snowsignal force-pushed the jane/server/ruff-source-actions branch from 4d28e57 to e5c43ec Compare April 16, 2024 04:11
@snowsignal
Copy link
Contributor Author

@MichaReiser @dhruvmanila I've tried to address your feedback as best as I could.

We now only return the ruff-specific code actions (source.*.ruff) in the code action response. I also reworked how we check supported code actions to account for this.

Here are the various configurations I've tested with the latest changes:

Configuration:

{
  "[python]": {
    "editor.codeActionsOnSave": {
      "source.fixAll": "explicit",
    },
  }
}

Result: Manually saving automatically fixes all problems in the file.

Configuration:

{
  "[python]": {
    "editor.codeActionsOnSave": {
      "source.fixAll.ruff": "explicit",
    },
  }
}

Result: Manually saving automatically fixes all problems in the file.

Configuration:

{
  "[python]": {
    "editor.codeActionsOnSave": {
      "source.fixAll": "never",
      "source.fixAll.ruff": "explicit"
    },
  }
}

Result: Manually saving automatically fixes all problems in the file.

Configuration:

{
  "[python]": {
    "editor.codeActionsOnSave": {
      "source.fixAll": "explicit",
      "source.fixAll.ruff": "explicit"
    },
  }
}

Result: Manually saving automatically fixes all problems in the file.

Configuration:

{
  "[python]": {
    "editor.codeActionsOnSave": {
      "source.fixAll": "explicit",
      "source.fixAll.ruff": "never"
    },
  }
}

Result: Manual saving does nothing.

It's the same for source.organizeImports. Do these match what you expected? I think these behave the same as ruff-lsp except for the last one.

One strange thing I noticed is that the always value for the setting seemed to have no difference compared to explicit - that is, it didn't apply the code action on auto-saves, and you still had to save manually. This is also what ruff-lsp did, so I'm not sure whether this is the expected behavior or not.

@dhruvmanila
Copy link
Member

It's the same for source.organizeImports. Do these match what you expected? I think these behave the same as ruff-lsp except for the last one.

Yeah, I think the behavior in this PR is correct for the last example. I also confirmed with ESLint with the following VS Code settings:

  "[javascript]": {
    "editor.codeActionsOnSave": {
      "source.fixAll": "explicit",
      "source.fixAll.eslint": "never"
    }
  }

And the following code:

/* eslint curly: "error"*/

if (foo) foo++;

I think ruff-lsp must be doing an "or" check where if the kind is either of them, return the said code action.

Copy link
Member

@dhruvmanila dhruvmanila left a comment

Choose a reason for hiding this comment

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

Thank you for addressing all the feedback and providing all of the test scenarios.

@dhruvmanila
Copy link
Member

Any reason why is this PR labeled as "bug"? I don't think there was support for it before this PR so I wouldn't classify it as a bug.

crates/ruff_server/src/server/api/requests/code_action.rs Outdated Show resolved Hide resolved
crates/ruff_server/src/server/api/requests/code_action.rs Outdated Show resolved Hide resolved
crates/ruff_server/src/server.rs Outdated Show resolved Hide resolved
@snowsignal
Copy link
Contributor Author

snowsignal commented Apr 16, 2024

@dhruvmanila:

Any reason why is this PR labeled as "bug"? I don't think there was support for it before this PR so I wouldn't classify it as a bug.

The original issue that this PR solves was that we should have supported this but that support was broken.

@snowsignal snowsignal enabled auto-merge (squash) April 16, 2024 18:14
@snowsignal snowsignal merged commit eab3c4e into main Apr 16, 2024
17 checks passed
@snowsignal snowsignal deleted the jane/server/ruff-source-actions branch April 16, 2024 18:21
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.

Ruff-specific source actions are not working with ruff server
3 participants