From 4c0cb063e5502c2cdba9bcbd2f72a178e14f8b7b Mon Sep 17 00:00:00 2001 From: Alexey Kudravtsev Date: Mon, 22 Nov 2021 16:09:06 +0100 Subject: [PATCH] clarify FileTypeIdentifiableByVirtualFile.isMyType() method as not exhaustive "file belongs to this file type" but only as one of the possible checks, because otherwise too much code would need to be duplicated GitOrigin-RevId: 3277a316a60082bd17f7ea7e263aed278ab93947 --- .../ex/FileTypeIdentifiableByVirtualFile.java | 6 +-- .../fileTypes/impl/FileTypeManagerImpl.java | 7 ++-- .../openapi/fileTypes/impl/FileTypesTest.java | 37 +++++++++++++------ .../kotlin/org/toml/lang/psi/ElementTypes.kt | 2 +- 4 files changed, 33 insertions(+), 19 deletions(-) diff --git a/platform/core-api/src/com/intellij/openapi/fileTypes/ex/FileTypeIdentifiableByVirtualFile.java b/platform/core-api/src/com/intellij/openapi/fileTypes/ex/FileTypeIdentifiableByVirtualFile.java index e178cd68b43c..be2648c30c77 100644 --- a/platform/core-api/src/com/intellij/openapi/fileTypes/ex/FileTypeIdentifiableByVirtualFile.java +++ b/platform/core-api/src/com/intellij/openapi/fileTypes/ex/FileTypeIdentifiableByVirtualFile.java @@ -30,9 +30,9 @@ import org.jetbrains.annotations.NotNull; */ public interface FileTypeIdentifiableByVirtualFile extends FileType { /** - * @return true if (and only if) this particular file should be treated as belonging to this file type. - * Please make sure your definition is consistent with all other definitions for this file type. - * For example, file type should not be associated with the file name pattern (see Settings|Editor|File Types) which is not mentioned in the {@code isMyFileType()} below. + * @return true if this particular file should be treated as belonging to this file type. + * Note that this file type can be associated with other files by other means as well (e.g., "Settings|Editor|File Types|Associate file name pattern..."), + * so this method is just one of the possible file type definitions. */ boolean isMyFileType(@NotNull VirtualFile file); diff --git a/platform/platform-impl/src/com/intellij/openapi/fileTypes/impl/FileTypeManagerImpl.java b/platform/platform-impl/src/com/intellij/openapi/fileTypes/impl/FileTypeManagerImpl.java index 2f95fc5633af..b676105ca6e5 100644 --- a/platform/platform-impl/src/com/intellij/openapi/fileTypes/impl/FileTypeManagerImpl.java +++ b/platform/platform-impl/src/com/intellij/openapi/fileTypes/impl/FileTypeManagerImpl.java @@ -668,10 +668,11 @@ public class FileTypeManagerImpl extends FileTypeManagerEx implements Persistent } } - if (requestedFileType instanceof FileTypeIdentifiableByVirtualFile) { - return ((FileTypeIdentifiableByVirtualFile)requestedFileType).isMyFileType(file); + if (requestedFileType instanceof FileTypeIdentifiableByVirtualFile + && ((FileTypeIdentifiableByVirtualFile)requestedFileType).isMyFileType(file)) { + return true; } - // otherwise we can skip all the mySpecialFileTypes because it's certain this file type is not one of them + // otherwise, we can skip all the mySpecialFileTypes because it's certain this file type is not one of them FileType fileType = getFileTypeByFileName(file.getNameSequence()); if (fileType == UnknownFileType.INSTANCE) { diff --git a/platform/platform-tests/testSrc/com/intellij/openapi/fileTypes/impl/FileTypesTest.java b/platform/platform-tests/testSrc/com/intellij/openapi/fileTypes/impl/FileTypesTest.java index 40c93edfbbf4..077fbb18475a 100644 --- a/platform/platform-tests/testSrc/com/intellij/openapi/fileTypes/impl/FileTypesTest.java +++ b/platform/platform-tests/testSrc/com/intellij/openapi/fileTypes/impl/FileTypesTest.java @@ -1408,15 +1408,26 @@ public class FileTypesTest extends HeavyPlatformTestCase { public void testIsFileOfTypeMustNotQueryAllFileTypesIdentifiableByVirtualFileForPerformanceReasons() throws IOException { Disposable disposable = Disposer.newDisposable(); try { - AtomicInteger count = new AtomicInteger(); + AtomicInteger myFileTypeCalledCount = new AtomicInteger(); class MyFileTypeIdentifiableByFile extends FakeFileType { - @Override public boolean isMyFileType(@NotNull VirtualFile file) { count.incrementAndGet(); return false; } + @Override public boolean isMyFileType(@NotNull VirtualFile file) { myFileTypeCalledCount.incrementAndGet(); return false; } @Override public @NotNull String getName() { return "myfake"; } @Override public @Nls @NotNull String getDisplayName() { return getName(); } @Override public @NotNull @NlsContexts.Label String getDescription() { return getName(); } } - FileType myType = new MyFileTypeIdentifiableByFile(); - myFileTypeManager.registerFileType(myType, List.of(), disposable); + myFileTypeManager.registerFileType(new MyFileTypeIdentifiableByFile(), List.of(), disposable); + + AtomicInteger otherFileTypeCalledCount = new AtomicInteger(); + class MyOtherFileTypeIdentifiableByFile extends FakeFileType { + @Override public boolean isMyFileType(@NotNull VirtualFile file) { + otherFileTypeCalledCount.incrementAndGet(); + return false; + } + @Override public @NotNull String getName() { return "myotherfake"; } + @Override public @Nls @NotNull String getDisplayName() { return getName(); } + @Override public @NotNull @NlsContexts.Label String getDescription() { return getName(); } + } + myFileTypeManager.registerFileType(new MyOtherFileTypeIdentifiableByFile(), List.of(), disposable); File f = createTempFile("xx.lkj_lkj_lkj_ljk", "a"); VirtualFile virtualFile = getVirtualFile(f); @@ -1424,7 +1435,7 @@ public class FileTypesTest extends HeavyPlatformTestCase { FakeVirtualFile vf = new FakeVirtualFile(virtualFile, "myname.myname") { @Override public @NotNull FileType getFileType() { - return myFileTypeManager.getFileTypeByFile(this); // otherwise this call will be redirected to FileTypeManger.getIsntance() which is not what we are testing + return myFileTypeManager.getFileTypeByFile(this); // otherwise this call will be redirected to FileTypeManger.getInstance() which is not what we are testing } @Override @@ -1434,17 +1445,19 @@ public class FileTypesTest extends HeavyPlatformTestCase { }; FileType ft = myFileTypeManager.getFileTypeByFile(vf); assertEquals(UnknownFileType.INSTANCE, ft); - assertTrue(count.toString(), count.get() > 0); + // during getFileType() we must check all possible file types + assertTrue(myFileTypeCalledCount.toString(), myFileTypeCalledCount.get() > 0); + assertTrue(otherFileTypeCalledCount.toString(), otherFileTypeCalledCount.get() > 0); - count.set(0); + myFileTypeCalledCount.set(0); + otherFileTypeCalledCount.set(0); assertFalse(myFileTypeManager.isFileOfType(vf, PlainTextFileType.INSTANCE)); - assertEquals(count.toString(), 0, count.get()); + assertEquals(myFileTypeCalledCount.toString(), 0, myFileTypeCalledCount.get()); + assertEquals(otherFileTypeCalledCount.toString(), 0, otherFileTypeCalledCount.get()); - class MyOtherFileTypeIdentifiableByFile extends MyFileTypeIdentifiableByFile { - @Override public boolean isMyFileType(@NotNull VirtualFile file) { return false; } - } assertFalse(myFileTypeManager.isFileOfType(vf, new MyOtherFileTypeIdentifiableByFile())); - assertEquals(count.toString(), 0, count.get()); + assertEquals(myFileTypeCalledCount.toString(), 0, myFileTypeCalledCount.get()); // must not call irrelevant file types + assertTrue(otherFileTypeCalledCount.toString(), otherFileTypeCalledCount.get() > 0); // must call requested file type } finally { Disposer.dispose(disposable); diff --git a/plugins/toml/core/src/main/kotlin/org/toml/lang/psi/ElementTypes.kt b/plugins/toml/core/src/main/kotlin/org/toml/lang/psi/ElementTypes.kt index b8f46b8caced..68410a8339dc 100644 --- a/plugins/toml/core/src/main/kotlin/org/toml/lang/psi/ElementTypes.kt +++ b/plugins/toml/core/src/main/kotlin/org/toml/lang/psi/ElementTypes.kt @@ -28,7 +28,7 @@ object TomlFileType : LanguageFileType(TomlLanguage), FileTypeIdentifiableByVirt override fun getCharset(file: VirtualFile, content: ByteArray): String = "UTF-8" override fun isMyFileType(file: VirtualFile): Boolean { - return file.name == "config" && file.parent?.name == ".cargo" || FileTypeManager.getInstance().getFileTypeByFileName(file.name) == this + return file.name == "config" && file.parent?.name == ".cargo" } }