[ASTImporter] Fix name conflict handling with different strategies

[martong]

Summary

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

[a_sidorin]

Hi Gabor,

Thank you for addressing the problem!

lib/AST/ASTImporter.cpp

2259–2260

if body is surrounded by braces, so it's better to surround else too.

2260–2261

Do we push the same decl twice?

8520–8525

Empty DeclarationName can be valid sometimes. Should we return ErrorOr<DeclarationName> instead? This can also simplify caller code a bit.

[martong]

lib/AST/ASTImporter.cpp

2260–2261

Uhh, thanks, that is rebase error I've made.

8520–8525

That's a good idea, thanks.
Though, I'd use Expected<T> rather, that would be more consistent with the rest of the code.

[martong]

• Add braces around else
• Remove falsely duplicated push_back
• Use Expected<T> in HandleNameConflict

[a_sidorin]

Thanks for the fixes! I'd like to clarify one moment, however.

lib/AST/ASTImporter.cpp

2154

Should we write else Name = *NameOrError?

[martong]

lib/AST/ASTImporter.cpp

2154

Atm, we implement the simplest strategy for name conflict handling: we just return with the error.
However, on a long term we should use the returned name indeed. I mean when we really do implement a renaming strategy via HandleNameConflict.

Also, for that we'd have to double check that the Name is indeed used when we create the AST node. So I'd rather leave this else branch up to that point. Hopefully, by that time we'll have unittests which would exercise this else branch, now we just don't have any.

[martong]

• Put back the call to GetOriginalDecl

[martong]

Ping @a_sidorin @shafik

[martong]

Ping

[a_sidorin]

LGTM, thanks!

[martong]

• Rebase to master

[shafik]

First round of review.

clang/lib/AST/ASTImporter.cpp

2632 ↗

(On Diff #211312)

Name -> SearchName

clang/unittests/AST/ASTImporterTest.cpp

2392 ↗

(On Diff #211312)

What about tests for name conflicts for:

NamespaceDecl
TypedefNameDecl
TypeAliasTemplateDecl
EnumConstantDecl
RecordDecl
VarDecl

Who were also modified above.

[shafik]

Just wanted to see if you were planning on landing this soon, the fix Name -> SearchName is probably an important one since we have seen several issues w/ bugs like that but I really would like to see more tests. Are you having issues coming up w/ tests?

[martong]

In D59692#1632138, @shafik wrote:

Just wanted to see if you were planning on landing this soon, the fix Name -> SearchName is probably an important one since we have seen several issues w/ bugs like that but I really would like to see more tests. Are you having issues coming up w/ tests?

Hey Shafik, I was busy lately with some non-ASTImporter related tasks, but just recently I could drive myself back to it.
I am planning to add the test cases in the following work days, but the latest by the end of the next week.

[martong]

• Name -> SearchName
• Add tests for conflicting declarations

[martong]

clang/unittests/AST/ASTImporterTest.cpp

2392 ↗

(On Diff #211312)

I added several new tests with a new test suite ConflictingDeclsTest, they cover all the modifications in ASTImporter.cpp except for VarTemplateDecls.
In case of VarTemplateDecls we don't have a proper structural eq check implemented yet, so I disabled that test.

[martong]

@shafik Ping

[martong]

• Use resulting Name from HandleNameConflict if set
• Add ODRHandling strategies
• Refactor tests, to avoid some code repetition

[martong]

@shafik I have updated the patch with ODR handling strategies as per our discusson.

For the record I copy our discussion here:

On Sat, Aug 24, 2019 at 12:50 AM Shafik Yaghmour <[e-mail]> wrote:

Apologies, I missed this earlier!

On Wed, Aug 21, 2019 at 2:44 AM Gábor Márton <[e-mail]> wrote:

Hi Shafik,

Right now you will end up w/ an arbitrary one of them but we do want to support

a way to choose between them eventually.

Actually, right now (if the patch is not merged) we end up having both of them in the AST. Some parts of the AST reference the existing definition, while some other parts reference the new definition. Also, the regular lookup will find both definitions.

If the patch is merged then only the first (the existing) definition is kept and an error is reported.

AFAICT this would prevent such a solution. At least that is how the

new test case for RecordDecl make it appear

Yes, you will never be able to remove an existing node from the AST, so I don't think an either-or choosing mechanism is feasible. But you may be able to decide whether you want to add the new and conflicting node. And you may be able to use a different name for the new conflicting node. This may help clients to see which node they are using.
I want to create an additional patch which builds on this patch. Here is the essence of what I'd like to have:

if (!ConflictingDecls.empty()) {
  Expected<DeclarationName> Resolution = Importer.HandleNameConflict(
      Name, DC, Decl::IDNS_Member, ConflictingDecls.data(),
      ConflictingDecls.size());
  if (Resolution)
    Name = Resolution.get();
  else
    return Resolution.takeError();
}

Consider the "else" branch. I'd like to have such an "else" branch everywhere. The point is to use the result of HandleNameConflict (if it is set to something). This way it is possible to create any kind of ODR handling by overwriting HandleNameConflict in the descendant classes.

We identified 3 possible strategies so far:

Conservative. In case of name conflict propagate the error. This should be the default behavior.
Liberal. In case of name conflict create a new node with the same name and do not propagate the error.
Rename. In case of name conflict create a new node with a different name (e.g. with a prefix of the TU's name). Do not propagate the error.

If we add a new conflicting node beside the exiting one, then some clients of the AST which rely on lookup will be confused. The CTU client does not rely on lookup so that is not really a problem there, but I don't know what this could cause with LLDB. Perhaps the renaming strategy could work there too.
The Conservative and Liberal strategies are very easy to implement, and I am going to create patches for that if we have consensus.

We know currently we do have cases where we have ODR violations w/
RecordDecl due to use of an opaque struct in the API headers and a
concrete instance internally e.g.:

//opaque
struct A {

char buf[16];

};

//concrete
struct A {

double d;
int64_t x;

};

and we don't want this to become an error.

I think we would at least one the choice of Conservative or Liberal to
be configurable and maybe LLDB would default to Liberal. This would
enable us to keep the status quo and not break existing cases any
worse than they already are.

I would prefer that would be done in this PR since I don't want to be
in a situation where we branch internally and we have this change but
not the follow-up change.

I don't see how like you comment says this does not effect CXXRecordDecl

In my comment I do not say that CXXRecordDecls are exceptions from the general way of handling ODR violations.
The comment is about that we shall not report ODR errors if we are not 100% sure that we face one.
So we should report an ODR error only if the found Decl and the newly imported Decl have the same kind.
I.e. both are CXXRecordDecls.
For example, let's assume we import a CXXRecordDecl and we find an existing ClassTemplateDecl with the very same Name.
Then we should not report an ODR violation.

Thank you for the clarification, I misunderstood the comment, now it
makes more sense.

Thanks,
Gabor

On Mon, Aug 19, 2019 at 5:46 PM Shafik Yaghmour <[e-mail]> wrote:

Have a nice vacation :-)

On Mon, Aug 19, 2019 at 8:40 AM Gábor Márton <[e-mail]> wrote:

Hi Shafik,

I'll have an answer for you on Wednesday, I'm on vacation until then.

Thanks,
Gábor

On Sat, 17 Aug 2019, 04:30 Shafik Yaghmour, <[e-mail]> wrote:

Hello Gábor,

I was looking at;

https://reviews.llvm.org/D59692

again and I appreciate you added the new tests.

I don't know how I missed this before but we would probably like to
support ODR violations in some reasonable manner. For example we have
one API that is using an opaque struct for the public API but the
private API used a non-opaque definition.

I would have to come up with some sort of test case which is hard to
do, or at least I did not have success last time I tried. Right now
you will end up w/ an arbitrary one of them but we do want to support
a way to choose between them eventually.

AFAICT this would prevent such a solution. At least that is how the
new test case for RecordDecl make it appear and I don't see how like
you comment says this does not effect CXXRecordDecl.

-Shafik

[shafik]

Other than my two comment this LGTM

clang/lib/AST/ASTImporter.cpp

3452 ↗

(On Diff #217135)

I am a little concerned about this change. I am wondering if this was previously handled differently as a band-aid for some other issues that only showed up in rare cases when they iterated over all the results but not when they errored out on the first.

Could we make the changes to VisitFieldDecl a separate PR so it is easier to revert later on if we do find this causes a regression we are not currently covering?

clang/unittests/AST/ASTImporterTest.cpp

5615 ↗

(On Diff #217135)

Since the "Liberal" strategy should be the current status quo, I think it might make sense to have a first PR that just has the *LiberalStrategy tests to verify that indeed this is the current behavior as we expect.

[martong]

clang/lib/AST/ASTImporter.cpp

3452 ↗

(On Diff #217135)

Ok, I reverted these hunks. I am going to create a follow-up patch which will include these changes.

clang/unittests/AST/ASTImporterTest.cpp

5615 ↗

(On Diff #217135)

Ok, I removed the test suite ConflictingDeclsWithConservativeStrategy. I am going to create a subsequent patch which adds comprehensive testing for both strategies.

[martong]

• Revert changes in VisitFieldDecl and in VisitIndirectFieldDecl
• Remove test suite ConflictingDeclsWithConservativeStrategy

[martong]

• Apply clang-format

[martong]

Jenkins is not green http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/743/
However, the newly failing test is TestTargetCommand.py, which seems to be unrelated.

[labath]

In D59692#1646990, @martong wrote:

Jenkins is not green http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/743/
However, the newly failing test is TestTargetCommand.py, which seems to be unrelated.

That ought to be fixed by r370057.

include/clang/AST/ASTImporter.h

public:
     ///
     /// \param IDNS the identifier namespace in which the name will be found.
     ///
     /// \param Decls the set of declarations with the same name as the
     /// declaration being imported.
     ///
     /// \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; }
 
     /// Retrieve the context that AST nodes are being imported from.
     ASTContext &getFromContext() const { return FromContext; }
 
     /// Retrieve the file manager that AST nodes are being imported into.
public:
 
     /// Subclasses can override this function to observe all of the \c From ->
     /// \c To declaration mappings as they are imported.
     virtual void Imported(Decl *From, Decl *To) {}
 
     /// Store and assign the imported declaration to its counterpart.
     Decl *MapImported(Decl *From, Decl *To);
 
+    /// Deprecated. FIXME use [[deprecated]] once Clang enables C++14.
     /// Called by StructuralEquivalenceContext.  If a RecordDecl is
     /// being compared to another RecordDecl as part of import, completing the
     /// other RecordDecl may trigger importation of the first RecordDecl. This
     /// happens especially for anonymous structs.  If the original of the second
     /// RecordDecl can be found, we can complete it without the need for
     /// importation, eliminating this loop.
     virtual Decl *GetOriginalDecl(Decl *To) { return nullptr; }
 

lib/AST/ASTImporter.cpp

namespace clang {
   using llvm::make_error;
   using llvm::Error;
   using llvm::Expected;
   using ExpectedType = llvm::Expected<QualType>;
   using ExpectedStmt = llvm::Expected<Stmt *>;
   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.
     switch (Error) {
     case NameConflict:
       return "NameConflict";
     case UnsupportedConstruct:
       return "UnsupportedConstruct";
StructuralEquivalenceContext Ctx(
       Importer.getFromContext(), Importer.getToContext(),
       Importer.getNonEquivalentDecls(), getStructuralEquivalenceKind(Importer),
       false, Complain);
   return Ctx.IsEquivalent(From, To);
 }
 
 bool ASTNodeImporter::IsStructuralMatch(RecordDecl *FromRecord,
                                         RecordDecl *ToRecord, bool Complain) {
-  // Eliminate a potential failure point where we attempt to re-import
-  // something we're trying to import while completing ToRecord.
-  Decl *ToOrigin = Importer.GetOriginalDecl(ToRecord);
-  if (ToOrigin) {
-    auto *ToOriginRecord = dyn_cast<RecordDecl>(ToOrigin);
-    if (ToOriginRecord)
-      ToRecord = ToOriginRecord;
-  }
-
   StructuralEquivalenceContext Ctx(Importer.getFromContext(),
                                    ToRecord->getASTContext(),
                                    Importer.getNonEquivalentDecls(),
                                    getStructuralEquivalenceKind(Importer),
                                    false, Complain);
   return Ctx.IsEquivalent(FromRecord, ToRecord);
 }
 
for (auto *FoundDecl : FoundDecls) {
         ConflictingDecls.clear();
         break;
       }
 
       ConflictingDecls.push_back(FoundDecl);
     }
 
     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(),
+      if (!NameOrErr)
+        return NameOrErr.takeError();
     }
   }
 
   ExpectedSLoc BeginLocOrErr = import(D->getBeginLoc());
   if (!BeginLocOrErr)
     return BeginLocOrErr.takeError();
 
   // Create the "to" namespace, if needed.
   NamespaceDecl *ToNamespace = MergeWithNamespace;
for (auto *FoundDecl : FoundDecls) {
       if (auto *FoundTypedef = dyn_cast<TypedefNameDecl>(FoundDecl)) {
         QualType FromUT = D->getUnderlyingType();
         QualType FoundUT = FoundTypedef->getUnderlyingType();
         if (Importer.IsStructurallyEquivalent(FromUT, FoundUT)) {
           // If the "From" context has a complete underlying type but we
           // already have a complete underlying type then return with that.
           if (!FromUT->isIncompleteType() && !FoundUT->isIncompleteType())
             return Importer.MapImported(D, FoundTypedef);
+        } else {
+          ConflictingDecls.push_back(FoundDecl);
         }
-        // FIXME Handle redecl chain.
-        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)
+        return NameOrErr.takeError();
     }
   }
 
   QualType ToUnderlyingType;
   TypeSourceInfo *ToTypeSourceInfo;
   SourceLocation ToBeginLoc;
   if (auto Imp = importSeq(
       D->getUnderlyingType(), D->getTypeSourceInfo(), D->getBeginLoc()))
for (auto *FoundDecl : FoundDecls) {
       if (!FoundDecl->isInIdentifierNamespace(IDNS))
         continue;
       if (auto *FoundAlias = dyn_cast<TypeAliasTemplateDecl>(FoundDecl))
         return Importer.MapImported(D, FoundAlias);
       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)
+        return NameOrErr.takeError();
     }
   }
 
   TemplateParameterList *ToTemplateParameters;
   TypeAliasDecl *ToTemplatedDecl;
   if (auto Imp = importSeq(D->getTemplateParameters(), D->getTemplatedDecl()))
     std::tie(ToTemplateParameters, ToTemplatedDecl) = *Imp;
   else
for (auto *FoundDecl : FoundDecls) {
       if (auto *Typedef = dyn_cast<TypedefNameDecl>(FoundDecl)) {
         if (const auto *Tag = Typedef->getUnderlyingType()->getAs<TagType>())
           FoundDecl = Tag->getDecl();
       }
 
       if (auto *FoundEnum = dyn_cast<EnumDecl>(FoundDecl)) {
         if (IsStructuralMatch(D, FoundEnum))
           return Importer.MapImported(D, FoundEnum);
-      }
-
         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)
+        return NameOrErr.takeError();
     }
   }
 
   SourceLocation ToBeginLoc;
   NestedNameSpecifierLoc ToQualifierLoc;
   QualType ToIntegerType;
   if (auto Imp = importSeq(
       D->getBeginLoc(), D->getQualifierLoc(), D->getIntegerType()))
for (auto *FoundDecl : FoundDecls) {
                 // because implicit methods are created only if they are used.
                 if (Error Err = ImportImplicitMethods(DCXX, FoundCXX))
                   return std::move(Err);
             }
           }
           PrevDecl = FoundRecord->getMostRecentDecl();
           break;
         }
-      }
-
         ConflictingDecls.push_back(FoundDecl);
+      } // kind is RecordDecl
     } // for
 
     if (!ConflictingDecls.empty() && SearchName) {
-      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)
+        return NameOrErr.takeError();
     }
   }
 
   ExpectedSLoc BeginLocOrErr = import(D->getBeginLoc());
   if (!BeginLocOrErr)
     return BeginLocOrErr.takeError();
 
   // Create the record declaration.
if (!LexicalDC->isFunctionOrMethod()) {
     auto FoundDecls = Importer.findDeclsInToCtx(DC, Name);
     for (auto *FoundDecl : FoundDecls) {
       if (!FoundDecl->isInIdentifierNamespace(IDNS))
         continue;
 
       if (auto *FoundEnumConstant = dyn_cast<EnumConstantDecl>(FoundDecl)) {
         if (IsStructuralMatch(D, FoundEnumConstant))
           return Importer.MapImported(D, FoundEnumConstant);
-      }
-
         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)
+        return NameOrErr.takeError();
     }
   }
 
   ExpectedType TypeOrErr = import(D->getType());
   if (!TypeOrErr)
     return TypeOrErr.takeError();
 
   ExpectedExpr InitOrErr = import(D->getInitExpr());
for (auto *FoundDecl : FoundDecls) {
         if (Importer.getToContext().getLangOpts().CPlusPlus)
           continue;
 
         // Complain about inconsistent function types.
         Importer.ToDiag(Loc, diag::err_odr_function_type_inconsistent)
             << Name << D->getType() << FoundFunction->getType();
         Importer.ToDiag(FoundFunction->getLocation(), diag::note_odr_value_here)
             << FoundFunction->getType();
-      }
-
         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)
+        return NameOrErr.takeError();
     }
   }
 
   // We do not allow more than one in-class declaration of a function. This is
   // because AST clients like VTableBuilder asserts on this. VTableBuilder
   // assumes there is only one in-class declaration. Building a redecl
   // chain would result in more than one in-class declaration for
   // overrides (even if they are part of the same redecl chain inside the
for (auto *FoundDecl : FoundDecls) {
             break;
           }
         }
 
         Importer.ToDiag(Loc, diag::err_odr_variable_type_inconsistent)
           << Name << D->getType() << FoundVar->getType();
         Importer.ToDiag(FoundVar->getLocation(), diag::note_odr_value_here)
           << FoundVar->getType();
-      }
-
         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)
+        return NameOrErr.takeError();
     }
   }
 
   QualType ToType;
   TypeSourceInfo *ToTypeSourceInfo;
   SourceLocation ToInnerLocStart;
   NestedNameSpecifierLoc ToQualifierLoc;
   if (auto Imp = importSeq(
for (auto *FoundDecl : FoundDecls) {
           ClassTemplateDecl *TemplateWithDef =
               getTemplateDefinition(FoundTemplate);
           if (D->isThisDeclarationADefinition() && TemplateWithDef) {
             return Importer.MapImported(D, TemplateWithDef);
           }
           FoundByLookup = FoundTemplate;
           break;
         }
-      }
-
         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(),
+      if (!NameOrErr)
+        return NameOrErr.takeError();
     }
-
-    if (!Name)
-      return make_error<ImportError>(ImportError::NameConflict);
   }
 
   CXXRecordDecl *FromTemplated = D->getTemplatedDecl();
 
   // Create the declaration that is being templated.
   CXXRecordDecl *ToTemplated;
   if (Error Err = importInto(ToTemplated, FromTemplated))
     return std::move(Err);
for (auto *FoundDecl : FoundDecls) {
     Decl *Found = FoundDecl;
     if (VarTemplateDecl *FoundTemplate = dyn_cast<VarTemplateDecl>(Found)) {
       if (IsStructuralMatch(D, FoundTemplate)) {
         // The variable templates structurally match; call it the same template.
         Importer.MapImported(D->getTemplatedDecl(),
                              FoundTemplate->getTemplatedDecl());
         return Importer.MapImported(D, FoundTemplate);
       }
-    }
-
       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(),
+    if (!NameOrErr)
+      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.
   // FIXME: Value not used?
   ExpectedType TypeOrErr = import(DTemplated->getType());
   if (!TypeOrErr)
     return TypeOrErr.takeError();
 
Expected<Selector> ASTImporter::Import_New(Selector FromSel) {
 
   SmallVector<IdentifierInfo *, 4> Idents;
   Idents.push_back(Import(FromSel.getIdentifierInfoForSlot(0)));
   for (unsigned I = 1, N = FromSel.getNumArgs(); I < N; ++I)
     Idents.push_back(Import(FromSel.getIdentifierInfoForSlot(I)));
   return ToContext.Selectors.getSelector(FromSel.getNumArgs(), Idents.data());
 }
 
-DeclarationName ASTImporter::HandleNameConflict(DeclarationName Name,
+Expected<DeclarationName> ASTImporter::HandleNameConflict(DeclarationName Name,
                                                           DeclContext *DC,
                                                           unsigned IDNS,
                                                           NamedDecl **Decls,
                                                           unsigned NumDecls) {
-  return Name;
+  return make_error<ImportError>(ImportError::NameConflict);
 }
 Selector ASTImporter::Import(Selector From) {
   llvm::Expected<Selector> To = Import_New(From);
   if (To)
     return *To;
   else
     llvm::consumeError(To.takeError());
   return {};

test/Analysis/Inputs/ctu-other.c

 // Test typedef and global variable in function.
 typedef struct {
   int a;
   int b;
 } FooBar;
 FooBar fb;
 int f(int i) {
   if (fb.a) {
     fb.b = i;
   }
   return 1;
 }
 
 // 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
 #define MYMACRO(ctx) \
   ctx->a;
 struct S {
   int a;
 };

unittests/AST/ASTImporterTest.cpp

TEST_P(ASTImporterOptionSpecificTestBase, AnonymousRecordsReversed) {
   auto *X1 =
       FirstDeclMatcher<RecordDecl>().match(FromTU1, recordDecl(hasName("X")));
   Import(X0, Lang_C);
   Import(X1, Lang_C);
 
   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"))));
 }
 
 TEST_P(ASTImporterOptionSpecificTestBase, ImportDoesUpdateUsedFlag) {
   auto Pattern = varDecl(hasName("x"));
   VarDecl *Imported1;
   {
     Decl *FromTU = getTuDecl("extern int x;", Lang_CXX, "input0.cc");
TEST_P(ImportFunctions,
   auto *ToD = Import(FromD, Lang_CXX);
   EXPECT_TRUE(ToD);
   Decl *ToTU = ToAST->getASTContext().getTranslationUnitDecl();
   EXPECT_TRUE(MatchVerifier<TranslationUnitDecl>().match(
       ToTU, translationUnitDecl(hasDescendant(
                 functionDecl(hasName("f"), hasDescendant(declRefExpr()))))));
 }
 
+struct ImportFunctionTemplates : ASTImporterOptionSpecificTestBase {};
+
+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) {
   auto Pattern = functionDecl(hasName("f"));
 
   Decl *FromTU = getTuDecl("struct X { friend void f(); };"
                            "void f();",
                            Lang_CXX,
INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportDecl,
                         DefaultTestValuesForRunOptions, );
 
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, ASTImporterOptionSpecificTestBase,
                         DefaultTestValuesForRunOptions, );
 
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportFunctions,
                         DefaultTestValuesForRunOptions, );
 
+INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportFunctionTemplates,
+                        DefaultTestValuesForRunOptions, );
+
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportFriendFunctionTemplates,
                         DefaultTestValuesForRunOptions, );
 
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportClasses,
                         DefaultTestValuesForRunOptions, );
 
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportFriendFunctions,
                         DefaultTestValuesForRunOptions, );