[ASTImporter] Added visibility context check for TypedefNameDecl.

[balazske]

Summary

ASTImporter makes now difference between typedefs and type aliases
with same name in different translation units
if these are not visible outside.

[shafik]

lib/AST/ASTImporter.cpp

949 ↗

(On Diff #208931)

I am not sure what case this covers? Can you elaborate? I see the case the condition above is catching.

unittests/AST/ASTImporterVisibilityTest.cpp

348 ↗

(On Diff #208931)

Perhaps ExpectLink would be better as ExpectLinkedDeclChain and the same for ExpectNotLink.

It was actually confusing at first because link is an overload term.

352 ↗

(On Diff #208931)

We need another set of tests for typedefs and using e.g. ExternTypedef and ExternUsing since they are equivalent.

[shafik]

It is worth noting that:

typedef int T;
typedef int T;

is not valid C99 see godbolt

[balazske]

Improved the tests.

[balazske]

• Removed unneeded structs.

[balazske]

In D64480#1653629, @shafik wrote:

It is worth noting that:

typedef int T;
typedef int T;

is not valid C99 see godbolt

Should we handle this case? This can be special for C99 only when the declarations must be merged instead of linked. Probably this does not cause functional problems if we leave it as is.

lib/AST/ASTImporter.cpp

949 ↗

(On Diff #208931)

This (last line) should mean that root level in source file (non-anonymous namespace) in different files is treated as same visibility context (like "extern" declarations). If namespace is different (anonymous and non-anonymous) the context is different.
For TypedefNameDecl there is no possibility of external formal linkage (it is like always external).

unittests/AST/ASTImporterVisibilityTest.cpp

348 ↗

(On Diff #208931)

I can change it in a later patch.

[shafik]

In D64480#1654354, @balazske wrote:

In D64480#1653629, @shafik wrote:

It is worth noting that:

typedef int T;
typedef int T;

is not valid C99 see godbolt

Should we handle this case? This can be special for C99 only when the declarations must be merged instead of linked. Probably this does not cause functional problems if we leave it as is.

I don't think it is critical to handle this case but I was surprised when I learned this, it is an edge case and it would be good to note this perhaps as a comment in the code.

[shafik]

clang/unittests/AST/ASTImporterVisibilityTest.cpp

343

I don't have any objections to changing the names ExpectLink and ExpectNotLink in a subsequent patch but we should update then b/c they are confusing (because they are overloaded terms) and it takes some time to work backwards to understand the intent.

[balazske]

• Added comment about repeated enums in C99.

[balazske]

Rename of constants:
https://reviews.llvm.org/D67174

clang/lib/AST/ASTImporter.cpp

if (From.isPackExpansion())
     if (Error Err = importInto(EllipsisLoc, From.getEllipsisLoc()))
       return std::move(Err);
 
   return LambdaCapture(
       *LocationOrErr, From.isImplicit(), From.getCaptureKind(), Var,
       EllipsisLoc);
 }
 
+template <typename T>
+bool ASTNodeImporter::hasSameVisibilityContext(T *Found, T *From) {
+  if (From->hasExternalFormalLinkage())
+    return Found->hasExternalFormalLinkage();
+  if (Importer.GetFromTU(Found) != From->getTranslationUnitDecl())
+    return false;
+  if (From->isInAnonymousNamespace())
+    return Found->isInAnonymousNamespace();
+  else
+    return !Found->isInAnonymousNamespace() &&
+           !Found->hasExternalFormalLinkage();
+}
+
+template <>
+bool ASTNodeImporter::hasSameVisibilityContext(TypedefNameDecl *Found,
+                                               TypedefNameDecl *From) {
+  if (From->isInAnonymousNamespace() && Found->isInAnonymousNamespace())
+    return Importer.GetFromTU(Found) == From->getTranslationUnitDecl();
+  return From->isInAnonymousNamespace() == Found->isInAnonymousNamespace();
+}
+
 } // namespace clang
 
 //----------------------------------------------------------------------------
 // Import Types
 //----------------------------------------------------------------------------
 
 using namespace clang;
 
ASTNodeImporter::VisitTypedefNameDecl(TypedefNameDecl *D, bool IsAlias) {
   if (Error Err = ImportDeclParts(D, DC, LexicalDC, Name, ToD, Loc))
     return std::move(Err);
   if (ToD)
     return ToD;
 
   // If this typedef is not in block scope, determine whether we've
   // seen a typedef with the same name (that we can merge with) or any
   // other entity by that name (which name lookup could conflict with).
+  // Note: Repeated typedefs are not valid in C99:
+  // 'typedef int T; typedef int T;' is invalid
+  // We do not care about this now.
   if (!DC->isFunctionOrMethod()) {
     SmallVector<NamedDecl *, 4> ConflictingDecls;
     unsigned IDNS = Decl::IDNS_Ordinary;
     auto FoundDecls = Importer.findDeclsInToCtx(DC, Name);
     for (auto *FoundDecl : FoundDecls) {
       if (!FoundDecl->isInIdentifierNamespace(IDNS))
         continue;
       if (auto *FoundTypedef = dyn_cast<TypedefNameDecl>(FoundDecl)) {
+        if (!hasSameVisibilityContext(FoundTypedef, D))
+          continue;
+
         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);
           // FIXME Handle redecl chain. When you do that make consistent changes
if (Stmt *FromBody = FromFD->getBody()) {
     if (ExpectedStmt ToBodyOrErr = import(FromBody))
       ToFD->setBody(*ToBodyOrErr);
     else
       return ToBodyOrErr.takeError();
   }
   return Error::success();
 }
 
-template <typename T>
-bool ASTNodeImporter::hasSameVisibilityContext(T *Found, T *From) {
-  if (From->hasExternalFormalLinkage())
-    return Found->hasExternalFormalLinkage();
-  if (Importer.GetFromTU(Found) != From->getTranslationUnitDecl())
-    return false;
-  if (From->isInAnonymousNamespace())
-    return Found->isInAnonymousNamespace();
-  else
-    return !Found->isInAnonymousNamespace() &&
-           !Found->hasExternalFormalLinkage();
-}
-
 ExpectedDecl ASTNodeImporter::VisitFunctionDecl(FunctionDecl *D) {
 
   SmallVector<Decl *, 2> Redecls = getCanonicalForwardRedeclChain(D);
   auto RedeclIt = Redecls.begin();
   // Import the first part of the decl chain. I.e. import all previous
   // declarations starting from the canonical decl.
   for (; RedeclIt != Redecls.end() && *RedeclIt != D; ++RedeclIt) {
     ExpectedDecl ToRedeclOrErr = import(*RedeclIt);

clang/unittests/AST/ASTImporterVisibilityTest.cpp

 struct GetClassPattern {
   using DeclTy = CXXRecordDecl;
   BindableMatcher<Decl> operator()() { return cxxRecordDecl(hasName("X")); }
 };
 struct GetEnumPattern {
   using DeclTy = EnumDecl;
   BindableMatcher<Decl> operator()() { return enumDecl(hasName("E")); }
 };
+struct GetTypedefNamePattern {
+  using DeclTy = TypedefNameDecl;
+  BindableMatcher<Decl> operator()() { return typedefNameDecl(hasName("T")); }
+};
 
 // Values for the value-parameterized test fixtures.
 // FunctionDecl:
 const auto *ExternF = "void f();";
 const auto *StaticF = "static void f();";
 const auto *AnonF = "namespace { void f(); }";
 // VarDecl:
 const auto *ExternV = "extern int v;";
 const auto *StaticV = "static int v;";
 const auto *AnonV = "namespace { extern int v; }";
 // CXXRecordDecl:
 const auto *ExternC = "class X;";
 const auto *AnonC = "namespace { class X; }";
 // EnumDecl:
 const auto *ExternE = "enum E {};";
 const auto *AnonE = "namespace { enum E {}; }";
+// TypedefNameDecl:
+const auto *ExternTypedef = "typedef int T;";
+const auto *AnonTypedef = "namespace { typedef int T; }";
+const auto *ExternUsing = "using T = int;";
+const auto *AnonUsing = "namespace { using T = int; }";
 
 // First value in tuple: Compile options.
 // Second value in tuple: Source code to be used in the test.
 using ImportVisibilityChainParams =
     ::testing::WithParamInterface<std::tuple<ArgVector, const char *>>;
 // Fixture to test the redecl chain of Decls with the same visibility. Gtest
 // makes it possible to have either value-parameterized or type-parameterized
 // fixtures. However, we cannot have both value- and type-parameterized test
EXPECT_EQ(0u, ToF0->getTranslationUnitDecl()
                       .getDiagnostics()
                       .getNumWarnings());
   }
 };
 using ImportFunctionsVisibility = ImportVisibility<GetFunPattern>;
 using ImportVariablesVisibility = ImportVisibility<GetVarPattern>;
 using ImportClassesVisibility = ImportVisibility<GetClassPattern>;
 using ImportEnumsVisibility = ImportVisibility<GetEnumPattern>;
+using ImportTypedefNameVisibility = ImportVisibility<GetTypedefNamePattern>;
 
 // FunctionDecl.
 TEST_P(ImportFunctionsVisibility, ImportAfter) {
   TypedTest_ImportAfter();
 }
 TEST_P(ImportFunctionsVisibility, ImportAfterImport) {
   TypedTest_ImportAfterImport();
 }

 }
 // EnumDecl.
 TEST_P(ImportEnumsVisibility, ImportAfter) {
   TypedTest_ImportAfterWithMerge();
 }
 TEST_P(ImportEnumsVisibility, ImportAfterImport) {
   TypedTest_ImportAfterImportWithMerge();
 }
+// TypedefNameDecl.
+TEST_P(ImportTypedefNameVisibility, ImportAfter) {
+  TypedTest_ImportAfterWithMerge();
+}
+TEST_P(ImportTypedefNameVisibility, ImportAfterImport) {
+  TypedTest_ImportAfterImportWithMerge();
+}
 
 const bool ExpectLink = true;
 const bool ExpectNotLink = false;
 
 INSTANTIATE_TEST_CASE_P(
     ParameterizedTests, ImportFunctionsVisibility,
     ::testing::Combine(
         DefaultTestValuesForRunOptions,

 INSTANTIATE_TEST_CASE_P(
     ParameterizedTests, ImportEnumsVisibility,
     ::testing::Combine(
         DefaultTestValuesForRunOptions,
         ::testing::Values(std::make_tuple(ExternE, ExternE, ExpectLink),
                           std::make_tuple(ExternE, AnonE, ExpectNotLink),
                           std::make_tuple(AnonE, ExternE, ExpectNotLink),
                           std::make_tuple(AnonE, AnonE, ExpectNotLink))), );
+INSTANTIATE_TEST_CASE_P(
+    ParameterizedTests, ImportTypedefNameVisibility,
+    ::testing::Combine(
+        DefaultTestValuesForRunOptions,
+        ::testing::Values(
+            std::make_tuple(ExternTypedef, ExternTypedef, ExpectLink),
+            std::make_tuple(ExternTypedef, AnonTypedef, ExpectNotLink),
+            std::make_tuple(AnonTypedef, ExternTypedef, ExpectNotLink),
+            std::make_tuple(AnonTypedef, AnonTypedef, ExpectNotLink),
+
+            std::make_tuple(ExternUsing, ExternUsing, ExpectLink),
+            std::make_tuple(ExternUsing, AnonUsing, ExpectNotLink),
+            std::make_tuple(AnonUsing, ExternUsing, ExpectNotLink),
+            std::make_tuple(AnonUsing, AnonUsing, ExpectNotLink),
+
+            std::make_tuple(ExternUsing, ExternTypedef, ExpectLink),
+            std::make_tuple(ExternUsing, AnonTypedef, ExpectNotLink),
+            std::make_tuple(AnonUsing, ExternTypedef, ExpectNotLink),
+            std::make_tuple(AnonUsing, AnonTypedef, ExpectNotLink),
+
+            std::make_tuple(ExternTypedef, ExternUsing, ExpectLink),
+            std::make_tuple(ExternTypedef, AnonUsing, ExpectNotLink),
+            std::make_tuple(AnonTypedef, ExternUsing, ExpectNotLink),
+            std::make_tuple(AnonTypedef, AnonUsing, ExpectNotLink))), );
 
 } // end namespace ast_matchers
 } // end namespace clang