ClangRefactoring: Don't update the database if something went wrong

We can get an compile error. In that case we should not update the
database. In the future we should have a mechanism to report about the
database state.

Task-number: QTCREATORBUG-21949
Change-Id: I203346d536b007171f7bf255047409431c44a85a
Reviewed-by: Ivan Donchevskii <ivan.donchevskii@qt.io>

src/tools/clangrefactoringbackend/source/collectmacrossourcefilecallbacks.cpp

@@ -45,8 +45,6 @@ bool CollectMacrosSourceFileCallbacks::handleBeginSource(clang::CompilerInstance
                 compilerInstance.getPreprocessorPtr(),
                 m_sourcesManager);
 
-    compilerInstance.getLangOpts().DelayedTemplateParsing = false;
-    compilerInstance.getPreprocessorPtr()->SetSuppressIncludeNotFoundError(true);
     compilerInstance.getPreprocessorPtr()->addPPCallbacks(std::move(callbacks));
 
     return true;

src/tools/clangrefactoringbackend/source/collectsymbolsaction.h

@@ -34,6 +34,7 @@
 
 #include <filepathcachingfwd.h>
 
+#include <clang/Frontend/CompilerInstance.h>
 #include <clang/Frontend/FrontendAction.h>
 #include <clang/Index/IndexingAction.h>
 
@@ -52,19 +53,22 @@ public:
     std::unique_ptr<clang::ASTConsumer> newASTConsumer(clang::CompilerInstance &compilerInstance,
                                                        llvm::StringRef inFile);
 private:
-    class WrappedIndexAction : public clang::WrapperFrontendAction
+    class WrappedIndexAction final : public clang::WrapperFrontendAction
     {
     public:
-        WrappedIndexAction(std::shared_ptr<clang::index::IndexDataConsumer> m_indexDataConsumer,
+        WrappedIndexAction(std::shared_ptr<clang::index::IndexDataConsumer> indexDataConsumer,
                            clang::index::IndexingOptions indexingOptions)
             : clang::WrapperFrontendAction(
-                  clang::index::createIndexingAction(m_indexDataConsumer, indexingOptions, nullptr))
+                  clang::index::createIndexingAction(indexDataConsumer, indexingOptions, nullptr))
         {}
 
         std::unique_ptr<clang::ASTConsumer>
         CreateASTConsumer(clang::CompilerInstance &compilerInstance,
                           llvm::StringRef inFile) override
         {
+            compilerInstance.getPreprocessor().SetSuppressIncludeNotFoundError(true);
+            compilerInstance.getLangOpts().DelayedTemplateParsing = false;
+
             return WrapperFrontendAction::CreateASTConsumer(compilerInstance, inFile);
         }
     };

src/tools/clangrefactoringbackend/source/indexdataconsumer.cpp

@@ -124,33 +124,23 @@ bool IndexDataConsumer::skipSymbol(clang::FileID fileId, clang::index::SymbolRol
     return isParsedDeclaration || isParsedReference;
 }
 
-bool IndexDataConsumer::handleDeclOccurence(const clang::Decl *declaration,
+bool IndexDataConsumer::handleDeclOccurence(
-                                            clang::index::SymbolRoleSet symbolRoles,
+    const clang::Decl *declaration,
-                                            llvm::ArrayRef<clang::index::SymbolRelation> symbolRelations,
+    clang::index::SymbolRoleSet symbolRoles,
-#if LLVM_VERSION_MAJOR >= 7
+    llvm::ArrayRef<clang::index::SymbolRelation> /*symbolRelations*/,
-                                            clang::SourceLocation sourceLocation,
+    clang::SourceLocation sourceLocation,
-#else
+    IndexDataConsumer::ASTNodeInfo /*astNodeInfo*/)
-                                            clang::FileID fileId,
-                                            unsigned offset,
-#endif
-                                            IndexDataConsumer::ASTNodeInfo astNodeInfo)
 {
     const auto *namedDeclaration = clang::dyn_cast<clang::NamedDecl>(declaration);
     if (namedDeclaration) {
         if (!namedDeclaration->getIdentifier())
             return true;
 
-#if LLVM_VERSION_MAJOR >= 7
         if (skipSymbol(m_sourceManager->getFileID(sourceLocation), symbolRoles))
-#else
+
-        if (skipSymbol(fileId, symbolRoles))
-#endif
             return true;
 
         SymbolIndex globalId = toSymbolIndex(declaration->getCanonicalDecl());
-#if LLVM_VERSION_MAJOR < 7
-        clang::SourceLocation sourceLocation = m_sourceManager->getLocForStartOfFile(fileId).getLocWithOffset(offset);
-#endif
 
         auto found = m_symbolEntries.find(globalId);
         if (found == m_symbolEntries.end()) {

src/tools/clangrefactoringbackend/source/indexdataconsumer.h

@@ -55,12 +55,7 @@ public:
     bool handleDeclOccurence(const clang::Decl *declaration,
                              clang::index::SymbolRoleSet symbolRoles,
                              llvm::ArrayRef<clang::index::SymbolRelation> symbolRelations,
-#if LLVM_VERSION_MAJOR >= 7
                              clang::SourceLocation sourceLocation,
-#else
-                             clang::FileID fileId,
-                             unsigned offset,
-#endif
                              ASTNodeInfo astNodeInfo) override;
 
 private:

src/tools/clangrefactoringbackend/source/symbolindexer.cpp

@@ -117,27 +117,32 @@ void SymbolIndexer::updateProjectPart(ProjectPartContainer &&projectPart)
     std::vector<SymbolIndexerTask> symbolIndexerTask;
     symbolIndexerTask.reserve(projectPart.sourcePathIds.size());
     for (FilePathId sourcePathId : projectPart.sourcePathIds) {
-        auto indexing = [projectPartId, arguments = commandLineBuilder.commandLine, sourcePathId, this](
+        auto indexing = [projectPartId,
-                            SymbolsCollectorInterface &symbolsCollector) {
+                         arguments = commandLineBuilder.commandLine,
+                         sourcePathId,
+                         this](SymbolsCollectorInterface &symbolsCollector) {
             symbolsCollector.setFile(sourcePathId, arguments);
 
-            symbolsCollector.collectSymbols();
+            bool success = symbolsCollector.collectSymbols();
 
-            Sqlite::ImmediateTransaction transaction{m_transactionInterface};
+            if (success) {
+                Sqlite::ImmediateTransaction transaction{m_transactionInterface};
 
-            m_symbolStorage.addSymbolsAndSourceLocations(symbolsCollector.symbols(),
+                m_symbolStorage.addSymbolsAndSourceLocations(symbolsCollector.symbols(),
-                                                         symbolsCollector.sourceLocations());
+                                                             symbolsCollector.sourceLocations());
 
-            m_symbolStorage.updateProjectPartSources(projectPartId, symbolsCollector.sourceFiles());
+                m_symbolStorage.updateProjectPartSources(projectPartId,
+                                                         symbolsCollector.sourceFiles());
 
-            m_buildDependencyStorage.insertOrUpdateUsedMacros(symbolsCollector.usedMacros());
+                m_buildDependencyStorage.insertOrUpdateUsedMacros(symbolsCollector.usedMacros());
 
-            m_buildDependencyStorage.insertFileStatuses(symbolsCollector.fileStatuses());
+                m_buildDependencyStorage.insertFileStatuses(symbolsCollector.fileStatuses());
 
-            m_buildDependencyStorage.insertOrUpdateSourceDependencies(
+                m_buildDependencyStorage.insertOrUpdateSourceDependencies(
-                symbolsCollector.sourceDependencies());
+                    symbolsCollector.sourceDependencies());
 
-            transaction.commit();
+                transaction.commit();
+            }
         };
 
         symbolIndexerTask.emplace_back(sourcePathId, projectPartId, std::move(indexing));
@@ -185,27 +190,31 @@ void SymbolIndexer::updateChangedPath(FilePathId filePathId,
     CommandLineBuilder<ProjectPartArtefact, Utils::SmallStringVector>
         builder{artefact, artefact.toolChainArguments, InputFileType::Source, {}, {}, pchPath};
 
-    auto indexing = [projectPartId = artefact.projectPartId, arguments=builder.commandLine, filePathId, this](
+    auto indexing = [projectPartId = artefact.projectPartId,
-                        SymbolsCollectorInterface &symbolsCollector) {
+                     arguments = builder.commandLine,
+                     filePathId,
+                     this](SymbolsCollectorInterface &symbolsCollector) {
         symbolsCollector.setFile(filePathId, arguments);
 
-        symbolsCollector.collectSymbols();
+        bool success = symbolsCollector.collectSymbols();
 
-        Sqlite::ImmediateTransaction transaction{m_transactionInterface};
+        if (success) {
+            Sqlite::ImmediateTransaction transaction{m_transactionInterface};
 
-        m_symbolStorage.addSymbolsAndSourceLocations(symbolsCollector.symbols(),
+            m_symbolStorage.addSymbolsAndSourceLocations(symbolsCollector.symbols(),
-                                                     symbolsCollector.sourceLocations());
+                                                         symbolsCollector.sourceLocations());
 
-        m_symbolStorage.updateProjectPartSources(projectPartId, symbolsCollector.sourceFiles());
+            m_symbolStorage.updateProjectPartSources(projectPartId, symbolsCollector.sourceFiles());
 
-        m_buildDependencyStorage.insertOrUpdateUsedMacros(symbolsCollector.usedMacros());
+            m_buildDependencyStorage.insertOrUpdateUsedMacros(symbolsCollector.usedMacros());
 
-        m_buildDependencyStorage.insertFileStatuses(symbolsCollector.fileStatuses());
+            m_buildDependencyStorage.insertFileStatuses(symbolsCollector.fileStatuses());
 
-        m_buildDependencyStorage.insertOrUpdateSourceDependencies(
+            m_buildDependencyStorage.insertOrUpdateSourceDependencies(
-            symbolsCollector.sourceDependencies());
+                symbolsCollector.sourceDependencies());
 
-        transaction.commit();
+            transaction.commit();
+        }
     };
 
     symbolIndexerTask.emplace_back(filePathId, optionalArtefact->projectPartId, std::move(indexing));

src/tools/clangrefactoringbackend/source/symbolscollector.cpp

@@ -121,12 +121,14 @@ newFrontendActionFactory(Factory *consumerFactory,
       new FrontendActionFactoryAdapter(consumerFactory, sourceFileCallbacks));
 }
 
-void SymbolsCollector::collectSymbols()
+bool SymbolsCollector::collectSymbols()
 {
     auto tool = m_clangTool.createTool();
 
-    tool.run(ClangBackEnd::newFrontendActionFactory(&m_collectSymbolsAction,
+    auto actionFactory = ClangBackEnd::newFrontendActionFactory(&m_collectSymbolsAction,
-                                                    &m_collectMacrosSourceFileCallbacks).get());
+                                                                &m_collectMacrosSourceFileCallbacks);
+
+    return tool.run(actionFactory.get()) != 1;
 }
 
 void SymbolsCollector::doInMainThreadAfterFinished()

src/tools/clangrefactoringbackend/source/symbolscollector.h

@@ -52,7 +52,7 @@ public:
 
     void clear() override;
 
-    void collectSymbols() override;
+    bool collectSymbols() override;
 
     void doInMainThreadAfterFinished() override;
 

src/tools/clangrefactoringbackend/source/symbolscollectorinterface.h

@@ -44,7 +44,7 @@ class SymbolsCollectorInterface : public ProcessorInterface
 {
 public:
     virtual void setFile(FilePathId filePathId, const Utils::SmallStringVector &arguments) = 0;
-    virtual void collectSymbols() = 0;
+    virtual bool collectSymbols() = 0;
 
     virtual const SymbolEntries &symbols() const = 0;
     virtual const SourceLocationEntries &sourceLocations() const = 0;

tests/unit/unittest/symbolindexer-test.cpp

@@ -114,6 +114,7 @@ protected:
         ON_CALL(mockCollector, sourceDependencies()).WillByDefault(ReturnRef(sourceDependencies));
         ON_CALL(mockSymbolStorage, fetchProjectPartArtefact(A<FilePathId>())).WillByDefault(Return(artefact));
         ON_CALL(mockBuildDependenciesStorage, fetchLowestLastModifiedTime(A<FilePathId>())).WillByDefault(Return(-1));
+        ON_CALL(mockCollector, collectSymbols()).WillByDefault(Return(true));
 
         mockCollector.setIsUsed(false);
 
@@ -590,6 +591,61 @@ TEST_F(SymbolIndexer, UpdateProjectPartsCallsInOrderWithProjectPartArtifact)
     indexer.updateProjectParts({projectPart1});
 }
 
+TEST_F(SymbolIndexer, UpdateProjectPartsCallsInOrderButGetsAnErrorForCollectingSymbols)
+{
+    InSequence s;
+
+    EXPECT_CALL(mockSqliteTransactionBackend, immediateBegin());
+    EXPECT_CALL(mockSymbolStorage,
+                fetchProjectPartArtefact(TypedEq<Utils::SmallStringView>(projectPart1.projectPartId)))
+        .WillOnce(Return(nullArtefact));
+    EXPECT_CALL(mockSymbolStorage,
+                insertOrUpdateProjectPart(Eq(projectPart1.projectPartId),
+                                          Eq(projectPart1.toolChainArguments),
+                                          Eq(projectPart1.compilerMacros),
+                                          Eq(projectPart1.systemIncludeSearchPaths),
+                                          Eq(projectPart1.projectIncludeSearchPaths),
+                                          Eq(Utils::Language::Cxx),
+                                          Eq(Utils::LanguageVersion::CXX14),
+                                          Eq(Utils::LanguageExtension::None)))
+        .WillOnce(Return(12));
+    EXPECT_CALL(mockSymbolStorage, fetchPrecompiledHeader(Eq(12)));
+    EXPECT_CALL(mockBuildDependenciesStorage, fetchLowestLastModifiedTime(Eq(main1PathId))).Times(0);
+    EXPECT_CALL(mockSqliteTransactionBackend, commit());
+    EXPECT_CALL(mockCollector,
+                setFile(main1PathId,
+                        ElementsAre("clang++",
+                                    "-Wno-pragma-once-outside-header",
+                                    "-x",
+                                    "c++",
+                                    "-std=c++14",
+                                    "-nostdinc",
+                                    "-nostdinc++",
+                                    "-DBAR=1",
+                                    "-DFOO=1",
+                                    "-I",
+                                    "/project/includes",
+                                    "-I",
+                                    "/other/project/includes",
+                                    "-isystem",
+                                    TESTDATA_DIR,
+                                    "-isystem",
+                                    "/other/includes",
+                                    "-isystem",
+                                    "/includes")));
+    EXPECT_CALL(mockCollector, collectSymbols()).WillOnce(Return(false));
+    EXPECT_CALL(mockSqliteTransactionBackend, immediateBegin()).Times(0);
+    EXPECT_CALL(mockSymbolStorage, addSymbolsAndSourceLocations(symbolEntries, sourceLocations)).Times(0);
+    EXPECT_CALL(mockSymbolStorage, updateProjectPartSources(TypedEq<int>(12), Eq(sourceFileIds))).Times(0);
+    EXPECT_CALL(mockBuildDependenciesStorage, insertOrUpdateUsedMacros(Eq(usedMacros))).Times(0);
+    EXPECT_CALL(mockBuildDependenciesStorage, insertFileStatuses(Eq(fileStatus))).Times(0);
+    EXPECT_CALL(mockBuildDependenciesStorage, insertOrUpdateSourceDependencies(Eq(sourceDependencies)))
+        .Times(0);
+    EXPECT_CALL(mockSqliteTransactionBackend, commit()).Times(0);
+
+    indexer.updateProjectParts({projectPart1});
+}
+
 TEST_F(SymbolIndexer, CallSetNotifier)
 {
     EXPECT_CALL(mockPathWatcher, setNotifier(_));
@@ -667,6 +723,50 @@ TEST_F(SymbolIndexer, HandleEmptyOptionalArtifactInUpdateChangedPath)
     indexer.pathsChanged({sourceFileIds[0]});
 }
 
+TEST_F(SymbolIndexer, UpdateChangedPathCallsInOrderButGetsAnErrorForCollectingSymbols)
+{
+    InSequence s;
+
+    EXPECT_CALL(mockSqliteTransactionBackend, deferredBegin());
+    EXPECT_CALL(mockSymbolStorage, fetchProjectPartArtefact(TypedEq<FilePathId>(sourceFileIds[0])))
+        .WillOnce(Return(artefact));
+    EXPECT_CALL(mockSymbolStorage, fetchPrecompiledHeader(Eq(artefact.projectPartId)));
+    EXPECT_CALL(mockSqliteTransactionBackend, commit());
+    EXPECT_CALL(mockCollector,
+                setFile(Eq(sourceFileIds[0]),
+                        ElementsAre("clang++",
+                                    "-DFOO",
+                                    "-x",
+                                    "c++",
+                                    "-std=c++14",
+                                    "-nostdinc",
+                                    "-nostdinc++",
+                                    "-DBAR=1",
+                                    "-DFOO=1",
+                                    "-I",
+                                    "/project/includes",
+                                    "-I",
+                                    "/other/project/includes",
+                                    "-isystem",
+                                    TESTDATA_DIR,
+                                    "-isystem",
+                                    "/other/includes",
+                                    "-isystem",
+                                    "/includes")));
+    EXPECT_CALL(mockCollector, collectSymbols()).WillOnce(Return(false));
+    EXPECT_CALL(mockSqliteTransactionBackend, immediateBegin()).Times(0);
+    EXPECT_CALL(mockSymbolStorage, addSymbolsAndSourceLocations(symbolEntries, sourceLocations)).Times(0);
+    EXPECT_CALL(mockSymbolStorage, updateProjectPartSources(artefact.projectPartId, Eq(sourceFileIds)))
+        .Times(0);
+    EXPECT_CALL(mockBuildDependenciesStorage, insertOrUpdateUsedMacros(Eq(usedMacros))).Times(0);
+    EXPECT_CALL(mockBuildDependenciesStorage, insertFileStatuses(Eq(fileStatus))).Times(0);
+    EXPECT_CALL(mockBuildDependenciesStorage, insertOrUpdateSourceDependencies(Eq(sourceDependencies)))
+        .Times(0);
+    EXPECT_CALL(mockSqliteTransactionBackend, commit()).Times(0);
+
+    indexer.pathsChanged({sourceFileIds[0]});
+}
+
 TEST_F(SymbolIndexer, UpdateChangedPathIsUsingPrecompiledHeader)
 {
     ON_CALL(mockSymbolStorage, fetchProjectPartArtefact(TypedEq<FilePathId>(sourceFileIds[0])))