Introduce `EagerStringFormatting` check #1139

Stephan202 · 2024-04-14T17:41:13Z

~~❗ 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:

Introduce `EagerStringFormatting` check (#1139)

This new check flags code that can be simplified and/or optimized by
deferring certain string formatting operations.

sonarqubecloud · 2024-04-14T17:43:26Z

Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
89.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

github-actions · 2024-04-14T17:43:56Z

Surviving mutants in this change: 20
Killed mutants in this change: 69

class	surviving	killed
🧟`tech.picnic.errorprone.bugpatterns.EagerStringFormatting$StringFormatExpression`	13	26
🧟`tech.picnic.errorprone.bugpatterns.EagerStringFormatting`	7	43

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

sonarqubecloud · 2024-08-04T10:55:07Z

Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
89.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

github-actions · 2024-08-04T10:55:50Z

Surviving mutants in this change: 20
Killed mutants in this change: 69

class	surviving	killed
🧟`tech.picnic.errorprone.bugpatterns.EagerStringFormatting$StringFormatExpression`	13	26
🧟`tech.picnic.errorprone.bugpatterns.EagerStringFormatting`	7	43

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

sonarqubecloud · 2024-09-21T15:21:56Z

Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
89.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

github-actions · 2024-09-21T15:22:57Z

Surviving mutants in this change: 20
Killed mutants in this change: 69

class	surviving	killed
🧟`tech.picnic.errorprone.bugpatterns.EagerStringFormatting$StringFormatExpression`	13	26
🧟`tech.picnic.errorprone.bugpatterns.EagerStringFormatting`	7	43

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

github-actions · 2024-12-14T14:22:58Z

Surviving mutants in this change: 20
Killed mutants in this change: 69

class	surviving	killed
🧟`tech.picnic.errorprone.bugpatterns.EagerStringFormatting$StringFormatExpression`	13	26
🧟`tech.picnic.errorprone.bugpatterns.EagerStringFormatting`	7	43

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

github-actions · 2024-12-27T13:04:25Z

Surviving mutants in this change: 2
Killed mutants in this change: 93

class	surviving	killed
🧟`tech.picnic.errorprone.bugpatterns.EagerStringFormatting`	1	55
🧟`tech.picnic.errorprone.bugpatterns.EagerStringFormatting$StringFormatExpression`	1	38

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

github-actions · 2024-12-27T13:34:44Z

Surviving mutants in this change: 2
Killed mutants in this change: 96

class	surviving	killed
🧟`tech.picnic.errorprone.bugpatterns.EagerStringFormatting`	1	58
🧟`tech.picnic.errorprone.bugpatterns.EagerStringFormatting$StringFormatExpression`	1	38

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

github-actions · 2024-12-27T18:30:02Z

Surviving mutants in this change: 2
Killed mutants in this change: 98

class	surviving	killed
🧟`tech.picnic.errorprone.bugpatterns.EagerStringFormatting`	1	58
🧟`tech.picnic.errorprone.bugpatterns.EagerStringFormatting$StringFormatExpression`	1	40

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

Stephan202 · 2024-12-27T18:52:29Z

@rickie @mohamedsamehsalah this one is finally ready for review. :)

github-actions · 2024-12-27T18:54:28Z

Surviving mutants in this change: 2
Killed mutants in this change: 98

class	surviving	killed
🧟`tech.picnic.errorprone.bugpatterns.EagerStringFormatting`	1	58
🧟`tech.picnic.errorprone.bugpatterns.EagerStringFormatting$StringFormatExpression`	1	40

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

mohamedsamehsalah

🧠 Saves a lot of review comments ❤️ ❤️

mohamedsamehsalah · 2024-12-27T19:55:19Z

...rone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/EagerStringFormattingTest.java

+  @Test
+  void identification() {
+    CompilationTestHelper.newInstance(EagerStringFormatting.class, getClass())
+        .expectErrorMessage("DEFER", m -> m.contains("String formatting can be deferred\n"))


is the \n character necessary? 👀

Yep, cause otherwise this is a strictly weaker predicate than the DEFER_* predicates below.

Makes sense 😄

mohamedsamehsalah · 2024-12-27T19:57:26Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/EagerStringFormatting.java

+import tech.picnic.errorprone.utils.SourceCode;
+
+/**
+ * A {@link BugChecker} that flags {@link String#format(String, Object...)}, {@link


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 👍 )

I could do {@link String#format} and {@link String#formatted} :)

mohamedsamehsalah · 2024-12-27T20:13:24Z

...rone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/EagerStringFormattingTest.java

+import com.google.errorprone.CompilationTestHelper;
+import org.junit.jupiter.api.Test;
+
+final class EagerStringFormattingTest {


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",

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 👀

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.

Stephan202

Thanks for taking such a quick look @mohamedsamehsalah! I added a commit.

Stephan202 · 2024-12-28T09:02:05Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/EagerStringFormatting.java

+import tech.picnic.errorprone.utils.SourceCode;
+
+/**
+ * A {@link BugChecker} that flags {@link String#format(String, Object...)}, {@link


I could do {@link String#format} and {@link String#formatted} :)

Stephan202 · 2024-12-28T09:02:50Z

...rone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/EagerStringFormattingTest.java

+  @Test
+  void identification() {
+    CompilationTestHelper.newInstance(EagerStringFormatting.class, getClass())
+        .expectErrorMessage("DEFER", m -> m.contains("String formatting can be deferred\n"))


Yep, cause otherwise this is a strictly weaker predicate than the DEFER_* predicates below.

Stephan202 · 2024-12-28T16:21:10Z

...rone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/EagerStringFormattingTest.java

+import com.google.errorprone.CompilationTestHelper;
+import org.junit.jupiter.api.Test;
+
+final class EagerStringFormattingTest {


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.

github-actions · 2024-12-28T16:49:01Z

Surviving mutants in this change: 2
Killed mutants in this change: 98

class	surviving	killed
🧟`tech.picnic.errorprone.bugpatterns.EagerStringFormatting`	1	58
🧟`tech.picnic.errorprone.bugpatterns.EagerStringFormatting$StringFormatExpression`	1	40

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

mohamedsamehsalah

🧯

rickie

Look at those IT 😍 , so nice! Flushing my two comments I have so far. It's a big one to get through with all the cases, but it will be such a nice one to release this.

rickie · 2025-01-09T17:12:56Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/EagerStringFormatting.java

+    linkType = CUSTOM,
+    severity = WARNING,
+    tags = {PERFORMANCE, SIMPLIFICATION})
+public final class EagerStringFormatting extends BugChecker implements MethodInvocationTreeMatcher {


Could you elaborate on why we use "Eager" in the name of the check. In somewhat similar cases we use "Redundant", curious why we don't do that here :).

Yep. Basically this check flags cases where string formatting happens unconditionally, rather than happening only if some other condition is met (precondition failure, log statement at enabled log level, ...). So the check flags "eager" formatting operations that could be "deferred" instead.

rickie · 2025-01-09T17:22:01Z

...rone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/EagerStringFormattingTest.java

+            "",
+            "    checkArgument(true, String.format(\"Vacuous format string %%\"));",
+            "    checkNotNull(\"never-null\", \"Format string: %s %s%%\".formatted(1, 2));",
+            "    checkState(false, String.format(Locale.US, \"Format string with locale: %s\", 3));",


I'm curious, if we drop the Locale, that could be non-behavior preserving, right?

IIUC this should actually be fine, because we explicitly handle java.util.Formattable arguments by not suggesting a rewrite when those are present.

This new check flags code that can be simplified and/or optimized by deferring certain string formatting operations.

sonarqubecloud · 2025-01-11T13:26:53Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

github-actions · 2025-01-11T13:27:49Z

Surviving mutants in this change: 2
Killed mutants in this change: 98

class	surviving	killed
🧟`tech.picnic.errorprone.bugpatterns.EagerStringFormatting`	1	58
🧟`tech.picnic.errorprone.bugpatterns.EagerStringFormatting$StringFormatExpression`	1	40

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

rickie

Nice work @Stephan202 , really curious to see this on our codebase!

Stephan202 added this to the 0.17.0 milestone Apr 14, 2024

Stephan202 marked this pull request as draft April 14, 2024 17:41

Stephan202 mentioned this pull request Apr 14, 2024

Introduce Check{Argument,State}WithMessageAndArguments Refaster rules #1135

Closed

Stephan202 modified the milestones: 0.17.0, 0.18.0 Jul 20, 2024

Stephan202 force-pushed the sschroevers/introduce-redundant-string-escape-check branch from 1b69726 to 96c8208 Compare August 4, 2024 10:39

Stephan202 added the new feature label Aug 4, 2024

Stephan202 force-pushed the sschroevers/eager-string-formatting-check branch from 0caa780 to 9675a51 Compare August 4, 2024 10:52

Stephan202 modified the milestones: 0.18.0, 0.19.0 Aug 11, 2024

Stephan202 force-pushed the sschroevers/introduce-redundant-string-escape-check branch from 96c8208 to 8ee3ce8 Compare September 21, 2024 15:00

Stephan202 force-pushed the sschroevers/eager-string-formatting-check branch from 9675a51 to 459da80 Compare September 21, 2024 15:19

rickie modified the milestones: 0.19.0, 0.20.0 Oct 23, 2024

rickie force-pushed the sschroevers/introduce-redundant-string-escape-check branch from 8ee3ce8 to 4ce0955 Compare November 12, 2024 10:11

Stephan202 force-pushed the sschroevers/introduce-redundant-string-escape-check branch 2 times, most recently from 3e30dc5 to f200a92 Compare November 18, 2024 19:06

Base automatically changed from sschroevers/introduce-redundant-string-escape-check to master November 18, 2024 19:33

Stephan202 force-pushed the sschroevers/eager-string-formatting-check branch from 459da80 to 49a376b Compare December 14, 2024 14:19

Stephan202 force-pushed the sschroevers/eager-string-formatting-check branch from 49a376b to edb6cc7 Compare December 27, 2024 13:01

Stephan202 force-pushed the sschroevers/eager-string-formatting-check branch from 992e5fb to 4bc7f8b Compare December 27, 2024 18:27

Stephan202 force-pushed the sschroevers/eager-string-formatting-check branch from 4bc7f8b to caff523 Compare December 27, 2024 18:51

Stephan202 requested review from rickie and mohamedsamehsalah December 27, 2024 18:51

Stephan202 marked this pull request as ready for review December 27, 2024 18:51

mohamedsamehsalah reviewed Dec 27, 2024

View reviewed changes

Stephan202 commented Dec 28, 2024

View reviewed changes

mohamedsamehsalah approved these changes Dec 29, 2024

View reviewed changes

rickie reviewed Jan 10, 2025

View reviewed changes

Stephan202 added 2 commits January 11, 2025 14:24

Introduce EagerStringFormatting check

733aa14

This new check flags code that can be simplified and/or optimized by deferring certain string formatting operations.

Tweaks

fa59f63

rickie force-pushed the sschroevers/eager-string-formatting-check branch from 3af3cf2 to fa59f63 Compare January 11, 2025 13:24

rickie approved these changes Jan 11, 2025

View reviewed changes

rickie merged commit a42353b into master Jan 11, 2025
16 checks passed

rickie deleted the sschroevers/eager-string-formatting-check branch January 11, 2025 13:42

Introduce EagerStringFormatting check #1139

Introduce EagerStringFormatting check #1139

Conversation

Stephan202 commented Apr 14, 2024 • edited Loading

sonarqubecloud bot commented Apr 14, 2024

Quality Gate passed

github-actions bot commented Apr 14, 2024

sonarqubecloud bot commented Aug 4, 2024

Quality Gate passed

github-actions bot commented Aug 4, 2024

sonarqubecloud bot commented Sep 21, 2024

Quality Gate passed

github-actions bot commented Sep 21, 2024

github-actions bot commented Dec 14, 2024

github-actions bot commented Dec 27, 2024

github-actions bot commented Dec 27, 2024

github-actions bot commented Dec 27, 2024

Stephan202 commented Dec 27, 2024

github-actions bot commented Dec 27, 2024

mohamedsamehsalah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mohamedsamehsalah Dec 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stephan202 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Dec 28, 2024

mohamedsamehsalah left a comment

Choose a reason for hiding this comment

rickie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Jan 11, 2025

Quality Gate passed

github-actions bot commented Jan 11, 2025

rickie left a comment

Choose a reason for hiding this comment

Introduce `EagerStringFormatting` check #1139

Introduce `EagerStringFormatting` check #1139

Stephan202 commented Apr 14, 2024 •

edited

Loading

mohamedsamehsalah Dec 27, 2024 •

edited

Loading