Using quick fix action to rename an identifier does not rename occurrences in other files

[FancyBanana]

The "change to" fix action does not use "rename" action, it only changed the identifier it was called on.

System information

Output of clangd --version:

clangd --version
Ubuntu clangd version 14.0.0-1ubuntu1
Features: linux+grpc
Platform: x86_64-pc-linux-gnu

Also clangd 15.0.6 that is downloaded with clangd extension in VSCode

Editor/LSP plugin:
VSCode/ llvm-vs-code-extensions.vscode-clangd

Operating system:
Ubuntu 22.04.2 LTS

[HighCommander4]

Could you provide a code example please, and indicate where in the code you are performing the code action?

[FancyBanana]

Here's some example code that has this issue:

ExampleClass.hpp

#pragma once

class test {

    public:

    test();

    float ThisIsAVariable;

    float ThisIsAFunction(float);

};

ExampleClass.cpp

#include "ExampleClass.hpp"

test::test() : ThisIsAVariable(1) {};

float test::ThisIsAFunction(float b){
    return ThisIsAVariable * 2 - b;
}

main.cpp

#include "ExampleClass.hpp"
#include <iostream>

int main() {


  std::cout << "test " << test().ThisIsAFunction(22) << std::endl;

}

.clang-tidy

Checks: "bugprone-*,
  modernize-*,
  -modernize-use-trailing-return-type,
  -modernize-use-using,
  readability-*,
  clang-analyzer-*,
  clang-diagnostic-*,
  cppcoreguidelines-*,
  -cppcoreguidelines-owning-memory,
  performance-*,
  -fuchsia-*,
  -*-use-auto,
  -cert-err58-cpp,
  -google-readability-todo,
  "
HeaderFilterRegex: ".*"
CheckOptions:
  - {key: readability-implicit-bool-conversion.AllowPointerConditions, value: 1}
  - { key: readability-identifier-naming.NamespaceCase,                value: CamelCase }
  - { key: readability-identifier-naming.TypedefCase,                  value: CamelCase }
  - { key: readability-identifier-naming.TypedefSuffix,                value: _t }
  - { key: readability-identifier-naming.TypeAliasCase,                value: CamelCase }
  - { key: readability-identifier-naming.TypeAliasSuffix,              value: _t }
  - { key: readability-identifier-naming.ClassCase,                    value: CamelCase}
  - { key: readability-identifier-naming.TemplateCase,                 value: CamelCase}
  - { key: readability-identifier-naming.TemplatePrefix,               value: T}
  - { key: readability-identifier-naming.StructCase,                   value: CamelCase }
  - { key: readability-identifier-naming.AbstractClassCase,            value: CamelCase}
  - { key: readability-identifier-naming.AbstractClassPrefix,          value: I}
  - { key: readability-identifier-naming.PrivateMemberCase,            value: CamelCase }
  - { key: readability-identifier-naming.PrivateMemberPrefix,          value: m_ }
  - { key: readability-identifier-naming.ProtectedMemberCase,          value: CamelCase }
  - { key: readability-identifier-naming.ProtectedMemberPrefix,        value: m_ }
  - { key: readability-identifier-naming.PublicMemberCase,             value: camelBack}
  - { key: readability-identifier-naming.ParameterCase,                value: camelBack }
  - { key: readability-identifier-naming.LocalVariable,                value: camelBack}
  - { key: readability-identifier-naming.ConstantCase,                 value: CamelCase}
  - { key: readability-identifier-naming.ConstantPrefix,               value: c_}
  - { key: readability-identifier-naming.EnumCase,                     value: CamelCase}
  - { key: readability-identifier-naming.EnumConstantCase,             value: CamelCase }
  - { key: readability-identifier-naming.ProtectedMethodCase,          value: camelBack }
  - { key: readability-identifier-naming.PublicMethodCase,             value: camelBack}
  - { key: readability-identifier-naming.PrivateMethodCase,            value: camelBack}
  - { key: readability-identifier-naming.PrivateMethodPrefix,          value: _}
  - { key: readability-identifier-naming.MacroDefinitionCase,          value: UPPER_CASE }

The issue happens when you try to do the auto fix (or just fix) float ThisIsAVariable or float ThisIsAFunction(float); in ExampleClass.hpp

[HighCommander4]

Thanks, I see now: when invoking the quick fix for clang-tidy's "invalid case style", which is to rename the variable, only occurrences in the current file are renamed, not occurrences in other files.

This is kind of an interesting problem. Currently, clangd just passes through the quick fix produced by the clang-tidy checker. The clang-tidy checker operates on a translation unit level, so it doesn't have the capability to produce a quick fix that makes edits to other translation units.

On the other hand, clangd does have the capability to rename a variable across translation units, using its index of the project. In theory, clangd could "upgrade" the clang-tidy quick fix from a "local rename" to a "global rename". But to do this, clangd would need to understand that the quick-fix is "rename symbol X", rather than "make these edits to this file" which is the current information clang-tidy provides.

[HighCommander4]

I wonder if perhaps a solution could build upon the proposal in this RFC to "enhance clang-tidy with project-level knowledge". Then perhaps clang-tidy could produce the multi-TU quick fix itself.

[tom-anders]

For neovim users, here's a (slightly modified) snippet from my config that takes a clang-tidy rename code action and forwards it to clangd (Probably something similar could be built for vscode):

require'lspconfig'.clangd.setup {
   on_attach = function(client, bufnr)
        vim.api.nvim_create_user_command('ClangdApplyRename', function() apply_next_rename(client) end, {})
   end
}

 function apply_next_rename(clangd)
    local params = vim.lsp.util.make_range_params()
    params.context = {
        diagnostics = vim.lsp.diagnostic.get_line_diagnostics()
    }
    local result = clangd.request_sync('textDocument/codeAction', params, 1000)

    if result.err then
        vim.notify(vim.inspect(result.err), "error")
        return
    end

    local current_column = vim.fn.col('.') - 1 -- LSP is 0-indexed

    local find_name = function(result)
        for _, action in pairs(result) do
            if (action.diagnostics and action.diagnostics[1] and action.diagnostics[1].code == 'readability-identifier-naming') then
                local range = action.diagnostics[1].range
                if (range.start.character <= current_column and current_column < range['end'].character) then
                    -- Simply use the first new text, they should all be the same
                    for file, change in pairs(action.edit.changes) do
                        return change[1].newText
                    end
                end
            end
        end
    end
    local newName = find_name(result.result)

    if not newName then
        vim.notify("No rename code action at this line!", "error")
        return
    end

    params = vim.lsp.util.make_position_params()
    params.newName = newName

    result = clangd.request_sync('textDocument/rename', params, 1000)
    if result.err then 
        vim.notify(vim.inspect(result.err), "error")
        return
    end

    vim.lsp.util.apply_workspace_edit(result.result, clangd.offset_encoding)
end

[HighCommander4]

@tom-anders very interesting :)

In theory, clangd could "upgrade" the clang-tidy quick fix from a "local rename" to a "global rename". But to do this, clangd would need to understand that the quick-fix is "rename symbol X", rather than "make these edits to this file" which is the current information clang-tidy provides.

So your script solves this problem by hardcoding the information, "for the readability-identifier-naming diagnostic, the quick-fix is conceptually a rename, with the newText field of the first change storing the new name".

I guess if we are fine with hard-coding such things in clangd, there's no reason the server couldn't do this, and replace the quick-fix with a Command that performs a workspace-wide rename when invoked.

[tom-anders]

@tom-anders very interesting :)

In theory, clangd could "upgrade" the clang-tidy quick fix from a "local rename" to a "global rename". But to do this, clangd would need to understand that the quick-fix is "rename symbol X", rather than "make these edits to this file" which is the current information clang-tidy provides.

So your script solves this problem by hardcoding the information, "for the readability-identifier-naming diagnostic, the quick-fix is conceptually a rename, with the newText field of the first change storing the new name".

I guess if we are fine with hard-coding such things in clangd, there's no reason the server couldn't do this, and replace the quick-fix with a Command that performs a workspace-wide rename when invoked.

Yeah IMO hardcoding this should be fine, the "API" of the quick-fix should never change in the future, and if clang-tidy at some point gains the ability to do a "global rename", it would not even break our hardcoded logic.

I can try implementing this in clangd and see what the feedback is - if it's not accpected, at least then we'll have an editor-agnostic patch that people can use.

[HighCommander4]

I can try implementing this in clangd and see what the feedback is - if it's not accpected, at least then we'll have an editor-agnostic patch that people can use.

Sure! I'm happy to help by reviewing.

[tom-anders]

@HighCommander4 Here's a draft: llvm/llvm-project#78454
Feel free to add some comments when you have time - We should probably add a lit-test, I'll do that once we agree on implementation details