ClangRefactoring: Improve symbol parsing

Declarations are only indexed if their file has been changed and references
has to be indexed if the file or any included file has been changed.

Change-Id: I07c6de1379bce2462c1e0fad34d4378a3da4397b
Reviewed-by: Ivan Donchevskii <ivan.donchevskii@qt.io>

src/tools/clangrefactoringbackend/source/collectsymbolsaction.h

@@ -75,7 +75,7 @@ private:
         clang::index::IndexingOptions options;
 
         options.SystemSymbolFilter = clang::index::IndexingOptions::SystemSymbolFilterKind::None;
-        options.IndexFunctionLocals = false;
+        options.IndexFunctionLocals = true;
 
         return options;
     }

src/tools/clangrefactoringbackend/source/indexdataconsumer.cpp

@@ -99,11 +99,29 @@ SymbolKindAndTags symbolKindAndTags(const clang::Decl *declaration)
     return visitor.Visit(declaration);
 }
 
-bool isContextIndependentDeclaration(const clang::Decl *declaration)
+bool isDeclaration(clang::index::SymbolRoleSet symbolRoles)
 {
-    return clang::dyn_cast<clang::ValueDecl>(declaration)
+    using namespace clang::index;
-            || clang::dyn_cast<clang::TypeDecl>(declaration);
+
+    return symbolRoles & (uint(SymbolRole::Declaration) | uint(SymbolRole::Definition));
 }
+
+bool isReference(clang::index::SymbolRoleSet symbolRoles)
+{
+    using namespace clang::index;
+
+    return symbolRoles & (uint(SymbolRole::Reference) | uint(SymbolRole::Call));
+}
+
+}
+
+bool IndexDataConsumer::skipSymbol(clang::FileID fileId, clang::index::SymbolRoleSet symbolRoles)
+{
+    bool alreadyParsed = isAlreadyParsed(fileId);
+    bool isParsedDeclaration = alreadyParsed && isDeclaration(symbolRoles);
+    bool isParsedReference = alreadyParsed && !dependentFilesAreModified() && isReference(symbolRoles);
+
+    return isParsedDeclaration || isParsedReference;
 }
 
 bool IndexDataConsumer::handleDeclOccurence(const clang::Decl *declaration,
@@ -118,7 +136,7 @@ bool IndexDataConsumer::handleDeclOccurence(const clang::Decl *declaration,
         if (!namedDeclaration->getIdentifier())
             return true;
 
-        if (alreadyParsed(fileId) && isContextIndependentDeclaration(declaration))
+        if (skipSymbol(fileId, symbolRoles))
             return true;
 
         SymbolIndex globalId = toSymbolIndex(declaration->getCanonicalDecl());

src/tools/clangrefactoringbackend/source/indexdataconsumer.h

@@ -59,6 +59,9 @@ public:
                              unsigned offset,
                              ASTNodeInfo astNodeInfo) override;
 
+private:
+    bool skipSymbol(clang::FileID fileId, clang::index::SymbolRoleSet symbolRoles);
+
 private:
     SymbolEntries &m_symbolEntries;
     SourceLocationEntries &m_sourceLocationEntries;

src/tools/clangrefactoringbackend/source/sourcesmanager.h

@@ -59,7 +59,7 @@ public:
         if (!upToDate)
            addOrUpdateNewEntry(filePathId, modifiedTime);
 
-        m_dependendFilesModified = m_dependendFilesModified || !upToDate;
+        m_dependentFilesModified = m_dependentFilesModified || !upToDate;
 
         return upToDate ;
     }
@@ -82,17 +82,17 @@ public:
 
         m_modifiedTimeStamps = std::move(mergedModifiedTimeStamps);
         m_newModifiedTimeStamps.clear();
-        m_dependendFilesModified = false;
+        m_dependentFilesModified = false;
     }
 
-    bool dependendFilesModified() const
+    bool dependentFilesModified() const
     {
-        return m_dependendFilesModified;
+        return m_dependentFilesModified;
     }
 
-    bool alreadyParsedAllDependFiles(FilePathId filePathId, std::time_t modifiedTime)
+    bool alreadyParsedAllDependentFiles(FilePathId filePathId, std::time_t modifiedTime)
     {
-        return alreadyParsed(filePathId, modifiedTime) && !dependendFilesModified();
+        return alreadyParsed(filePathId, modifiedTime) && !dependentFilesModified();
     }
 
 private:
@@ -114,7 +114,7 @@ private:
 private:
     std::vector<FilePathIdTime> m_modifiedTimeStamps;
     std::vector<FilePathIdTime> m_newModifiedTimeStamps;
-    bool m_dependendFilesModified = false;
+    bool m_dependentFilesModified = false;
 };
 
 }

src/tools/clangrefactoringbackend/source/symbolscollector.cpp

@@ -51,6 +51,7 @@ void SymbolsCollector::addUnsavedFiles(const V2::FileContainers &unsavedFiles)
 
 void SymbolsCollector::clear()
 {
+    m_indexDataConsumer->clear();
     m_collectMacrosSourceFileCallbacks.clear();
     m_symbolEntries.clear();
     m_sourceLocationEntries.clear();

src/tools/clangrefactoringbackend/source/symbolsvisitorbase.h

@@ -60,7 +60,12 @@ public:
         return filePathId(fileEntry);
     }
 
-    bool alreadyParsed(clang::FileID fileId)
+    bool dependentFilesAreModified()
+    {
+        return m_sourcesManager.dependentFilesModified();
+    }
+
+    bool isAlreadyParsed(clang::FileID fileId)
     {
         const clang::FileEntry *fileEntry = m_sourceManager->getFileEntryForID(fileId);
 
@@ -70,7 +75,7 @@ public:
 
     bool alreadyParsed(clang::SourceLocation sourceLocation)
     {
-        return alreadyParsed(m_sourceManager->getFileID(sourceLocation));
+        return isAlreadyParsed(m_sourceManager->getFileID(sourceLocation));
     }
 
     FilePathId filePathId(const clang::FileEntry *fileEntry)
@@ -151,6 +156,11 @@ public:
                     m_sourceManager->getSLocEntry(fileId).getFile().getFileCharacteristic());
     }
 
+    void clear()
+    {
+        m_filePathIndices.clear();
+    }
+
 protected:
     std::vector<FilePathId> m_filePathIndices;
     FilePathCachingInterface &m_filePathCache;

tests/unit/unittest/data/include/symbolscollector_unmodified_header.h

@@ -3,9 +3,20 @@
 #define HEADER_DEFINE
 
 void HeaderFunction();
+void HeaderFunctionReference();
+void HeaderFunctionReferenceInMainFile();
 
 class Class
 {
 public:
     int Member = 20;
+    int MemberReference = 10;
+
+    void InlineHeaderMemberFunction()
+    {
+        int FunctionLocalVariable = 90;
+        MemberReference = 30;
+        HeaderFunctionReference();
+    }
 };
+

tests/unit/unittest/data/include/symbolscollector_unmodified_header2.h

@@ -0,0 +1,3 @@
+#pragma once
+
+void TouchHeaderFunction();

tests/unit/unittest/data/symbolscollector_unmodified.cpp

@@ -2,4 +2,7 @@
 
 #define MAINFILE_DEFINE
 
-void MainFileFunction();
+void MainFileFunction()
+{
+    HeaderFunctionReferenceInMainFile();
+}

tests/unit/unittest/data/symbolscollector_unmodified2.cpp

@@ -2,4 +2,8 @@
 
 #define MAINFILE_DEFINE
 
-void MainFileFunction();
+void MainFileFunction()
+{
+    HeaderFunctionReferenceInMainFile();
+}
+

tests/unit/unittest/data/symbolscollector_unmodified3.cpp

@@ -0,0 +1,9 @@
+#include <symbolscollector_unmodified_header.h>
+#include <symbolscollector_unmodified_header2.h>
+
+#define MAINFILE_DEFINE
+
+void MainFileFunction()
+{
+    HeaderFunctionReferenceInMainFile();
+}

tests/unit/unittest/sourcesmanager-test.cpp

@@ -138,14 +138,14 @@ TEST_F(SourcesManager, TimeIsUpdated)
 
 TEST_F(SourcesManager, AnyDependFileIsNotModifiedAfterInitialization)
 {
-    ASSERT_FALSE(sources.dependendFilesModified());
+    ASSERT_FALSE(sources.dependentFilesModified());
 }
 
 TEST_F(SourcesManager, AnyDependFileIsModified)
 {
     sources.alreadyParsed({1, 1}, 56);
 
-    ASSERT_TRUE(sources.dependendFilesModified());
+    ASSERT_TRUE(sources.dependentFilesModified());
 }
 
 TEST_F(SourcesManager, AnyDependFileIsModifiedAfterParsingTwoTimesSameTimeStamp)
@@ -153,7 +153,7 @@ TEST_F(SourcesManager, AnyDependFileIsModifiedAfterParsingTwoTimesSameTimeStamp)
     sources.alreadyParsed({1, 1}, 56);
     sources.alreadyParsed({1, 1}, 56);
 
-    ASSERT_TRUE(sources.dependendFilesModified());
+    ASSERT_TRUE(sources.dependentFilesModified());
 }
 
 TEST_F(SourcesManager, AnyDependFileIsNotModifiedAfterUpdate)
@@ -161,7 +161,7 @@ TEST_F(SourcesManager, AnyDependFileIsNotModifiedAfterUpdate)
     sources.alreadyParsed({1, 1}, 56);
     sources.updateModifiedTimeStamps();
 
-    ASSERT_FALSE(sources.dependendFilesModified());
+    ASSERT_FALSE(sources.dependentFilesModified());
 }
 
 TEST_F(SourcesManager, AnyDependFileIsNotModifiedAfterNotAlreadyPared)
@@ -171,7 +171,7 @@ TEST_F(SourcesManager, AnyDependFileIsNotModifiedAfterNotAlreadyPared)
 
     sources.alreadyParsed({1, 1}, 56);
 
-    ASSERT_FALSE(sources.dependendFilesModified());
+    ASSERT_FALSE(sources.dependentFilesModified());
 }
 
 TEST_F(SourcesManager, AnyDependFileIsNotModifiedAfterAlreadyPared)
@@ -181,7 +181,7 @@ TEST_F(SourcesManager, AnyDependFileIsNotModifiedAfterAlreadyPared)
 
     sources.alreadyParsed({1, 1}, 57);
 
-    ASSERT_TRUE(sources.dependendFilesModified());
+    ASSERT_TRUE(sources.dependentFilesModified());
 }
 
 TEST_F(SourcesManager, AnyDependFileIsModifiedAfterUpdateNewTimeStamp)
@@ -193,41 +193,41 @@ TEST_F(SourcesManager, AnyDependFileIsModifiedAfterUpdateNewTimeStamp)
 
     sources.alreadyParsed({1, 2}, 56);
 
-    ASSERT_TRUE(sources.dependendFilesModified());
+    ASSERT_TRUE(sources.dependentFilesModified());
 }
 
 TEST_F(SourcesManager, AlreadyParsedWithDependencyAfterUpdateNewTimeStamp)
 {
-    sources.alreadyParsedAllDependFiles({1, 1}, 56);
+    sources.alreadyParsedAllDependentFiles({1, 1}, 56);
-    sources.alreadyParsedAllDependFiles({1, 2}, 56);
+    sources.alreadyParsedAllDependentFiles({1, 2}, 56);
     sources.updateModifiedTimeStamps();
-    sources.alreadyParsedAllDependFiles({1, 1}, 57);
+    sources.alreadyParsedAllDependentFiles({1, 1}, 57);
 
-    bool alreadyParsed = sources.alreadyParsedAllDependFiles({1, 2}, 56);
+    bool alreadyParsed = sources.alreadyParsedAllDependentFiles({1, 2}, 56);
 
     ASSERT_FALSE(alreadyParsed);
 }
 
 TEST_F(SourcesManager, AlreadyParsedWithDependencyAfterUpdateNewSecondTimeStamp)
 {
-    sources.alreadyParsedAllDependFiles({1, 1}, 56);
+    sources.alreadyParsedAllDependentFiles({1, 1}, 56);
-    sources.alreadyParsedAllDependFiles({1, 2}, 56);
+    sources.alreadyParsedAllDependentFiles({1, 2}, 56);
     sources.updateModifiedTimeStamps();
-    sources.alreadyParsedAllDependFiles({1, 1}, 56);
+    sources.alreadyParsedAllDependentFiles({1, 1}, 56);
 
-    bool alreadyParsed = sources.alreadyParsedAllDependFiles({1, 2}, 57);
+    bool alreadyParsed = sources.alreadyParsedAllDependentFiles({1, 2}, 57);
 
     ASSERT_FALSE(alreadyParsed);
 }
 
 TEST_F(SourcesManager, AlreadyParsedWithDependencyAfterUpdateSameTimeStamps)
 {
-    sources.alreadyParsedAllDependFiles({1, 1}, 56);
+    sources.alreadyParsedAllDependentFiles({1, 1}, 56);
-    sources.alreadyParsedAllDependFiles({1, 2}, 56);
+    sources.alreadyParsedAllDependentFiles({1, 2}, 56);
     sources.updateModifiedTimeStamps();
-    sources.alreadyParsedAllDependFiles({1, 1}, 56);
+    sources.alreadyParsedAllDependentFiles({1, 1}, 56);
 
-    bool alreadyParsed = sources.alreadyParsedAllDependFiles({1, 2}, 56);
+    bool alreadyParsed = sources.alreadyParsedAllDependentFiles({1, 2}, 56);
 
     ASSERT_TRUE(alreadyParsed);
 }

tests/unit/unittest/symbolscollector-test.cpp

@@ -36,6 +36,12 @@
 #include <QDateTime>
 #include <QDir>
 
+#ifdef _WIN32
+#include <windows.h>
+#else
+#include <utime.h>
+#endif
+
 using testing::PrintToString;
 using testing::AllOf;
 using testing::Contains;
@@ -154,6 +160,15 @@ protected:
         return 0;
     }
 
+    void touchFile(const char *filePath)
+    {
+#ifdef _WIN32
+        QFile::resize(QString::fromUtf8(filePath), QFileInfo(QString::fromUtf8(filePath)).size());
+#else
+        utime(filePath, nullptr);
+#endif
+    }
+
 protected:
     Sqlite::Database database{":memory:", Sqlite::JournalMode::Memory};
     ClangBackEnd::RefactoringDatabaseInitializer<Sqlite::Database> initializer{database};
@@ -707,8 +722,24 @@ TEST_F(SymbolsCollector, IsVariableSymbol)
                         HasSymbolKind(SymbolKind::Variable))));
 }
 
+TEST_F(SymbolsCollector, IndexUnmodifiedHeaderFilesAtFirstRun)
+{
+    collector.addFiles({filePathId(TESTDATA_DIR "/symbolscollector_unmodified.cpp")}, {"cc", "-I", {TESTDATA_DIR, "/include"}});
 
-TEST_F(SymbolsCollector, DontIndexUnmodifiedHeaderFiles)
+    collector.collectSymbols();
+
+    ASSERT_THAT(collector.symbols(),
+                AllOf(
+                    Contains(HasSymbolName("MemberReference")),
+                    Contains(HasSymbolName("HeaderFunction")),
+                    Contains(HasSymbolName("HeaderFunctionReference")),
+                    Contains(HasSymbolName("Class")),
+                    Contains(HasSymbolName("Member")),
+                    Contains(HasSymbolName("HEADER_DEFINE")),
+                    Contains(HasSymbolName("FunctionLocalVariable"))));
+}
+
+TEST_F(SymbolsCollector, DontIndexUnmodifiedHeaderFilesAtSecondRun)
 {
     collector.addFiles({filePathId(TESTDATA_DIR "/symbolscollector_unmodified.cpp")}, {"cc", "-I", {TESTDATA_DIR, "/include"}});
     collector.collectSymbols();
@@ -719,6 +750,29 @@ TEST_F(SymbolsCollector, DontIndexUnmodifiedHeaderFiles)
 
     ASSERT_THAT(collector.symbols(),
                 AllOf(
+                    Contains(HasSymbolName("HeaderFunctionReferenceInMainFile")),
+                    Not(Contains(HasSymbolName("MemberReference"))),
+                    Not(Contains(HasSymbolName("HeaderFunctionReference"))),
+                    Not(Contains(HasSymbolName("HeaderFunction"))),
+                    Not(Contains(HasSymbolName("Class"))),
+                    Not(Contains(HasSymbolName("Member")))));
+}
+
+TEST_F(SymbolsCollector, DontIndexUnmodifiedHeaderFilesAtTouchHeader)
+{
+    collector.addFiles({filePathId(TESTDATA_DIR "/symbolscollector_unmodified3.cpp")}, {"cc", "-I", {TESTDATA_DIR, "/include"}});
+    collector.collectSymbols();
+    collector.clear();
+    touchFile(TESTDATA_DIR "/include/symbolscollector_unmodified_header2.h");
+    collector.addFiles({filePathId(TESTDATA_DIR "/symbolscollector_unmodified3.cpp")}, {"cc", "-I", {TESTDATA_DIR, "/include"}});
+
+    collector.collectSymbols();
+
+    ASSERT_THAT(collector.symbols(),
+                AllOf(
+                    Contains(HasSymbolName("HeaderFunctionReferenceInMainFile")),
+                    Not(Contains(HasSymbolName("MemberReference"))),
+                    Not(Contains(HasSymbolName("HeaderFunctionReference"))),
                     Not(Contains(HasSymbolName("HeaderFunction"))),
                     Not(Contains(HasSymbolName("Class"))),
                     Not(Contains(HasSymbolName("Member")))));
@@ -732,9 +786,13 @@ TEST_F(SymbolsCollector, DontIndexSystemIncudes)
 
     ASSERT_THAT(collector.symbols(),
                 AllOf(
+                    Contains(HasSymbolName("MainFileFunction")),
+                    Contains(HasSymbolName("HeaderFunctionReferenceInMainFile")),
+                    Not(Contains(HasSymbolName("MemberReference"))),
                     Not(Contains(HasSymbolName("HeaderFunction"))),
                     Not(Contains(HasSymbolName("Class"))),
                     Not(Contains(HasSymbolName("Member"))),
-                    Not(Contains(HasSymbolName("HEADER_DEFINE")))));
+                    Not(Contains(HasSymbolName("HEADER_DEFINE"))),
+                    Not(Contains(HasSymbolName("FunctionLocalVariable")))));
 }
 }

tests/unit/unittest/unittest.pro

@@ -248,4 +248,4 @@ HEADERS += \
     testclangtool.h \
 }
 
-OTHER_FILES += $$files(data/*)
+OTHER_FILES += $$files(data/*) $$files(data/include/*)