[ASTImporter] Update isUsed flag at Decl import.

[martong]

Summary

When a Decl is imported over an already existing similar one,
the isUsed flag is updated if it is set in the imported Decl.
This can happen if the Decl is used somewhere in the imported code
but was not used in the existing code.

[a.sidorin]

Hello Gabor,

I have a strong feeling of duplication with attribute and flags merging move in D47632. Maybe it is better to be resolved in that review by using the same code for attr/flag merging for both newly-created and mapped decls?

[a.sidorin]

Whoops, sorry Balázs. Didn't look at the review author :(

[balazske]

Originally this change was made after the patch in D47632 was done. It is here back-ported to the state before it. I like the idea of integrating this change into D47632. (This is a new functionality: Attributes are updated when something is found to be already imported, in contrast to copy of the attributes when a new object is created.)

[martong]

I have a strong feeling of duplication with attribute and flags merging move in D47632. Maybe it is better to be resolved in that review by using the same code for attr/flag merging for both newly-created and mapped decls?

Ok, then I'll integrate this into https://reviews.llvm.org/D47632.
Thanks for the review!

[martong]

Closed in favor of https://reviews.llvm.org/D47632

lib/AST/ASTImporter.cpp

namespace clang {
   }
 
   SmallVector<Decl*, 2> getCanonicalForwardRedeclChain(Decl* D) {
     // Currently only FunctionDecl is supported
     auto FD = cast<FunctionDecl>(D);
     return getCanonicalForwardRedeclChain<FunctionDecl>(FD);
   }
 
+  void updateAttrAndFlags(const Decl *From, Decl *To) {
+    // Check if some flags or attrs are new in 'From' and copy into 'To'.
+    // FIXME: Other flags or attrs?
+    if (From->isUsed(false) && !To->isUsed(false))
+      To->setIsUsed();
+  }
+
   class ASTNodeImporter : public TypeVisitor<ASTNodeImporter, QualType>,
                           public DeclVisitor<ASTNodeImporter, Decl *>,
                           public StmtVisitor<ASTNodeImporter, Stmt *> {
     ASTImporter &Importer;
 
   public:
     explicit ASTNodeImporter(ASTImporter &Importer) : Importer(Importer) {}
 
for (auto *FoundDecl : FoundDecls) {
       
       ConflictingDecls.push_back(FoundDecl);
     }
 
     if (MergeWithVar) {
       // An equivalent variable with external linkage has been found. Link 
       // the two declarations, then merge them.
       Importer.Imported(D, MergeWithVar);
+      updateAttrAndFlags(D, MergeWithVar);
       
       if (VarDecl *DDef = D->getDefinition()) {
         if (VarDecl *ExistingDef = MergeWithVar->getDefinition()) {
           Importer.ToDiag(ExistingDef->getLocation(), 
                           diag::err_odr_variable_multiple_def)
             << Name;
           Importer.FromDiag(DDef->getLocation(), diag::note_odr_defined_here);
         } else {
Decl *ASTImporter::Import(Decl *FromD) {
 
   ASTNodeImporter Importer(*this);
 
   // Check whether we've already imported this declaration.  
   llvm::DenseMap<Decl *, Decl *>::iterator Pos = ImportedDecls.find(FromD);
   if (Pos != ImportedDecls.end()) {
     Decl *ToD = Pos->second;
     Importer.ImportDefinitionIfNeeded(FromD, ToD);
+    // If FromD has some updated flags after last import, apply it.
+    updateAttrAndFlags(FromD, ToD);
     return ToD;
   }
   
   // Import the type
   Decl *ToD = Importer.Visit(FromD);
   if (!ToD)
     return nullptr;
 

unittests/AST/ASTImporterTest.cpp

class ASTImporterTestBase : public ParameterizedTestsFixture {
   std::string ToCode;
 
   struct TU {
     // Buffer for the context, must live in the test scope.
     std::string Code;
     std::string FileName;
     std::unique_ptr<ASTUnit> Unit;
     TranslationUnitDecl *TUDecl = nullptr;
+    std::unique_ptr<ASTImporter> Importer;
     TU(StringRef Code, StringRef FileName, ArgVector Args)
         : Code(Code), FileName(FileName),
           Unit(tooling::buildASTFromCodeWithArgs(this->Code, Args,
                                                  this->FileName)),
           TUDecl(Unit->getASTContext().getTranslationUnitDecl()) {
       Unit->enableSourceFileDiagnostics();
     }
+
+    Decl *import(ASTUnit *ToAST, Decl *FromDecl) {
+      assert(ToAST);
+      if (!Importer) {
+        Importer.reset(new ASTImporter(
+            ToAST->getASTContext(), ToAST->getFileManager(),
+            Unit->getASTContext(), Unit->getFileManager(), false));
+      }
+      return Importer->Import(FromDecl);
+    }
   };
 
   // We may have several From contexts and related translation units. In each
   // AST, the buffers for the source are handled via references and are set
   // during the creation of the AST. These references must point to a valid
   // buffer until the AST is alive. Thus, we must use a list in order to avoid
   // moving of the stored objects because that would mean breaking the
   // references in the AST. By using a vector a move could happen when the
getImportedDecl(StringRef FromSrcCode, Language FromLang, StringRef ToSrcCode,
     FromTUs.emplace_back(FromSrcCode, InputFileName, FromArgs);
     TU &FromTU = FromTUs.back();
 
     ToCode = ToSrcCode;
     assert(!ToAST);
     ToAST = tooling::buildASTFromCodeWithArgs(ToCode, ToArgs, OutputFileName);
     ToAST->enableSourceFileDiagnostics();
 
-    ASTContext &FromCtx = FromTU.Unit->getASTContext(),
-               &ToCtx = ToAST->getASTContext();
+    ASTContext &FromCtx = FromTU.Unit->getASTContext();
 
     createVirtualFileIfNeeded(ToAST.get(), InputFileName, FromTU.Code);
 
-    ASTImporter Importer(ToCtx, ToAST->getFileManager(), FromCtx,
-                         FromTU.Unit->getFileManager(), false);
-
     IdentifierInfo *ImportedII = &FromCtx.Idents.get(Identifier);
     assert(ImportedII && "Declaration with the given identifier "
                          "should be specified in test!");
     DeclarationName ImportDeclName(ImportedII);
     SmallVector<NamedDecl *, 4> FoundDecls;
     FromCtx.getTranslationUnitDecl()->localUncachedLookup(ImportDeclName,
                                                           FoundDecls);
 
     assert(FoundDecls.size() == 1);
 
-    Decl *Imported = Importer.Import(FoundDecls.front());
+    Decl *Imported = FromTU.import(ToAST.get(), FoundDecls.front());
+
     assert(Imported);
     return std::make_tuple(*FoundDecls.begin(), Imported);
   }
 
   // Creates a TU decl for the given source code which can be used as a From
   // context.  May be called several times in a given test (with different file
   // name).
   TranslationUnitDecl *getTuDecl(StringRef SrcCode, Language Lang,
Decl *Import(Decl *From, Language ToLang) {
     // which we want to import the `From` Decl. Without this source locations
     // will be invalid in the ToCtx.
     auto It = std::find_if(FromTUs.begin(), FromTUs.end(), [From](const TU &E) {
       return E.TUDecl == From->getTranslationUnitDecl();
     });
     assert(It != FromTUs.end());
     createVirtualFileIfNeeded(ToAST.get(), It->FileName, It->Code);
 
-    ASTContext &FromCtx = From->getASTContext(),
-               &ToCtx = ToAST->getASTContext();
-    ASTImporter Importer(ToCtx, ToAST->getFileManager(), FromCtx,
-                         FromCtx.getSourceManager().getFileManager(), false);
-    return Importer.Import(From);
+    return It->import(ToAST.get(), From);
   }
 
   ~ASTImporterTestBase() {
     if (!::testing::Test::HasFailure()) return;
 
     for (auto &Tu : FromTUs) {
       assert(Tu.Unit);
       llvm::errs() << "FromAST:\n";
TEST_P(CanonicalRedeclChain, ShouldBeSameForAllDeclInTheChain) {
   auto RedeclsD0 = getCanonicalForwardRedeclChain(D0);
   auto RedeclsD1 = getCanonicalForwardRedeclChain(D1);
   auto RedeclsD2 = getCanonicalForwardRedeclChain(D2);
 
   EXPECT_THAT(RedeclsD0, ::testing::ContainerEq(RedeclsD1));
   EXPECT_THAT(RedeclsD1, ::testing::ContainerEq(RedeclsD2));
 }
 
+TEST_P(ASTImporterTestBase, ImportDoesUpdateUsedFlag) {
+  auto Pattern = varDecl(hasName("x"));
+  VarDecl *Imported1;
+  {
+    Decl *FromTU = getTuDecl("extern int x;", Lang_CXX, "input0.cc");
+    auto *FromD = FirstDeclMatcher<VarDecl>().match(FromTU, Pattern);
+    Imported1 = cast<VarDecl>(Import(FromD, Lang_CXX));
+  }
+  VarDecl *Imported2;
+  {
+    Decl *FromTU = getTuDecl("int x;", Lang_CXX, "input1.cc");
+    auto *FromD = FirstDeclMatcher<VarDecl>().match(FromTU, Pattern);
+    Imported2 = cast<VarDecl>(Import(FromD, Lang_CXX));
+  }
+  EXPECT_EQ(Imported1->getCanonicalDecl(), Imported2->getCanonicalDecl());
+  EXPECT_FALSE(Imported2->isUsed(false));
+  {
+    Decl *FromTU = getTuDecl(
+        "extern int x; int f() { return x; }", Lang_CXX, "input2.cc");
+    auto *FromD =
+        FirstDeclMatcher<FunctionDecl>().match(FromTU, functionDecl());
+    Import(FromD, Lang_CXX);
+  }
+  EXPECT_TRUE(Imported2->isUsed(false));
+}
+
+TEST_P(ASTImporterTestBase, ReimportWithUsedFlag) {
+  auto Pattern = varDecl(hasName("x"));
+  
+  Decl *FromTU = getTuDecl("int x;", Lang_CXX, "input0.cc");
+  auto *FromD = FirstDeclMatcher<VarDecl>().match(FromTU, Pattern);
+  
+  auto *Imported1 = cast<VarDecl>(Import(FromD, Lang_CXX));
+  
+  ASSERT_FALSE(Imported1->isUsed(false));
+
+  FromD->setIsUsed();
+  auto *Imported2 = cast<VarDecl>(Import(FromD, Lang_CXX));
+  
+  EXPECT_EQ(Imported1, Imported2);
+  EXPECT_TRUE(Imported2->isUsed(false));
+}
 
 TEST_P(ImportExpr, ImportStringLiteral) {
   MatchVerifier<Decl> Verifier;
   testImport("void declToImport() { \"foo\"; }",
              Lang_CXX, "", Lang_CXX, Verifier,
              functionDecl(
                hasBody(
                  compoundStmt(