Java: Add new quality query to detect `String#replaceAll` with non-regex first argument by owen-mc · Pull Request #19115 · github/codeql

owen-mc · 2025-03-25T14:31:39Z

Added a new quality query, java/string-replace-all-with-non-regex, to detect uses of String#replaceAll when the first argument is not a regular expression, which should use String#replace instead for better performance.

Copilot

Pull Request Overview

This PR adds a new quality query to detect cases where String#replaceAll is used with a non-regular expression as the first argument, recommending the use of String#replace for better performance.

Introduces a test case to validate the new query.
Adds a change note to document the new quality query.
Provides a performance guideline detailing the issue and recommendation.

Reviewed Changes

Copilot reviewed 3 out of 6 changed files in this pull request and generated no comments.

File	Description
java/ql/test/query-tests/StringReplaceAllWithNonRegex/Test.java	Added test cases demonstrating compliant and non-compliant usage.
java/ql/src/change-notes/2025-03-25-string-replace-all-with-non-regex.md	Added change note for the new quality query.
java/ql/src/Performance/StringReplaceAllWithNonRegex.md	Added performance guideline explaining why String#replace is preferable when a non-regex literal is used.

Files not reviewed (3)

java/ql/src/Performance/StringReplaceAllWithNonRegex.ql: Language not supported
java/ql/test/query-tests/StringReplaceAllWithNonRegex/StringReplaceAllWithNonRegex.expected: Language not supported
java/ql/test/query-tests/StringReplaceAllWithNonRegex/StringReplaceAllWithNonRegex.qlref: Language not supported

Tip: Copilot code review supports C#, Go, Java, JavaScript, Markdown, Python, Ruby and TypeScript, with more languages coming soon. Learn more

smowton

Looks plausible

tamasvajk · 2025-03-26T07:39:49Z

java/ql/src/Performance/StringReplaceAllWithNonRegex.md

+## References
+
+- Java SE Documentation: [String.replaceAll](https://docs.oracle.com/en/java/javase/20/docs/api/java.base/java/lang/String.html#replaceAll(java.lang.String,java.lang.String)).
+- Common Weakness Enumeration: [CWE-1176](https://cwe.mitre.org/data/definitions/1176.html)


Minor: a . is missing from the end of line.

tamasvajk · 2025-03-26T07:40:54Z

java/ql/src/Performance/StringReplaceAllWithNonRegex.ql

+  //only contains characters that could be a simple string
+  firstArg.getValue().regexpMatch("^[a-zA-Z0-9]+$")
+select replaceAllCall,
+  "This call to 'replaceAll' should be a call `replace` as its $@ is not a regular expression.",


Does ` work in the alert message? replaceAll and replace should probably be quoted the same way.

In VS Code, ` doesn't seem to do anything, so I've changed it to '.

owen-mc · 2025-03-26T10:26:13Z

I ran MRVA. There are 417 results in the top 1,000 repos. 114 repos had at least one result. The most in one repo is 86 but that's an outlier - the next highest is 15. All the results look like TPs to me.

owen-mc · 2025-03-27T15:14:57Z

I have validated autofixes. As I expected, they are all good (though we tend to fix multiple instances at once rather than just the specified one). So I think this is ready to merge now.

owen-mc · 2025-03-27T15:16:15Z

@knewbury01 for awareness.

tamasvajk

Should this query be added to the ~~ccr.qls~~ code-quality.qls?

mchammer01 · 2025-03-31T06:10:48Z

I will review this for Docs today.

mchammer01

@owen-mc 👋🏻 - Approving on behalf of Docs ✨
Highlighted minor things + asked some questions for my own understanding.

mchammer01 · 2025-03-31T07:26:46Z

java/ql/src/Performance/StringReplaceAllWithNonRegex.md

+
+## Overview
+
+The underlying implementation of `String#replaceAll` uses `Pattern#compile` and expects a regular expression as its first argument. However in cases where the argument could be represented by just a plain `String` that does not represent an interesting regular expression, a call to `String#replace` may be more performant as it does not need to compile the regular expression.


For my own understanding: what is an "interesting" regular expression? 🤔
Also the clause inside "however" is a bit long. Would it be possible to shorten/simplify it?

Here, "interesting" means "uses any of the special syntax allowed in regular expressions to do more than just match text verbatim". I will think about how to improve.

mchammer01 · 2025-03-31T07:28:55Z

java/ql/src/Performance/StringReplaceAllWithNonRegex.md

+
+## References
+
+- Java SE Documentation: [String.replaceAll](https://docs.oracle.com/en/java/javase/20/docs/api/java.base/java/lang/String.html#replaceAll(java.lang.String,java.lang.String)).


Minor. I don't see String.replaceAll mentioned at all in the link provided. This seems to link to the String class.

It should go to the replaceAll method in the page on the String class. It seems to work for me.

mchammer01 · 2025-03-31T07:35:38Z

java/ql/src/Performance/StringReplaceAllWithNonRegex.ql

+/**
+ * @id java/string-replace-all-with-non-regex
+ * @name Use of `String#replaceAll` with a first argument which is not a regular expression
+ * @description Using `String#replaceAll` is less performant than `String#replace` when the first


As you know, I am not a developer, so I would be grateful if you could explain what the problem is with this being less performant. Does it take more time to execute?

Would something like be correct/acceptable?
(Sorry I hadn't hear of the adjective "performant" before. Discovered since that it is indeed a word in the engineering/computing area 😅 )

Using String#replaceAll instead of String#replace when the first argument is not a regular expression causes performance issues.

Feel free to ignore if you think that developers will understand what this query does with the description as-is.

mchammer01

@owen-mc - thanks for the tweaks, LGTM ⚡

mchammer01 · 2025-04-01T09:19:33Z

java/ql/src/Performance/StringReplaceAllWithNonRegex.md

 ## Overview

-The underlying implementation of `String#replaceAll` uses `Pattern#compile` and expects a regular expression as its first argument. However in cases where the argument could be represented by just a plain `String` that does not represent an interesting regular expression, a call to `String#replace` may be more performant as it does not need to compile the regular expression.
+The `String#replaceAll` method is designed to work with regular expressions as its first parameter. When you use a simple string without any regex patterns (like special characters or syntax), it's more efficient to use `String#replace` instead. This is because `replaceAll` has to compile the input as a regular expression first, which adds unnecessary overhead when you are just replacing literal text.


Love it, it is much clearer to me ✨

The credit should go to Claude 3.7 Sonnet 😆 .

Glad to see that someone is using Copilot 🤣

mchammer01 · 2025-04-01T09:20:04Z

java/ql/src/Performance/StringReplaceAllWithNonRegex.ql

 * @name Use of `String#replaceAll` with a first argument which is not a regular expression
- * @description Using `String#replaceAll` is less performant than `String#replace` when the first
- *              argument is not a regular expression.
+ * @description Using `String#replaceAll` with a first argument which is not a regular expression


👍🏻 for replacing "performant" with "more efficient"

CWE-1176: Inefficient CPU Computation

owen-mc · 2025-04-10T14:08:38Z

I think this is now ready to merge.

tamasvajk

LGTM

joefarebrother · 2025-04-14T08:35:49Z

java/ql/src/Performance/StringReplaceAllWithNonRegex.md

+        String s1 = "test";
+        s1 = s1.replaceAll("t", "x"); // NON_COMPLIANT
+        s1 = s1.replaceAll(".*", "x"); // COMPLIANT


Just a drive-by comment after this is already merged - would it be better to label these cases as GOOD and BAD as is more common in qhelp docs; and also include a case that uses replace?

We've been using COMPLIANT and NON_COMPLIANT for all the queries we are porting. If we do want to change it, the most efficient way would be to wait until they're all ported and then do a find-replace. Otherwise we have to remember to do it 40 times.

I can see a small benefit in including something like the following in the example (and tests, I guess). Is that what you mean?

s1 = s1.replace("t", "x"); // COMPLIANT

Copilot AI review requested due to automatic review settings March 25, 2025 14:31

owen-mc requested a review from a team as a code owner March 25, 2025 14:31

Copilot AI reviewed Mar 25, 2025

View reviewed changes

github-actions bot added documentation Java labels Mar 25, 2025

smowton previously approved these changes Mar 25, 2025

View reviewed changes

tamasvajk reviewed Mar 26, 2025

View reviewed changes

owen-mc dismissed smowton’s stale review via b01df5e March 26, 2025 10:19

tamasvajk previously approved these changes Mar 27, 2025

View reviewed changes

owen-mc dismissed tamasvajk’s stale review via 1ff90bf March 27, 2025 15:13

tamasvajk reviewed Mar 27, 2025

View reviewed changes

owen-mc added the ready-for-doc-review This PR requires and is ready for review from the GitHub docs team. label Mar 27, 2025

mchammer01 self-requested a review March 31, 2025 06:07

mchammer01 previously approved these changes Mar 31, 2025

View reviewed changes

owen-mc dismissed mchammer01’s stale review via de3d3e4 March 31, 2025 21:45

owen-mc requested a review from mchammer01 April 1, 2025 09:12

mchammer01 previously approved these changes Apr 1, 2025

View reviewed changes

owen-mc added 9 commits April 10, 2025 11:46

Java: Add initial version of string replaceAll with no regex query

041adcd

Adjust query name

ff2947a

Convert test to inline expectations

b5b252b

Use existing class StringReplaceAllCall

441c79e

Update qhelp

fea3d10

Adjust alert message

042fe07

Add quality and cwe tag to query

c4e56b1

CWE-1176: Inefficient CPU Computation

Fix punctuation

626a7d5

Update test expectations

04ec1d7

owen-mc added 5 commits April 10, 2025 12:21

Keep COMPLIANT and NON_COMPLIANT comments in test

e1c5517

Add query to code-quality query suite

3ea5cc1

Tweak documentation

ad89e79

Update tags

576f4cf

Sort ids in java-code-quality.qls

acfcc6d

owen-mc dismissed mchammer01’s stale review via acfcc6d April 10, 2025 11:40

owen-mc force-pushed the java/port/java/string-replace-all-with-non-regex branch from de3d3e4 to acfcc6d Compare April 10, 2025 11:40

owen-mc added the no-change-note-required This PR does not need a change note label Apr 10, 2025

Add new query to java-code-quality.qls.expected

4f5bdbb

owen-mc requested a review from tamasvajk April 10, 2025 14:01

tamasvajk approved these changes Apr 11, 2025

View reviewed changes

owen-mc merged commit 472bfa2 into github:main Apr 11, 2025
17 checks passed

owen-mc deleted the java/port/java/string-replace-all-with-non-regex branch April 11, 2025 09:31

joefarebrother reviewed Apr 14, 2025

View reviewed changes


		## Overview

		The underlying implementation of `String#replaceAll` uses `Pattern#compile` and expects a regular expression as its first argument. However in cases where the argument could be represented by just a plain `String` that does not represent an interesting regular expression, a call to `String#replace` may be more performant as it does not need to compile the regular expression.


		## References

		- Java SE Documentation: [String.replaceAll](https://docs.oracle.com/en/java/javase/20/docs/api/java.base/java/lang/String.html#replaceAll(java.lang.String,java.lang.String)).

Conversation

owen-mc commented Mar 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

smowton left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

owen-mc commented Mar 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

owen-mc commented Mar 27, 2025

Uh oh!

owen-mc commented Mar 27, 2025

Uh oh!

tamasvajk left a comment

Choose a reason for hiding this comment

Uh oh!

mchammer01 commented Mar 31, 2025

Uh oh!

mchammer01 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mchammer01 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

owen-mc Apr 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

owen-mc commented Apr 10, 2025

Uh oh!

tamasvajk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

owen-mc commented Mar 25, 2025 •

edited

Loading

owen-mc commented Mar 26, 2025 •

edited

Loading

owen-mc Apr 1, 2025 •

edited

Loading