-
Notifications
You must be signed in to change notification settings - Fork 706
SONARJAVA-4698 Fix false positive in S1191 when variable is named "sun" #5352
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
base: master
Are you sure you want to change the base?
Changes from 1 commit
66bc890
e7e84f4
72dd782
d568c34
4329430
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 |
|---|---|---|
| @@ -1,13 +1,15 @@ | ||
| import java.util.ArrayList; | ||
|
|
||
| class SunPackagesUsedCheckSample { | ||
| private Object sun; // variable named "sun" | ||
|
|
||
| private void f() { | ||
| java.util.List a; | ||
| sun.Foo b; // Noncompliant | ||
| // ^^^^^^^ | ||
| sun.Foo.toto.asd c; // secondary | ||
| // ^^^^^^^^^^^^^^^^< | ||
|
|
||
| } | ||
|
|
||
| public Object uselessMethod() { | ||
|
|
@@ -16,4 +18,18 @@ public Object uselessMethod() { | |
| } | ||
| return null; | ||
| } | ||
|
|
||
| // SONARJAVA-4698: False positive when variable is named "sun" | ||
| public void fooWithFieldNamedSun() { | ||
| sun.toString(); // Compliant - FP: "sun" is a field of type Object, not a sun.* package class | ||
| } | ||
|
|
||
| public void barWithParameterNamedSun(Object sun) { | ||
| sun.toString(); // Compliant - FP: "sun" is a parameter of type Object, not a sun.* package class | ||
|
||
| } | ||
|
|
||
| public void bazWithLocalVariableNamedSun() { | ||
| Object sun = new Object(); | ||
| sun.toString(); // Compliant - FP: "sun" is a local variable of type Object, not a sun.* package class | ||
|
||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -66,7 +66,7 @@ private void reportIssueWithSecondaries(JavaFileScannerContext context) { | |
| @Override | ||
| public void visitMemberSelectExpression(MemberSelectExpressionTree tree) { | ||
| String reference = ExpressionsHelper.concatenate(tree); | ||
| if (!isExcluded(reference) && isSunClass(reference)) { | ||
| if (!isExcluded(reference) && isSunClass(reference) && isActuallySunPackage(tree)) { | ||
| reportedTrees.add(tree); | ||
| } | ||
| } | ||
|
|
@@ -75,6 +75,19 @@ private static boolean isSunClass(String reference) { | |
| return reference.startsWith("sun."); | ||
| } | ||
|
|
||
| private static boolean isActuallySunPackage(MemberSelectExpressionTree tree) { | ||
| // Check if the expression's type actually comes from a sun.* package | ||
| // This prevents false positives when a variable is named "sun" | ||
| var type = tree.expression().symbolType(); | ||
| if (type.isUnknown()) { | ||
| // If we can't determine the type, we should still check to avoid missing real issues | ||
|
||
| // In this case, rely on the string-based check | ||
|
||
| return true; | ||
| } | ||
| String fullyQualifiedName = type.fullyQualifiedName(); | ||
| return fullyQualifiedName.startsWith("sun."); | ||
|
||
| } | ||
|
|
||
| private boolean isExcluded(String reference) { | ||
| for (String str : excludePackages) { | ||
| if (!str.isEmpty() && reference.startsWith(str)) { | ||
|
|
||
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.
I don't think we should mention "FP" in the comment, it sounds confusing to me: I would rather leave only
// Compliant - "sun" is a field of type Object, not a sun.* package class