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

Fix for readability-redundant-member-init leaves non-compilable code #42928

Closed
igor-akhmetov opened this issue Oct 7, 2019 · 9 comments
Closed
Assignees
Labels
bugzilla Issues migrated from bugzilla clang-tidy invalid Resolved as invalid, i.e. not a bug

Comments

@igor-akhmetov
Copy link

Bugzilla Link 43583
Resolution INVALID
Resolved on Oct 10, 2019 08:05
Version unspecified
OS Windows NT
CC @EugeneZelenko,@pepsiman

Extended Description

With clang-tidy 9 apply fix to the example from the check documentation:

class Foo {
public:
Foo() : s() {}

private:
std::string s;
};

Result:

class Foo {
public:
Foo() : {}

private:
std::string s;
};

@igor-akhmetov
Copy link
Author

assigned to @pepsiman

@pepsiman
Copy link
Contributor

pepsiman commented Oct 8, 2019

Clang-tidy checks produce fixes that sometimes need to be cleaned up to remove redundant colons or commas.

The cleanup is performed after applying the fixes.

clang-tidy 9 performs this cleanup correctly:

$ cat test.cpp
#include

class Foo {
public:
Foo() : s() {}

private:
std::string s;
};

$ clang-tidy-9 --checks=readability-redundant-member-init test.cpp --fix --
1 warning generated.
test.cpp:5:11: warning: initializer for member 's' is redundant [readability-redundant-member-init]
Foo() : s() {}
^~~~
test.cpp:5:11: note: FIX-IT applied suggested code changes
clang-tidy applied 1 of 1 suggested fixes.

$ cat test.cpp
#include

class Foo {
public:
Foo() {}

private:
std::string s;
};

Which tool are you using to apply the fixes?

@igor-akhmetov
Copy link
Author

Thanks, I was not aware that some additional cleanup is taking place. Is it performed by clang-format, or by some logic in clang-tidy itself?

I'm using ReSharper C++, which just applies the fixit provided by clang-tidy. In this case the text edit is:

MainSourceFile:  'C:\tmp\ConsoleApplication19\ConsoleApplication19.cpp'
Diagnostics:
  - DiagnosticName:  readability-redundant-member-init
    DiagnosticMessage:
      Message:         'initializer for member ''s'' is redundant'
      FilePath:        'C:\tmp\ConsoleApplication19\ConsoleApplication19.cpp'
      FileOffset:      53
      Replacements:
        - FilePath:        'C:\tmp\ConsoleApplication19\ConsoleApplication19.cpp'
          Offset:          53
          Length:          4
          ReplacementText: ''

That said, it should be possible to change the fixit provided by this check so that no additional cleanup is necessary. This would help all external tools that consume clang-tidy fixits.

@llvmbot
Copy link
Collaborator

llvmbot commented Oct 8, 2019

The cleanup is implemented in the clang::format::cleanupAroundReplacements function. clang-tidy calls it here: clang-tidy/ClangTidy.cpp:205. Cleanup is done separately from producing the fixes, since it may become necessary when applying multiple independent fixes together, e.g. consider this code:

...
A() : a(...), b(...), c(...) {}
...

If three different checks independently try to remove initializers for a, b and c, which of them should remove commas and the semicolon? If we decide, that the check should remove the comma after an initializer list member, then when removing c(...), we'll not be able to remove a comma after it, but we'll leave a comma before it. Same if we decide to remove a comma before the member (now the issue is with a(...)). Same with the semicolon (we only need to remove it, when removing all initializer list items.

A similar problem exists for inserting/removing #include directives.

We can't clean up replacements when exporting them, because they are grouped by diagnostic and can be applied one by one.

The only good solution is for the tool that applies the fix to clean up code. If ReSharper doesn't do this, it's wrong. It should call clang::format::cleanupAroundReplacements before applying replacements.

@igor-akhmetov
Copy link
Author

For context, ReSharper/CLion run clang-tidy in an external process, in order to:

  1. Be able to use custom user-provided clang-tidy binaries.
  2. Be resilient to clang crashes.

It is also desirable to be able to apply a fix for a specific single inspection from the IDE. This means we can't use cleanupAroundReplacements with the current clang-tidy CLI. To do that we'd need to teach the clang-tidy to apply a single fix (--line-filter is insufficient for that), and to print the resulting replacements together with replacements from cleanupAroundReplacements instead of applying them to the physical file.

An alternative check implementation would output a single check result for a given initialization list which would contain several fixes for all the redundant initializers (so that additional cleanup would not be necessary), but I do understand that the current design is simpler.

@pepsiman
Copy link
Contributor

pepsiman commented Oct 8, 2019

The clang-apply-replacements tool can be used to apply a fix and cleanup.

@igor-akhmetov
Copy link
Author

clang-apply-replacements is not really useful to us, as we'd like to be able to apply replacements inside the IDE itself. The usual workflow after applying a fix in an IDE is to change the text in the IDE buffer, not the physical file itself. The user can then evaluate the changes, and save the file when he wants to or revert the changes back.

@llvmbot
Copy link
Collaborator

llvmbot commented Oct 9, 2019

It seems like the best workaround is to duplicate what 'cleanupAroundReplacements' is doing (copy-paste, rewrite to C#; it's open-source after all) in ReSharper and hope this function does not change too often (which is probably the case).

@llvmbot
Copy link
Collaborator

llvmbot commented Oct 10, 2019

There are multiple options here. In no particular order (and without any guarantees that these would actually work or would be desired features for the relevant tools):

  1. add a mode to clang-apply-replacements to operate on stdin/stdout, so that you can use it for virtual buffers without temporary files
  2. add a tool (or a flag to clang-format) to expose just the cleanupAroundReplacements functionality
  3. add a mode to clang-tidy to format a subset of fixes (but -line-filter wouldn't be enough there, since there may be multiple fixes on the same line)
  4. wrap cleanupAroundReplacements into a native or managed DLL and use it from C# (and yes, it doesn't seem to change frequently)
  5. reimplement cleanupAroundReplacements in C#, as Ilya suggested
  6. use clangd to run clang-tidy checks and apply replacements.

@llvmbot llvmbot transferred this issue from llvm/llvm-bugzilla-archive Dec 10, 2021
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugzilla Issues migrated from bugzilla clang-tidy invalid Resolved as invalid, i.e. not a bug
Projects
None yet
Development

No branches or pull requests

3 participants