From b258289149fc55d8115e5c835ea5043379cf4d3f Mon Sep 17 00:00:00 2001 From: Ruslan Cheremin Date: Thu, 18 Jun 2026 18:06:28 +0200 Subject: [PATCH] [vfs] IJPL-247444: prohibit VirtualFile.rename() for already deleted VirtualFiles + `VirtualFile.rename()` should throw an exception if the file is already deleted because of general rule "accessing !valid files is illegal" + `VirtualFile.delete()` is unnatural to behave this way, so it just log warning (cherry picked from commit bcf565ddf9d2014d1e95fe6451c6a9629d7b0ad3) IJ-CR-211437 GitOrigin-RevId: 529bec2d3da89e0c707bd8e813694c4e95e1979f --- .../InvalidVirtualFileAccessException.java | 2 +- .../com/intellij/openapi/vfs/VirtualFile.java | 11 ++++--- .../newvfs/impl/VirtualFileSystemEntry.java | 31 +++++++++++++++++++ .../junit5/src/fixture/fixtures.kt | 4 ++- .../intellij/testFramework/VfsTestUtil.java | 4 ++- .../fixtures/impl/TempDirTestFixtureImpl.java | 4 ++- 6 files changed, 48 insertions(+), 8 deletions(-) diff --git a/platform/core-api/src/com/intellij/openapi/vfs/InvalidVirtualFileAccessException.java b/platform/core-api/src/com/intellij/openapi/vfs/InvalidVirtualFileAccessException.java index ed0d430d7ed5..2bf33facc4d8 100644 --- a/platform/core-api/src/com/intellij/openapi/vfs/InvalidVirtualFileAccessException.java +++ b/platform/core-api/src/com/intellij/openapi/vfs/InvalidVirtualFileAccessException.java @@ -23,7 +23,7 @@ public class InvalidVirtualFileAccessException extends RuntimeException { private static @NonNls String composeMessage(@NotNull VirtualFile file) { String url = file.getUrl(); - @NonNls String message = "Accessing invalid virtual file: " + url; + @NonNls String message = "Accessing invalid (=!isValid) virtual file: " + url; String reason = getInvalidationReason(file); if (reason != null) { message += "; reason: " + reason; diff --git a/platform/core-api/src/com/intellij/openapi/vfs/VirtualFile.java b/platform/core-api/src/com/intellij/openapi/vfs/VirtualFile.java index 35f65cda7324..2f8599f4ae68 100644 --- a/platform/core-api/src/com/intellij/openapi/vfs/VirtualFile.java +++ b/platform/core-api/src/com/intellij/openapi/vfs/VirtualFile.java @@ -260,10 +260,10 @@ public abstract class VirtualFile extends UserDataHolderBase implements Modifica /// Checks whether this `VirtualFile` is valid. File can be invalidated either by deleting it or one of its /// parents with [#delete] method or by an external change. - /// If file is not valid only [#equals], [#hashCode], + /// If the file is not valid only [#equals], [#hashCode], /// [#getName()], [#getPath()], [#getUrl()], [#getPresentableUrl()] and methods from /// [UserDataHolder] can be called for it. Using any other methods for an invalid [VirtualFile] instance - /// produce unpredictable results. + /// produces unpredictable results, usually an exception. /// /// @return `true` if this is a valid file, `false` otherwise public abstract boolean isValid(); @@ -419,11 +419,14 @@ public abstract class VirtualFile extends UserDataHolderBase implements Modifica /// @param requestor any object to control who called this method. Note that /// it is considered to be an external change if `requestor` is `null`. /// See [VirtualFileEvent#getRequestor] - /// @throws IOException if file failed to be deleted + /// @throws IOException if the file failed to be deleted @RequiresWriteLock public void delete(Object requestor) throws IOException { ApplicationManager.getApplication().assertWriteAccessAllowed(); - LOG.assertTrue(isValid(), "Deleting invalid file"); + if (!isValid()) { + LOG.warn("Deleting invalid (already deleted?) file: " + this + " -> nothing to delete, skip"); + return; + } getFileSystem().deleteFile(requestor, this); } diff --git a/platform/platform-impl/src/com/intellij/openapi/vfs/newvfs/impl/VirtualFileSystemEntry.java b/platform/platform-impl/src/com/intellij/openapi/vfs/newvfs/impl/VirtualFileSystemEntry.java index fc7f6bcd58d6..136e1d5957d9 100644 --- a/platform/platform-impl/src/com/intellij/openapi/vfs/newvfs/impl/VirtualFileSystemEntry.java +++ b/platform/platform-impl/src/com/intellij/openapi/vfs/newvfs/impl/VirtualFileSystemEntry.java @@ -497,6 +497,28 @@ public abstract class VirtualFileSystemEntry extends NewVirtualFile { @Override public void delete(Object requestor) throws IOException { ApplicationManager.getApplication().assertWriteAccessAllowed(); + + if (!isValid()) { + //We have a general rule "Accessing !valid VirtualFile is incorrect, except for a limited set of methods needed + // for identifying a VirtualFile, like id/path/toString" -- see VirtualFile.isValid jdocs. + // According to this rule, an attempt to _delete_ VirtualFile which is !valid -- should be an error. + // But it is a bit of unnatural behavior for .delete(): .delete()-like methods are _usually_ idempotent, i.e., + // safe to be called repeatedly -- and people expect them to behave that way. + // And historically .delete() didn't throw an exception in this case -- and as a result, we have a number of + // usages in our codebase (and, probably, in plugins too) that don't expect .delete() to throw an exception + // if called on an already deleted file. + // So, for the sake of backward compatibility, we keep supporting that legacy behavior here: log warning (which + // should contain a stacktrace of an original deleter), and return unharmed. + // Luckily, the rule in VirtualFile.isValid() does specify that accessing an invalid file is "incorrect", but does + // NOT specify that kind of bad things will follow -- so just "warn" is pretty legit. + + Logger.getInstance(VirtualFileSystemEntry.class).warn( + "Deleting invalid (already deleted?) file -> nothing to delete, skip", + new InvalidVirtualFileAccessException(this) + ); + return; + } + owningPersistentFS().deleteFile(requestor, this); } @@ -504,6 +526,8 @@ public abstract class VirtualFileSystemEntry extends NewVirtualFile { @Override public void rename(Object requestor, @NotNull @NonNls String newName) throws IOException { ApplicationManager.getApplication().assertWriteAccessAllowed(); + failIfFileIsInvalid(); + if (getName().equals(newName)) return; validateName(newName); owningPersistentFS().renameFile(requestor, this, newName); @@ -512,10 +536,17 @@ public abstract class VirtualFileSystemEntry extends NewVirtualFile { @RequiresWriteLock @Override public @NotNull VirtualFile createChildData(Object requestor, @NotNull String name) throws IOException { + failIfFileIsInvalid(); validateName(name); return owningPersistentFS().createChildFile(requestor, this, name); } + private void failIfFileIsInvalid() { + if (!isValid()) { + throw new InvalidVirtualFileAccessException(this); + } + } + @Override public boolean isWritable() { return getFlagInt(VfsDataFlags.IS_WRITABLE_FLAG); diff --git a/platform/testFramework/junit5/src/fixture/fixtures.kt b/platform/testFramework/junit5/src/fixture/fixtures.kt index 7bf7775799b1..df2f2fad22d7 100644 --- a/platform/testFramework/junit5/src/fixture/fixtures.kt +++ b/platform/testFramework/junit5/src/fixture/fixtures.kt @@ -376,7 +376,9 @@ fun TestFixture.virtualFileFixture( } initialized(file) { edtWriteAction { - file.delete(dirFixture) + if (file.isValid) { + file.delete(dirFixture) + } } } } diff --git a/platform/testFramework/src/com/intellij/testFramework/VfsTestUtil.java b/platform/testFramework/src/com/intellij/testFramework/VfsTestUtil.java index e92885e4f5c6..fa368a8ad979 100644 --- a/platform/testFramework/src/com/intellij/testFramework/VfsTestUtil.java +++ b/platform/testFramework/src/com/intellij/testFramework/VfsTestUtil.java @@ -130,7 +130,9 @@ public final class VfsTestUtil { public static void deleteFile(@NotNull VirtualFile file) { try { // requestor must be notnull (for GlobalUndoTest) - WriteAction.runAndWait(() -> file.delete(file)); + WriteAction.runAndWait(() -> { + if(file.isValid()) file.delete(file); + }); } catch (Throwable throwable) { ExceptionUtil.rethrow(throwable); diff --git a/platform/testFramework/src/com/intellij/testFramework/fixtures/impl/TempDirTestFixtureImpl.java b/platform/testFramework/src/com/intellij/testFramework/fixtures/impl/TempDirTestFixtureImpl.java index cdb1ebcfc9f1..ff1464b43181 100644 --- a/platform/testFramework/src/com/intellij/testFramework/fixtures/impl/TempDirTestFixtureImpl.java +++ b/platform/testFramework/src/com/intellij/testFramework/fixtures/impl/TempDirTestFixtureImpl.java @@ -140,7 +140,9 @@ public class TempDirTestFixtureImpl extends BaseFixture implements TempDirTestFi VirtualFile virtualFile = LocalFileSystem.getInstance().findFileByPath(FileUtil.toSystemIndependentName(myTempDir.toString())); if (virtualFile != null) { WriteAction.runAndWait(() -> { - virtualFile.delete(this); + if(virtualFile.isValid()) { + virtualFile.delete(this); + } }); } }