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

[igor-akhmetov]

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; };

[llvmbot]

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]

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).

[igor-akhmetov]

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.

[pepsiman]

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

[igor-akhmetov]

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.

[llvmbot]

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]

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.

[pepsiman]

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]

assigned to @pepsiman