Support String.formatted with FormatString Bug Pattern

[codylerum]

Added in JDK 15 is the ability for callformatted on a String.

https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/lang/String.html#formatted(java.lang.Object...)

"foo %s".formatted("bar");

It would be great if the https://errorprone.info/bugpattern/FormatString check was enforced on this.

[cushon]

SGTM, I'd be happy to take a PR for this

For the test coverage, we'll need something like the following to exclude the tests on Java < 15:

assumeTrue(RuntimeVersion.isAtLeast15());

[codylerum]

@cushon any guidance on how you would fit that in with the current checks? FormatString seems pretty tied to it being a method like .format(String pattern, Object... args) where this is on an instance of a String like"foo%s".formatted(Object... args)

Or would you create an entirely new check?

[cushon]

I think it would be possible to adapt the existing FormatString check.

It uses this method to get the list of arguments to e.g. String.format, and that list of expressions is passed through to FormatStringValidation.validate:

error-prone/core/src/main/java/com/google/errorprone/bugpatterns/formatstring/FormatStringUtils.java

Line 55 in 65eb009

public static ImmutableList<ExpressionTree> formatMethodArguments(

Perhaps formatMethodArguments could be updated to recognize e.g. foo.format("bar", "baz"), and then convert that to the list of expressions starting with the LHS, and continuing with the arguments: [foo, "bar", "baz"]

[codylerum]

@cushon Unfortunately I'm not able to figure out how to get Intellij to execute a unit test for debugging with everything on the classpath. Anyone have any hints or steps to make that happen?

[Stephan202]

@codylerum not sure what error you get, but if IntelliJ complains about duplicated classes on the classpath, then one workaround is to edit the run/debug configuration and click Modify Options -> Add before launch task -> Build, no error check, then drop the original Build step.

(You might also see an error about Stream#takeWhile being unknown. For that one raise the source level to 11.)

[codylerum]

@Stephan202

What I get is

"C:\Program Files\Eclipse Foundation\jdk-17.0.0.35-hotspot\bin\java.exe" -agentlib:jdwp=transport=dt_socket,address=127.0.0.1:51014,suspend=y,server=n -ea -Xmx1g --add-exports=jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED --add-exports=jdk.compiler/com.sun.tools.javac.file=ALL-UNNAMED --add-exports=jdk.compiler/com.sun.tools.javac.main=ALL-UNNAMED --add-exports=jdk.compiler/com.sun.tools.javac.model=ALL-UNNAMED --add-exports=jdk.compiler/com.sun.tools.javac.parser=ALL-UNNAMED --add-exports=jdk.compiler/com.sun.tools.javac.processing=ALL-UNNAMED --add-exports=jdk.compiler/com.sun.tools.javac.tree=ALL-UNNAMED --add-exports=jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED --add-opens=jdk.compiler/com.sun.tools.javac.code=ALL-UNNAMED --add-opens=jdk.compiler/com.sun.tools.javac.comp=ALL-UNNAMED --add-opens=java.base/java.math=ALL-UNNAMED --add-opens=java.base/java.nio=ALL-UNNAMED -Didea.test.cyclic.buffer.size=1048576 -javaagent:C:\Users\cody.lerum\AppData\Local\JetBrains\Toolbox\apps\IDEA-U\ch-1\213.6461.79\plugins\java\lib\rt\debugger-agent.jar -Dfile.encoding=UTF-8 @C:\Users\cody.lerum\AppData\Local\Temp\idea_arg_file644136494 com.intellij.rt.junit.JUnitStarter -ideVersion5 -junit4 com.google.errorprone.bugpatterns.formatstring.FormatStringTest,stringFormatted
Connected to the target VM, address: '127.0.0.1:51014', transport: 'socket'

java.lang.NoSuchMethodError: 'com.google.common.collect.ImmutableMap com.google.common.collect.ImmutableMap$Builder.buildOrThrow()'

	at com.google.errorprone.scanner.ScannerSupplier.defaultSeverities(ScannerSupplier.java:64)
	at com.google.errorprone.scanner.ScannerSupplier.fromBugCheckerInfos(ScannerSupplier.java:85)
	at com.google.errorprone.scanner.ScannerSupplier.fromBugCheckerClasses(ScannerSupplier.java:74)
	at com.google.errorprone.scanner.ScannerSupplier.fromBugCheckerClasses(ScannerSupplier.java:55)
	at com.google.errorprone.CompilationTestHelper.newInstance(CompilationTestHelper.java:116)
	at com.google.errorprone.bugpatterns.formatstring.FormatStringTest.<init>(FormatStringTest.java:32)
	at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
	at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:77)
	at java.base/jdk.internal.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
	at java.base/java.lang.reflect.Constructor.newInstanceWithCaller(Constructor.java:499)
	at java.base/java.lang.reflect.Constructor.newInstance(Constructor.java:480)
	at org.junit.runners.BlockJUnit4ClassRunner.createTest(BlockJUnit4ClassRunner.java:250)
	at org.junit.runners.BlockJUnit4ClassRunner.createTest(BlockJUnit4ClassRunner.java:260)
	at org.junit.runners.BlockJUnit4ClassRunner$2.runReflectiveCall(BlockJUnit4ClassRunner.java:309)
	at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
	at org.junit.runners.BlockJUnit4ClassRunner.methodBlock(BlockJUnit4ClassRunner.java:306)
	at org.junit.runners.BlockJUnit4ClassRunner$1.evaluate(BlockJUnit4ClassRunner.java:100)
	at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:366)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:103)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:63)
	at org.junit.runners.ParentRunner$4.run(ParentRunner.java:331)
	at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:79)
	at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:329)
	at org.junit.runners.ParentRunner.access$100(ParentRunner.java:66)
	at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:293)
	at org.junit.runners.ParentRunner$3.evaluate(ParentRunner.java:306)
	at org.junit.runners.ParentRunner.run(ParentRunner.java:413)
	at org.junit.runner.JUnitCore.run(JUnitCore.java:137)
	at com.intellij.junit4.JUnit4IdeaTestRunner.startRunnerWithArgs(JUnit4IdeaTestRunner.java:69)
	at com.intellij.rt.junit.IdeaTestRunner$Repeater$1.execute(IdeaTestRunner.java:38)
	at com.intellij.rt.execution.junit.TestsRepeater.repeat(TestsRepeater.java:11)
	at com.intellij.rt.junit.IdeaTestRunner$Repeater.startRunnerWithArgs(IdeaTestRunner.java:35)
	at com.intellij.rt.junit.JUnitStarter.prepareStreamsAndStart(JUnitStarter.java:235)
	at com.intellij.rt.junit.JUnitStarter.main(JUnitStarter.java:54)

which also shows as a compile error in intellij for ScannerSupplier on line 64

[Stephan202]

@codylerum looks like the classpath somehow contains an older version of Guava. Does your checkout contain any modifications that could cause an older Guava dependency to be pulled in? (The buildOrThrow method was introduced in version 31.0, while Error Prone currently uses 31.0.1. So this should just work.)

(This issue reminds me of #2138 (comment) and #2745 (comment), but there the issue was with using Error Prone, while you're trying to build Error Prone itself. 🤔)

[codylerum]

@Stephan202 Thanks for the pointers I went through and made intellij invalidate all it's caches and it seems to be compiling now.

Intellij seems to frequently have issues when a dependency version changes and it doesn't blow away the old one out of the build.

[codylerum]

I made some progress, but I'm out of my wheelhouse when it comes to walking around the complier here.

At line 116 I'm into the validator and I have my two expected arguments for my pattern of %s %s".formatted("foo", "baz");

https://github.com/codylerum/error-prone/blob/string_formatted/core/src/main/java/com/google/errorprone/bugpatterns/formatstring/FormatStringValidation.java#L116

However I'm lost at how to get the value of the string which is the "owner" of this method and the part that is the pattern %s %s I'm guessing this is a solved problem somewhere else.

[Stephan202]

@codylerum I won't have time for an in-dept analysis, but the formatMethodSymbol available in that method lacks the relevant information: it's the symbol associated with the String#formatted method, not linked to a particular string instance.

In the context of that specific method, when running FormatStringTest.stringFormatted, we have that state.getPath().getLeaf() points to the associated MethodInvocationTree, so the "%s %s" string can be retrieved as follows:

ASTHelpers.getReceiver((ExpressionTree) state.getPath().getLeaf())

However, this approach is very fragile (certainly that cast should be guarded, and there are likely other issues with it) and validate's signature/contract should likely be updated instead. Unfortunately I don't have more time to look into this now. But hopefully this helps somewhat :)

[codylerum]

Thanks @Stephan202 and @cushon

I was able to work up a PR that implements this which is now linked. Let me know if that direction works or if you want me to look at another way