[path-annotations] Refactor path annotation inspection and relax validation rules

* Simplify path validation by consolidating first and subsequent argument checks
* Allow `@LocalPath` annotation to be used directly in `Path.of()`
* Make error messages more specific about required annotations
* Reduce code duplication in validation logic

GitOrigin-RevId: 03e09d19dd17b374e921b0a387fa5cf19dfed5df
This commit is contained in:
Alexander Koshevoy
2025-05-19 18:41:59 +00:00
committed by intellij-monorepo-bot
parent 69fa89210b
commit 5bbf9ebced
9 changed files with 78 additions and 120 deletions
@@ -767,5 +767,5 @@ inspections.message.string.without.path.annotation.used.in.path.constructor.or.f
inspections.message.string.without.path.annotation.used.in.path.resolve.method=String without path annotation is used in Path.resolve() method
inspections.message.first.argument.fs.getpath.should.be.annotated.with.nativepath=First argument of FileSystem.getPath() should be annotated with @NativePath
inspections.message.more.parameters.in.fs.getpath.should.be.annotated.with.nativepath.or.filename=Elements of 'more' parameter in FileSystem.getPath() should be annotated with either @NativePath or @Filename
inspections.message.first.argument.path.of.should.be.annotated.with.multiroutingfilesystempath=First argument of Path.of() should be annotated with @MultiRoutingFileSystemPath
inspections.message.first.argument.path.of.should.be.annotated.with.multiroutingfilesystempath=First argument of Path.of() should be annotated with @MultiRoutingFileSystemPath or @Filename
inspections.message.more.parameters.in.path.of.should.be.annotated.with.multiroutingfilesystempath.or.filename=Elements of 'more' parameter in Path.of() should be annotated with either @MultiRoutingFileSystemPath or @Filename
@@ -54,113 +54,63 @@ class PathAnnotationInspection : DevKitUastInspectionBase() {
// Check if the method is a Path constructor or factory method
if (isPathConstructorOrFactory(target)) {
val arguments = node.valueArguments
if (arguments.isNotEmpty()) {
// Check first argument
val firstArg = arguments[0]
val firstArgInfo = PathAnnotationInfo.forExpression(firstArg)
for (i in 0 until arguments.size) {
val arg = arguments[i]
when (firstArgInfo) {
is PathAnnotationInfo.Native -> {
// Report error: @NativePath string used in Path constructor or factory method
holder.registerProblem(
sourcePsi,
DevKitBundle.message("inspections.message.nativepath.should.not.be.used.directly.constructing.path"),
AddMultiRoutingAnnotationFix()
)
}
is PathAnnotationInfo.Unspecified -> {
// Report normal warning: non-annotated string used in Path constructor or factory method
holder.registerProblem(
sourcePsi,
DevKitBundle.message("inspections.message.string.without.path.annotation.used.in.path.constructor.or.factory.method"),
AddMultiRoutingAnnotationFix()
)
}
is PathAnnotationInfo.FilenameInfo -> {
// Report error: first argument of Path.of() should be annotated with @MultiRoutingFileSystemPath
holder.registerProblem(
firstArg.sourcePsi ?: sourcePsi,
DevKitBundle.message("inspections.message.first.argument.path.of.should.be.annotated.with.multiroutingfilesystempath"),
AddMultiRoutingAnnotationFix()
)
}
is PathAnnotationInfo.LocalPathInfo -> {
// Report error: @LocalPath string used in Path constructor or factory method
holder.registerProblem(
sourcePsi,
DevKitBundle.message("inspections.message.nativepath.should.not.be.used.directly.constructing.path"),
AddMultiRoutingAnnotationFix()
)
}
is PathAnnotationInfo.MultiRouting -> {
// This is the correct annotation, no need to report anything
// Check if the argument is a string literal that denotes a valid filename
if (arg is UInjectionHost) {
val stringValue = arg.evaluateToString()
if (stringValue != null && PathAnnotationInfo.isValidFilename(stringValue)) {
// If it's a valid filename, don't register any problems
continue
}
}
// Check remaining arguments
if (arguments.size > 1) {
for (i in 1 until arguments.size) {
val arg = arguments[i]
// Check if the argument is a string literal that denotes a valid filename
if (arg is UInjectionHost) {
val stringValue = arg.evaluateToString()
if (stringValue != null && PathAnnotationInfo.isValidFilename(stringValue)) {
// Check if the argument is a reference to a variable with a string constant initializer that denotes a valid filename
if (arg is UReferenceExpression) {
val resolved = arg.resolve()
if (resolved is com.intellij.psi.PsiVariable) {
val initializer = resolved.initializer
if (initializer != null) {
// Try to evaluate the initializer as a string constant
val constantValue = com.intellij.psi.JavaPsiFacade.getInstance(resolved.project)
.constantEvaluationHelper.computeConstantExpression(initializer)
if (constantValue is String && PathAnnotationInfo.isValidFilename(constantValue)) {
// If it's a valid filename, don't register any problems
continue
}
}
}
}
// Check if the argument is a reference to a variable with a string constant initializer that denotes a valid filename
if (arg is UReferenceExpression) {
val resolved = arg.resolve()
if (resolved is com.intellij.psi.PsiVariable) {
val initializer = resolved.initializer
if (initializer != null) {
// Try to evaluate the initializer as a string constant
val constantValue = com.intellij.psi.JavaPsiFacade.getInstance(resolved.project)
.constantEvaluationHelper.computeConstantExpression(initializer)
if (constantValue is String && PathAnnotationInfo.isValidFilename(constantValue)) {
// If it's a valid filename, don't register any problems
continue
}
}
}
val argInfo = PathAnnotationInfo.forExpression(arg)
if (argInfo !is PathAnnotationInfo.MultiRouting && argInfo !is PathAnnotationInfo.FilenameInfo && argInfo !is PathAnnotationInfo.LocalPathInfo) {
// Report error: elements of 'more' parameter should be annotated with either @MultiRoutingFileSystemPath or @Filename
when (argInfo) {
is PathAnnotationInfo.Native -> {
if (i == 0)
holder.registerProblem(
arg.sourcePsi ?: sourcePsi,
DevKitBundle.message("inspections.message.nativepath.should.not.be.used.directly.constructing.path"),
AddMultiRoutingAnnotationFix()
)
}
val argInfo = PathAnnotationInfo.forExpression(arg)
if (argInfo !is PathAnnotationInfo.MultiRouting && argInfo !is PathAnnotationInfo.FilenameInfo) {
// Report error: elements of 'more' parameter should be annotated with either @MultiRoutingFileSystemPath or @Filename
when (argInfo) {
is PathAnnotationInfo.Native -> {
holder.registerProblem(
arg.sourcePsi ?: sourcePsi,
DevKitBundle.message("inspections.message.more.parameters.in.path.of.should.be.annotated.with.multiroutingfilesystempath.or.filename"),
AddMultiRoutingAnnotationFix()
)
}
is PathAnnotationInfo.LocalPathInfo -> {
holder.registerProblem(
arg.sourcePsi ?: sourcePsi,
DevKitBundle.message("inspections.message.more.parameters.in.path.of.should.be.annotated.with.multiroutingfilesystempath.or.filename"),
AddMultiRoutingAnnotationFix()
)
}
is PathAnnotationInfo.Unspecified -> {
holder.registerProblem(
arg.sourcePsi ?: sourcePsi,
DevKitBundle.message("inspections.message.more.parameters.in.path.of.should.be.annotated.with.multiroutingfilesystempath.or.filename"),
AddMultiRoutingAnnotationFix()
)
}
else -> {
// This should not happen, but we need to handle all cases
holder.registerProblem(
arg.sourcePsi ?: sourcePsi,
DevKitBundle.message("inspections.message.more.parameters.in.path.of.should.be.annotated.with.multiroutingfilesystempath.or.filename")
)
}
}
is PathAnnotationInfo.Unspecified -> {
holder.registerProblem(
arg.sourcePsi ?: sourcePsi,
if (i == 0)
DevKitBundle.message("inspections.message.first.argument.path.of.should.be.annotated.with.multiroutingfilesystempath")
else
DevKitBundle.message("inspections.message.more.parameters.in.path.of.should.be.annotated.with.multiroutingfilesystempath.or.filename"),
AddMultiRoutingAnnotationFix()
)
}
else -> {
// This should not happen, but we need to handle all cases
holder.registerProblem(
arg.sourcePsi ?: sourcePsi,
DevKitBundle.message("inspections.message.more.parameters.in.path.of.should.be.annotated.with.multiroutingfilesystempath.or.filename")
)
}
}
}
@@ -226,7 +226,7 @@ abstract class PathAnnotationInspectionTestBase : LightJavaCodeInsightFixtureTes
public void testMethod() {
// Test @LocalPath in Path.of()
@LocalPath String localPath = "/local/path";
Path path1 = <warning descr="${message("inspections.message.nativepath.should.not.be.used.directly.constructing.path")}">Path.of(localPath)</warning>; // Warning, @LocalPath should not be used directly in Path.of()
Path path1 = Path.of(localPath); // @LocalPath can be used directly in Path.of()
// Test @LocalPath in Path.resolve()
@MultiRoutingFileSystemPath String basePath = "/base/path";
@@ -52,7 +52,7 @@ class PathAnnotationInspectionJavaTest : PathAnnotationInspectionTestBase() {
public void testMethod() {
@NativePath String nativePath = "/usr/local/bin";
// This should be highlighted as an error because @NativePath strings should not be used directly in Path.of()
Path path = <warning descr="${message("inspections.message.nativepath.should.not.be.used.directly.constructing.path")}">Path.of(nativePath)</warning>;
Path path = Path.of(<warning descr="${message("inspections.message.nativepath.should.not.be.used.directly.constructing.path")}">nativePath</warning>);
}
}
""".trimIndent())
@@ -95,10 +95,10 @@ class PathAnnotationInspectionJavaTest : PathAnnotationInspectionTestBase() {
public void testMethod() {
String nonAnnotatedPath = "/usr/local/bin";
// This should be highlighted as a normal warning because non-annotated strings should be annotated with @MultiRoutingFileSystemPath
Path path = <warning descr="${message("inspections.message.string.without.path.annotation.used.in.path.constructor.or.factory.method")}">Path.of(nonAnnotatedPath)</warning>;
Path path = Path.of(<warning descr="${message("inspections.message.first.argument.path.of.should.be.annotated.with.multiroutingfilesystempath")}">nonAnnotatedPath</warning>);
// Direct string literal should also be highlighted
Path directPath = <warning descr="${message("inspections.message.string.without.path.annotation.used.in.path.constructor.or.factory.method")}">Path.of("/another/path")</warning>;
Path directPath = Path.of(<warning descr="${message("inspections.message.first.argument.path.of.should.be.annotated.with.multiroutingfilesystempath")}">"/another/path"</warning>);
}
}
""".trimIndent())
@@ -113,7 +113,7 @@ class PathAnnotationInspectionJavaTest : PathAnnotationInspectionTestBase() {
public class NonAnnotatedStringInPathResolve {
public void testMethod() {
Path basePath = <warning descr="${message("inspections.message.string.without.path.annotation.used.in.path.constructor.or.factory.method")}">Paths.get("/base/path")</warning>;
Path basePath = Paths.get(<warning descr="${message("inspections.message.first.argument.path.of.should.be.annotated.with.multiroutingfilesystempath")}">"/base/path"</warning>);
String nonAnnotatedPath = "invalid/path";
// This should be highlighted as a warning because non-annotated strings should be annotated with @MultiRoutingFileSystemPath
@@ -140,7 +140,7 @@ class PathAnnotationInspectionJavaTest : PathAnnotationInspectionTestBase() {
public class FilenameAnnotatedStringInPathResolve {
public void testMethod() {
Path basePath = <warning descr="${message("inspections.message.string.without.path.annotation.used.in.path.constructor.or.factory.method")}">Paths.get("/base/path")</warning>;
Path basePath = Paths.get(<warning descr="${message("inspections.message.first.argument.path.of.should.be.annotated.with.multiroutingfilesystempath")}">"/base/path"</warning>);
// Non-annotated string should be highlighted
String nonAnnotatedPath = "invalid/path";
@@ -175,9 +175,9 @@ class PathAnnotationInspectionJavaTest : PathAnnotationInspectionTestBase() {
String nonAnnotatedMore = "invalid/path";
Path path1 = Path.of(basePath, <warning descr="${message("inspections.message.more.parameters.in.path.of.should.be.annotated.with.multiroutingfilesystempath.or.filename")}">nonAnnotatedMore</warning>);
// @NativePath string in 'more' parameter should be highlighted
// @NativePath string in 'more' parameter should not be highlighted
@NativePath String nativeMore = "invalid/path";
Path path2 = Path.of(basePath, <warning descr="${message("inspections.message.more.parameters.in.path.of.should.be.annotated.with.multiroutingfilesystempath.or.filename")}">nativeMore</warning>);
Path path2 = Path.of(basePath, nativeMore);
// @Filename string in 'more' parameter should not be highlighted
@Filename String filenameMore = "file.txt";
@@ -1,6 +1,8 @@
// Copyright 2000-2025 JetBrains s.r.o. and contributors. Use of this source code is governed by the Apache 2.0 license.
package org.jetbrains.idea.devkit.inspections.path
import org.jetbrains.idea.devkit.DevKitBundle
import org.jetbrains.idea.devkit.DevKitBundle.message
import org.jetbrains.idea.devkit.inspections.PathAnnotationInspectionTestBase
class PathAnnotationInspectionOtherPathMethodsTest : PathAnnotationInspectionTestBase() {
@@ -15,8 +17,8 @@ class PathAnnotationInspectionOtherPathMethodsTest : PathAnnotationInspectionTes
public class OtherPathMethods {
public void testMethod() {
// Create paths
Path base = <warning descr="String without path annotation is used in Path constructor or factory method">Path.of("/base/path")</warning>;
Path other = <warning descr="String without path annotation is used in Path constructor or factory method">Path.of("/other/path")</warning>;
Path base = Path.of(<warning descr="${message("inspections.message.first.argument.path.of.should.be.annotated.with.multiroutingfilesystempath")}">"/base/path"</warning>);
Path other = Path.of(<warning descr="${message("inspections.message.first.argument.path.of.should.be.annotated.with.multiroutingfilesystempath")}">"/other/path"</warning>);
// Test Path.relativize(Path) - this should not be highlighted
Path relativized = base.relativize(other);
@@ -46,7 +48,7 @@ class PathAnnotationInspectionOtherPathMethodsTest : PathAnnotationInspectionTes
public class MixedPathMethods {
public void testMethod() {
// Create paths
Path base = <warning descr="String without path annotation is used in Path constructor or factory method">Path.of("/base/path")</warning>;
Path base = Path.of(<warning descr="${message("inspections.message.first.argument.path.of.should.be.annotated.with.multiroutingfilesystempath")}">"/base/path"</warning>);
// Test Path.resolveSibling(String) - this should be highlighted if not annotated
Path resolvedSibling1 = base.resolveSibling(<weak_warning descr="String without path annotation is used in Path.resolve() method">"other/path"</weak_warning>);
@@ -26,6 +26,11 @@ class PathAnnotationInspectionPathOfTest : PathAnnotationInspectionTestBase() {
final String validFilename1 = "file.txt";
final String validFilename2 = "another.txt";
Path path4 = Path.of(basePath, validFilename1, validFilename2);
Path path10 = Path.of("hello", "world");
final String validDirectoryName = "hello";
Path path11 = Path.of(validDirectoryName, "world");
}
}
""".trimIndent())
@@ -41,15 +46,15 @@ class PathAnnotationInspectionPathOfTest : PathAnnotationInspectionTestBase() {
public void testMethod() {
// First argument is NOT correctly annotated - this should be highlighted
String nonAnnotatedPath = "/base/path";
Path path1 = <warning descr="${message("inspections.message.string.without.path.annotation.used.in.path.constructor.or.factory.method")}">Path.of(nonAnnotatedPath, "file.txt")</warning>;
Path path1 = Path.of(<warning descr="${message("inspections.message.first.argument.path.of.should.be.annotated.with.multiroutingfilesystempath")}">nonAnnotatedPath</warning>, "file.txt");
// Even if other arguments are correctly annotated, it should still be highlighted
@Filename String filename = "file.txt";
@MultiRoutingFileSystemPath String subdir = "subdir";
Path path2 = <warning descr="${message("inspections.message.string.without.path.annotation.used.in.path.constructor.or.factory.method")}">Path.of(nonAnnotatedPath, filename, subdir)</warning>;
Path path2 = Path.of(<warning descr="${message("inspections.message.first.argument.path.of.should.be.annotated.with.multiroutingfilesystempath")}">nonAnnotatedPath</warning>, filename, subdir);
// Direct string literal as first argument should also be highlighted
Path path3 = <warning descr="${message("inspections.message.string.without.path.annotation.used.in.path.constructor.or.factory.method")}">Path.of("/another/path", "file.txt")</warning>;
Path path3 = Path.of(<warning descr="${message("inspections.message.first.argument.path.of.should.be.annotated.with.multiroutingfilesystempath")}">"/another/path"</warning>, "file.txt");
}
}
""".trimIndent())
@@ -1,6 +1,7 @@
// Copyright 2000-2025 JetBrains s.r.o. and contributors. Use of this source code is governed by the Apache 2.0 license.
package org.jetbrains.idea.devkit.inspections.path
import org.jetbrains.idea.devkit.DevKitBundle.message
import org.jetbrains.idea.devkit.inspections.PathAnnotationInspectionTestBase
class PathAnnotationInspectionPathResolvePathTest : PathAnnotationInspectionTestBase() {
@@ -15,10 +16,10 @@ class PathAnnotationInspectionPathResolvePathTest : PathAnnotationInspectionTest
public class PathResolvePath {
public void testMethod() {
// Create a base path
Path base = <warning descr="String without path annotation is used in Path constructor or factory method">Path.of("/base/path")</warning>;
Path base = Path.of(<warning descr="${message("inspections.message.first.argument.path.of.should.be.annotated.with.multiroutingfilesystempath")}">"/base/path"</warning>);
// Create another path
Path otherPath = <warning descr="String without path annotation is used in Path constructor or factory method">Path.of("/other/path")</warning>;
Path otherPath = Path.of(<warning descr="${message("inspections.message.first.argument.path.of.should.be.annotated.with.multiroutingfilesystempath")}">"/other/path"</warning>);
// Test Path.resolve(Path) - this should not be highlighted
Path resolvedPath = base.resolve(otherPath);
@@ -28,7 +29,7 @@ class PathAnnotationInspectionPathResolvePathTest : PathAnnotationInspectionTest
}
private Path getPath() {
return <warning descr="String without path annotation is used in Path constructor or factory method">Path.of("/some/path")</warning>;
return Path.of(<warning descr="${message("inspections.message.first.argument.path.of.should.be.annotated.with.multiroutingfilesystempath")}">"/some/path"</warning>);
}
}
""".trimIndent())
@@ -43,15 +43,15 @@ class PathAnnotationInspectionPathsGetTest : PathAnnotationInspectionTestBase()
public void testMethod() {
// First argument is NOT correctly annotated - this should be highlighted
String nonAnnotatedPath = "/base/path";
Path path1 = <warning descr="${message("inspections.message.string.without.path.annotation.used.in.path.constructor.or.factory.method")}">Paths.get(nonAnnotatedPath, "file.txt")</warning>;
Path path1 = Paths.get(<warning descr="${message("inspections.message.first.argument.path.of.should.be.annotated.with.multiroutingfilesystempath")}">nonAnnotatedPath</warning>, "file.txt");
// Even if other arguments are correctly annotated, it should still be highlighted
@Filename String filename = "file.txt";
@MultiRoutingFileSystemPath String subdir = "subdir";
Path path2 = <warning descr="${message("inspections.message.string.without.path.annotation.used.in.path.constructor.or.factory.method")}">Paths.get(nonAnnotatedPath, filename, subdir)</warning>;
Path path2 = Paths.get(<warning descr="${message("inspections.message.first.argument.path.of.should.be.annotated.with.multiroutingfilesystempath")}">nonAnnotatedPath</warning>, filename, subdir);
// Direct string literal as first argument should also be highlighted
Path path3 = <warning descr="${message("inspections.message.string.without.path.annotation.used.in.path.constructor.or.factory.method")}">Paths.get("/another/path", "file.txt")</warning>;
Path path3 = Paths.get(<warning descr="${message("inspections.message.first.argument.path.of.should.be.annotated.with.multiroutingfilesystempath")}">"/another/path"</warning>, "file.txt");
}
}
""".trimIndent())
@@ -58,7 +58,7 @@ class PathAnnotationInspectionQuickFixTest : PathAnnotationInspectionTestBase()
public void testMethod() {
String nonAnnotatedPath = "/usr/local/bin";
// This should be highlighted as a normal warning because non-annotated strings should be annotated with @MultiRoutingFileSystemPath
Path path = <warning descr="${message("inspections.message.string.without.path.annotation.used.in.path.constructor.or.factory.method")}">Path.of(nonAnnotatedPath)</warning>;
Path path = Path.of(<warning descr="${message("inspections.message.first.argument.path.of.should.be.annotated.with.multiroutingfilesystempath")}">nonAnnotatedPath</warning>);
}
}
""".trimIndent())
@@ -78,7 +78,7 @@ class PathAnnotationInspectionQuickFixTest : PathAnnotationInspectionTestBase()
public void testMethod() {
String nonAnnotatedPath = "/usr/local/bin";
// This should be highlighted as a normal warning because non-annotated strings should be annotated with @MultiRoutingFileSystemPath
Path path = <warning descr="${message("inspections.message.string.without.path.annotation.used.in.path.constructor.or.factory.method")}">Path.of(nonAnnotatedPath)</warning>;
Path path = Path.of(<warning descr="${message("inspections.message.first.argument.path.of.should.be.annotated.with.multiroutingfilesystempath")}">nonAnnotatedPath</warning>);
}
}
"""