[clang-tidy] remove duplicate fixes of alias checkers

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

Reviewed By: aaron.ballman, njames93

Differential Revision: https://reviews.llvm.org/D80753

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

@@ -122,6 +122,8 @@ public:
     {
       auto Level = static_cast<DiagnosticsEngine::Level>(Error.DiagLevel);
       std::string Name = Error.DiagnosticName;
+      if (!Error.EnabledDiagnosticAliases.empty())
+        Name += "," + llvm::join(Error.EnabledDiagnosticAliases, ",");
       if (Error.IsWarningAsError) {
         Name += ",-warnings-as-errors";
         Level = DiagnosticsEngine::Error;

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

@@ -27,6 +27,7 @@
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/Support/Regex.h"
+#include "llvm/Support/FormatVariadic.h"
 #include <tuple>
 #include <vector>
 using namespace clang;
@@ -634,6 +635,8 @@ void ClangTidyDiagnosticConsumer::removeIncompatibleErrors() {
     std::tuple<unsigned, EventType, int, int, unsigned> Priority;
   };
 
+  removeDuplicatedDiagnosticsOfAliasCheckers();
+
   // Compute error sizes.
   std::vector<int> Sizes;
   std::vector<
@@ -728,3 +731,59 @@ std::vector<ClangTidyError> ClangTidyDiagnosticConsumer::take() {
     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::removeDuplicatedDiagnosticsOfAliasCheckers() {
+  using UniqueErrorSet =
+      std::set<ClangTidyError *, LessClangTidyErrorWithoutDiagnosticName>;
+  UniqueErrorSet UniqueErrors;
+
+  auto IT = Errors.begin();
+  while (IT != Errors.end()) {
+    ClangTidyError &Error = *IT;
+    std::pair<UniqueErrorSet::iterator, bool> Inserted =
+        UniqueErrors.insert(&Error);
+
+    // Unique error, we keep it and move along.
+    if (Inserted.second) {
+      ++IT;
+    } else {
+      ClangTidyError &ExistingError = **Inserted.first;
+      const llvm::StringMap<tooling::Replacements> &CandidateFix =
+          Error.Message.Fix;
+      const llvm::StringMap<tooling::Replacements> &ExistingFix =
+          (*Inserted.first)->Message.Fix;
+
+      if (CandidateFix != ExistingFix) {
+
+        // In case of a conflict, don't suggest any fix-it.
+        ExistingError.Message.Fix.clear();
+        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 ('{0}', '{1}') or "
+                          "ensure they are both configured the same",
+                          ExistingError.DiagnosticName, Error.DiagnosticName)
+                .str());
+      }
+
+      if (Error.IsWarningAsError)
+        ExistingError.IsWarningAsError = true;
+
+      // Since it is the same error, we should take it as alias and remove it.
+      ExistingError.EnabledDiagnosticAliases.emplace_back(Error.DiagnosticName);
+      IT = Errors.erase(IT);
+    }
+  }
+}

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

@@ -48,6 +48,7 @@ struct ClangTidyError : tooling::Diagnostic {
                  bool IsWarningAsError);
 
   bool IsWarningAsError;
+  std::vector<std::string> EnabledDiagnosticAliases;
 };
 
 /// Contains displayed and ignored diagnostic counters for a ClangTidy
@@ -246,6 +247,7 @@ public:
 private:
   void finalizeLastError();
   void removeIncompatibleErrors();
+  void removeDuplicatedDiagnosticsOfAliasCheckers();
 
   /// Returns the \c HeaderFilter constructed for the options set in the
   /// context.

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

@@ -0,0 +1,23 @@
+// 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,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 ('cppcoreguidelines-pro-type-member-init', 'hicpp-member-init') or ensure they are both configured the same
+  {
+    _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

@@ -0,0 +1,39 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-pro-type-member-init,hicpp-member-init,modernize-use-emplace,hicpp-use-emplace %t
+
+namespace std {
+
+template <typename T>
+class vector {
+public:
+  void push_back(const T &) {}
+  void push_back(T &&) {}
+
+  template <typename... Args>
+  void emplace_back(Args &&... args){};
+};
+} // namespace std
+
+class Foo {
+public:
+  Foo() : _num1(0)
+  // CHECK-MESSAGES: warning: constructor does not initialize these fields: _num2 [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{};
+};
+
+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,modernize-use-emplace]
+}
+

clang-tools-extra/test/clang-tidy/infrastructure/duplicate-reports.cpp

@@ -2,8 +2,7 @@
 
 void alwaysThrows() {
   int ex = 42;
-  // CHECK-MESSAGES: warning: throw expression should throw anonymous temporary values instead [cert-err09-cpp]
+  // CHECK-MESSAGES: warning: throw expression should throw anonymous temporary values instead [cert-err09-cpp,cert-err61-cpp]
-  // CHECK-MESSAGES: warning: throw expression should throw anonymous temporary values instead [cert-err61-cpp]
   throw ex;
 }
 

llvm/include/llvm/ADT/StringMap.h

@@ -248,6 +248,26 @@ public:
     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;
+  }
+
+  bool operator!=(const StringMap &RHS) const { return !(*this == RHS); }
+
   /// 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.

llvm/unittests/ADT/StringMapTest.cpp

@@ -387,6 +387,70 @@ TEST_F(StringMapTest, MoveAssignment) {
   ASSERT_EQ(B.count("x"), 0u);
 }
 
+TEST_F(StringMapTest, EqualEmpty) {
+  StringMap<int> A;
+  StringMap<int> B;
+  ASSERT_TRUE(A == B);
+  ASSERT_FALSE(A != B);
+  ASSERT_TRUE(A == A); // self check
+}
+
+TEST_F(StringMapTest, EqualWithValues) {
+  StringMap<int> A;
+  A["A"] = 1;
+  A["B"] = 2;
+  A["C"] = 3;
+  A["D"] = 3;
+
+  StringMap<int> B;
+  B["A"] = 1;
+  B["B"] = 2;
+  B["C"] = 3;
+  B["D"] = 3;
+
+  ASSERT_TRUE(A == B);
+  ASSERT_TRUE(B == A);
+  ASSERT_FALSE(A != B);
+  ASSERT_FALSE(B != A);
+  ASSERT_TRUE(A == A); // self check
+}
+
+TEST_F(StringMapTest, NotEqualMissingKeys) {
+  StringMap<int> A;
+  A["A"] = 1;
+  A["B"] = 2;
+
+  StringMap<int> B;
+  B["A"] = 1;
+  B["B"] = 2;
+  B["C"] = 3;
+  B["D"] = 3;
+
+  ASSERT_FALSE(A == B);
+  ASSERT_FALSE(B == A);
+  ASSERT_TRUE(A != B);
+  ASSERT_TRUE(B != A);
+}
+
+TEST_F(StringMapTest, NotEqualWithDifferentValues) {
+  StringMap<int> A;
+  A["A"] = 1;
+  A["B"] = 2;
+  A["C"] = 100;
+  A["D"] = 3;
+
+  StringMap<int> B;
+  B["A"] = 1;
+  B["B"] = 2;
+  B["C"] = 3;
+  B["D"] = 3;
+
+  ASSERT_FALSE(A == B);
+  ASSERT_FALSE(B == A);
+  ASSERT_TRUE(A != B);
+  ASSERT_TRUE(B != A);
+}
+
 struct Countable {
   int &InstanceCount;
   int Number;