-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Java: Add new quality query to detect String#replaceAll
with non-regex first argument
#19115
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
041adcd
ff2947a
b5b252b
441c79e
fea3d10
042fe07
c4e56b1
626a7d5
04ec1d7
e1c5517
3ea5cc1
ad89e79
576f4cf
acfcc6d
4f5bdbb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
# Use of `String#replaceAll` with a first argument which is not a regular expression | ||
|
||
Using `String#replaceAll` is less performant than `String#replace` when the first argument is not a regular expression. | ||
|
||
## Overview | ||
|
||
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. | ||
|
||
## Recommendation | ||
|
||
Use `String#replace` instead where a `replaceAll` call uses a trivial string as its first argument. | ||
|
||
## Example | ||
|
||
```java | ||
public class Test { | ||
void f() { | ||
String s1 = "test"; | ||
s1 = s1.replaceAll("t", "x"); // NON_COMPLIANT | ||
s1 = s1.replaceAll(".*", "x"); // COMPLIANT | ||
Comment on lines
+18
to
+20
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
} | ||
} | ||
|
||
``` | ||
|
||
## 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)). | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor. I don't see There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It should go to the |
||
- Common Weakness Enumeration: [CWE-1176](https://cwe.mitre.org/data/definitions/1176.html). |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
/** | ||
* @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` with a first argument which is not a regular expression | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍🏻 for replacing "performant" with "more efficient" |
||
* is less efficient than using `String#replace`. | ||
* @kind problem | ||
* @precision very-high | ||
* @problem.severity recommendation | ||
* @tags quality | ||
* reliability | ||
* performance | ||
* external/cwe/cwe-1176 | ||
*/ | ||
|
||
import java | ||
|
||
from StringReplaceAllCall replaceAllCall, StringLiteral firstArg | ||
where | ||
firstArg = replaceAllCall.getArgument(0) and | ||
//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 to 'replace' as its $@ is not a regular expression.", | ||
firstArg, "first argument" |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,14 +1,15 @@ | ||
- queries: . | ||
- include: | ||
id: | ||
- java/suspicious-date-format | ||
- java/integer-multiplication-cast-to-long | ||
- java/equals-on-unrelated-types | ||
- java/contradictory-type-checks | ||
- java/reference-equality-of-boxed-types | ||
- java/equals-on-unrelated-types | ||
- java/inconsistent-equals-and-hashcode | ||
- java/unchecked-cast-in-equals | ||
- java/unused-container | ||
- java/input-resource-leak | ||
- java/integer-multiplication-cast-to-long | ||
- java/output-resource-leak | ||
- java/type-variable-hides-type | ||
- java/reference-equality-of-boxed-types | ||
- java/string-replace-all-with-non-regex | ||
- java/suspicious-date-format | ||
- java/type-variable-hides-type | ||
- java/unchecked-cast-in-equals | ||
- java/unused-container |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
| Test.java:4:14:4:36 | replaceAll(...) | This call to 'replaceAll' should be a call to 'replace' as its $@ is not a regular expression. | Test.java:4:28:4:30 | "t" | first argument | |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
query: Performance/StringReplaceAllWithNonRegex.ql | ||
postprocess: utils/test/InlineExpectationsTestQuery.ql |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
public class Test { | ||
void f() { | ||
String s1 = "test"; | ||
s1 = s1.replaceAll("t", "x"); // $ Alert // NON_COMPLIANT | ||
s1 = s1.replaceAll(".*", "x"); // COMPLIANT | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love it, it is much clearer to me ✨
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The credit should go to Claude 3.7 Sonnet 😆 .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Glad to see that someone is using Copilot 🤣