[ASTImporter] Various source location and range import fixes.

[balazske]

Summary

ASTImporter contained wrong or missing imports of SourceLocation
and SourceRange for some objects. At least a part of such errors
is fixed now.
Tests for SourceLocation import do not exist yet. A separate
patch is needed to add a general SourceLocation import test
that runs on every import as part of the already existing tests.

[balazske]

• Import BraceRange of EnumDecl.

[shafik]

I don't see any regressions but I am a little uncomfortable since there are no tests. So I would feel better if this was split into three parts: Namespaces, Enums and Templates.

Are there small test programs that fail due to the missing data? We can add them as regression tests.

lib/AST/ASTImporter.cpp

6127–6128

Curious why you decided to add the new arguments to the front as opposed to the end?

7148

Can you explain why E->getNameInfo().getLoc() is more correct than E->getExprLoc()?

7223

We still want to import a few lines down even if !E->getTemplateKeywordLoc().isValid()?

[balazske]

There is a test for the SourceLocation import in https://reviews.llvm.org/D60463 (but this test does not work until the ordering of imported Decl is same as original).

[balazske]

lib/AST/ASTImporter.cpp

6127–6128

This overload of ImportTemplateArgumentListInfo already exists before the patch. It looks like that the last argument is the output value and the arguments before are input values.

7148

The getExprLoc returns (if I am correct) the begin location and getNameInfo().getLoc() returns in "X<T>::value" location of "value" (this should not be the same as BeginLoc, may be it is the EndLoc?). (The begin and end location is not imported explicitly, it is obtained from other location values in the expression object.) I just observed that E->getLocation() can be used instead of E->getNameInfo().getLoc().
(The reason why this is correct that the test in D60463 indicates failure if getExprLoc is used, the begin and end locations are the same. That test works only in our Ericsson fork because Decl reordering issue.)

7223

The TemplateKeywordLoc can be invalid if the optional template keyword is missing. The import function can import an invalid SourceLocation (as invalid but not error).

[shafik]

So an alternative to testing could be matching the AST output from a clang-import-test like we did here:

https://reviews.llvm.org/D61140

although it unfortunately looks like only only the AST dump of NamespaceDecl output the SourceLocation you are fixing, see it on godbolt when I try a clang-import-test it looks like it is missing that SourceLocation:

|-NamespaceDecl 0x7f8c1f884750 <enum.cpp:1:1, <invalid sloc>> col:11

and with your patch applied I see the SourceLocation:

|-NamespaceDecl 0x7fe3e6882f50 <enum.cpp:1:1, line:7:1> line:1:11 A

[shafik]

I think the AST dump for EnumDecl and ClassTemplateSpecializationDecl should be dumping the missing SourceLocations but I am having trouble tracking down where that should be done.

[balazske]

It looks like that the same test can be applied as in D60463 but check only the first line of the AST dump. The first line contains information about the actual Decl only. This checks less than checking the full AST dump but finds some of wrong SourceLocation import. The problem is that the test can not be added yet because there are other failures. At least one new patch is needed with corrections related to import of TypeSourceInfo. Anyway this change (and the others) can not be added together with the test because multiple patches (including this) are needed to make the test not fail.

Source locations can be observed here:
http://ce.steveire.com/z/WB9VAu

[shafik]

Actually I was mistaken, we can see the difference for EnumDecl and ClassTemplateSpecializationDecl as well.

For EnumDecl before:

EnumDecl 0x7fd0ae884800 <line:2:1, col:6> col:6 referenced B

After:

EnumDecl 0x7fa703882600 <line:2:1, line:6:1> line:2:6 referenced B

For ClassTemplateSpecializationDecl before:

ClassTemplateSpecializationDecl 0x7f8c338846a0 <line:4:1, line:5:7> col:7 class A definition

after:

ClassTemplateSpecializationDecl 0x7fca150832a0 <line:4:1, line:7:1> line:5:7 class A definition

So once we have tests similar to D61140 I would feel good about this change. Especially since it is not clear when the other changes will be in and tests are important to guard against regressions.

[balazske]

• Import BraceRange of EnumDecl.
• Added lit tests.

[balazske]

Added lit tests for the simple (Decl) cases. The Expr and type related changes are more tricky to do. But I do not like this approach to add new tests because the test function in D60463 does almost the same thing for every import in ASTTests without adding lot of new checks manually.

[balazske]

Ping

[martong]

Ping @shafik @balazske

[martong]

@shafik Ping
As I see, balazske added the desired test, Shafik could you please take a look?

[balazske]

Rebased to monorepo and newer master.

[shafik]

LGTM

[dyung]

This change caused the Import/namespace/struct-and-var/test.cpp test to fail on ARM due to an extra line with `-CXXRecordDecl being emitted by the compiler that was being matched instead of the intended line. I checked in a fix to tighten up the check a little more so that it gets the correct line in 757bc55. I don't think it should negatively affect the test, but please do review the change.

Sample test failure from build bot:
http://lab.llvm.org:8011/builders/llvm-clang-win-x-armv7l/builds/1131/steps/test-check-clang/logs/FAIL%3A%20Clang%3A%3Atest.cpp

******************** TEST 'Clang :: Import/struct-and-var/test.cpp' FAILED ********************
Script:
--
: 'RUN: at line 1';   clang-import-test -dump-ast --import C:\buildbot\as-builder-1\llvm-clang-win-x-armv7l\llvm-project\clang\test\Import\struct-and-var/Inputs/S1.cpp --import C:\buildbot\as-builder-1\llvm-clang-win-x-armv7l\llvm-project\clang\test\Import\struct-and-var/Inputs/S2.cpp -expression C:\buildbot\as-builder-1\llvm-clang-win-x-armv7l\llvm-project\clang\test\Import\struct-and-var\test.cpp | c:\buildbot\as-builder-1\llvm-clang-win-x-armv7l\build\bin\filecheck.exe C:\buildbot\as-builder-1\llvm-clang-win-x-armv7l\llvm-project\clang\test\Import\struct-and-var\test.cpp
--
Exit Code: 1

Command Output (stdout):
--
$ ":" "RUN: at line 1"
$ "clang-import-test" "-dump-ast" "--import" "C:\buildbot\as-builder-1\llvm-clang-win-x-armv7l\llvm-project\clang\test\Import\struct-and-var/Inputs/S1.cpp" "--import" "C:\buildbot\as-builder-1\llvm-clang-win-x-armv7l\llvm-project\clang\test\Import\struct-and-var/Inputs/S2.cpp" "-expression" "C:\buildbot\as-builder-1\llvm-clang-win-x-armv7l\llvm-project\clang\test\Import\struct-and-var\test.cpp"
$ "c:\buildbot\as-builder-1\llvm-clang-win-x-armv7l\build\bin\filecheck.exe" "C:\buildbot\as-builder-1\llvm-clang-win-x-armv7l\llvm-project\clang\test\Import\struct-and-var\test.cpp"
# command stderr:
C:\buildbot\as-builder-1\llvm-clang-win-x-armv7l\llvm-project\clang\test\Import\struct-and-var\test.cpp:4:16: error: CHECK-SAME: is not on the same line as the previous match

// CHECK-SAME: Inputs/S2.cpp:1:1, line:3:1> line:1:8 struct F

               ^

<stdin>:49:126: note: 'next' match was here

`-CXXRecordDecl 0x63d9992750 <C:\buildbot\as-builder-1\llvm-clang-win-x-armv7l\llvm-project\clang\test\Import\struct-and-var/Inputs/S2.cpp:1:1, line:3:1> line:1:8 struct F definition

                                                                                                                             ^

<stdin>:25:18: note: previous match ended here

| `-CXXRecordDecl 0x63d9992378 <<invalid sloc>> <invalid sloc> implicit <undeserialized declarations> struct __va_list definition

                 ^

[balazske]

The fix looks OK (other alternative is to remove the CHECK with CXXRecordDecl, or make a single line with regular expressions).

lib/AST/ASTImporter.cpp

if (!ConflictingDecls.empty()) {
       if (!Name)
         return make_error<ImportError>(ImportError::NameConflict);
     }
   }
 
   ExpectedSLoc BeginLocOrErr = import(D->getBeginLoc());
   if (!BeginLocOrErr)
     return BeginLocOrErr.takeError();
+  ExpectedSLoc RBraceLocOrErr = import(D->getRBraceLoc());
+  if (!RBraceLocOrErr)
+    return RBraceLocOrErr.takeError();
 
   // Create the "to" namespace, if needed.
   NamespaceDecl *ToNamespace = MergeWithNamespace;
   if (!ToNamespace) {
     if (GetImportedOrCreateDecl(
             ToNamespace, D, Importer.getToContext(), DC, D->isInline(),
             *BeginLocOrErr, Loc, Name.getAsIdentifierInfo(),
             /*PrevDecl=*/nullptr))
       return ToNamespace;
+    ToNamespace->setRBraceLoc(*RBraceLocOrErr);
     ToNamespace->setLexicalDeclContext(LexicalDC);
     LexicalDC->addDeclInternal(ToNamespace);
 
     // If this is an anonymous namespace, register it as the anonymous
     // namespace within its context.
     if (!Name) {
       if (auto *TU = dyn_cast<TranslationUnitDecl>(DC))
         TU->setAnonymousNamespace(ToNamespace);
if (auto *DCXX = dyn_cast<CXXRecordDecl>(D)) {
     if (GetImportedOrCreateDecl(D2, D, Importer.getToContext(),
                                 D->getTagKind(), DC, *BeginLocOrErr, Loc,
                                 Name.getAsIdentifierInfo(), PrevDecl))
       return D2;
     D2->setLexicalDeclContext(LexicalDC);
     LexicalDC->addDeclInternal(D2);
   }
 
+  if (auto BraceRangeOrErr = import(D->getBraceRange()))
+    D2->setBraceRange(*BraceRangeOrErr);
+  else
+    return BraceRangeOrErr.takeError();
   if (auto QualifierLocOrErr = import(D->getQualifierLoc()))
     D2->setQualifierInfo(*QualifierLocOrErr);
   else
     return QualifierLocOrErr.takeError();
 
   if (D->isAnonymousStructOrUnion())
     D2->setAnonymousStructOrUnion(true);
 
ExpectedDecl ASTNodeImporter::VisitClassTemplateSpecializationDecl(
   // Set the context of this specialization/instantiation.
   D2->setLexicalDeclContext(LexicalDC);
 
   // Add to the DC only if it was an explicit specialization/instantiation.
   if (D2->isExplicitInstantiationOrSpecialization()) {
     LexicalDC->addDeclInternal(D2);
   }
 
+  if (auto BraceRangeOrErr = import(D->getBraceRange()))
+    D2->setBraceRange(*BraceRangeOrErr);
+  else
+    return BraceRangeOrErr.takeError();
+
   // Import the qualifier, if any.
   if (auto LocOrErr = import(D->getQualifierLoc()))
     D2->setQualifierInfo(*LocOrErr);
   else
     return LocOrErr.takeError();
 
   if (auto *TSI = D->getTypeAsWritten()) {
     if (auto TInfoOrErr = import(TSI))
if (!FoundDOrErr)
       return FoundDOrErr.takeError();
     ToFoundD = *FoundDOrErr;
   }
 
   TemplateArgumentListInfo ToTAInfo;
   TemplateArgumentListInfo *ToResInfo = nullptr;
   if (E->hasExplicitTemplateArgs()) {
     if (Error Err =
-        ImportTemplateArgumentListInfo(E->template_arguments(), ToTAInfo))
+            ImportTemplateArgumentListInfo(E->getLAngleLoc(), E->getRAngleLoc(),
+                                           E->template_arguments(), ToTAInfo))
       return std::move(Err);
     ToResInfo = &ToTAInfo;
   }
 
   auto *ToE = DeclRefExpr::Create(
       Importer.getToContext(), ToQualifierLoc, ToTemplateKeywordLoc, ToDecl,
       E->refersToEnclosingVariableOrCapture(), ToLocation, ToType,
       E->getValueKind(), ToFoundD, ToResInfo);
ExpectedStmt ASTNodeImporter::VisitCXXDependentScopeMemberExpr(
   return CXXDependentScopeMemberExpr::Create(
       Importer.getToContext(), ToBase, ToType, E->isArrow(), ToOperatorLoc,
       ToQualifierLoc, ToTemplateKeywordLoc, ToFirstQualifierFoundInScope,
       ToMemberNameInfo, ResInfo);
 }
 
 ExpectedStmt
 ASTNodeImporter::VisitDependentScopeDeclRefExpr(DependentScopeDeclRefExpr *E) {
-  auto Imp = importSeq(
-      E->getQualifierLoc(), E->getTemplateKeywordLoc(), E->getDeclName(),
-      E->getExprLoc(), E->getLAngleLoc(), E->getRAngleLoc());
+  auto Imp = importSeq(E->getQualifierLoc(), E->getTemplateKeywordLoc(),
+                       E->getDeclName(), E->getNameInfo().getLoc(),
+                       E->getLAngleLoc(), E->getRAngleLoc());
   if (!Imp)
     return Imp.takeError();
 
   NestedNameSpecifierLoc ToQualifierLoc;
-  SourceLocation ToTemplateKeywordLoc, ToExprLoc, ToLAngleLoc, ToRAngleLoc;
+  SourceLocation ToTemplateKeywordLoc, ToNameLoc, ToLAngleLoc, ToRAngleLoc;
   DeclarationName ToDeclName;
-  std::tie(
-      ToQualifierLoc, ToTemplateKeywordLoc, ToDeclName, ToExprLoc,
+  std::tie(ToQualifierLoc, ToTemplateKeywordLoc, ToDeclName, ToNameLoc,
            ToLAngleLoc, ToRAngleLoc) = *Imp;
-
-  DeclarationNameInfo ToNameInfo(ToDeclName, ToExprLoc);
+  DeclarationNameInfo ToNameInfo(ToDeclName, ToNameLoc);
   if (Error Err = ImportDeclarationNameLoc(E->getNameInfo(), ToNameInfo))
     return std::move(Err);
 
   TemplateArgumentListInfo ToTAInfo(ToLAngleLoc, ToRAngleLoc);
   TemplateArgumentListInfo *ResInfo = nullptr;
   if (E->hasExplicitTemplateArgs()) {
     if (Error Err =
         ImportTemplateArgumentListInfo(E->template_arguments(), ToTAInfo))
ASTNodeImporter::VisitUnresolvedLookupExpr(UnresolvedLookupExpr *E) {
 
   UnresolvedSet<8> ToDecls;
   for (auto *D : E->decls())
     if (auto ToDOrErr = import(D))
       ToDecls.addDecl(cast<NamedDecl>(*ToDOrErr));
     else
       return ToDOrErr.takeError();
 
-  if (E->hasExplicitTemplateArgs() && E->getTemplateKeywordLoc().isValid()) {
+  if (E->hasExplicitTemplateArgs()) {
     TemplateArgumentListInfo ToTAInfo;
     if (Error Err = ImportTemplateArgumentListInfo(
         E->getLAngleLoc(), E->getRAngleLoc(), E->template_arguments(),
         ToTAInfo))
       return std::move(Err);
 
     ExpectedSLoc ToTemplateKeywordLocOrErr = import(E->getTemplateKeywordLoc());
     if (!ToTemplateKeywordLocOrErr)
for (Decl *D : E->decls())
     if (auto ToDOrErr = import(D))
       ToDecls.addDecl(cast<NamedDecl>(*ToDOrErr));
     else
       return ToDOrErr.takeError();
 
   TemplateArgumentListInfo ToTAInfo;
   TemplateArgumentListInfo *ResInfo = nullptr;
   if (E->hasExplicitTemplateArgs()) {
-    if (Error Err =
-        ImportTemplateArgumentListInfo(E->template_arguments(), ToTAInfo))
+    TemplateArgumentListInfo FromTAInfo;
+    E->copyTemplateArgumentsInto(FromTAInfo);
+    if (Error Err = ImportTemplateArgumentListInfo(FromTAInfo, ToTAInfo))
       return std::move(Err);
     ResInfo = &ToTAInfo;
   }
 
   Expr *ToBase = nullptr;
   if (!E->isImplicitAccess()) {
     if (ExpectedExpr ToBaseOrErr = import(E->getBase()))
       ToBase = *ToBaseOrErr;
while (!NestedNames.empty()) {
 
     case NestedNameSpecifier::TypeSpec:
     case NestedNameSpecifier::TypeSpecWithTemplate: {
       SourceLocation ToTLoc;
       if (Error Err = importInto(ToTLoc, NNS.getTypeLoc().getBeginLoc()))
         return std::move(Err);
       TypeSourceInfo *TSI = getToContext().getTrivialTypeSourceInfo(
             QualType(Spec->getAsType(), 0), ToTLoc);
+      if (Kind == NestedNameSpecifier::TypeSpecWithTemplate)
+        // ToLocalBeginLoc is here the location of the 'template' keyword.
         Builder.Extend(getToContext(), ToLocalBeginLoc, TSI->getTypeLoc(),
                        ToLocalEndLoc);
+      else
+        // No location for 'template' keyword here.
+        Builder.Extend(getToContext(), SourceLocation{}, TSI->getTypeLoc(),
+                       ToLocalEndLoc);
       break;
     }
 
     case NestedNameSpecifier::Global:
       Builder.MakeGlobal(getToContext(), ToLocalBeginLoc);
       break;
 
     case NestedNameSpecifier::Super: {