Skip to content
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.

Sign up for GitHub

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

SONARJAVA-5232 S1192: Do not consider arguments in Throwables when checking for duplicated strings #4984

Closed
wants to merge 13 commits into from
Closed

SONARJAVA-5232 S1192: Do not consider arguments in Throwables when checking for duplicated strings #4984

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
c00f35a
S1192: Do not consider arguments in Throwables when checking for dupl…
tomasz-tylenda-sonarsource Jan 20, 2025
644966b
cleanup
tomasz-tylenda-sonarsource Jan 29, 2025
424d246
Use Optional instead of if-then
tomasz-tylenda-sonarsource Jan 29, 2025
ba694ef
Update ruling goldens
tomasz-tylenda-sonarsource Jan 30, 2025
6d63e1a
Update ruling goldens - part 2
tomasz-tylenda-sonarsource Jan 30, 2025
7ec939d
Update java-checks/src/main/java/org/sonar/java/checks/StringLiteralD…
tomasz-tylenda-sonarsource Feb 4, 2025
d2f6f2a
Update java-checks-test-sources/default/src/main/java/checks/StringLi…
tomasz-tylenda-sonarsource Feb 4, 2025
1710a4d
Update java-checks-test-sources/default/src/main/java/checks/StringLi…
tomasz-tylenda-sonarsource Feb 4, 2025
13cef4e
test case with concatenation
tomasz-tylenda-sonarsource Feb 4, 2025
a29a193
test without semantics
tomasz-tylenda-sonarsource Feb 4, 2025
edf21d3
concatenation in arguments
tomasz-tylenda-sonarsource Feb 4, 2025
1549244
Updating goldens after adding support for string concat
tomasz-tylenda-sonarsource Feb 4, 2025
f85ffb6
Updating golden for concatenation - part 2
tomasz-tylenda-sonarsource Feb 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 0 additions & 11 deletions its/ruling/src/test/resources/commons-beanutils/java-S1192.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,17 +14,10 @@
220
],
"commons-beanutils:commons-beanutils:src/main/java/org/apache/commons/beanutils2/LazyDynaBean.java": [
235,
360,
407,
728
],
"commons-beanutils:commons-beanutils:src/main/java/org/apache/commons/beanutils2/LazyDynaClass.java": [
231
],
"commons-beanutils:commons-beanutils:src/main/java/org/apache/commons/beanutils2/LazyDynaMap.java": [
224
],
"commons-beanutils:commons-beanutils:src/main/java/org/apache/commons/beanutils2/MappedPropertyDescriptor.java": [
91
],
Expand All @@ -34,7 +27,6 @@
"commons-beanutils:commons-beanutils:src/main/java/org/apache/commons/beanutils2/PropertyUtilsBean.java": [
292,
292,
363,
420,
429,
430,
Expand All @@ -47,9 +39,6 @@
1627,
1630
],
"commons-beanutils:commons-beanutils:src/main/java/org/apache/commons/beanutils2/ResultSetIterator.java": [
98
],
"commons-beanutils:commons-beanutils:src/main/java/org/apache/commons/beanutils2/WrapDynaBean.java": [
141,
145
Expand Down
16 changes: 1 addition & 15 deletions its/ruling/src/test/resources/eclipse-jetty-similar-to-main/java-S1192.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,6 @@
"org.eclipse.jetty:jetty-project:jetty-http/src/main/java/org/eclipse/jetty/http/HttpGenerator.java": [
276
],
"org.eclipse.jetty:jetty-project:jetty-http/src/main/java/org/eclipse/jetty/http/HttpParser.java": [
974
],
"org.eclipse.jetty:jetty-project:jetty-http/src/main/java/org/eclipse/jetty/http/MimeTypes.java": [
106
],
Expand All @@ -19,8 +16,7 @@
],
"org.eclipse.jetty:jetty-project:jetty-io/src/main/java/org/eclipse/jetty/io/ssl/SslConnection.java": [
644,
932,
1013
932
],
"org.eclipse.jetty:jetty-project:jetty-server/src/main/java/org/eclipse/jetty/server/CustomRequestLog.java": [
597
Expand All @@ -33,7 +29,6 @@
],
"org.eclipse.jetty:jetty-project:jetty-server/src/main/java/org/eclipse/jetty/server/HttpOutput.java": [
680,
716,
1677
],
"org.eclipse.jetty:jetty-project:jetty-server/src/main/java/org/eclipse/jetty/server/InclusiveByteRange.java": [
Expand All @@ -52,7 +47,6 @@
198
],
"org.eclipse.jetty:jetty-project:jetty-server/src/main/java/org/eclipse/jetty/server/ShutdownMonitor.java": [
188,
359,
360,
366
Expand All @@ -66,27 +60,19 @@
"org.eclipse.jetty:jetty-project:jetty-server/src/main/java/org/eclipse/jetty/server/session/AbstractSessionCache.java": [
444
],
"org.eclipse.jetty:jetty-project:jetty-server/src/main/java/org/eclipse/jetty/server/session/AbstractSessionDataStore.java": [
161
],
"org.eclipse.jetty:jetty-project:jetty-server/src/main/java/org/eclipse/jetty/server/session/DatabaseAdaptor.java": [
107,
109
],
"org.eclipse.jetty:jetty-project:jetty-server/src/main/java/org/eclipse/jetty/server/session/JDBCSessionDataStore.java": [
277,
324,
326,
346,
352,
353,
353,
355
],
"org.eclipse.jetty:jetty-project:jetty-server/src/main/java/org/eclipse/jetty/server/session/Session.java": [
332
],
"org.eclipse.jetty:jetty-project:jetty-server/src/main/java/org/eclipse/jetty/server/session/SessionHandler.java": [
1410
]
}
22 changes: 1 addition & 21 deletions its/ruling/src/test/resources/eclipse-jetty/java-S1192.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,6 @@
"org.eclipse.jetty:jetty-project:jetty-http/src/main/java/org/eclipse/jetty/http/HttpGenerator.java": [
276
],
"org.eclipse.jetty:jetty-project:jetty-http/src/main/java/org/eclipse/jetty/http/HttpParser.java": [
974
],
"org.eclipse.jetty:jetty-project:jetty-http/src/main/java/org/eclipse/jetty/http/MimeTypes.java": [
106
],
Expand All @@ -19,8 +16,7 @@
],
"org.eclipse.jetty:jetty-project:jetty-io/src/main/java/org/eclipse/jetty/io/ssl/SslConnection.java": [
644,
932,
1013
932
],
"org.eclipse.jetty:jetty-project:jetty-server/src/main/java/org/eclipse/jetty/server/CustomRequestLog.java": [
597
Expand All @@ -33,7 +29,6 @@
],
"org.eclipse.jetty:jetty-project:jetty-server/src/main/java/org/eclipse/jetty/server/HttpOutput.java": [
680,
716,
1677
],
"org.eclipse.jetty:jetty-project:jetty-server/src/main/java/org/eclipse/jetty/server/InclusiveByteRange.java": [
Expand All @@ -52,7 +47,6 @@
198
],
"org.eclipse.jetty:jetty-project:jetty-server/src/main/java/org/eclipse/jetty/server/ShutdownMonitor.java": [
188,
359,
360,
366
Expand All @@ -66,18 +60,13 @@
"org.eclipse.jetty:jetty-project:jetty-server/src/main/java/org/eclipse/jetty/server/session/AbstractSessionCache.java": [
444
],
"org.eclipse.jetty:jetty-project:jetty-server/src/main/java/org/eclipse/jetty/server/session/AbstractSessionDataStore.java": [
161
],
"org.eclipse.jetty:jetty-project:jetty-server/src/main/java/org/eclipse/jetty/server/session/DatabaseAdaptor.java": [
107,
109
],
"org.eclipse.jetty:jetty-project:jetty-server/src/main/java/org/eclipse/jetty/server/session/JDBCSessionDataStore.java": [
277,
324,
326,
346,
352,
353,
353,
Expand All @@ -86,9 +75,6 @@
"org.eclipse.jetty:jetty-project:jetty-server/src/main/java/org/eclipse/jetty/server/session/Session.java": [
332
],
"org.eclipse.jetty:jetty-project:jetty-server/src/main/java/org/eclipse/jetty/server/session/SessionHandler.java": [
1410
],
"org.eclipse.jetty:jetty-project:jetty-slf4j-impl/src/main/java/org/eclipse/jetty/logging/JettyLoggerConfiguration.java": [
241,
242
Expand All @@ -106,9 +92,6 @@
"org.eclipse.jetty:jetty-project:jetty-util/src/main/java/org/eclipse/jetty/util/IO.java": [
93
],
"org.eclipse.jetty:jetty-project:jetty-util/src/main/java/org/eclipse/jetty/util/IntrospectionUtil.java": [
55
],
"org.eclipse.jetty:jetty-project:jetty-util/src/main/java/org/eclipse/jetty/util/IteratingCallback.java": [
289
],
Expand Down Expand Up @@ -139,9 +122,6 @@
"org.eclipse.jetty:jetty-project:jetty-util/src/main/java/org/eclipse/jetty/util/resource/URLResource.java": [
76
],
"org.eclipse.jetty:jetty-project:jetty-util/src/main/java/org/eclipse/jetty/util/ssl/SslContextFactory.java": [
1047
],
"org.eclipse.jetty:jetty-project:jetty-util/src/main/java/org/eclipse/jetty/util/thread/ReservedThreadExecutor.java": [
251
],
Expand Down
6 changes: 0 additions & 6 deletions its/ruling/src/test/resources/guava/java-S1192.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,6 @@
"com.google.guava:guava:src/com/google/common/collect/ConcurrentHashMultiset.java": [
223
],
"com.google.guava:guava:src/com/google/common/collect/Cut.java": [
141
],
"com.google.guava:guava:src/com/google/common/collect/FilteredKeyMultimap.java": [
121
],
Expand All @@ -28,8 +25,5 @@
"com.google.guava:guava:src/com/google/common/math/MathPreconditions.java": [
32,
53
],
"com.google.guava:guava:src/com/google/common/util/concurrent/MoreExecutors.java": [
781
]
}
62 changes: 62 additions & 0 deletions ...-checks-test-sources/default/src/main/java/checks/StringLiteralDuplicatedCheckSample.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,3 +113,65 @@ class Coverage {
private final String prevLeftNull = "SELECT" + 3;
private final String prevRightNull = 3 + "SELECT";
}

class ExceptionArguments {
private void simple(int r) {
tomasz-tylenda-sonarsource marked this conversation as resolved.
Show resolved Hide resolved
if(r == 0) {
throw new IllegalArgumentException("simple IAE");
} else if (r == 1) {
throw new IllegalArgumentException("simple IAE");
} else if (r == 2) {
throw new IllegalArgumentException("simple IAE");
}
}

static class MyException extends Exception {
public MyException(String message, Throwable throwable) {
super(message, throwable);
}
}

private void twoArgs(int r, Throwable throwable) throws MyException {
if(r == 0) {
throw new MyException("my exception message", throwable);
} else if (r == 1) {
throw new MyException("my exception message", throwable);
} else if (r == 2) {
throw new MyException("my exception message", throwable);
}
}

private Throwable buildButDoNotThrow(int r) {
if(r == 0) {
return new NullPointerException("build message"); // Noncompliant {{Define a constant instead of duplicating this literal "build message" 3 times.}}
} else if (r == 1) {
return new IllegalArgumentException("build message");
} else {
return new AssertionError("build message");
}
}

private void nestedString1(int r) {
if(r == 0) {
throw new IllegalArgumentException("nested string 1".toLowerCase());
} else if (r == 1) {
throw new IllegalArgumentException("nested string 1".toUpperCase());
} else if (r == 2) {
throw new IllegalArgumentException("nested string 1");
}
}

private String transform(String s, String suffix) {
return s + suffix;
}

private void nestedString2(int r) {
tomasz-tylenda-sonarsource marked this conversation as resolved.
Show resolved Hide resolved
if(r == 0) {
throw new IllegalArgumentException(transform("nested string 2", "AAA")); // Noncompliant {{Define a constant instead of duplicating this literal "nested string 2" 3 times.}}
} else if (r == 1) {
throw new IllegalArgumentException(transform("nested string 2", "BBB"));
} else if (r == 2) {
throw new IllegalArgumentException(transform("nested string 2", "CCC"));
}
}
}
19 changes: 18 additions & 1 deletion java-checks/src/main/java/org/sonar/java/checks/StringLiteralDuplicatedCheck.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import javax.annotation.Nullable;
import org.sonar.check.Rule;
import org.sonar.check.RuleProperty;
Expand Down Expand Up @@ -89,13 +90,29 @@ private static List<JavaFileScannerContext.Location> secondaryLocations(Collecti
public void visitLiteral(LiteralTree tree) {
if (tree.is(Tree.Kind.STRING_LITERAL, Tree.Kind.TEXT_BLOCK)) {
String literal = tree.value();
if (literal.length() >= MINIMAL_LITERAL_LENGTH && !isStringLiteralFragment(tree)) {
if (literal.length() >= MINIMAL_LITERAL_LENGTH && !isStringLiteralFragment(tree) && !isThrowableArgument(tree)) {
String stringValue = LiteralUtils.getAsStringValue(tree).replace("\\n", "\n");
occurrences.computeIfAbsent(stringValue, key -> new ArrayList<>()).add(tree);
}
}
}

/**
* Verify that <code>tree</code> is an argument in
* <code>throw new SomeException(arg1, arg2, ...)</code>.
* For simplicity and to avoid surprises there is no recursion on arguments.
*/
private static boolean isThrowableArgument(LiteralTree tree) {
return
Optional.ofNullable(tree.parent())
tomasz-tylenda-sonarsource marked this conversation as resolved.
Show resolved Hide resolved
.filter(t -> t.is(Tree.Kind.ARGUMENTS))
tomasz-tylenda-sonarsource marked this conversation as resolved.
Show resolved Hide resolved
.map(Tree::parent)
.filter(t -> t.is(Tree.Kind.NEW_CLASS))
.map(Tree::parent)
.filter(t -> t.is(Tree.Kind.THROW_STATEMENT))
.isPresent();
Comment on lines +115 to +120
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The approach should be robust enough to work with broken semantics.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a test with .withoutSemantic(). Please reopen if you meant something else. We don't need any external dependencies for this rule, but the file should complile.

}

private static boolean isStringLiteralFragment(ExpressionTree tree) {
return isStringLiteral(tree) && (isStringLiteral(getNextOperand(tree)) || isStringLiteral(getPreviousOperand(tree)));
}
Expand Down