[clang][AST] Check context of record in structural equivalence.

[balazske]

Summary

The AST structural equivalence check did not differentiate between
a struct and a struct with same name in different namespace. When
type of a member is checked it is possible to encounter such a case
and wrongly decide that the types are similar. This problem is fixed
by check for the namespaces of a record declaration.

[steakhal]

I very much like the code.
L1364 is uncovered according to my coverage results.

clang/lib/AST/ASTStructuralEquivalence.cpp

1360–1364

Shouldn't you also check the name as well?

1379

So, the previous DC1->isTranslationUnit() guards the DC1->getParent() to never be NULL; same for DC2.
Pretty neat.

clang/unittests/AST/StructuralEquivalenceTest.cpp

982–988

The outer-most namespace could also be inline.
I'd love to see a test about that as well.

1004–1009

extern "C" might be nested within other namespaces.
Probably worth testing that as well.

1025–1032

Please, also test if the name of the structs is mismatched.

[shafik]

How does it handle this case:

struct A {int x;};

struct A f(void) {
    struct A{} a;

    return a;

and this case:

struct b{};

struct a {int x;}           // visible outside of f
f(struct b {int x; } b1) {  // not visibile out of f
  return (struct a){.x = 1};
}

[balazske]

In the last case the the RecordDecl's a and b can be matched (not internal statements in a function) and this is similar to the existing test cases. The struct a and struct b already fails to match because different name.

clang/lib/AST/ASTStructuralEquivalence.cpp

1360–1364

Here DC1 or DC2 is already a function or translation unit, the name of it is not relevant.

1379

Change to !DC1->getParent() (from DC1->isTranslationUnit())? (The line can be moved to here to increase readability, so the break statement is at end of the loop.)

[balazske]

Updated tests.

[martong]

Thanks @balazske for updating the tests, I think this addresses @shafik 's questions. LGTM! Ping @shafik

[balazske]

Applied lint reformattings.

[martong]

Still LGTM! Let's land it!

[thakis]

Looks like this breaks tests on windows: http://45.33.8.238/win/49558/step_7.txt

Please take a look and revert for now if it takes a while to fix.

[balazske]

Windows test failures occurred because wrong AST matchers in the tests. Tests will be updated to use named match.

[balazske]

Using correct AST match in the tests to fix failures on Windows.

[shafik]

clang/unittests/AST/StructuralEquivalenceTest.cpp

1037

I expect this to be false the outer Param is not the same type as the Param in the parameter.

1051

I expect this to be false as well.

[balazske]

clang/unittests/AST/StructuralEquivalenceTest.cpp

1037

There is a test ctu-main.c that fails if this is false. In that code a similar construct as here (structInProto) is imported and the import fails because unsupported construct. If these are structurally not equivalent then the failure is a ODR like error (the Param inside the function prototype is different than the Param outside of it). I do not know which is the correct behavior, I think the intent in that test is that import of this construct should work (if it would be supported by ASTImporter) at least in the C case.

[shafik]

clang/unittests/AST/StructuralEquivalenceTest.cpp

1037

They are compatible types, so if that is the criteria then that looks reasonable.

clang/lib/AST/ASTStructuralEquivalence.cpp

assert(D1->isLambda() && D2->isLambda() &&
          "Must be called on lambda classes");
   if (!IsStructurallyEquivalent(Context, D1->getLambdaCallOperator(),
                                 D2->getLambdaCallOperator()))
     return false;
 
   return true;
 }
 
+/// Determine if context of a class is equivalent.
+static bool IsRecordContextStructurallyEquivalent(RecordDecl *D1,
+                                                  RecordDecl *D2) {
+  // The context should be completely equal, including anonymous and inline
+  // namespaces.
+  // We compare objects as part of full translation units, not subtrees of
+  // translation units.
+  DeclContext *DC1 = D1->getDeclContext()->getNonTransparentContext();
+  DeclContext *DC2 = D2->getDeclContext()->getNonTransparentContext();
+  while (true) {
+    // Special case: We allow a struct defined in a function to be equivalent
+    // with a similar struct defined outside of a function.
+    if ((DC1->isFunctionOrMethod() && DC2->isTranslationUnit()) ||
+        (DC2->isFunctionOrMethod() && DC1->isTranslationUnit()))
+      return true;
+
+    if (DC1->getDeclKind() != DC2->getDeclKind())
+      return false;
+    if (DC1->isTranslationUnit())
+      break;
+    if (DC1->isInlineNamespace() != DC2->isInlineNamespace())
+      return false;
+    if (const auto *ND1 = dyn_cast<NamedDecl>(DC1)) {
+      const auto *ND2 = cast<NamedDecl>(DC2);
+      if (!DC1->isInlineNamespace() &&
+          !IsStructurallyEquivalent(ND1->getIdentifier(), ND2->getIdentifier()))
+        return false;
+    }
+
+    DC1 = DC1->getParent()->getNonTransparentContext();
+    DC2 = DC2->getParent()->getNonTransparentContext();
+  }
+
+  return true;
+}
+
 /// Determine structural equivalence of two records.
 static bool IsStructurallyEquivalent(StructuralEquivalenceContext &Context,
                                      RecordDecl *D1, RecordDecl *D2) {
 
   // Check for equivalent structure names.
   IdentifierInfo *Name1 = D1->getIdentifier();
   if (!Name1 && D1->getTypedefNameForAnonDecl())
     Name1 = D1->getTypedefNameForAnonDecl()->getIdentifier();
if (Optional<unsigned> Index1 =
               StructuralEquivalenceContext::findUntaggedStructOrUnionIndex(
                   D2)) {
         if (*Index1 != *Index2)
           return false;
       }
     }
   }
 
+  // If the records occur in different context (namespace), these should be
+  // different. This is specially important if the definition of one or both
+  // records is missing.
+  if (!IsRecordContextStructurallyEquivalent(D1, D2))
+    return false;
+
   // If both declarations are class template specializations, we know
   // the ODR applies, so check the template and template arguments.
   const auto *Spec1 = dyn_cast<ClassTemplateSpecializationDecl>(D1);
   const auto *Spec2 = dyn_cast<ClassTemplateSpecializationDecl>(D2);
   if (Spec1 && Spec2) {
     // Check that the specialized templates are the same.
     if (!IsStructurallyEquivalent(Context, Spec1->getSpecializedTemplate(),
                                   Spec2->getSpecializedTemplate()))

clang/unittests/AST/StructuralEquivalenceTest.cpp

auto Decls = makeNamedDecls("struct foo {explicit(true) foo(int);};",
                               "struct foo {explicit(true) foo(int);};", Lang_CXX20);
   CXXConstructorDecl *First = FirstDeclMatcher<CXXConstructorDecl>().match(
       get<0>(Decls), cxxConstructorDecl(hasName("foo")));
   CXXConstructorDecl *Second = FirstDeclMatcher<CXXConstructorDecl>().match(
       get<1>(Decls), cxxConstructorDecl(hasName("foo")));
   EXPECT_TRUE(testStructuralMatch(First, Second));
 }
 
+struct StructuralEquivalenceRecordContextTest : StructuralEquivalenceTest {};
+
+TEST_F(StructuralEquivalenceRecordContextTest, NamespaceNoVsNamed) {
+  auto Decls =
+      makeNamedDecls("class X;", "namespace N { class X; }", Lang_CXX03, "X");
+  EXPECT_FALSE(testStructuralMatch(Decls));
+}
+
+TEST_F(StructuralEquivalenceRecordContextTest, NamespaceNamedVsNamed) {
+  auto Decls = makeNamedDecls("namespace A { class X; }",
+                              "namespace B { class X; }", Lang_CXX03, "X");
+  EXPECT_FALSE(testStructuralMatch(Decls));
+}
+
+TEST_F(StructuralEquivalenceRecordContextTest, NamespaceAnonVsNamed) {
+  auto Decls = makeNamedDecls("namespace { class X; }",
+                              "namespace N { class X; }", Lang_CXX03, "X");
+  EXPECT_FALSE(testStructuralMatch(Decls));
+}
+
+TEST_F(StructuralEquivalenceRecordContextTest, NamespaceNoVsAnon) {
+  auto Decls =
+      makeNamedDecls("class X;", "namespace { class X; }", Lang_CXX03, "X");
+  EXPECT_FALSE(testStructuralMatch(Decls));
+}
+
+TEST_F(StructuralEquivalenceRecordContextTest, NamespaceAnonVsAnon) {
+  auto Decls = makeNamedDecls("namespace { class X; }",
+                              "namespace { class X; }", Lang_CXX03, "X");
+  EXPECT_TRUE(testStructuralMatch(Decls));
+}
+
+TEST_F(StructuralEquivalenceRecordContextTest, NamespaceAnonVsAnonAnon) {
+  auto Decls =
+      makeNamedDecls("namespace { class X; }",
+                     "namespace { namespace { class X; } }", Lang_CXX03, "X");
+  EXPECT_FALSE(testStructuralMatch(Decls));
+}
+
+TEST_F(StructuralEquivalenceRecordContextTest,
+       NamespaceNamedNamedVsNamedNamed) {
+  auto Decls = makeNamedDecls("namespace A { namespace N { class X; } }",
+                              "namespace B { namespace N { class X; } }",
+                              Lang_CXX03, "X");
+  EXPECT_FALSE(testStructuralMatch(Decls));
+}
+
+TEST_F(StructuralEquivalenceRecordContextTest, NamespaceNamedVsInline) {
+  auto Decls = makeNamedDecls("namespace A { namespace A { class X; } }",
+                              "namespace A { inline namespace A { class X; } }",
+                              Lang_CXX17, "X");
+  EXPECT_FALSE(testStructuralMatch(Decls));
+}
+
+TEST_F(StructuralEquivalenceRecordContextTest, NamespaceInlineVsInline) {
+  auto Decls = makeNamedDecls("namespace A { inline namespace A { class X; } }",
+                              "namespace A { inline namespace B { class X; } }",
+                              Lang_CXX17, "X");
+  EXPECT_TRUE(testStructuralMatch(Decls));
+}
+
+TEST_F(StructuralEquivalenceRecordContextTest, NamespaceInlineTopLevel) {
+  auto Decls =
+      makeNamedDecls("inline namespace A { class X; } }",
+                     "inline namespace B { class X; } }", Lang_CXX17, "X");
+  EXPECT_TRUE(testStructuralMatch(Decls));
+}
+
+TEST_F(StructuralEquivalenceRecordContextTest, TransparentContext) {
+  auto Decls =
+      makeNamedDecls("extern \"C\" { class X; }", "class X;", Lang_CXX03, "X");
+  EXPECT_TRUE(testStructuralMatch(Decls));
+}
+
+TEST_F(StructuralEquivalenceRecordContextTest, TransparentContextNE) {
+  auto Decls = makeNamedDecls("extern \"C\" { class X; }",
+                              "namespace { class X; }", Lang_CXX03, "X");
+  EXPECT_FALSE(testStructuralMatch(Decls));
+}
+
+TEST_F(StructuralEquivalenceRecordContextTest, TransparentContextInNamespace) {
+  auto Decls = makeNamedDecls("extern \"C\" { namespace N { class X; } }",
+                              "namespace N { extern \"C\" { class X; } }",
+                              Lang_CXX03, "X");
+  EXPECT_TRUE(testStructuralMatch(Decls));
+}
+
+TEST_F(StructuralEquivalenceTest, NamespaceOfRecordMember) {
+  auto Decls = makeNamedDecls(
+      R"(
+      class X;
+      class Y { X* x; };
+      )",
+      R"(
+      namespace N { class X; }
+      class Y { N::X* x; };
+      )",
+      Lang_CXX03, "Y");
+  EXPECT_FALSE(testStructuralMatch(Decls));
+}
+
+TEST_F(StructuralEquivalenceTest, StructDefinitionInPrototype) {
+  auto Decls =
+      makeNamedDecls("struct Param { int a; }; void foo(struct Param *p);",
+                     "void foo(struct Param { int a; } *p);", Lang_C89);
+  EXPECT_TRUE(testStructuralMatch(Decls));
+}
+
+TEST_F(StructuralEquivalenceTest, StructDefinitionInPrototypeDifferentName) {
+  auto Decls =
+      makeNamedDecls("struct Param1 { int a; }; void foo(struct Param1 *p);",
+                     "void foo(struct Param2 { int a; } *p);", Lang_C89);
+  EXPECT_FALSE(testStructuralMatch(Decls));
+}
+
+TEST_F(StructuralEquivalenceRecordContextTest, RecordInsideFunction) {
+  auto Decls = makeNamedDecls("struct Param { int a; };",
+                              "void f() { struct Param { int a; }; }", Lang_C89,
+                              "Param");
+  EXPECT_TRUE(testStructuralMatch(Decls));
+}
+
 struct StructuralEquivalenceEnumTest : StructuralEquivalenceTest {};
 
 TEST_F(StructuralEquivalenceEnumTest, FwdDeclEnumShouldBeEqualWithFwdDeclEnum) {
   auto t = makeNamedDecls("enum class foo;", "enum class foo;", Lang_CXX11);
   EXPECT_TRUE(testStructuralMatch(t));
 }
 
 TEST_F(StructuralEquivalenceEnumTest,