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

Add clang-tidy editor.codeActionsOnSave entry point #12684

Closed
thernstig opened this issue Sep 4, 2024 · 14 comments
Closed

Add clang-tidy editor.codeActionsOnSave entry point #12684

thernstig opened this issue Sep 4, 2024 · 14 comments
Labels
enhancement Improvement to an existing feature Feature: Code Analysis Related to integration with clang-tidy, cppcheck, cl.exe /analyze, etc. help wanted Can be fixed in the public (open source) repo. Language Service more votes needed Issues that have been postponed until more community members upvote it
Milestone

Comments

@thernstig
Copy link

thernstig commented Sep 4, 2024

Feature Request

The proper way for extensions to add linting fixes (not formatting fixes) is to add entries like this in the VS Code config:

  "editor.codeActionsOnSave": {
    "source.organizeImports": "always",
    "source.fixAll.eslint": "always",
    "source.fixAll.markdownlint": "always",
    "source.fixAll.stylelint": "always"
  },

It would be nice if this extension could add a source.fixAll.clangTidy that would then automatically fix all fixable problems on save.

References

https://stackoverflow.com/a/61051832/1853417
https://code.visualstudio.com/api/references/vscode-api (search for CodeActionProvider)
https://code.visualstudio.com/updates/v1_83#_code-actions-on-save-and-auto-save

@sean-mcmanus sean-mcmanus added Language Service enhancement Improvement to an existing feature Feature: Code Analysis Related to integration with clang-tidy, cppcheck, cl.exe /analyze, etc. labels Sep 4, 2024
Copy link

github-actions bot commented Nov 4, 2024

This feature request is being closed due to insufficient upvotes. Please leave a 👍-upvote or 👎-downvote reaction on the issue to help us prioritize it. When enough upvotes are received, this issue will be eligible for our backlog.

@github-actions github-actions bot added the more votes needed Issues that have been postponed until more community members upvote it label Nov 4, 2024
@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Nov 4, 2024
@github-actions github-actions bot added this to the Triage milestone Nov 4, 2024
@github-project-automation github-project-automation bot moved this to Done in cpptools Nov 4, 2024
@thernstig
Copy link
Author

@sean-mcmanus it seems impossible to get enough upvotes when there is so little activity in the repo and only a 2 month time for something to be upvoted.

This is also the defacto standard for how to handle this from VS Code, almost to the point that this should be considered a bug.

@bobbrow
Copy link
Member

bobbrow commented Nov 4, 2024

@thernstig, please don't take it personally, we add the "more votes needed" tag to highlight that this feature request can be considered in the future and the bot will reopen it automatically when the votes are received. With over 1000 active issues and a very small team, we have to set some rules for feature requests, not only to help us manage the overall backlog size, but also to set expectations about the chances that something may make it into the product.

We do have a modest amount of activity in the repo, but we also set the bar very low for the number of upvotes required. The VS Code team requires 20. We only require 3 and we allow the requestor to vote on their own request.

@sean-mcmanus
Copy link
Contributor

@thernstig What exactly do these statements mean: add entries like this in the VS Code config and extension could add a source.fixAll.clangTidy mean? Is something you could add via modifying our TypeScript implementation? Do you know of any examples of this being added for other extensions? i.e. I'd have to do research into how that would be implemented.

@thernstig
Copy link
Author

thernstig commented Nov 5, 2024

@bobbrow oh, thanks @bobbrow 😄. Understandable. I am not as angry as I might sound. I am just a huge DX proponent, and I know how many other languages extensions handles this in VS Code, that feel more modern in key areas around how, as far as my understanding is, should be done. This extensions is great though! I am and want to continue to use it very much, and am merely striving to lift some key areas that I find most language extensions should have (again, from my own experience). Maybe I should send in a resume to see if I can join the team instead of writing issues 😄🤚

@sean-mcmanus see https://github.com/microsoft/vscode-eslint?tab=readme-ov-file#version-204 and https://github.com/astral-sh/ruff-vscode?tab=readme-ov-file#configuring-vs-code for two very large extensions, for the huge languages JavaScript and Python.

@sean-mcmanus
Copy link
Contributor

@thernstig I looked at those examples repos, but I still don't see what API they're calling to register a particular fixit values like "source.fixAll.clangTidy". Maybe we can just check the editor.codeActionsOnSave for "source.fixAll" or "source.fixAll.clangTidy", but I don't think it would appear as a completion option in the settings UI.

@thernstig
Copy link
Author

@sean-mcmanus https://code.visualstudio.com/api/references/vscode-api#CodeActionKind maybe?

It is the https://code.visualstudio.com/api/references/vscode-api and CodeActionProvider that is used that:

Change applied on save by the editor.codeActionsOnSave setting.

This all started way back here the discussion around this: microsoft/vscode#47621

@sean-mcmanus
Copy link
Contributor

@thernstig Yeah, thanks, that looks like it's it -- we currently use the QuickFix type. I can change it to a new kind to see if it works. I'm not sure if it'll correctly run the formatter after the fix and before saving though.

@sean-mcmanus sean-mcmanus self-assigned this Nov 5, 2024
@sean-mcmanus sean-mcmanus added quick fix investigate: costing Investigate how much work is required and removed more votes needed Issues that have been postponed until more community members upvote it labels Nov 5, 2024
@sean-mcmanus sean-mcmanus modified the milestones: Triage, 1.23 Nov 5, 2024
@sean-mcmanus sean-mcmanus reopened this Nov 5, 2024
@sean-mcmanus sean-mcmanus moved this from Done to Triage in cpptools Nov 5, 2024
@thernstig
Copy link
Author

@sean-mcmanus please note this is only for fixing linting issues that are auto-fixable, such as for clang-tidy. It is not supposed to be used for formatters like clang-format.

I.e. it is supposed to be used for auto-fixable problems as listed here: https://clang.llvm.org/extra/clang-tidy/checks/list.html

@bobbrow
Copy link
Member

bobbrow commented Nov 5, 2024

Maybe I should send in a resume to see if I can join the team instead of writing issues 😄🤚

😄 Unfortunately, we aren't currently hiring, but I think I see you on Linkedin. I'll send you a request to connect.

@sean-mcmanus
Copy link
Contributor

@thernstig The formatter generally needs to run on the fixit's or they don't have any alignment.

@sean-mcmanus sean-mcmanus removed their assignment Nov 18, 2024
@sean-mcmanus sean-mcmanus added help wanted Can be fixed in the public (open source) repo. and removed investigate: costing Investigate how much work is required labels Nov 18, 2024
@sean-mcmanus
Copy link
Contributor

@thernstig With

    "C_Cpp.codeAnalysis.clangTidy.checks.enabled": ["readability-make-member-function-const", "readability-const-return-type"],
    "editor.codeActionsOnSave": {
        "source.fixAll": "always"
    }

and

class foo
{
int i;
public:
  int func() { return i; }
  int func2() { return i; }
  const int func3() { return 0; }
};

and changing the QuickFix type to SourceFixAll at https://github.com/microsoft/vscode-cpptools/blob/main/Extension/src/LanguageServer/codeAnalysis.ts#L127

It causes the "Fix all" option to disappear from the Quick Fix list (we could probably work around that by adding a different code action for that type), but it never invokes the C_Cpp.FixAllCodeAnalysisProblems command, so either I'm not doing something correct or there's some VS Code bug. I get the same result when I use const codeAnalysisFixAllMacroKind: vscode.CodeActionKind = vscode.CodeActionKind.SourceFixAll.append("clangTidy"); as the type.

@sean-mcmanus sean-mcmanus removed this from the 1.23 milestone Nov 18, 2024
@thernstig
Copy link
Author

@sean-mcmanus I have not tested this myself so I cannot give an exact answer, but there are other extensions that do this. See e.g. https://marketplace.visualstudio.com/items?itemName=dbaeumer.vscode-eslint and https://marketplace.visualstudio.com/items?itemName=charliermarsh.ruff that does this. Their code base should give some hint. (I would check it if I had the time, but I am swamped atm).

In addition, there should be a source.fixAll.clangTidy added. Below is an example of a real project of mine, where each extension/linter has its own config. Of course a source.fixAll should still work, enabling all linters that have a editor.codeActionsOnSave -> source.fixAll hook.

{
  "editor.codeActionsOnSave": {
    "source.organizeImports": "always",
    "source.fixAll.eslint": "always",
    "source.fixAll.markdownlint": "always",
    "source.fixAll.stylelint": "always"
  }
}

Copy link

This feature request is being closed due to insufficient upvotes. Please leave a 👍-upvote or 👎-downvote reaction on the issue to help us prioritize it. When enough upvotes are received, this issue will be eligible for our backlog.

@github-actions github-actions bot added the more votes needed Issues that have been postponed until more community members upvote it label Jan 20, 2025
@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Jan 20, 2025
@github-actions github-actions bot added this to the Triage milestone Jan 20, 2025
@github-project-automation github-project-automation bot moved this from Triage to Done in cpptools Jan 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improvement to an existing feature Feature: Code Analysis Related to integration with clang-tidy, cppcheck, cl.exe /analyze, etc. help wanted Can be fixed in the public (open source) repo. Language Service more votes needed Issues that have been postponed until more community members upvote it
Projects
Status: Done
Development

No branches or pull requests

3 participants