ProjectExplorer: add renameFiles() tests and refactor for crash safety

Replaces the monolithic ProjectExplorerPlugin::renameFiles() with a
three-stage implementation:
1. file-system rename
2. project update
3. diagnostics

Fixes crashes on nullptr nodes and subtree invalidation during QML
project reparses.

Change-Id: Ibe46db6e0f4498df7846e7cde2fd9a48394eed40
Reviewed-by: Tim Jenssen <tim.jenssen@qt.io>

src/plugins/projectexplorer/project.cpp

@@ -1387,13 +1387,29 @@ const QString TEST_PROJECT_MIMETYPE = "application/vnd.test.qmakeprofile";
 const QString TEST_PROJECT_DISPLAYNAME = "testProjectFoo";
 const char TEST_PROJECT_ID[] = "Test.Project.Id";
 
-class TestBuildSystem final : public BuildSystem
+class TestBuildSystem : public BuildSystem
 {
 public:
     using BuildSystem::BuildSystem;
 
-    void triggerParsing() final {}
+    void triggerParsing() override {}
-    QString name() const final { return QLatin1String("test"); }
+    QString name() const override { return QLatin1String("test"); }
+
+    virtual bool canRenameFile(
+        Node *context, const FilePath &oldFilePath, const FilePath &newFilePath) override
+    {
+        ++canRenameFileCount;
+        return true;
+    }
+
+    virtual bool renameFiles(
+        Node *context, const FilePairs &filesToRename, FilePaths *notRenamed) override
+    {
+        ++renameFilesCount;
+        return true;
+    }
+    int canRenameFileCount = 0;
+    int renameFilesCount = 0;
 };
 
 class TestProject : public Project
@@ -1565,6 +1581,222 @@ void ProjectExplorerTest::testProject_projectTree()
     QVERIFY(!project.rootProjectNode());
 }
 
+class RejectingAllRenameBuildSystem : public TestBuildSystem
+{
+public:
+    using TestBuildSystem::TestBuildSystem;
+    bool renameFiles(Node *, const FilePairs &, FilePaths *) override
+    {
+        ++renameFilesCount;
+        return false;
+    }
+};
+
+class PartiallyRejectingRenameBuildSystem : public TestBuildSystem
+{
+public:
+    using TestBuildSystem::TestBuildSystem;
+    static FilePaths pathsToReject;
+    bool renameFiles(Node *, const FilePairs &filePairs, FilePaths *notRenamed) override
+    {
+        ++renameFilesCount;
+        for (auto &[o, _] : filePairs)
+            if (pathsToReject.contains(o) && notRenamed)
+                notRenamed->append(o);
+        return true;
+    }
+};
+FilePaths PartiallyRejectingRenameBuildSystem::pathsToReject;
+
+class ReparsingBuildSystem : public TestBuildSystem
+{
+public:
+    using TestBuildSystem::TestBuildSystem;
+    static QPointer<Project> projectToReparse;
+    bool renameFiles(Node *originNode, const FilePairs &filePairs, FilePaths *notRenamed) override
+    {
+        const bool ok = TestBuildSystem::renameFiles(originNode, filePairs, notRenamed);
+        if (notRenamed) {
+            for (const auto &pair : filePairs)
+                notRenamed->append(pair.first);
+        }
+        if (projectToReparse) {
+            auto newRoot = std::make_unique<ProjectNode>(projectToReparse->projectFilePath());
+            projectToReparse->setRootProjectNode(std::move(newRoot));
+            if (ProjectTree::instance())
+                emit ProjectTree::instance()->subtreeChanged(nullptr);
+        }
+        return ok;
+    }
+};
+QPointer<Project> ReparsingBuildSystem::projectToReparse;
+
+class RenameTestProject : public Project
+{
+public:
+    explicit RenameTestProject(const TemporaryDirectory &td)
+        : Project(TEST_PROJECT_MIMETYPE, td.path())
+    {
+        setId(TEST_PROJECT_ID);
+        setDisplayName(TEST_PROJECT_DISPLAYNAME);
+    }
+
+    bool needsConfiguration() const final { return false; }
+
+    template<typename BS>
+    void initialize()
+    {
+        setBuildSystemCreator<BS>();
+        target = addTargetForKit(&kit);
+        createNodes();
+        Q_ASSERT(dynamic_cast<BS *>(target->buildSystem()));
+    }
+
+    TestBuildSystem *testBuildSystem()
+    {
+        return dynamic_cast<TestBuildSystem *>(target->buildSystem());
+    }
+    Kit kit;
+    Target *target = nullptr;
+    FilePath sourceFile, secondSourceFile;
+
+private:
+    void createNodes()
+    {
+        sourceFile = projectFilePath().pathAppended("test.cpp");
+        secondSourceFile = projectFilePath().pathAppended("test2.cpp");
+        QVERIFY(sourceFile.writeFileContents("content1"));
+        QVERIFY(secondSourceFile.writeFileContents("content2"));
+
+        auto root = std::make_unique<ProjectNode>(projectFilePath());
+        std::vector<std::unique_ptr<FileNode>> vec;
+        vec.emplace_back(std::make_unique<FileNode>(projectFilePath(), FileType::Project));
+        vec.emplace_back(std::make_unique<FileNode>(sourceFile, FileType::Source));
+        vec.emplace_back(std::make_unique<FileNode>(secondSourceFile, FileType::Source));
+        root->addNestedNodes(std::move(vec));
+        setRootProjectNode(std::move(root));
+    }
+};
+
+static FilePath makeRenamedFilePath(const FilePath &original)
+{
+    return original.chopped(4).stringAppended("_renamed.cpp");
+}
+
+void ProjectExplorerTest::testProject_renameFile()
+{
+    TemporaryDirectory tempDir("testProject_renameFile");
+    RenameTestProject testProject(tempDir);
+    testProject.initialize<TestBuildSystem>();
+    auto sourceFileNode = const_cast<Node *>(testProject.nodeForFilePath(testProject.sourceFile));
+    QVERIFY(sourceFileNode);
+    const FilePath testRenamed = makeRenamedFilePath(testProject.sourceFile);
+    QList<std::pair<Node *, FilePath>> nodesToRename{{sourceFileNode, testRenamed}};
+
+    const FilePairs result = ProjectExplorerPlugin::renameFiles(nodesToRename);
+
+    QCOMPARE(result.size(), 1);
+    QCOMPARE(testProject.testBuildSystem()->canRenameFileCount, 1);
+    QCOMPARE(testProject.testBuildSystem()->renameFilesCount, 1);
+    QCOMPARE(testProject.sourceFile.exists(), false);
+    QCOMPARE(testRenamed.exists(), true);
+}
+
+void ProjectExplorerTest::testProject_renameFile_NullNode()
+{
+    TemporaryDirectory tempDir("testProject_renameFile_NullNode");
+    RenameTestProject testProject(tempDir);
+    testProject.initialize<TestBuildSystem>();
+    const FilePath testRenamed = makeRenamedFilePath(testProject.sourceFile);
+    QList<std::pair<Node *, FilePath>> nodesToRename{{nullptr, testRenamed}};
+
+    const FilePairs result = ProjectExplorerPlugin::renameFiles(nodesToRename);
+
+    QVERIFY(result.isEmpty());
+}
+
+void ProjectExplorerTest::testProject_renameMultipleFiles()
+{
+    TemporaryDirectory tempDir("testProject_renameMultipleFiles");
+    RenameTestProject testProject(tempDir);
+    testProject.initialize<TestBuildSystem>();
+    const FilePath renamedFilePath1 = makeRenamedFilePath(testProject.sourceFile);
+    const FilePath renamedFilePath2 = makeRenamedFilePath(testProject.secondSourceFile);
+    auto testNode1 = const_cast<Node *>(testProject.nodeForFilePath(testProject.sourceFile));
+    auto testNode2 = const_cast<Node *>(testProject.nodeForFilePath(testProject.secondSourceFile));
+    QList<std::pair<Node *, FilePath>>
+        nodesToRename{{testNode1, renamedFilePath1}, {testNode2, renamedFilePath2}};
+
+    const FilePairs result = ProjectExplorerPlugin::renameFiles(nodesToRename);
+
+    QCOMPARE(result.size(), 2);
+    QCOMPARE(testProject.testBuildSystem()->canRenameFileCount, 2);
+    QCOMPARE(testProject.testBuildSystem()->renameFilesCount, 1);
+    QCOMPARE(testProject.sourceFile.exists(), false);
+    QCOMPARE(testProject.secondSourceFile.exists(), false);
+    QCOMPARE(renamedFilePath1.exists(), true);
+    QCOMPARE(renamedFilePath2.exists(), true);
+}
+
+void ProjectExplorerTest::testProject_renameFile_BuildSystemRejectsAll()
+{
+    TemporaryDirectory tempDir("testProject_renameFile_BuildSystemRejectsAll");
+    RenameTestProject testProject(tempDir);
+    testProject.initialize<RejectingAllRenameBuildSystem>();
+    Node *sourcNode = const_cast<Node *>(testProject.nodeForFilePath(testProject.sourceFile));
+    const FilePath renamedPath = makeRenamedFilePath(testProject.sourceFile);
+    QList<std::pair<Node *, FilePath>> toRename{{sourcNode, renamedPath}};
+
+    const FilePairs result = ProjectExplorerPlugin::renameFiles(toRename);
+
+    QVERIFY(result.isEmpty());
+    QCOMPARE(testProject.testBuildSystem()->canRenameFileCount, 1);
+    QCOMPARE(testProject.testBuildSystem()->renameFilesCount, 1);
+    QCOMPARE(testProject.sourceFile.exists(), false);
+    QCOMPARE(renamedPath.exists(), true);
+}
+
+void ProjectExplorerTest::testProject_renameFile_BuildSystemRejectsPartial()
+{
+    TemporaryDirectory tempDir("testProject_renameFile_BuildSystemRejectsPartial");
+    RenameTestProject testProject(tempDir);
+    testProject.initialize<PartiallyRejectingRenameBuildSystem>();
+    PartiallyRejectingRenameBuildSystem::pathsToReject = {testProject.sourceFile};
+    const FilePath renamed1 = makeRenamedFilePath(testProject.sourceFile);
+    const FilePath renamed2 = makeRenamedFilePath(testProject.secondSourceFile);
+    Node *node1 = const_cast<Node *>(testProject.nodeForFilePath(testProject.sourceFile));
+    Node *node2 = const_cast<Node *>(testProject.nodeForFilePath(testProject.secondSourceFile));
+    QList<std::pair<Node *, FilePath>> toRename{{node1, renamed1}, {node2, renamed2}};
+
+    const FilePairs result = ProjectExplorerPlugin::renameFiles(toRename);
+
+    QCOMPARE(result.size(), 1);
+    QVERIFY(result.contains({testProject.secondSourceFile, renamed2}));
+    QCOMPARE(testProject.testBuildSystem()->canRenameFileCount, 2);
+    QCOMPARE(testProject.testBuildSystem()->renameFilesCount, 1);
+    QCOMPARE(testProject.sourceFile.exists(), false);
+    QCOMPARE(testProject.secondSourceFile.exists(), false);
+    QCOMPARE(renamed1.exists(), true);
+    QCOMPARE(renamed2.exists(), true);
+}
+
+void ProjectExplorerTest::testProject_renameFile_QmlCrashSimulation()
+{
+    TemporaryDirectory tempDir("testProject_renameFile_QmlCrashSimulation");
+    RenameTestProject testProject(tempDir);
+    testProject.initialize<ReparsingBuildSystem>();
+    ReparsingBuildSystem::projectToReparse = &testProject;
+    Node *sourceNode = const_cast<Node *>(testProject.nodeForFilePath(testProject.sourceFile));
+    const FilePath renamedPath = makeRenamedFilePath(testProject.sourceFile);
+    QList<std::pair<Node *, FilePath>> toRename{{sourceNode, renamedPath}};
+
+    const FilePairs result = ProjectExplorerPlugin::renameFiles(toRename);
+
+    QVERIFY(renamedPath.exists());
+    QCOMPARE(testProject.sourceFile.exists(), false);
+    QVERIFY(result.isEmpty());
+}
+
 void ProjectExplorerTest::testProject_multipleBuildConfigs()
 {
     // Find suitable kit.

src/plugins/projectexplorer/projectexplorer.cpp

@@ -110,6 +110,7 @@
 #include <coreplugin/imode.h>
 #include <coreplugin/iversioncontrol.h>
 #include <coreplugin/locator/directoryfilter.h>
+#include <coreplugin/messagebox.h>
 #include <coreplugin/messagemanager.h>
 #include <coreplugin/minisplitter.h>
 #include <coreplugin/modemanager.h>
@@ -2482,97 +2483,166 @@ void ProjectExplorerPlugin::showOutputPaneForRunControl(RunControl *runControl)
     appOutputPane().showOutputPaneForRunControl(runControl);
 }
 
+static QList<std::pair<FilePath, FilePath>> collectValidRenames(
+    const QList<std::pair<Node *, FilePath>> &nodesAndDesiredPaths,
+    QHash<FilePath, Node *> &oldPathToNode)
+{
+    QList<std::pair<FilePath, FilePath>> pendingRenames;
+
+    for (const std::pair<Node *, FilePath> &nodeAndPath : nodesAndDesiredPaths) {
+        Node *originalNode = nodeAndPath.first;
+        if (!originalNode)
+            continue;
+
+        const FilePath oldPath = originalNode->filePath();
+        const FilePath newPath = nodeAndPath.second;
+
+        if (oldPath.equalsCaseSensitive(newPath))
+            continue;
+
+        pendingRenames.append({oldPath, newPath});
+        oldPathToNode.insert(oldPath, originalNode);
+    }
+    return pendingRenames;
+}
+
 static HandleIncludeGuards canTryToRenameIncludeGuards(const Node *node)
 {
     return node->asFileNode() && node->asFileNode()->fileType() == FileType::Header
-            ? HandleIncludeGuards::Yes : HandleIncludeGuards::No;
+               ? HandleIncludeGuards::Yes
+               : HandleIncludeGuards::No;
+}
+
+static void applyFileSystemRenames(
+    const QList<std::pair<FilePath, FilePath>> &pendingRenames,
+    const QHash<FilePath, Node *> &oldPathToNode,
+    FilePairs &successfulRenames,
+    FilePaths &failedRenames)
+{
+    for (const std::pair<FilePath, FilePath> &oldAndNew : pendingRenames) {
+        Node *originalNode = oldPathToNode.value(oldAndNew.first);
+
+        const bool renameSucceeded = Core::FileUtils::renameFile(
+            oldAndNew.first, oldAndNew.second, canTryToRenameIncludeGuards(originalNode));
+
+        if (renameSucceeded)
+            successfulRenames.append(oldAndNew);
+        else
+            failedRenames.append(oldAndNew.first);
+    }
+}
+
+static void applyProjectTreeRenames(
+    const FilePairs &fileSystemSuccessfulRenames,
+    const QHash<FilePath, Node *> &oldPathToNode,
+    FilePairs &completelySuccessfulRenames,
+    FilePaths &projectUpdateFailures,
+    FilePaths &skippedDueToInvalidContext)
+{
+    QHash<FolderNode *, FilePairs> renamesGroupedByParent;
+
+    for (const std::pair<FilePath, FilePath> &oldAndNew : fileSystemSuccessfulRenames) {
+        const FilePath &oldPath = oldAndNew.first;
+        const FilePath &newPath = oldAndNew.second;
+
+        Node *originalNode = oldPathToNode.value(oldPath);
+        FolderNode *parentNode = originalNode ? originalNode->parentFolderNode() : nullptr;
+
+        if (parentNode) {
+            parentNode->canRenameFile(oldPath, newPath);
+            renamesGroupedByParent[parentNode].append(oldAndNew);
+        } else {
+            skippedDueToInvalidContext.append(oldPath);
+        }
+    }
+
+    for (auto it = renamesGroupedByParent.cbegin(); it != renamesGroupedByParent.cend(); ++it) {
+        FolderNode *folderNode = it.key();
+        const FilePairs &filePairsForFolder = it.value();
+
+        FilePaths notRenamedInBuildSystem;
+        const bool buildSystemReportedSuccess
+            = folderNode->renameFiles(filePairsForFolder, &notRenamedInBuildSystem);
+
+        for (const std::pair<FilePath, FilePath> &oldAndNew : filePairsForFolder) {
+            const FilePath &oldPath = oldAndNew.first;
+
+            const bool updatedInProject = buildSystemReportedSuccess
+                                          && !notRenamedInBuildSystem.contains(oldPath);
+            if (updatedInProject)
+                completelySuccessfulRenames.append(oldAndNew);
+            else
+                projectUpdateFailures.append(oldPath);
+        }
+    }
+}
+
+static void showRenameDiagnostics(
+    const FilePaths &fileSystemFailures,
+    const FilePaths &projectUpdateFailures,
+    const FilePaths &skippedRenames)
+{
+    if (fileSystemFailures.isEmpty() && projectUpdateFailures.isEmpty()
+        && skippedRenames.isEmpty()) {
+        return;
+    }
+
+    const auto pathsToHtmlList = [](const FilePaths &paths) {
+        QString html = "<ul>";
+        for (const FilePath &path : paths)
+            html += "<li>" + path.toUserOutput() + "</li>";
+        return html += "</ul>";
+    };
+
+    QString messageBody;
+    if (!fileSystemFailures.isEmpty())
+        messageBody += Tr::tr("The following files could not be renamed in the file system:%1")
+                           .arg(pathsToHtmlList(fileSystemFailures));
+
+    if (!projectUpdateFailures.isEmpty()) {
+        if (!messageBody.isEmpty())
+            messageBody += "<br>";
+        messageBody += Tr::tr("These files were renamed in the file system, but project files were "
+                              "not updated:%1")
+                           .arg(pathsToHtmlList(projectUpdateFailures));
+    }
+
+    if (!skippedRenames.isEmpty()) {
+        if (!messageBody.isEmpty())
+            messageBody += "<br>";
+        messageBody += Tr::tr("These files were renamed in the file system, but the project "
+                              "structure was not updated (context lost or unsupported):%1")
+                           .arg(pathsToHtmlList(skippedRenames));
+    }
+
+    Core::AsynchronousMessageBox::warning(Tr::tr("Renaming Issues"), messageBody);
 }
 
 FilePairs ProjectExplorerPlugin::renameFiles(
-    const QList<std::pair<Node *, FilePath>> &nodesAndNewFilePaths)
+    const QList<std::pair<Node *, FilePath>> &nodesAndDesiredNewPaths)
 {
-    const QList<std::pair<Node *, FilePath>> nodesAndNewFilePathsFiltered
+    QHash<FilePath, Node *> oldPathToNode;
-            = Utils::filtered(nodesAndNewFilePaths, [](const std::pair<Node *, FilePath> &elem) {
+    const QList<std::pair<FilePath, FilePath>> pendingRenames
-        return !elem.first->filePath().equalsCaseSensitive(elem.second);
+        = collectValidRenames(nodesAndDesiredNewPaths, oldPathToNode);
-    });
 
-    // The same as above, for use when the nodes might no longer exist.
+    if (pendingRenames.isEmpty())
-    const QList<std::pair<FilePath, FilePath>> oldAndNewFilePathsFiltered
+        return {};
-            = Utils::transform(nodesAndNewFilePathsFiltered, [](const std::pair<Node *, FilePath> &p) {
-        return std::make_pair(p.first->filePath(), p.second);
-    });
 
-    FilePaths renamedOnly;
+    FilePairs fileSystemSuccess;
-    FilePaths failedRenamings;
+    FilePaths fileSystemFailures;
-    const auto renameFile = [&failedRenamings](const Node *node, const FilePath &newFilePath) {
+    applyFileSystemRenames(pendingRenames, oldPathToNode, fileSystemSuccess, fileSystemFailures);
-        if (!Core::FileUtils::renameFile(
-                    node->filePath(), newFilePath, canTryToRenameIncludeGuards(node))) {
-            failedRenamings << node->filePath();
-            return false;
-        }
-        return true;
-    };
-    QHash<FolderNode *, QList<std::pair<Node *, FilePath>>> renamingsPerParentNode;
-    for (const auto &elem : nodesAndNewFilePathsFiltered) {
-        if (FolderNode * const folderNode = elem.first->parentFolderNode())
-            renamingsPerParentNode[folderNode] << elem;
-        else if (renameFile(elem.first, elem.second))
-            renamedOnly << elem.first->filePath();
-    }
 
-    for (auto it = renamingsPerParentNode.cbegin(); it != renamingsPerParentNode.cend(); ++it) {
+    FilePairs projectSuccess;
-        FilePairs toUpdateInProject;
+    FilePaths projectFailures;
-        for (const std::pair<Node *, FilePath> &elem : it.value()) {
+    FilePaths skippedRenames;
-            const bool canUpdateProject
+    applyProjectTreeRenames(
-                    = it.key()->canRenameFile(elem.first->filePath(), elem.second);
+        fileSystemSuccess, oldPathToNode, projectSuccess, projectFailures, skippedRenames);
-            if (renameFile(elem.first, elem.second)) {
-                if (canUpdateProject )
-                    toUpdateInProject << std::make_pair(elem.first->filePath(), elem.second);
-                else
-                    renamedOnly << elem.first->filePath();
-            }
-        }
-        if (toUpdateInProject.isEmpty())
-            continue;
-        FilePaths notRenamed;
-        if (!it.key()->renameFiles(toUpdateInProject, &notRenamed))
-            renamedOnly << notRenamed;
-    }
 
-    if (!failedRenamings.isEmpty() || !renamedOnly.isEmpty()) {
+    showRenameDiagnostics(fileSystemFailures, projectFailures, skippedRenames);
-        const auto pathsAsHtmlList = [](const FilePaths &files) {
-            QString s("<ul>");
-            for (const FilePath &f : files)
-                s.append("<li>").append(f.toUserOutput()).append("</li>");
-            return s.append("</ul>");
-        };
-        QString failedRenamingsString;
-        if (!failedRenamings.isEmpty()) {
-            failedRenamingsString = Tr::tr("The following files could not be renamed: %1")
-                    .arg(pathsAsHtmlList(failedRenamings));
-        }
-        QString renamedOnlyString;
-        if (!renamedOnly.isEmpty()) {
-            renamedOnlyString
-                = "<br>"
-                  + Tr::tr("The following files were renamed, but their project files could not "
-                           "be updated accordingly: %1")
-                        .arg(pathsAsHtmlList(renamedOnly));
-        }
-        QTimer::singleShot(
-                    0, m_instance, [message = QString(failedRenamingsString + renamedOnlyString)] {
-            QMessageBox::warning(
-                        ICore::dialogParent(), Tr::tr("Renaming Did Not Fully Succeed"), message);
-        });
-    }
 
-    FilePairs allRenamedFiles;
+    if (!projectSuccess.isEmpty())
-    for (const std::pair<FilePath, FilePath> &candidate : oldAndNewFilePathsFiltered) {
+        emit instance()->filesRenamed(projectSuccess);
-        if (!failedRenamings.contains(candidate.first))
+    return projectSuccess;
-            allRenamedFiles.emplaceBack(candidate.first, candidate.second);
-    }
-    emit instance()->filesRenamed(allRenamedFiles);
-    return allRenamedFiles;
 }
 
 #ifdef WITH_TESTS

src/plugins/projectexplorer/projectexplorer_test.h

@@ -80,6 +80,13 @@ private slots:
     void testProject_parsingSuccess();
     void testProject_parsingFail();
     void testProject_projectTree();
+    void testProject_renameFile();
+    void testProject_renameFile_NullNode();
+    void testProject_renameMultipleFiles();
+    void testProject_renameFile_BuildSystemRejectsAll();
+    void testProject_renameFile_BuildSystemRejectsPartial();
+    void testProject_renameFile_QmlCrashSimulation();
+
     void testProject_multipleBuildConfigs();
 
     void testSourceToBinaryMapping();
@@ -87,7 +94,6 @@ private slots:
 
     void testSessionSwitch();
 
-private:
     friend class ::ProjectExplorer::ProjectExplorerPlugin;
 };
 

src/plugins/projectexplorer/projectmodels.cpp

@@ -412,6 +412,8 @@ void FlatModel::parsingStateChanged(Project *project)
 
 void FlatModel::updateSubtree(FolderNode *node)
 {
+    if (!node)
+        return;
     // FIXME: This is still excessive, should be limited to the affected subtree.
     while (FolderNode *parent = node->parentFolderNode())
         node = parent;