[ASTImporter] Use InjectedClassNameType at import of templated record.

[balazske]

Summary

At import of a record describing a template set its type to
InjectedClassNameType (instead of RecordType).

[martong]

LGTM! But let's wait for someone else's review too.

@a.sidorin
We discovered this error during the CTU analysis of google/protobuf and we could reduce the erroneous C file with c-reduce to the minimal example presented in the test file.

[martong]

Adding Rafael too as a reviewer, because he has been working also on the ASTImporter recently.

[r.stahl]

LGTM!

I'm not really too confident to approve changes yet, but with a second opinion it should be fine.

lib/AST/ASTImporter.cpp

2146

The only thing I'm wondering is whether the Decls looked up here are always known to be already imported.

[balazske]

lib/AST/ASTImporter.cpp

2146

These are in the 'To' context. It may be that the Injected is not found here, probably because not yet imported (in this case the import may be part of a not completed recursive process).

[r.stahl]

lib/AST/ASTImporter.cpp

2146

As far as I understand that corner case could be covered by doing the lookup on DCXX instead and then importing the injected decl. But then you wouldn't find it if it is only in the To context (if that is possible).

I mean if a user calls ImportDecl in another order specifically. But such a case is probably really artificial and I'm not sure if it's even makes sense or is already covered by ImportDeclParts.

It should be fine as it is.

[a.sidorin]

Hello, Balázs,

You can find my comments inline.

lib/AST/ASTImporter.cpp

2138

Maybe we should fix it a bit upper (line 2100)?

test/ASTMerge/injected-class-name-decl-1/Inputs/inject1.cpp

16 ↗

(On Diff #148805)

This looks like raw creduce output. Is there a way to simplify this or make human-readable? Do we really need nested namespaces, unused decls and other stuff not removed by creduce? I know that creduce is bad at reducing templates because we often meet similar output internally. But it is usually not a problem to resolve some redundancy manually to assist creduce. In this case, we can start by removing k and n.
We can leave this code as-is only if removing declarations or simplifying templates affects import order causing the bug to disappear. But even in this case we have to humanize the test.

[balazske]

lib/AST/ASTImporter.cpp

2138

The ToDescribed is used in this code, not available before the new record is created.

[balazske]

test/ASTMerge/injected-class-name-decl-1/Inputs/inject1.cpp

16 ↗

(On Diff #148805)

Probably remove this test? There was some bug in a previous version of the fix that was triggered by this test. Before that fix (and on current master) this test does not fail so it is not possible to simplify it.

[martong]

test/ASTMerge/injected-class-name-decl-1/Inputs/inject1.cpp

16 ↗

(On Diff #148805)

I vote on deleting this test then. We already have another clear and simple test.

[a_sidorin]

Hello Balázs,

Please clang-format the tests and delete injected-class-name-decl-1. Don't see any other issues.

lib/AST/ASTImporter.cpp

2139

Nit: "an InjectedClassNameType".

test/ASTMerge/injected-class-name-decl-1/Inputs/inject1.cpp

16 ↗

(On Diff #148805)

I think we should delete this test. As I see, it passes even in upstream clang, so it doesn't really make sense to keep it.

[balazske]

Small fixes.

[a.sidorin]

LGTM!

lib/AST/ASTImporter.cpp

if (auto *DCXX = dyn_cast<CXXRecordDecl>(D)) {
 
       if (ClassTemplateDecl *FromDescribed =
           DCXX->getDescribedClassTemplate()) {
         auto *ToDescribed = cast_or_null<ClassTemplateDecl>(
             Importer.Import(FromDescribed));
         if (!ToDescribed)
           return nullptr;
         D2CXX->setDescribedClassTemplate(ToDescribed);
+        if (!DCXX->isInjectedClassName()) {
+          // In a record describing a template the type should be an
+          // InjectedClassNameType (see Sema::CheckClassTemplate). Update the
+          // previously set type to the correct value here (ToDescribed is not
+          // available at record create).
+          // FIXME: The previous type is cleared but not removed from
+          // ASTContext's internal storage.
+          CXXRecordDecl *Injected = nullptr;
+          for (NamedDecl *Found : D2CXX->noload_lookup(Name)) {
+            auto *Record = dyn_cast<CXXRecordDecl>(Found);
+            if (Record && Record->isInjectedClassName()) {
+              Injected = Record;
+              break;
+            }
+          }
+          D2CXX->setTypeForDecl(nullptr);
+          Importer.getToContext().getInjectedClassNameType(D2CXX,
+              ToDescribed->getInjectedClassNameSpecialization());
+          if (Injected) {
+            Injected->setTypeForDecl(nullptr);
+            Importer.getToContext().getTypeDeclType(Injected, D2CXX);
+          }
+        }
       } else if (MemberSpecializationInfo *MemberInfo =
                    DCXX->getMemberSpecializationInfo()) {
         TemplateSpecializationKind SK =
             MemberInfo->getTemplateSpecializationKind();
         CXXRecordDecl *FromInst = DCXX->getInstantiatedFromMemberClass();
         auto *ToInst =
             cast_or_null<CXXRecordDecl>(Importer.Import(FromInst));
         if (FromInst && !ToInst)

test/ASTMerge/injected-class-name-decl/Inputs/inject1.cpp

+template <class X>
+class C { static X x; };

test/ASTMerge/injected-class-name-decl/Inputs/inject2.cpp

+template <class X>
+X C<X>::x;

test/ASTMerge/injected-class-name-decl/test.cpp

+// RUN: %clang_cc1 -std=c++1z -emit-pch -o %t.ast %S/Inputs/inject1.cpp
+// RUN: %clang_cc1 -std=c++1z -emit-obj -o /dev/null -ast-merge %t.ast %S/Inputs/inject2.cpp
+// expected-no-diagnostics