[ASTImporter] Fix name conflict handling with different strategies

There are numorous flaws about the name conflict handling, this patch
attempts fixes them. Changes in details:

* HandleNameConflict return with a false DeclarationName

Hitherto we effectively never returned with a NameConflict error, even
if the preceding StructuralMatch indicated a conflict.
Because we just simply returned with the parameter `Name` in
HandleNameConflict and that name is almost always `true` when converted to
`bool`.

* Add tests which indicate wrong NameConflict handling

* Add to ConflictingDecls only if decl kind is different

Note, we might not indicate an ODR error when there is an existing record decl
and a enum is imported with same name.  But there are other cases. E.g. think
about the case when we import a FunctionTemplateDecl with name f and we found a
simple FunctionDecl with name f. They overload.  Or in case of a
ClassTemplateDecl and CXXRecordDecl, the CXXRecordDecl could be the 'templated'
class, so it would be false to report error.  So I think we should report a
name conflict error only when we are 100% sure of that.  That is why I think it
should be a general pattern to report the error only if the kind is the same.

* Fix failing ctu test with EnumConstandDecl

In ctu-main.c we have the enum class 'A' which brings in the enum
constant 'x' with value 0 into the global namespace.
In ctu-other.c we had the enum class 'B' which brought in the same name
('x') as an enum constant but with a different enum value (42). This is clearly
an ODR violation in the global namespace. The solution was to rename the
second enum constant.

 * Introduce ODR handling strategies

Reviewers: a_sidorin, shafik

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

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@370045 91177308-0d34-0410-b5e6-96231b3b80d8

include/clang/AST/ASTImporter.h

@@ -90,6 +90,8 @@ class TypeSourceInfo;
     using FileIDImportHandlerType =
         std::function<void(FileID /*ToID*/, FileID /*FromID*/)>;
 
+    enum class ODRHandlingType { Conservative, Liberal };
+
     // An ImportPath is the list of the AST nodes which we visit during an
     // Import call.
     // If node `A` depends on node `B` then the path contains an `A`->`B` edge.
@@ -236,6 +238,8 @@ class TypeSourceInfo;
     /// Whether to perform a minimal import.
     bool Minimal;
 
+    ODRHandlingType ODRHandling;
+
     /// Whether the last diagnostic came from the "from" context.
     bool LastDiagFromFrom = false;
 
@@ -326,6 +330,8 @@ class TypeSourceInfo;
     /// to-be-completed forward declarations when possible.
     bool isMinimalImport() const { return Minimal; }
 
+    void setODRHandling(ODRHandlingType T) { ODRHandling = T; }
+
     /// \brief Import the given object, returns the result.
     ///
     /// \param To Import the object into this variable.
@@ -517,12 +523,11 @@ class TypeSourceInfo;
     ///
     /// \param NumDecls the number of conflicting declarations in \p Decls.
     ///
-    /// \returns the name that the newly-imported declaration should have.
-    virtual DeclarationName HandleNameConflict(DeclarationName Name,
-                                               DeclContext *DC,
-                                               unsigned IDNS,
-                                               NamedDecl **Decls,
-                                               unsigned NumDecls);
+    /// \returns the name that the newly-imported declaration should have. Or
+    /// an error if we can't handle the name conflict.
+    virtual Expected<DeclarationName>
+    HandleNameConflict(DeclarationName Name, DeclContext *DC, unsigned IDNS,
+                       NamedDecl **Decls, unsigned NumDecls);
 
     /// Retrieve the context that AST nodes are being imported into.
     ASTContext &getToContext() const { return ToContext; }

lib/AST/ASTImporter.cpp

@@ -80,6 +80,7 @@ namespace clang {
   using ExpectedExpr = llvm::Expected<Expr *>;
   using ExpectedDecl = llvm::Expected<Decl *>;
   using ExpectedSLoc = llvm::Expected<SourceLocation>;
+  using ExpectedName = llvm::Expected<DeclarationName>;
 
   std::string ImportError::toString() const {
     // FIXME: Improve error texts.
@@ -2247,11 +2248,13 @@ ExpectedDecl ASTNodeImporter::VisitNamespaceDecl(NamespaceDecl *D) {
     }
 
     if (!ConflictingDecls.empty()) {
-      Name = Importer.HandleNameConflict(Name, DC, Decl::IDNS_Namespace,
-                                         ConflictingDecls.data(),
-                                         ConflictingDecls.size());
-      if (!Name)
-        return make_error<ImportError>(ImportError::NameConflict);
+      ExpectedName NameOrErr = Importer.HandleNameConflict(
+          Name, DC, Decl::IDNS_Namespace, ConflictingDecls.data(),
+          ConflictingDecls.size());
+      if (NameOrErr)
+        Name = NameOrErr.get();
+      else
+        return NameOrErr.takeError();
     }
   }
 
@@ -2355,21 +2358,21 @@ ASTNodeImporter::VisitTypedefNameDecl(TypedefNameDecl *D, bool IsAlias) {
           // already have a complete underlying type then return with that.
           if (!FromUT->isIncompleteType() && !FoundUT->isIncompleteType())
             return Importer.MapImported(D, FoundTypedef);
+          // FIXME Handle redecl chain. When you do that make consistent changes
+          // in ASTImporterLookupTable too.
+        } else {
+          ConflictingDecls.push_back(FoundDecl);
         }
-        // FIXME Handle redecl chain. When you do that make consistent changes
-        // in ASTImporterLookupTable too.
-        break;
       }
-
-      ConflictingDecls.push_back(FoundDecl);
     }
 
     if (!ConflictingDecls.empty()) {
-      Name = Importer.HandleNameConflict(Name, DC, IDNS,
-                                         ConflictingDecls.data(),
-                                         ConflictingDecls.size());
-      if (!Name)
-        return make_error<ImportError>(ImportError::NameConflict);
+      ExpectedName NameOrErr = Importer.HandleNameConflict(
+          Name, DC, IDNS, ConflictingDecls.data(), ConflictingDecls.size());
+      if (NameOrErr)
+        Name = NameOrErr.get();
+      else
+        return NameOrErr.takeError();
     }
   }
 
@@ -2442,11 +2445,12 @@ ASTNodeImporter::VisitTypeAliasTemplateDecl(TypeAliasTemplateDecl *D) {
     }
 
     if (!ConflictingDecls.empty()) {
-      Name = Importer.HandleNameConflict(Name, DC, IDNS,
-                                         ConflictingDecls.data(),
-                                         ConflictingDecls.size());
-      if (!Name)
-        return make_error<ImportError>(ImportError::NameConflict);
+      ExpectedName NameOrErr = Importer.HandleNameConflict(
+          Name, DC, IDNS, ConflictingDecls.data(), ConflictingDecls.size());
+      if (NameOrErr)
+        Name = NameOrErr.get();
+      else
+        return NameOrErr.takeError();
     }
   }
 
@@ -2550,17 +2554,18 @@ ExpectedDecl ASTNodeImporter::VisitEnumDecl(EnumDecl *D) {
           continue;
         if (IsStructuralMatch(D, FoundEnum))
           return Importer.MapImported(D, FoundEnum);
+        ConflictingDecls.push_back(FoundDecl);
       }
-
-      ConflictingDecls.push_back(FoundDecl);
     }
 
     if (!ConflictingDecls.empty()) {
-      Name = Importer.HandleNameConflict(SearchName, DC, IDNS,
-                                         ConflictingDecls.data(),
-                                         ConflictingDecls.size());
-      if (!Name)
-        return make_error<ImportError>(ImportError::NameConflict);
+      ExpectedName NameOrErr = Importer.HandleNameConflict(
+          SearchName, DC, IDNS, ConflictingDecls.data(),
+          ConflictingDecls.size());
+      if (NameOrErr)
+        Name = NameOrErr.get();
+      else
+        return NameOrErr.takeError();
     }
   }
 
@@ -2685,17 +2690,18 @@ ExpectedDecl ASTNodeImporter::VisitRecordDecl(RecordDecl *D) {
           PrevDecl = FoundRecord->getMostRecentDecl();
           break;
         }
-      }
-
-      ConflictingDecls.push_back(FoundDecl);
+        ConflictingDecls.push_back(FoundDecl);
+      } // kind is RecordDecl
     } // for
 
     if (!ConflictingDecls.empty() && SearchName) {
-      Name = Importer.HandleNameConflict(SearchName, DC, IDNS,
-                                         ConflictingDecls.data(),
-                                         ConflictingDecls.size());
-      if (!Name)
-        return make_error<ImportError>(ImportError::NameConflict);
+      ExpectedName NameOrErr = Importer.HandleNameConflict(
+          SearchName, DC, IDNS, ConflictingDecls.data(),
+          ConflictingDecls.size());
+      if (NameOrErr)
+        Name = NameOrErr.get();
+      else
+        return NameOrErr.takeError();
     }
   }
 
@@ -2854,17 +2860,17 @@ ExpectedDecl ASTNodeImporter::VisitEnumConstantDecl(EnumConstantDecl *D) {
       if (auto *FoundEnumConstant = dyn_cast<EnumConstantDecl>(FoundDecl)) {
         if (IsStructuralMatch(D, FoundEnumConstant))
           return Importer.MapImported(D, FoundEnumConstant);
+        ConflictingDecls.push_back(FoundDecl);
       }
-
-      ConflictingDecls.push_back(FoundDecl);
     }
 
     if (!ConflictingDecls.empty()) {
-      Name = Importer.HandleNameConflict(Name, DC, IDNS,
-                                         ConflictingDecls.data(),
-                                         ConflictingDecls.size());
-      if (!Name)
-        return make_error<ImportError>(ImportError::NameConflict);
+      ExpectedName NameOrErr = Importer.HandleNameConflict(
+          Name, DC, IDNS, ConflictingDecls.data(), ConflictingDecls.size());
+      if (NameOrErr)
+        Name = NameOrErr.get();
+      else
+        return NameOrErr.takeError();
     }
   }
 
@@ -3102,17 +3108,17 @@ ExpectedDecl ASTNodeImporter::VisitFunctionDecl(FunctionDecl *D) {
             << Name << D->getType() << FoundFunction->getType();
         Importer.ToDiag(FoundFunction->getLocation(), diag::note_odr_value_here)
             << FoundFunction->getType();
+        ConflictingDecls.push_back(FoundDecl);
       }
-
-      ConflictingDecls.push_back(FoundDecl);
     }
 
     if (!ConflictingDecls.empty()) {
-      Name = Importer.HandleNameConflict(Name, DC, IDNS,
-                                         ConflictingDecls.data(),
-                                         ConflictingDecls.size());
-      if (!Name)
-        return make_error<ImportError>(ImportError::NameConflict);
+      ExpectedName NameOrErr = Importer.HandleNameConflict(
+          Name, DC, IDNS, ConflictingDecls.data(), ConflictingDecls.size());
+      if (NameOrErr)
+        Name = NameOrErr.get();
+      else
+        return NameOrErr.takeError();
     }
   }
 
@@ -3758,17 +3764,17 @@ ExpectedDecl ASTNodeImporter::VisitVarDecl(VarDecl *D) {
           << Name << D->getType() << FoundVar->getType();
         Importer.ToDiag(FoundVar->getLocation(), diag::note_odr_value_here)
           << FoundVar->getType();
+        ConflictingDecls.push_back(FoundDecl);
       }
-
-      ConflictingDecls.push_back(FoundDecl);
     }
 
     if (!ConflictingDecls.empty()) {
-      Name = Importer.HandleNameConflict(Name, DC, IDNS,
-                                         ConflictingDecls.data(),
-                                         ConflictingDecls.size());
-      if (!Name)
-        return make_error<ImportError>(ImportError::NameConflict);
+      ExpectedName NameOrErr = Importer.HandleNameConflict(
+          Name, DC, IDNS, ConflictingDecls.data(), ConflictingDecls.size());
+      if (NameOrErr)
+        Name = NameOrErr.get();
+      else
+        return NameOrErr.takeError();
     }
   }
 
@@ -5106,19 +5112,19 @@ ExpectedDecl ASTNodeImporter::VisitClassTemplateDecl(ClassTemplateDecl *D) {
           // see ASTTests test ImportExistingFriendClassTemplateDef.
           continue;
         }
+        ConflictingDecls.push_back(FoundDecl);
       }
-
-      ConflictingDecls.push_back(FoundDecl);
     }
 
     if (!ConflictingDecls.empty()) {
-      Name = Importer.HandleNameConflict(Name, DC, Decl::IDNS_Ordinary,
-                                         ConflictingDecls.data(),
-                                         ConflictingDecls.size());
+      ExpectedName NameOrErr = Importer.HandleNameConflict(
+          Name, DC, Decl::IDNS_Ordinary, ConflictingDecls.data(),
+          ConflictingDecls.size());
+      if (NameOrErr)
+        Name = NameOrErr.get();
+      else
+        return NameOrErr.takeError();
     }
-
-    if (!Name)
-      return make_error<ImportError>(ImportError::NameConflict);
   }
 
   CXXRecordDecl *FromTemplated = D->getTemplatedDecl();
@@ -5391,22 +5397,20 @@ ExpectedDecl ASTNodeImporter::VisitVarTemplateDecl(VarTemplateDecl *D) {
                              FoundTemplate->getTemplatedDecl());
         return Importer.MapImported(D, FoundTemplate);
       }
+      ConflictingDecls.push_back(FoundDecl);
     }
-
-    ConflictingDecls.push_back(FoundDecl);
   }
 
   if (!ConflictingDecls.empty()) {
-    Name = Importer.HandleNameConflict(Name, DC, Decl::IDNS_Ordinary,
-                                       ConflictingDecls.data(),
-                                       ConflictingDecls.size());
+    ExpectedName NameOrErr = Importer.HandleNameConflict(
+        Name, DC, Decl::IDNS_Ordinary, ConflictingDecls.data(),
+        ConflictingDecls.size());
+    if (NameOrErr)
+      Name = NameOrErr.get();
+    else
+      return NameOrErr.takeError();
   }
 
-  if (!Name)
-    // FIXME: Is it possible to get other error than name conflict?
-    // (Put this `if` into the previous `if`?)
-    return make_error<ImportError>(ImportError::NameConflict);
-
   VarDecl *DTemplated = D->getTemplatedDecl();
 
   // Import the type.
@@ -7817,7 +7821,7 @@ ASTImporter::ASTImporter(ASTContext &ToContext, FileManager &ToFileManager,
                          std::shared_ptr<ASTImporterSharedState> SharedState)
     : SharedState(SharedState), ToContext(ToContext), FromContext(FromContext),
       ToFileManager(ToFileManager), FromFileManager(FromFileManager),
-      Minimal(MinimalImport) {
+      Minimal(MinimalImport), ODRHandling(ODRHandlingType::Conservative) {
 
   // Create a default state without the lookup table: LLDB case.
   if (!SharedState) {
@@ -8744,12 +8748,17 @@ Expected<Selector> ASTImporter::Import(Selector FromSel) {
   return ToContext.Selectors.getSelector(FromSel.getNumArgs(), Idents.data());
 }
 
-DeclarationName ASTImporter::HandleNameConflict(DeclarationName Name,
-                                                DeclContext *DC,
-                                                unsigned IDNS,
-                                                NamedDecl **Decls,
-                                                unsigned NumDecls) {
-  return Name;
+Expected<DeclarationName> ASTImporter::HandleNameConflict(DeclarationName Name,
+                                                          DeclContext *DC,
+                                                          unsigned IDNS,
+                                                          NamedDecl **Decls,
+                                                          unsigned NumDecls) {
+  if (ODRHandling == ODRHandlingType::Conservative)
+    // Report error at any name conflict.
+    return make_error<ImportError>(ImportError::NameConflict);
+  else
+    // Allow to create the new Decl with the same name.
+    return Name;
 }
 
 DiagnosticBuilder ASTImporter::ToDiag(SourceLocation Loc, unsigned DiagID) {

test/Analysis/Inputs/ctu-other.c

@@ -12,11 +12,11 @@ int f(int i) {
 }
 
 // Test enums.
-enum B { x = 42,
-         l,
-         s };
+enum B { x2 = 42,
+         y2,
+         z2 };
 int enumCheck(void) {
-  return x;
+  return x2;
 }
 
 // Test reporting an error in macro definition

unittests/AST/ASTImporterFixtures.cpp

@@ -39,10 +39,12 @@ void createVirtualFileIfNeeded(ASTUnit *ToAST, StringRef FileName,
 }
 
 ASTImporterTestBase::TU::TU(StringRef Code, StringRef FileName, ArgVector Args,
-                            ImporterConstructor C)
+                            ImporterConstructor C,
+                            ASTImporter::ODRHandlingType ODRHandling)
     : Code(Code), FileName(FileName),
       Unit(tooling::buildASTFromCodeWithArgs(this->Code, Args, this->FileName)),
-      TUDecl(Unit->getASTContext().getTranslationUnitDecl()), Creator(C) {
+      TUDecl(Unit->getASTContext().getTranslationUnitDecl()), Creator(C),
+      ODRHandling(ODRHandling) {
   Unit->enableSourceFileDiagnostics();
 
   // If the test doesn't need a specific ASTImporter, we just create a
@@ -63,10 +65,12 @@ void ASTImporterTestBase::TU::lazyInitImporter(
     const std::shared_ptr<ASTImporterSharedState> &SharedState,
     ASTUnit *ToAST) {
   assert(ToAST);
-  if (!Importer)
+  if (!Importer) {
     Importer.reset(Creator(ToAST->getASTContext(), ToAST->getFileManager(),
                            Unit->getASTContext(), Unit->getFileManager(), false,
                            SharedState));
+    Importer->setODRHandling(ODRHandling);
+  }
   assert(&ToAST->getASTContext() == &Importer->getToContext());
   createVirtualFileIfNeeded(ToAST, FileName, Code);
 }
@@ -83,6 +87,13 @@ Decl *ASTImporterTestBase::TU::import(
   }
 }
 
+llvm::Expected<Decl *> ASTImporterTestBase::TU::importOrError(
+    const std::shared_ptr<ASTImporterSharedState> &SharedState, ASTUnit *ToAST,
+    Decl *FromDecl) {
+  lazyInitImporter(SharedState, ToAST);
+  return Importer->Import(FromDecl);
+}
+
 QualType ASTImporterTestBase::TU::import(
     const std::shared_ptr<ASTImporterSharedState> &SharedState, ASTUnit *ToAST,
     QualType FromType) {
@@ -132,7 +143,8 @@ ASTImporterTestBase::getImportedDecl(StringRef FromSrcCode, Language FromLang,
   ArgVector FromArgs = getArgVectorForLanguage(FromLang),
             ToArgs = getArgVectorForLanguage(ToLang);
 
-  FromTUs.emplace_back(FromSrcCode, InputFileName, FromArgs, Creator);
+  FromTUs.emplace_back(FromSrcCode, InputFileName, FromArgs, Creator,
+                       ODRHandling);
   TU &FromTU = FromTUs.back();
 
   assert(!ToAST);
@@ -165,7 +177,7 @@ TranslationUnitDecl *ASTImporterTestBase::getTuDecl(StringRef SrcCode,
          }) == FromTUs.end());
 
   ArgVector Args = getArgVectorForLanguage(Lang);
-  FromTUs.emplace_back(SrcCode, FileName, Args, Creator);
+  FromTUs.emplace_back(SrcCode, FileName, Args, Creator, ODRHandling);
   TU &Tu = FromTUs.back();
 
   return Tu.TUDecl;
@@ -187,6 +199,16 @@ Decl *ASTImporterTestBase::Import(Decl *From, Language ToLang) {
   return To;
 }
 
+llvm::Expected<Decl *> ASTImporterTestBase::importOrError(Decl *From,
+                                                          Language ToLang) {
+  lazyInitToAST(ToLang, "", OutputFileName);
+  TU *FromTU = findFromTU(From);
+  assert(SharedStatePtr);
+  llvm::Expected<Decl *> To =
+      FromTU->importOrError(SharedStatePtr, ToAST.get(), From);
+  return To;
+}
+
 QualType ASTImporterTestBase::ImportType(QualType FromType, Decl *TUDecl,
                                          Language ToLang) {
   lazyInitToAST(ToLang, "", OutputFileName);

unittests/AST/ASTImporterFixtures.h

@@ -17,12 +17,16 @@
 #include "gmock/gmock.h"
 
 #include "clang/AST/ASTImporter.h"
-#include "clang/Frontend/ASTUnit.h"
 #include "clang/AST/ASTImporterSharedState.h"
+#include "clang/Frontend/ASTUnit.h"
+#include "llvm/Support/Error.h"
+#include "llvm/Support/ErrorHandling.h"
 
 #include "DeclMatcher.h"
 #include "Language.h"
 
+#include <sstream>
+
 namespace clang {
 
 class ASTImporter;
@@ -82,6 +86,9 @@ public:
       const std::shared_ptr<ASTImporterSharedState> &SharedState)>
       ImporterConstructor;
 
+  // ODR handling type for the AST importer.
+  ASTImporter::ODRHandlingType ODRHandling;
+
   // The lambda that constructs the ASTImporter we use in this test.
   ImporterConstructor Creator;
 
@@ -99,9 +106,12 @@ private:
     TranslationUnitDecl *TUDecl = nullptr;
     std::unique_ptr<ASTImporter> Importer;
     ImporterConstructor Creator;
+    ASTImporter::ODRHandlingType ODRHandling;
 
     TU(StringRef Code, StringRef FileName, ArgVector Args,
-       ImporterConstructor C = ImporterConstructor());
+       ImporterConstructor C = ImporterConstructor(),
+       ASTImporter::ODRHandlingType ODRHandling =
+           ASTImporter::ODRHandlingType::Conservative);
     ~TU();
 
     void
@@ -109,6 +119,9 @@ private:
                      ASTUnit *ToAST);
     Decl *import(const std::shared_ptr<ASTImporterSharedState> &SharedState,
                  ASTUnit *ToAST, Decl *FromDecl);
+    llvm::Expected<Decl *>
+    importOrError(const std::shared_ptr<ASTImporterSharedState> &SharedState,
+                  ASTUnit *ToAST, Decl *FromDecl);
     QualType import(const std::shared_ptr<ASTImporterSharedState> &SharedState,
                     ASTUnit *ToAST, QualType FromType);
   };
@@ -162,8 +175,14 @@ public:
     return cast_or_null<DeclT>(Import(cast<Decl>(From), Lang));
   }
 
+  // Import the given Decl into the ToCtx.
+  // Same as Import but returns the result of the import which can be an error.
+  llvm::Expected<Decl *> importOrError(Decl *From, Language ToLang);
+
   QualType ImportType(QualType FromType, Decl *TUDecl, Language ToLang);
 
+  ASTImporterTestBase()
+      : ODRHandling(ASTImporter::ODRHandlingType::Conservative) {}
   ~ASTImporterTestBase();
 };
 
@@ -174,6 +193,46 @@ protected:
   ArgVector getExtraArgs() const override { return GetParam(); }
 };
 
+template <class T>
+::testing::AssertionResult isSuccess(llvm::Expected<T> &ValOrErr) {
+  if (ValOrErr)
+    return ::testing::AssertionSuccess() << "Expected<> contains no error.";
+  else
+    return ::testing::AssertionFailure()
+           << "Expected<> contains error: " << toString(ValOrErr.takeError());
+}
+
+template <class T>
+::testing::AssertionResult isImportError(llvm::Expected<T> &ValOrErr,
+                                         ImportError::ErrorKind Kind) {
+  if (ValOrErr) {
+    return ::testing::AssertionFailure() << "Expected<> is expected to contain "
+                                            "error but does contain value \""
+                                         << (*ValOrErr) << "\"";
+  } else {
+    std::ostringstream OS;
+    bool Result = false;
+    auto Err = llvm::handleErrors(
+        ValOrErr.takeError(), [&OS, &Result, Kind](clang::ImportError &IE) {
+          if (IE.Error == Kind) {
+            Result = true;
+            OS << "Expected<> contains an ImportError " << IE.toString();
+          } else {
+            OS << "Expected<> contains an ImportError " << IE.toString()
+               << " instead of kind " << Kind;
+          }
+        });
+    if (Err) {
+      OS << "Expected<> contains unexpected error: "
+         << toString(std::move(Err));
+    }
+    if (Result)
+      return ::testing::AssertionSuccess() << OS.str();
+    else
+      return ::testing::AssertionFailure() << OS.str();
+  }
+}
+
 } // end namespace ast_matchers
 } // end namespace clang
 

unittests/AST/ASTImporterTest.cpp

@@ -1879,7 +1879,7 @@ TEST_P(ASTImporterOptionSpecificTestBase, AnonymousRecordsReversed) {
   auto *ToTU = ToAST->getASTContext().getTranslationUnitDecl();
   // We expect one (ODR) warning during the import.
   EXPECT_EQ(1u, ToTU->getASTContext().getDiagnostics().getNumWarnings());
-  EXPECT_EQ(2u,
+  EXPECT_EQ(1u,
             DeclCounter<RecordDecl>().match(ToTU, recordDecl(hasName("X"))));
 }
 
@@ -2432,6 +2432,62 @@ TEST_P(ImportFunctionTemplates,
   EXPECT_EQ(ToD1, ToD2);
 }
 
+TEST_P(ImportFunctionTemplates,
+       ImportFunctionWhenThereIsAFunTemplateWithSameName) {
+  getToTuDecl(
+      R"(
+      template <typename T>
+      void foo(T) {}
+      void foo();
+      )",
+      Lang_CXX);
+  Decl *FromTU = getTuDecl("void foo();", Lang_CXX);
+  auto *FromD = FirstDeclMatcher<FunctionDecl>().match(
+      FromTU, functionDecl(hasName("foo")));
+  auto *ImportedD = Import(FromD, Lang_CXX);
+  EXPECT_TRUE(ImportedD);
+}
+
+TEST_P(ImportFunctionTemplates,
+       ImportConstructorWhenThereIsAFunTemplateWithSameName) {
+  auto Code =
+      R"(
+      struct Foo {
+        template <typename T>
+        Foo(T) {}
+        Foo();
+      };
+      )";
+  getToTuDecl(Code, Lang_CXX);
+  Decl *FromTU = getTuDecl(Code, Lang_CXX);
+  auto *FromD =
+      LastDeclMatcher<CXXConstructorDecl>().match(FromTU, cxxConstructorDecl());
+  auto *ImportedD = Import(FromD, Lang_CXX);
+  EXPECT_TRUE(ImportedD);
+}
+
+TEST_P(ImportFunctionTemplates,
+       ImportOperatorWhenThereIsAFunTemplateWithSameName) {
+  getToTuDecl(
+      R"(
+      template <typename T>
+      void operator<(T,T) {}
+      struct X{};
+      void operator<(X, X);
+      )",
+      Lang_CXX);
+  Decl *FromTU = getTuDecl(
+      R"(
+      struct X{};
+      void operator<(X, X);
+      )",
+      Lang_CXX);
+  auto *FromD = LastDeclMatcher<FunctionDecl>().match(
+      FromTU, functionDecl(hasOverloadedOperatorName("<")));
+  auto *ImportedD = Import(FromD, Lang_CXX);
+  EXPECT_TRUE(ImportedD);
+}
+
 struct ImportFriendFunctions : ImportFunctions {};
 
 TEST_P(ImportFriendFunctions, ImportFriendFunctionRedeclChainProto) {
@@ -5127,15 +5183,6 @@ TEST_P(ErrorHandlingTest,
   }
 }
 
-INSTANTIATE_TEST_CASE_P(ParameterizedTests, ErrorHandlingTest,
-                        DefaultTestValuesForRunOptions, );
-
-INSTANTIATE_TEST_CASE_P(ParameterizedTests, DeclContextTest,
-                        ::testing::Values(ArgVector()), );
-
-INSTANTIATE_TEST_CASE_P(ParameterizedTests, CanonicalRedeclChain,
-                        ::testing::Values(ArgVector()), );
-
 TEST_P(ASTImporterOptionSpecificTestBase, LambdaInFunctionBody) {
   Decl *FromTU = getTuDecl(
       R"(
@@ -5233,8 +5280,8 @@ TEST_P(ASTImporterOptionSpecificTestBase,
   // prototype (inside 'friend') for it comes first in the AST and is not
   // linked to the definition.
   EXPECT_EQ(ImportedDef, ToClassDef);
-}  
-  
+}
+
 struct LLDBLookupTest : ASTImporterOptionSpecificTestBase {
   LLDBLookupTest() {
     Creator = [](ASTContext &ToContext, FileManager &ToFileManager,
@@ -5362,10 +5409,197 @@ TEST_P(ASTImporterOptionSpecificTestBase,
   EXPECT_EQ(ImportedX->isAggregate(), FromX->isAggregate());
 }
 
+struct ConflictingDeclsWithLiberalStrategy : ASTImporterOptionSpecificTestBase {
+  ConflictingDeclsWithLiberalStrategy() {
+    this->ODRHandling = ASTImporter::ODRHandlingType::Liberal;
+  }
+};
+
+// Check that a Decl has been successfully imported into a standalone redecl
+// chain.
+template <typename DeclTy, typename PatternTy>
+static void CheckImportedAsNew(llvm::Expected<Decl *> &Result, Decl *ToTU,
+                               PatternTy Pattern) {
+  ASSERT_TRUE(isSuccess(Result));
+  Decl *ImportedD = *Result;
+  ASSERT_TRUE(ImportedD);
+  auto *ToD = FirstDeclMatcher<DeclTy>().match(ToTU, Pattern);
+  EXPECT_NE(ImportedD, ToD);
+  EXPECT_FALSE(ImportedD->getPreviousDecl());
+  EXPECT_EQ(DeclCounter<DeclTy>().match(ToTU, Pattern), 2u);
+}
+
+TEST_P(ConflictingDeclsWithLiberalStrategy, Typedef) {
+  Decl *ToTU = getToTuDecl(
+      R"(
+      typedef int X;
+      )",
+      Lang_CXX11);
+  Decl *FromTU = getTuDecl(
+      R"(
+      typedef double X;
+      )",
+      Lang_CXX11);
+  auto Pattern = typedefNameDecl(hasName("X"));
+  auto *FromX = FirstDeclMatcher<TypedefNameDecl>().match(FromTU, Pattern);
+
+  Expected<Decl *> Result = importOrError(FromX, Lang_CXX11);
+  CheckImportedAsNew<TypedefNameDecl>(Result, ToTU, Pattern);
+}
+
+TEST_P(ConflictingDeclsWithLiberalStrategy, TypeAlias) {
+  Decl *ToTU = getToTuDecl(
+      R"(
+      using X = int;
+      )",
+      Lang_CXX11);
+  Decl *FromTU = getTuDecl(
+      R"(
+      using X = double;
+      )",
+      Lang_CXX11);
+  auto Pattern = typedefNameDecl(hasName("X"));
+  auto *FromX = FirstDeclMatcher<TypedefNameDecl>().match(FromTU, Pattern);
+  Expected<Decl *> Result = importOrError(FromX, Lang_CXX11);
+  CheckImportedAsNew<TypedefNameDecl>(Result, ToTU, Pattern);
+}
+
+TEST_P(ConflictingDeclsWithLiberalStrategy, EnumDecl) {
+  Decl *ToTU = getToTuDecl(
+      R"(
+      enum X { a, b };
+      )",
+      Lang_CXX11);
+  Decl *FromTU = getTuDecl(
+      R"(
+      enum X { a, b, c };
+      )",
+      Lang_CXX11);
+  auto Pattern = enumDecl(hasName("X"));
+  auto *FromX = FirstDeclMatcher<EnumDecl>().match(FromTU, Pattern);
+  Expected<Decl *> Result = importOrError(FromX, Lang_CXX11);
+  CheckImportedAsNew<EnumDecl>(Result, ToTU, Pattern);
+}
+
+TEST_P(ConflictingDeclsWithLiberalStrategy, EnumConstantDecl) {
+  Decl *ToTU = getToTuDecl(
+      R"(
+      enum E { X = 0 };
+      )",
+      Lang_CXX11);
+  Decl *FromTU = getTuDecl(
+      R"(
+      enum E { X = 1 };
+      )",
+      Lang_CXX11);
+  auto Pattern = enumConstantDecl(hasName("X"));
+  auto *FromX = FirstDeclMatcher<EnumConstantDecl>().match(FromTU, Pattern);
+  Expected<Decl *> Result = importOrError(FromX, Lang_CXX11);
+  CheckImportedAsNew<EnumConstantDecl>(Result, ToTU, Pattern);
+}
+
+TEST_P(ConflictingDeclsWithLiberalStrategy, RecordDecl) {
+  Decl *ToTU = getToTuDecl(
+      R"(
+      class X { int a; };
+      )",
+      Lang_CXX11);
+  Decl *FromTU = getTuDecl(
+      R"(
+      class X { int b; };
+      )",
+      Lang_CXX11);
+  auto Pattern = cxxRecordDecl(hasName("X"), unless(isImplicit()));
+  auto *FromX = FirstDeclMatcher<RecordDecl>().match(FromTU, Pattern);
+  Expected<Decl *> Result = importOrError(FromX, Lang_CXX11);
+  CheckImportedAsNew<RecordDecl>(Result, ToTU, Pattern);
+}
+
+TEST_P(ConflictingDeclsWithLiberalStrategy, VarDecl) {
+  Decl *ToTU = getToTuDecl(
+      R"(
+      int X;
+      )",
+      Lang_CXX11);
+  Decl *FromTU = getTuDecl(
+      R"(
+      double X;
+      )",
+      Lang_CXX11);
+  auto Pattern = varDecl(hasName("X"));
+  auto *FromX = FirstDeclMatcher<VarDecl>().match(FromTU, Pattern);
+  Expected<Decl *> Result = importOrError(FromX, Lang_CXX11);
+  CheckImportedAsNew<VarDecl>(Result, ToTU, Pattern);
+}
+
+TEST_P(ConflictingDeclsWithLiberalStrategy, FunctionDecl) {
+  Decl *ToTU = getToTuDecl(
+      R"(
+      void X(int);
+      )",
+      Lang_C); // C, no overloading!
+  Decl *FromTU = getTuDecl(
+      R"(
+      void X(double);
+      )",
+      Lang_C);
+  auto Pattern = functionDecl(hasName("X"));
+  auto *FromX = FirstDeclMatcher<FunctionDecl>().match(FromTU, Pattern);
+  Expected<Decl *> Result = importOrError(FromX, Lang_CXX11);
+  CheckImportedAsNew<FunctionDecl>(Result, ToTU, Pattern);
+}
+
+TEST_P(ConflictingDeclsWithLiberalStrategy, ClassTemplateDecl) {
+  Decl *ToTU = getToTuDecl(
+      R"(
+      template <class>
+      struct X;
+      )",
+      Lang_CXX11);
+  Decl *FromTU = getTuDecl(
+      R"(
+      template <int>
+      struct X;
+      )",
+      Lang_CXX11);
+  auto Pattern = classTemplateDecl(hasName("X"));
+  auto *FromX = FirstDeclMatcher<ClassTemplateDecl>().match(FromTU, Pattern);
+  Expected<Decl *> Result = importOrError(FromX, Lang_CXX11);
+  CheckImportedAsNew<ClassTemplateDecl>(Result, ToTU, Pattern);
+}
+
+TEST_P(ConflictingDeclsWithLiberalStrategy, DISABLED_VarTemplateDecl) {
+  const internal::VariadicDynCastAllOfMatcher<Decl, VarTemplateDecl>
+      varTemplateDecl;
+  Decl *ToTU = getToTuDecl(
+      R"(
+      template <class T>
+      constexpr T X;
+      )",
+      Lang_CXX14);
+  Decl *FromTU = getTuDecl(
+      R"(
+      template <int>
+      constexpr int X = 0;
+      )",
+      Lang_CXX14);
+  auto Pattern = varTemplateDecl(hasName("X"));
+  auto *FromX = FirstDeclMatcher<VarTemplateDecl>().match(
+      FromTU, varTemplateDecl(hasName("X")));
+  Expected<Decl *> Result = importOrError(FromX, Lang_CXX11);
+  CheckImportedAsNew<VarTemplateDecl>(Result, ToTU, Pattern);
+}
+
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, SVEBuiltins,
                         ::testing::Values(ArgVector{"-target",
                                                     "aarch64-linux-gnu"}), );
 
+INSTANTIATE_TEST_CASE_P(ParameterizedTests, DeclContextTest,
+                        ::testing::Values(ArgVector()), );
+
+INSTANTIATE_TEST_CASE_P(ParameterizedTests, CanonicalRedeclChain,
+                        ::testing::Values(ArgVector()), );
+
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, ASTImporterLookupTableTest,
                         DefaultTestValuesForRunOptions, );
 
@@ -5384,19 +5618,22 @@ INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportDecl,
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, ASTImporterOptionSpecificTestBase,
                         DefaultTestValuesForRunOptions, );
 
+INSTANTIATE_TEST_CASE_P(ParameterizedTests, ErrorHandlingTest,
+                        DefaultTestValuesForRunOptions, );
+
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, RedirectingImporterTest,
                         DefaultTestValuesForRunOptions, );
 
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportFunctions,
                         DefaultTestValuesForRunOptions, );
 
-INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportFriendFunctionTemplates,
+INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportFunctionTemplates,
                         DefaultTestValuesForRunOptions, );
 
-INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportClasses,
+INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportFriendFunctionTemplates,
                         DefaultTestValuesForRunOptions, );
 
-INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportFunctionTemplates,
+INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportClasses,
                         DefaultTestValuesForRunOptions, );
 
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportFriendFunctions,
@@ -5418,6 +5655,8 @@ INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportVariables,
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, LLDBLookupTest,
                         DefaultTestValuesForRunOptions, );
 
+INSTANTIATE_TEST_CASE_P(ParameterizedTests, ConflictingDeclsWithLiberalStrategy,
+                        DefaultTestValuesForRunOptions, );
 
 } // end namespace ast_matchers
 } // end namespace clang