VA_FORMAT_STRING_USES_NEWLINE handling of text blocks
#2881
Conversation
Text blocks line endings should be escaped when used in a format, the String.formatted() method should be checked as we are already checking the static version String.format()
fix: check calls to String.formatted()
gtoison
changed the title
VA_FORMAT_STRING_USES_NEWLINE handling of text blocks
|
I'm clearly not an expert in this field by any means, but imho this is not a fix, but an ugly workaround which goes against the goal of the textblocks. JEP 378 clearly states that the goal is to not explicitliy write line breaks: "Simplify the task of writing Java programs by making it easy to express strings that span several lines of source code, while avoiding escape sequences in common cases." Sprinkling %n everywhere does therefore not sound sane. Maybe this is more or less a bug for openjdk and such? |
|
This rule is flagging issues when a string is fed into a formatter, in other cases (merely declaring a string via a text block) it shouldn't complain. I agree it is ugly but you should write |
If you don't want to have line breaks in your string, then you can simply skip the line breaks at the end of the line with As far as I understand yes, this is rather a compiler problem, and since SpotBugs works on the compiled code, and I'm not sure whether we can differentiate a simple string literal and the text blocks introduced in Java 15 on the bytecode level (maybe from the line numbers). Also, this is rather about the programmer's intent, whether he/she wants a line break or not. |
| || "java/io/PrintStream".equals(cl) && "printf".equals(nm) | ||
| || cl.endsWith("Writer") && "format".equals(nm) | ||
| || cl.endsWith("Writer") && "printf".equals(nm)) | ||
| || cl.endsWith("Logger") && nm.endsWith("fmt")) { |
There was a problem hiding this comment.
Thanks for the formatting. It's much more readable 😄
|
|
||
| if ("java/lang/String".equals(cl) && "formatted".equals(nm)) { |
There was a problem hiding this comment.
Can you please check the signature as well?
| if ("java/lang/String".equals(cl) && "formatted".equals(nm)) { | ||
| Object stackItem = stack.getStackItem(1).getConstant(); | ||
| if (stackItem instanceof String) { | ||
| String string = (String) stackItem; |
There was a problem hiding this comment.
It looks like formatString which is set in line 79 is exactly the same. Is there any reason not to use that?
| bugReporter.reportBug(new BugInstance(this, "VA_FORMAT_STRING_USES_NEWLINE", NORMAL_PRIORITY) | ||
| .addClassAndMethod(this).addCalledMethod(this).addString(string) | ||
| .describe(StringAnnotation.FORMAT_STRING_ROLE).addSourceLine(this)); | ||
| } |
There was a problem hiding this comment.
This is the duplication of 117-121. I think it would be better if the check for java.lang.String.formatted() would be added to the condition from 107.
There was a problem hiding this comment.
I've made changes to merge the check with the existing one, I think it addresses your remarks
Thank you for reviewing!
There was a problem hiding this comment.
Think we were good going to merge.
|
Other than concerns on this raised, I think it looks ok. +1 from me. |
The text blocks introduced in Java 15 pose problem when used in a formatter because the line endings are not platform specific and SpotBugs correctly raises a VA_FORMAT_STRING_USES_NEWLINE
Text blocks line endings should be escaped when used in a format (using
%n\). The bug description should give that solution.The String.formatted() method (added in Java 15) should also be checked as we are already checking the static version String.format()
This should fix #1877