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

[Quuxplusone]

Bugzilla Link	PR43583
Status	RESOLVED INVALID
Importance	P normal
Reported by	Igor Akhmetov (igor.akhmetov@gmail.com)
Reported on	2019-10-07 02:07:42 -0700
Last modified on	2019-10-10 08:05:48 -0700
Version	unspecified
Hardware	PC Windows NT
CC	alexfh@google.com, djasper@google.com, eugene.zelenko@gmail.com, ibiryukov@google.com, klimek@google.com, malcolm.parsons@gmail.com
Fixed by commit(s)
Attachments
Blocks
Blocked by
See also

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

[Quuxplusone]

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 <string>

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 <string>

class Foo {
public:
  Foo()  {}

private:
  std::string s;
};

Which tool are you using to apply the fixes?

[Quuxplusone]

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.

[Quuxplusone]

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.

[Quuxplusone]

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.

[Quuxplusone]

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

[Quuxplusone]

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.

[Quuxplusone]

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

[Quuxplusone]

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.