Introduce EagerStringFormatting check
#1139
Conversation
|
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
Stephan202
force-pushed
the
sschroevers/introduce-redundant-string-escape-check
branch
from
1b69726 to
96c8208
Compare
Stephan202
force-pushed
the
sschroevers/eager-string-formatting-check
branch
from
0caa780 to
9675a51
Compare
|
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
Stephan202
force-pushed
the
sschroevers/introduce-redundant-string-escape-check
branch
from
96c8208 to
8ee3ce8
Compare
Stephan202
force-pushed
the
sschroevers/eager-string-formatting-check
branch
from
9675a51 to
459da80
Compare
|
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
rickie
force-pushed
the
sschroevers/introduce-redundant-string-escape-check
branch
from
8ee3ce8 to
4ce0955
Compare
Stephan202
force-pushed
the
sschroevers/introduce-redundant-string-escape-check
branch
2 times, most recently
from
3e30dc5 to
f200a92
Compare
Stephan202
force-pushed
the
sschroevers/eager-string-formatting-check
branch
from
459da80 to
49a376b
Compare
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
Stephan202
force-pushed
the
sschroevers/eager-string-formatting-check
branch
from
49a376b to
edb6cc7
Compare
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
Stephan202
force-pushed
the
sschroevers/eager-string-formatting-check
branch
from
992e5fb to
4bc7f8b
Compare
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
This new check flags code that can be simplified and/or optimized by deferring certain string formatting operations.
Stephan202
force-pushed
the
sschroevers/eager-string-formatting-check
branch
from
4bc7f8b to
caff523
Compare
|
@rickie @mohamedsamehsalah this one is finally ready for review. :) |
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
| @Test | ||
| void identification() { | ||
| CompilationTestHelper.newInstance(EagerStringFormatting.class, getClass()) | ||
| .expectErrorMessage("DEFER", m -> m.contains("String formatting can be deferred\n")) |
There was a problem hiding this comment.
is the \n character necessary? 👀
There was a problem hiding this comment.
Yep, cause otherwise this is a strictly weaker predicate than the DEFER_* predicates below.
| import tech.picnic.errorprone.utils.SourceCode; | ||
|
|
||
| /** | ||
| * A {@link BugChecker} that flags {@link String#format(String, Object...)}, {@link |
There was a problem hiding this comment.
Maybe either just something along the lines of "String#format(String) and it's overloaded siblings", or format (heh) in a <ul> tag.
(No strong opinion, this is also readable 👍 )
There was a problem hiding this comment.
I could do {@link String#format} and {@link String#formatted} :)
| import com.google.errorprone.CompilationTestHelper; | ||
| import org.junit.jupiter.api.Test; | ||
|
|
||
| final class EagerStringFormattingTest { |
There was a problem hiding this comment.
Shouldn't we also check for flag the case when the potential String#format invocations is stored and passed in a variable to the "parent" method?
Index: error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/EagerStringFormattingTest.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>ISO-8859-1
===================================================================
diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/EagerStringFormattingTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/EagerStringFormattingTest.java
--- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/EagerStringFormattingTest.java (revision caff523200614c14f23100fe4957dbf3d020bb4a)
+++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/EagerStringFormattingTest.java (date 1735330311287)
@@ -65,6 +65,9 @@
"",
" requireNonNull(\"never-null\");",
" requireNonNull(\"never-null\", () -> String.format(\"Format string: %s\", nonFinalField));",
+ " // BUG: Diagnostic matches: DEFER_PARAM",
+ " String loggedMessage = String.format(\"Never-null format string: %s\", nonFinalField)",
+ " requireNonNull(loggedMessage);",
" // BUG: Diagnostic matches: VACUOUS",
" requireNonNull(String.format(\"Never-null format string: %s\", nonFinalField));",
" // BUG: Diagnostic matches: VACUOUS",
There was a problem hiding this comment.
is stored and passed in a variable to the "parent" method?
Or even go far and check if it's returned from another method invocation 👀
There was a problem hiding this comment.
Indeed, none of these cases are currently covered. For now I'll call out these limitations in a comment; the check/PR is already complex enough as it is.
There was a problem hiding this comment.
Thanks for taking such a quick look @mohamedsamehsalah! I added a commit.
| import tech.picnic.errorprone.utils.SourceCode; | ||
|
|
||
| /** | ||
| * A {@link BugChecker} that flags {@link String#format(String, Object...)}, {@link |
There was a problem hiding this comment.
I could do {@link String#format} and {@link String#formatted} :)
| @Test | ||
| void identification() { | ||
| CompilationTestHelper.newInstance(EagerStringFormatting.class, getClass()) | ||
| .expectErrorMessage("DEFER", m -> m.contains("String formatting can be deferred\n")) |
There was a problem hiding this comment.
Yep, cause otherwise this is a strictly weaker predicate than the DEFER_* predicates below.
| import com.google.errorprone.CompilationTestHelper; | ||
| import org.junit.jupiter.api.Test; | ||
|
|
||
| final class EagerStringFormattingTest { |
There was a problem hiding this comment.
Indeed, none of these cases are currently covered. For now I'll call out these limitations in a comment; the check/PR is already complex enough as it is.
|
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
❗ This PR is on top of #1138. ❗This is a draft PR; a bit more work (especially around tests) is required. I'm opening the PR already, as I'd like to reference it.
Suggested commit message: