[clang-tidy] remove duplicate fixes of alias checkers

[Daniel599]

Summary

when both a check and its alias are enabled, we should only take the fixes of one of them and not both.
This patch fixes bug 45577
https://bugs.llvm.org/show_bug.cgi?id=45577

[Eugene.Zelenko]

clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp

750

Please don't use auto when type is not spelled in same statement or not iterator.

clang-tools-extra/test/clang-tidy/infrastructure/duplicate-fixes-of-alias-checkers.cpp

4

cstdio. Please also separate with newline.

53

Please add newline.

[njames93]

It may be worth verifying that the fix-its are identical too, multiple versions of a check could be running with differing options resulting in different fix-its generated, in that case we should let clang-tidy disable any conflicting fixes for us.
Side note would it not be nicer to just group the diagnostics into one, thinking either of these ways

CHECK-MESSAGES: warning: use emplace_back instead of push_back [hicpp-use-emplace, modernize-use-emplace]
CHECK-MESSAGES: warning: use emplace_back instead of push_back [hicpp-use-emplace] [modernize-use-emplace]

This would result in cleaner diagnostics emitted and remove the need for that note.

clang-tools-extra/test/clang-tidy/infrastructure/duplicate-fixes-of-alias-checkers.cpp

53

New line

[Daniel599]

clang-tools-extra/test/clang-tidy/infrastructure/duplicate-fixes-of-alias-checkers.cpp

4

I wanted to use cstdio but the test fails with 'file not found', don't know if it's specific on my host, anyway I`ll remove this include (and the printf)

[aaron.ballman]

In D80753#2061565, @njames93 wrote:

It may be worth verifying that the fix-its are identical too, multiple versions of a check could be running with differing options resulting in different fix-its generated, in that case we should let clang-tidy disable any conflicting fixes for us.
Side note would it not be nicer to just group the diagnostics into one, thinking either of these ways

CHECK-MESSAGES: warning: use emplace_back instead of push_back [hicpp-use-emplace, modernize-use-emplace]
CHECK-MESSAGES: warning: use emplace_back instead of push_back [hicpp-use-emplace] [modernize-use-emplace]

This would result in cleaner diagnostics emitted and remove the need for that note.

I think this is a nice approach. It also helps the case where a user sees a false positive diagnostic and tries to disable it in their config file. Under the removing duplicates behavior, the user would have a harder time discovering what tests to disable, but under the above approach, they'd have all the information they need up front.

clang-tools-extra/test/clang-tidy/infrastructure/duplicate-fixes-of-alias-checkers.cpp

4

<cstdio> would have been wrong anyway -- clang-tidy (like clang) tests should not include system headers (unless they're faked system header so they're consistent across build bots).

[Daniel599]

In D80753#2062586, @aaron.ballman wrote:

In D80753#2061565, @njames93 wrote:

It may be worth verifying that the fix-its are identical too, multiple versions of a check could be running with differing options resulting in different fix-its generated, in that case we should let clang-tidy disable any conflicting fixes for us.
Side note would it not be nicer to just group the diagnostics into one, thinking either of these ways

CHECK-MESSAGES: warning: use emplace_back instead of push_back [hicpp-use-emplace, modernize-use-emplace]
CHECK-MESSAGES: warning: use emplace_back instead of push_back [hicpp-use-emplace] [modernize-use-emplace]

This would result in cleaner diagnostics emitted and remove the need for that note.

I think this is a nice approach. It also helps the case where a user sees a false positive diagnostic and tries to disable it in their config file. Under the removing duplicates behavior, the user would have a harder time discovering what tests to disable, but under the above approach, they'd have all the information they need up front.

Regarding different fix-its generated:
you are right, in order to fix it, I had to add StringMap::operator == (soon I`ll update the diff), I hope it's OK.
I added the option cppcoreguidelines-pro-type-member-init.UseAssignment to check it, but the result I got was something like:

int _num2 = 0{};

I think that if the fix-its aren't the same, both of them should be disabled with warning about it, what do you think?

Regarding placing all errors of all aliases in one error:
My original goal was to fix the bug that was introduce since clang-tidy-9, what you are suggesting sounds really cool, but I am still new to this code,
Any idea how can I implement such feature? also what should I do if the options of the alias checks aren't the same?

[Daniel599]

I have made the needed changes in order to detect a conflict in duplicated fix-it of aliased checkers and also added a test for it.
Now I`ll try to work on combining aliased errors together, any tips regarding this feature?

[njames93]

In D80753#2064857, @Daniel599 wrote:

I have made the needed changes in order to detect a conflict in duplicated fix-it of aliased checkers and also added a test for it.
Now I`ll try to work on combining aliased errors together, any tips regarding this feature?

Here is a quick draft I built up for this ClangTidyDiagnosticsConsumer.cpp lmk what you think.
Some of the handling probably can be changed for conflicting fix its.

[Daniel599]

Thank you @njames93, I already started and took a bit different approach.
In case of a conflict, leaving it to clang-tidy itself didn't help as it added both of the fix-its together resulting in = 0{};, so I thought it will be better to disable both of them.
Sadly I didn't find 3 aliased checkers which can be configured to produce different code so I can't check this edge case.
Please let me know if another changes are needed for this patch

[njames93]

In D80753#2065067, @Daniel599 wrote:

Thank you @njames93, I already started and took a bit different approach.
In case of a conflict, leaving it to clang-tidy itself didn't help as it added both of the fix-its together resulting in = 0{};, so I thought it will be better to disable both of them.

Fair enough, your solution looks a lot nicer than what I suggested anyway.

Sadly I didn't find 3 aliased checkers which can be configured to produce different code so I can't check this edge case.
Please let me know if another changes are needed for this patch

You could create a unit test for 3 aliased checks with different options, however if there isn't a check that users can use to reproduce that edge case there is no point in worrying about it.

clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp

765–778

s/(*Inserted.first)->/ExistingError.

779

ExistingError.Notes.emplace_back(llvm::formatv(
    "cannot apply fix-it because an alias checker has "
    "suggested a different fix-it, please remove one of the checkers "
    "or make sure they are both configured the same. "
    "Aliased checkers: '{0}', '{1}'",
    ExistingError.DiagnosticName, Error.DiagnosticName).str());

clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h

51

Just ignore this, but do you ever get so bored and feel the need to save 24 bytes... https://gist.github.com/njames93/aac662ca52d345245e5e6f5dbc47d484 :)

[Daniel599]

clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h

51

That's a really cool trick, good to know :)

[Daniel599]

clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h

249

maybe I need to rename this method since now it's removing the errors also, I`ll do it when I get back from work.

[njames93]

clang-tools-extra/test/clang-tidy/infrastructure/duplicate-fixes-of-alias-checkers.cpp

5–9

This isn't needed for the test case and can safely be removed.

14–22

The 2 constructors can be removed as well as the destructor.

llvm/include/llvm/ADT/StringMap.h

251–268

This needs unit tests llvm/unittests/ADT/StringMapTest.cpp, also a small nit but could you add the corresponding operator!=

[Daniel599]

Added StringMap::operator!= and also unit tests

[njames93]

clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp

776

You don't really need the Aliased checkers note as that is wrapped in the initial diagnostic message.
Also if there was a check that could spew out 3 different fix-its for the same diagnostic, this would result in the duplication note being made twice, maybe the notes should be cleared too?

[Daniel599]

clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp

776

regarding your comment about 3 fix-it, as we walked before, there isn't a case like that (I didn't find any) as I wanted to make a test out of it.
I added the last line as it would show who are the two that conflict (supporting the future case of more than 2 aliased checkers),
I can remove it if it doesn't help, let me know.

maybe the notes should be cleared too?

I am open for suggestions about how to re-write this message :)
I also think it could be better

[Daniel599]

Any additional changes/fixes regarding this patch?
Thanks.

[Daniel599]

ping?

[aaron.ballman]

LGTM aside from a few small nits, but please wait a bit for @njames93 in case they have final thoughts.

clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp

757

Add full stops to the end of your comments (here and elsewhere).

776

I would reword it to be a run-on sentence: cannot apply fix-it because an alias checker has suggested a different fix-it; please remove one of the checkers ('{0}', '{1}') or ensure they are both configured the same

This also nicely removes the nit about terminating punctuation in a diagnostic.

clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h

249

I think this should still be renamed. How about removeDuplicatedDiagnosticsOfAliasCheckers()?

[njames93]

LG, I have nothing else to add here.

[Daniel599]

I fixed all as your comments suggested, thanks :)
If this patch is ready, can someone commit it on my behalf please? (I don't have write permissions)
Thank you.

clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h

 /// dependency on a SourceManager.
 ///
 /// FIXME: Make Diagnostics flexible enough to support this directly.
 struct ClangTidyError : tooling::Diagnostic {
   ClangTidyError(StringRef CheckName, Level DiagLevel, StringRef BuildDirectory,
                  bool IsWarningAsError);
 
   bool IsWarningAsError;
 };
 
 /// Contains displayed and ignored diagnostic counters for a ClangTidy
 /// run.
 struct ClangTidyStats {
   ClangTidyStats()
       : ErrorsDisplayed(0), ErrorsIgnoredCheckFilter(0), ErrorsIgnoredNOLINT(0),
         ErrorsIgnoredNonUserCode(0), ErrorsIgnoredLineFilter(0) {}
 
void HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
                         const Diagnostic &Info) override;
 
   // Retrieve the diagnostics that were captured.
   std::vector<ClangTidyError> take();
 
 private:
   void finalizeLastError();
   void removeIncompatibleErrors();
+  void removeDuplicatedFixesOfAliasCheckers();
 
   /// Returns the \c HeaderFilter constructed for the options set in the
   /// context.
   llvm::Regex *getHeaderFilter();
 
   /// Updates \c LastErrorRelatesToUserCode and LastErrorPassesLineFilter
   /// according to the diagnostic \p Location.
   void checkFilters(SourceLocation Location, const SourceManager &Sources);

clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp

struct Event {
     EventType Type;
     // The index of the error to which the interval that generated this event
     // belongs.
     unsigned ErrorId;
     // The events will be sorted based on this field.
     std::tuple<unsigned, EventType, int, int, unsigned> Priority;
   };
 
+  removeDuplicatedFixesOfAliasCheckers();
+
   // Compute error sizes.
   std::vector<int> Sizes;
   std::vector<
       std::pair<ClangTidyError *, llvm::StringMap<tooling::Replacements> *>>
       ErrorFixes;
   for (auto &Error : Errors) {
     if (const auto *Fix = tooling::selectFirstFix(Error))
       ErrorFixes.emplace_back(
std::vector<ClangTidyError> ClangTidyDiagnosticConsumer::take() {
 
   llvm::sort(Errors, LessClangTidyError());
   Errors.erase(std::unique(Errors.begin(), Errors.end(), EqualClangTidyError()),
                Errors.end());
   if (RemoveIncompatibleErrors)
     removeIncompatibleErrors();
   return std::move(Errors);
 }
+
+namespace {
+struct LessClangTidyErrorWithoutDiagnosticName {
+  bool operator()(const ClangTidyError *LHS, const ClangTidyError *RHS) const {
+    const tooling::DiagnosticMessage &M1 = LHS->Message;
+    const tooling::DiagnosticMessage &M2 = RHS->Message;
+
+    return std::tie(M1.FilePath, M1.FileOffset, M1.Message) <
+           std::tie(M2.FilePath, M2.FileOffset, M2.Message);
+  }
+};
+} // end anonymous namespace
+
+void ClangTidyDiagnosticConsumer::removeDuplicatedFixesOfAliasCheckers() {
+  using UniqueErrorSet =
+      std::set<ClangTidyError *, LessClangTidyErrorWithoutDiagnosticName>;
+  UniqueErrorSet UniqueErrors;
+
+  for (auto &Error : Errors) {
+    std::pair<UniqueErrorSet::iterator, bool> Inserted =
+        UniqueErrors.insert(&Error);
+
+    // If we already have an error like this, just with the different
+    // DiagnosticName
+    if (!Inserted.second) {
+      const llvm::StringMap<tooling::Replacements> &CandidateFix =
+          Error.Message.Fix;
+      const llvm::StringMap<tooling::Replacements> &CurrentFix =
+          (*Inserted.first)->Message.Fix;
+
+      if (!CandidateFix.empty() && !CurrentFix.empty()) {
+
+        // In case both of the checkers provide the exact same fix
+        // Remove its Fix since we don't need same fix twice
+        if (CandidateFix == CurrentFix) {
+          Error.Message.Fix.clear();
+          std::string FixAlreadyExists =
+              "this fix will not be applied because an alias checker has "
+              "already provided it, see '" +
+              (*Inserted.first)->DiagnosticName + "'";
+          Error.Notes.emplace_back(std::move(FixAlreadyExists));
+        } else {
+          std::string AliasedCheckerFixConflict =
+              "cannot apply fix-it because an alias checker has "
+              "suggested a different fix-it, please remove one of the checkers "
+              "or make sure they are both configured the same. "
+              "Aliased checkers: '" +
+              (*Inserted.first)->DiagnosticName + "', '" +
+              Error.DiagnosticName + "'";
+
+          Error.Message.Fix.clear();
+          Error.Notes.emplace_back(AliasedCheckerFixConflict);
+          (*Inserted.first)->Message.Fix.clear();
+          (*Inserted.first)
+              ->Notes.emplace_back(std::move(AliasedCheckerFixConflict));
+        }
+      }
+    }
+  }
+}

clang-tools-extra/test/clang-tidy/infrastructure/duplicate-conflicted-fixes-of-alias-checkers.cpp

+// RUN: %check_clang_tidy %s cppcoreguidelines-pro-type-member-init,hicpp-member-init,modernize-use-emplace,hicpp-use-emplace %t -- \
+//// RUN:     -config='{CheckOptions: [ \
+//// RUN:         {key: cppcoreguidelines-pro-type-member-init.UseAssignment, value: 1}, \
+//// RUN:     ]}'
+
+class Foo {
+public:
+  Foo() : _num1(0)
+  // CHECK-MESSAGES: warning: constructor does not initialize these fields: _num2 [cppcoreguidelines-pro-type-member-init]
+  // CHECK-MESSAGES: note: cannot apply fix-it because an alias checker has suggested a different fix-it, please remove one of the checkers or make sure they are both configured the same. Aliased checkers: 'cppcoreguidelines-pro-type-member-init', 'hicpp-member-init'
+  // CHECK-MESSAGES: warning: constructor does not initialize these fields: _num2 [hicpp-member-init]
+  // CHECK-MESSAGES: note: cannot apply fix-it because an alias checker has suggested a different fix-it, please remove one of the checkers or make sure they are both configured the same. Aliased checkers: 'cppcoreguidelines-pro-type-member-init', 'hicpp-member-init'
+  {
+    _num1 = 10;
+  }
+
+  int use_the_members() const {
+    return _num1 + _num2;
+  }
+
+private:
+  int _num1;
+  int _num2;
+  // CHECK-FIXES: _num2;
+};

clang-tools-extra/test/clang-tidy/infrastructure/duplicate-fixes-of-alias-checkers.cpp

+// RUN: %check_clang_tidy %s cppcoreguidelines-pro-type-member-init,hicpp-member-init,modernize-use-emplace,hicpp-use-emplace %t
+
+namespace std {
+template <typename>
+class initializer_list {
+public:
+  initializer_list() noexcept {}
+};
+
+template <typename T>
+class vector {
+public:
+  vector() = default;
+  vector(initializer_list<T>) {}
+
+  void push_back(const T &) {}
+  void push_back(T &&) {}
+
+  template <typename... Args>
+  void emplace_back(Args &&... args){};
+  ~vector();
+};
+} // namespace std
+
+class Foo {
+public:
+  Foo() : _num1(0)
+  // CHECK-MESSAGES: warning: constructor does not initialize these fields: _num2 [cppcoreguidelines-pro-type-member-init]
+  // CHECK-MESSAGES: warning: constructor does not initialize these fields: _num2 [hicpp-member-init]
+  // CHECK-MESSAGES: note: this fix will not be applied because an alias checker has already provided it, see 'cppcoreguidelines-pro-type-member-init'
+  {
+    _num1 = 10;
+  }
+
+  int use_the_members() const {
+    return _num1 + _num2;
+  }
+
+private:
+  int _num1;
+  int _num2;
+  // CHECK-FIXES: _num2{};
+};
+
+int should_use_emplace(std::vector<Foo> &v) {
+  v.push_back(Foo());
+  // CHECK-FIXES: v.emplace_back();
+  // CHECK-MESSAGES: warning: use emplace_back instead of push_back [hicpp-use-emplace]
+  // CHECK-MESSAGES: warning: use emplace_back instead of push_back [modernize-use-emplace]
+  // CHECK-MESSAGES: note: this fix will not be applied because an alias checker has already provided it, see 'hicpp-use-emplace'
+}
+

llvm/include/llvm/ADT/StringMap.h

public:
   /// count - Return 1 if the element is in the map, 0 otherwise.
   size_type count(StringRef Key) const { return find(Key) == end() ? 0 : 1; }
 
   template <typename InputTy>
   size_type count(const StringMapEntry<InputTy> &MapEntry) const {
     return count(MapEntry.getKey());
   }
 
+  /// equal - check whether both of the containers are equal
+  bool operator==(const StringMap &RHS) const {
+    if (size() != RHS.size())
+      return false;
+
+    for (const auto &KeyValue : *this) {
+      auto FindInRHS = RHS.find(KeyValue.getKey());
+
+      if (FindInRHS == RHS.end())
+        return false;
+
+      if (!(KeyValue.getValue() == FindInRHS->getValue()))
+        return false;
+    }
+
+    return true;
+  }
+
   /// insert - Insert the specified key/value pair into the map.  If the key
   /// already exists in the map, return false and ignore the request, otherwise
   /// insert it and return true.
   bool insert(MapEntryTy *KeyValue) {
     unsigned BucketNo = LookupBucketFor(KeyValue->getKey());
     StringMapEntryBase *&Bucket = TheTable[BucketNo];
     if (Bucket && Bucket != getTombstoneVal())
       return false; // Already exists in map.