Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
be4d139
feat: issue optional warning using '==' for wrapper types
dashorst Mar 20, 2024
5d9a86e
refactor: ComparingWrapper constant -> 1202
dashorst Apr 6, 2024
33b3448
docs: clarify ComparingWrapper detection of boxed types
dashorst Apr 6, 2024
f4e2db9
refactor: use already available severity computation
dashorst Apr 6, 2024
221dcd4
refactor: rename to unlikely equal expression argument type
dashorst Apr 6, 2024
dcd55f0
fix: also rename compiler option and document it
dashorst Apr 6, 2024
0784fef
refactor: UnlikelyEqualExpressionArgumenType -> UnlikelyReferenceComp…
dashorst Apr 8, 2024
389bdfb
refactor: add operator to unlikelyReferenceComparison message
dashorst Apr 8, 2024
b3140c8
fix: CompilerInvocationTests
dashorst Apr 8, 2024
4b3c272
feat: add support for more reference comparison detections
dashorst Apr 8, 2024
384a8ae
fix: UnlikelyReferenceComparison with null should not warn
dashorst Apr 9, 2024
843cc58
test: Ignore unlikely reference comparison in AssignmentTest
dashorst Apr 9, 2024
2eaf0f3
test: Ignore unlikely reference comparison in CastTest
dashorst Apr 9, 2024
ee3fa2d
test: ignore unlikely reference comparison in AutoBoxingTest
dashorst Apr 12, 2024
4372822
test: replace comparingWrapper with unlikelyReferenceComparison in Ba…
dashorst Apr 12, 2024
b07953b
fix: change default of unlikelyReferenceComparison to INFO in Irritan…
dashorst Apr 12, 2024
94fd84b
test: ignore unlikely reference comparison in NegativeLambdaExpressio…
dashorst Apr 12, 2024
89ea49a
test: fix BatchCompilerTest (correctly identified unlikely comparison)
dashorst Apr 12, 2024
b3cf7e2
refactor: unlikelyReferenceComparison -> dubiousReferenceComparison
dashorst May 9, 2024
d4eb747
fix: test runner gave errors on non-normalized paths
dashorst May 9, 2024
df2ef7e
fix: give better test failure messages
dashorst May 9, 2024
d6393d1
Update warning message from Unlikely -> Dubious
dashorst May 9, 2024
3c0d8ac
test: Normalise path differences in assertion
dashorst May 12, 2024
2ec884d
refactor: implement review comments
dashorst May 18, 2024
bf2fec3
clean: remove duplicate entry in CompilerOptions
dashorst May 18, 2024
5d54c68
Merge branch 'master' into dashorst:issue_2176
stephan-herrmann Jun 6, 2024
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
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
* Utility class to hold results of processor-based tests.
* All methods are static.
*/
@SuppressWarnings("reference-comparison")
public final class ProcessorTestStatus {

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2188,6 +2188,8 @@ public interface IProblem {
int UnlikelyCollectionMethodArgumentType = 1200;
/** @since 3.13 */
int UnlikelyEqualsArgumentType = 1201;
/** @since 3.38 */
int DubiousReferenceComparison = 1202;

/* Local-Variable Type Inference */
/** @since 3.14 */
Expand Down Expand Up @@ -2625,5 +2627,4 @@ public interface IProblem {
* @noreference preview feature
*/
int DisallowedStatementInPrologue = PreviewRelated + 2023;

}
Original file line number Diff line number Diff line change
Expand Up @@ -915,6 +915,25 @@ public TypeBinding resolveType(BlockScope scope) {
}
}
}

boolean leftTypeIsNumber = new String(leftType.readableName()).equals("java.lang.Number"); //$NON-NLS-1$
boolean rightTypeIsNumber = new String(rightType.readableName()).equals("java.lang.Number"); //$NON-NLS-1$

// Check for dubious comparisons of references, for both wrapper types or strings.
// Putting primitives (or calculations) in the mix results in unboxing the wrappers to their
// primitives, so this only warns when both sides are wrappers. e.g. (x1 == (x2 + 1)) results in
// (x2 + 1) becoming a primitive, and an unboxing of x1 as well, not necessitating a warning.
// When comparing a reference to null the assumption is that the developer is aware of comparing
// references, so the comparison is not unlikely
if((leftType != TypeBinding.NULL && rightType != TypeBinding.NULL) &&
((leftType.isBoxedPrimitiveType() && rightType.isBoxedPrimitiveType()) ||
(!leftType.isPrimitiveType() && (rightType.isBoxedPrimitiveType() || rightTypeIsNumber)) ||
((leftType.isBoxedPrimitiveType() || leftTypeIsNumber) && !rightType.isPrimitiveType()) ||
(leftType.id == rightType.id && (leftType.id == TypeIds.T_JavaLangString || leftType.isArrayType()))))
{
scope.problemReporter().dubiousComparison(this, operatorToString(), leftType, rightType);
}

// both base type
if (leftType.isBaseType() && rightType.isBaseType()) {
int leftTypeID = leftType.id;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3973,6 +3973,9 @@ private void handleErrorOrWarningToken(String token, boolean isEnabling, int sev
CompilerOptions.OPTION_ReportDeadCodeInTrivialIfStatement,
CompilerOptions.DISABLED);
return;
} else if (token.equals("dubiousReferenceComparison")) { //$NON-NLS-1$
setSeverity(CompilerOptions.OPTION_ReportDubiousReferenceComparison, severity, isEnabling);
return;
}
break;
case 'e' :
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -399,6 +399,9 @@ misc.usage.warn = {1} {2}\n\
\ dep-ann missing @Deprecated annotation\n\
\ deprecation + deprecation outside deprecated code\n\
\ discouraged + use of types matching a discouraged access rule\n\
\ dubiousReferenceComparison\n\
\ + dubious reference comparison of primitive wrappers\n\
\ or strings\n\
\ emptyBlock undocumented empty block\n\
\ enumIdentifier ''enum'' used as identifier\n\
\ enumSwitch incomplete enum switch\n\
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,7 @@ public class CompilerOptions {
public static final String OPTION_ReportUnlikelyCollectionMethodArgumentType = "org.eclipse.jdt.core.compiler.problem.unlikelyCollectionMethodArgumentType"; //$NON-NLS-1$
public static final String OPTION_ReportUnlikelyCollectionMethodArgumentTypeStrict = "org.eclipse.jdt.core.compiler.problem.unlikelyCollectionMethodArgumentTypeStrict"; //$NON-NLS-1$
public static final String OPTION_ReportUnlikelyEqualsArgumentType = "org.eclipse.jdt.core.compiler.problem.unlikelyEqualsArgumentType"; //$NON-NLS-1$
public static final String OPTION_ReportDubiousReferenceComparison = "org.eclipse.jdt.core.compiler.problem.dubiousReferenceComparison"; //$NON-NLS-1$

public static final String OPTION_ReportAPILeak = "org.eclipse.jdt.core.compiler.problem.APILeak"; //$NON-NLS-1$
public static final String OPTION_ReportUnstableAutoModuleName = "org.eclipse.jdt.core.compiler.problem.unstableAutoModuleName"; //$NON-NLS-1$
Expand Down Expand Up @@ -379,6 +380,7 @@ public class CompilerOptions {
// group 3
public static final int InsufficientResourceManagement = IrritantSet.GROUP3 | ASTNode.Bit1;
public static final int IncompatibleOwningContract = IrritantSet.GROUP3 | ASTNode.Bit2;
public static final int DubiousReferenceComparison = IrritantSet.GROUP3 | ASTNode.Bit3;


// Severity level for handlers
Expand Down Expand Up @@ -606,6 +608,7 @@ public class CompilerOptions {
"nls", //$NON-NLS-1$
"null", //$NON-NLS-1$
"rawtypes", //$NON-NLS-1$
"reference-comparison", //$NON-NLS-1$
"removal", //$NON-NLS-1$
"resource", //$NON-NLS-1$
"restriction", //$NON-NLS-1$
Expand Down Expand Up @@ -836,6 +839,8 @@ public static String optionKeyFromIrritant(int irritant) {
return OPTION_ReportUnlikelyCollectionMethodArgumentType;
case UnlikelyEqualsArgumentType:
return OPTION_ReportUnlikelyEqualsArgumentType;
case DubiousReferenceComparison :
return OPTION_ReportDubiousReferenceComparison;
case APILeak:
return OPTION_ReportAPILeak;
case UnstableAutoModuleName:
Expand Down Expand Up @@ -987,6 +992,7 @@ public static String[] warningOptionNames() {
OPTION_ReportDeprecationInDeprecatedCode,
OPTION_ReportDeprecationWhenOverridingDeprecatedMethod,
OPTION_ReportDiscouragedReference,
OPTION_ReportDubiousReferenceComparison,
OPTION_ReportEmptyStatement,
OPTION_ReportEnumIdentifier,
OPTION_ReportFallthroughCase,
Expand Down Expand Up @@ -1196,6 +1202,8 @@ public static String warningTokenFromIrritant(int irritant) {
case UnlikelyEqualsArgumentType:
case UnlikelyCollectionMethodArgumentType:
return "unlikely-arg-type"; //$NON-NLS-1$
case DubiousReferenceComparison:
return "reference-comparison"; //$NON-NLS-1$
case APILeak:
return "exports"; //$NON-NLS-1$
case UnstableAutoModuleName:
Expand Down Expand Up @@ -1268,6 +1276,8 @@ public static IrritantSet warningTokenToIrritants(String warningToken) {
case 'r' :
if ("rawtypes".equals(warningToken)) //$NON-NLS-1$
return IrritantSet.RAW;
if ("reference-comparison".equals(warningToken)) //$NON-NLS-1$
return IrritantSet.DUBIOUS_REFERENCE_COMPARISON;
if ("resource".equals(warningToken)) //$NON-NLS-1$
return IrritantSet.RESOURCE;
if ("restriction".equals(warningToken)) //$NON-NLS-1$
Expand Down Expand Up @@ -1414,6 +1424,7 @@ public Map<String, String> getMap() {
optionsMap.put(OPTION_EmulateJavacBug8031744, this.emulateJavacBug8031744 ? ENABLED : DISABLED);
optionsMap.put(OPTION_ReportRedundantSuperinterface, getSeverityString(RedundantSuperinterface));
optionsMap.put(OPTION_ReportComparingIdentical, getSeverityString(ComparingIdentical));
optionsMap.put(OPTION_ReportDubiousReferenceComparison, getSeverityString(DubiousReferenceComparison));
optionsMap.put(OPTION_ReportMissingSynchronizedOnInheritedMethod, getSeverityString(MissingSynchronizedModifierInInheritedMethod));
optionsMap.put(OPTION_ReportMissingHashCodeMethod, getSeverityString(ShouldImplementHashcode));
optionsMap.put(OPTION_ReportDeadCode, getSeverityString(DeadCode));
Expand Down Expand Up @@ -2009,6 +2020,7 @@ && getSeverity(ExplicitlyClosedAutoCloseable) == ProblemSeverities.Ignore) {
this.reportUnlikelyCollectionMethodArgumentTypeStrict = ENABLED.equals(optionValue);
}
if ((optionValue = optionsMap.get(OPTION_ReportUnlikelyEqualsArgumentType)) != null) updateSeverity(UnlikelyEqualsArgumentType, optionValue);
if ((optionValue = optionsMap.get(OPTION_ReportDubiousReferenceComparison)) != null) updateSeverity(DubiousReferenceComparison, optionValue);
if ((optionValue = optionsMap.get(OPTION_ReportAPILeak)) != null) updateSeverity(APILeak, optionValue);
if ((optionValue = optionsMap.get(OPTION_ReportUnstableAutoModuleName)) != null) updateSeverity(UnstableAutoModuleName, optionValue);
if ((optionValue = optionsMap.get(OPTION_AnnotationBasedNullAnalysis)) != null) {
Expand Down Expand Up @@ -2382,6 +2394,7 @@ public String toString() {
buf.append("\n\t- unlikely argument type for collection methods: ").append(getSeverityString(UnlikelyCollectionMethodArgumentType)); //$NON-NLS-1$
buf.append("\n\t- unlikely argument type for collection methods, strict check against expected type: ").append(this.reportUnlikelyCollectionMethodArgumentTypeStrict ? ENABLED : DISABLED); //$NON-NLS-1$
buf.append("\n\t- unlikely argument types for equals(): ").append(getSeverityString(UnlikelyEqualsArgumentType)); //$NON-NLS-1$
buf.append("\n\t- dubious operand type for reference comparison: ").append(getSeverityString(DubiousReferenceComparison)); //$NON-NLS-1$
buf.append("\n\t- API leak: ").append(getSeverityString(APILeak)); //$NON-NLS-1$
buf.append("\n\t- unstable auto module name: ").append(getSeverityString(UnstableAutoModuleName)); //$NON-NLS-1$
buf.append("\n\t- SuppressWarnings not fully analysed: ").append(getSeverityString(SuppressWarningsNotAnalysed)); //$NON-NLS-1$
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ public class IrritantSet {
public static final IrritantSet UNQUALIFIED_FIELD_ACCESS = new IrritantSet(CompilerOptions.UnqualifiedFieldAccess);
public static final IrritantSet RESOURCE = new IrritantSet(CompilerOptions.UnclosedCloseable);
public static final IrritantSet UNLIKELY_ARGUMENT_TYPE = new IrritantSet(CompilerOptions.UnlikelyCollectionMethodArgumentType);
public static final IrritantSet DUBIOUS_REFERENCE_COMPARISON = new IrritantSet(CompilerOptions.DubiousReferenceComparison);
public static final IrritantSet API_LEAK = new IrritantSet(CompilerOptions.APILeak);
public static final IrritantSet MODULE = new IrritantSet(CompilerOptions.UnstableAutoModuleName);

Expand All @@ -87,7 +88,10 @@ public class IrritantSet {
.set(
CompilerOptions.UnlikelyEqualsArgumentType
| CompilerOptions.SuppressWarningsNotAnalysed
| CompilerOptions.AnnotatedTypeArgumentToUnannotated);
| CompilerOptions.AnnotatedTypeArgumentToUnannotated)
// group-3 infos enabled by default
.set(CompilerOptions.DubiousReferenceComparison)
;

COMPILER_DEFAULT_WARNINGS
// group-0 warnings enabled by default
Expand Down Expand Up @@ -140,7 +144,8 @@ public class IrritantSet {
|CompilerOptions.UnstableAutoModuleName
|CompilerOptions.PreviewFeatureUsed)
.set(CompilerOptions.InsufficientResourceManagement
|CompilerOptions.IncompatibleOwningContract);
|CompilerOptions.IncompatibleOwningContract)
;
// default errors IF AnnotationBasedNullAnalysis is enabled:
COMPILER_DEFAULT_ERRORS.set(
CompilerOptions.NullSpecViolation
Expand Down Expand Up @@ -198,6 +203,9 @@ public class IrritantSet {

UNLIKELY_ARGUMENT_TYPE
.set(CompilerOptions.UnlikelyEqualsArgumentType);

DUBIOUS_REFERENCE_COMPARISON
.set(CompilerOptions.DubiousReferenceComparison);
}
// Internal state

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -719,6 +719,8 @@ public static int getIrritant(int problemID) {
return CompilerOptions.UnlikelyCollectionMethodArgumentType;
case IProblem.UnlikelyEqualsArgumentType:
return CompilerOptions.UnlikelyEqualsArgumentType;
case IProblem.DubiousReferenceComparison:
return CompilerOptions.DubiousReferenceComparison;

case IProblem.NonPublicTypeInAPI:
case IProblem.NotExportedTypeInAPI:
Expand Down Expand Up @@ -794,6 +796,7 @@ public static int getProblemCategory(int severity, int problemID) {
case CompilerOptions.NonNullTypeVariableFromLegacyInvocation :
case CompilerOptions.UnlikelyCollectionMethodArgumentType :
case CompilerOptions.UnlikelyEqualsArgumentType:
case CompilerOptions.DubiousReferenceComparison:
case CompilerOptions.APILeak:
case CompilerOptions.UnstableAutoModuleName:
return CategorizedProblem.CAT_POTENTIAL_PROGRAMMING_PROBLEM;
Expand Down Expand Up @@ -11667,6 +11670,15 @@ public void unlikelyArgumentType(Expression argument, MethodBinding method, Type
argument.sourceEnd);
}

public void dubiousComparison(EqualExpression comparison, String operator, TypeBinding leftType, TypeBinding rightType) {
this.handle(
IProblem.DubiousReferenceComparison,
new String[] {operator, new String(leftType.shortReadableName()), new String(rightType.shortReadableName())},
new String[] {operator, new String(leftType.readableName()), new String(rightType.readableName())},
Comment on lines +11676 to +11677
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ups, these lines should be swapped. This way the default rendering of the problem will use short names, which is friendlier to the eye :)
Frankly, I don't know off-hand where the long version will be used, but let's just play by that convention.

comparison.sourceStart,
comparison.sourceEnd);
}

public void nonPublicTypeInAPI(TypeBinding type, int sourceStart, int sourceEnd) {
handle(IProblem.NonPublicTypeInAPI,
new String[] { new String(type.readableName()) },
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -247,7 +247,6 @@
210 = Syntax error on keyword "{0}", no accurate correction available
211 = Comparing identical expressions
212 = Type {0} cannot be safely cast to {1}

220 = Unmatched bracket
221 = The primitive type {0} of {1} does not have a field {2}
222 = Invalid expression as statement
Expand Down Expand Up @@ -908,6 +907,7 @@
# more programming problems:
1200 = Unlikely argument type {0} for {1} on a {2}
1201 = Unlikely argument type for equals(): {0} seems to be unrelated to {2}
1202 = Dubious operand types for '{0}': {1} and {2} are references

### Autoclosable try
1251 = Duplicate resource reference {0}
Expand Down Expand Up @@ -1181,8 +1181,7 @@
# Statements before Super
2022 = Cannot use {0} in a pre-construction context
2023 = {0} statement not allowed in prologue



### ELABORATIONS
## Access restrictions
78592 = The type ''{1}'' is not API (restriction on classpath entry ''{0}'')
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -658,14 +658,16 @@ protected void runTest(boolean shouldCompileOK, String[] testFiles, Object extra
"Unexpected standard output for invocation with arguments ["
+ extraArguments + "]",
expectedOutOutputString,
outOutputString);
outputDirNormalizer
.normalized(outOutputString));
}
if (!errCompareOK) {
assertEquals(
"Unexpected error output for invocation with arguments ["
+ extraArguments + "]",
expectedErrOutputString,
errOutputString);
outputDirNormalizer
.normalized(errOutputString));
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ public AssignmentTest(String name) {
protected Map getCompilerOptions() {
Map options = super.getCompilerOptions();
options.put(CompilerOptions.OPTION_ReportDeadCode, CompilerOptions.IGNORE);
options.put(CompilerOptions.OPTION_ReportDubiousReferenceComparison, CompilerOptions.IGNORE);
options.put(CompilerOptions.OPTION_ReportNullReference, CompilerOptions.ERROR);
options.put(CompilerOptions.OPTION_ReportPotentialNullReference, CompilerOptions.ERROR);
options.put(CompilerOptions.OPTION_ReportRedundantNullCheck, CompilerOptions.ERROR);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ public AutoBoxingTest(String name) {
protected Map getCompilerOptions() {
Map defaultOptions = super.getCompilerOptions();
defaultOptions.put(CompilerOptions.OPTION_ReportAutoboxing, CompilerOptions.WARNING);
defaultOptions.put(CompilerOptions.OPTION_ReportDubiousReferenceComparison, CompilerOptions.IGNORE);
return defaultOptions;
}

Expand Down
Loading