Skip to content

Commit

Permalink
feat: issue optional warning using '==' for wrapper types
Browse files Browse the repository at this point in the history 
When comparing wrapper types or Strings using '==' this is done by
reference instead of value. Usually this is in error (e.g. Long only
caches values between -127 and +127). By flagging this as an optional
warning or even error (configurable) these cases can be easily weeded
out.

Fixes #2176
  • Loading branch information
@dashorst
dashorst committed Mar 20, 2024
1 parent 16766b5 commit 7ee8ffe
Show file tree
Hide file tree
Showing 8 changed files with 101 additions and 1 deletion.
5 changes: 5 additions & 0 deletions org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/core/compiler/IProblem.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2601,4 +2601,9 @@ public interface IProblem {
* @noreference preview feature
*/
int UnnamedVariableMustHaveInitializer = PreviewRelated + 2001;

/**
* @since 3.38
*/
int ComparingWrapper = Internal + 2011;
}
7 changes: 7 additions & 0 deletions ...se.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ast/EqualExpression.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -915,6 +915,13 @@ public TypeBinding resolveType(BlockScope scope) {
}
}
}

// both wrapper types or strings
if((leftType.isBoxedPrimitiveType() && rightType.isBoxedPrimitiveType()) || (leftType.id == rightType.id && leftType.id == TypeIds.T_JavaLangString))
{
scope.problemReporter().comparingWrapperExpressions(this);
}

// both base type
if (leftType.isBaseType() && rightType.isBaseType()) {
int leftTypeID = leftType.id;
Expand Down
3 changes: 3 additions & 0 deletions org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/batch/Main.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3943,6 +3943,9 @@ private void handleErrorOrWarningToken(String token, boolean isEnabling, int sev
} else if (token.equals("compareIdentical")) { //$NON-NLS-1$
setSeverity(CompilerOptions.OPTION_ReportComparingIdentical, severity, isEnabling);
return;
} else if (token.equals("compareWrapper")) { //$NON-NLS-1$
setSeverity(CompilerOptions.OPTION_ReportComparingWrapper, severity, isEnabling);
return;
} else if (token.equals("charConcat") /*|| token.equals("noImplicitStringConversion")/*backward compatible*/) {//$NON-NLS-1$
setSeverity(CompilerOptions.OPTION_ReportNoImplicitStringConversion, severity, isEnabling);
return;
Expand Down
1 change: 1 addition & 0 deletions ...e.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/batch/messages.properties
View file
Original file line number Diff line number Diff line change
Expand Up @@ -388,6 +388,7 @@ misc.usage.warn = {1} {2}\n\
\ boxing autoboxing conversion\n\
\ charConcat + char[] in String concat\n\
\ compareIdentical + comparing identical expressions\n\
\ compareWrapped + comparing wrapped primitive expressions\n\
\ conditionAssign possible accidental boolean assignment\n\
\ constructorName + method with constructor name\n\
\ deadCode + dead code excluding trivial if (DEBUG) check\n\
Expand Down
7 changes: 7 additions & 0 deletions ...e.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/impl/CompilerOptions.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,7 @@ public class CompilerOptions {
public static final String OPTION_EmulateJavacBug8031744 = "org.eclipse.jdt.core.compiler.emulateJavacBug8031744"; //$NON-NLS-1$
public static final String OPTION_ReportRedundantSuperinterface = "org.eclipse.jdt.core.compiler.problem.redundantSuperinterface"; //$NON-NLS-1$
public static final String OPTION_ReportComparingIdentical = "org.eclipse.jdt.core.compiler.problem.comparingIdentical"; //$NON-NLS-1$
public static final String OPTION_ReportComparingWrapper = "org.eclipse.jdt.core.compiler.problem.comparingWrapper"; //$NON-NLS-1$
public static final String OPTION_ReportMissingSynchronizedOnInheritedMethod = "org.eclipse.jdt.core.compiler.problem.missingSynchronizedOnInheritedMethod"; //$NON-NLS-1$
public static final String OPTION_ReportMissingHashCodeMethod = "org.eclipse.jdt.core.compiler.problem.missingHashCodeMethod"; //$NON-NLS-1$
public static final String OPTION_ReportDeadCode = "org.eclipse.jdt.core.compiler.problem.deadCode"; //$NON-NLS-1$
Expand Down Expand Up @@ -377,6 +378,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 ComparingWrapper = IrritantSet.GROUP3 | ASTNode.Bit3;


// Severity level for handlers
Expand Down Expand Up @@ -780,6 +782,8 @@ public static String optionKeyFromIrritant(int irritant) {
return OPTION_ReportRedundantSuperinterface;
case ComparingIdentical :
return OPTION_ReportComparingIdentical;
case ComparingWrapper :
return OPTION_ReportComparingWrapper;
case MissingSynchronizedModifierInInheritedMethod :
return OPTION_ReportMissingSynchronizedOnInheritedMethod;
case ShouldImplementHashcode :
Expand Down Expand Up @@ -976,6 +980,7 @@ public static String[] warningOptionNames() {
OPTION_ReportAssertIdentifier,
OPTION_ReportAutoboxing,
OPTION_ReportComparingIdentical,
OPTION_ReportComparingWrapper,
OPTION_ReportDeadCode,
OPTION_ReportDeadCodeInTrivialIfStatement,
OPTION_ReportDeprecation,
Expand Down Expand Up @@ -1409,6 +1414,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_ReportComparingWrapper, getSeverityString(ComparingWrapper));
optionsMap.put(OPTION_ReportMissingSynchronizedOnInheritedMethod, getSeverityString(MissingSynchronizedModifierInInheritedMethod));
optionsMap.put(OPTION_ReportMissingHashCodeMethod, getSeverityString(ShouldImplementHashcode));
optionsMap.put(OPTION_ReportDeadCode, getSeverityString(DeadCode));
Expand Down Expand Up @@ -1965,6 +1971,7 @@ public void set(Map<String, String> optionsMap) {
if ((optionValue = optionsMap.get(OPTION_ReportUnusedTypeArgumentsForMethodInvocation)) != null) updateSeverity(UnusedTypeArguments, optionValue);
if ((optionValue = optionsMap.get(OPTION_ReportRedundantSuperinterface)) != null) updateSeverity(RedundantSuperinterface, optionValue);
if ((optionValue = optionsMap.get(OPTION_ReportComparingIdentical)) != null) updateSeverity(ComparingIdentical, optionValue);
if ((optionValue = optionsMap.get(OPTION_ReportComparingWrapper)) != null) updateSeverity(ComparingWrapper, optionValue);
if ((optionValue = optionsMap.get(OPTION_ReportMissingSynchronizedOnInheritedMethod)) != null) updateSeverity(MissingSynchronizedModifierInInheritedMethod, optionValue);
if ((optionValue = optionsMap.get(OPTION_ReportMissingHashCodeMethod)) != null) updateSeverity(ShouldImplementHashcode, optionValue);
if ((optionValue = optionsMap.get(OPTION_ReportDeadCode)) != null) updateSeverity(DeadCode, optionValue);
Expand Down
14 changes: 14 additions & 0 deletions ...dt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/problem/ProblemReporter.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -663,6 +663,8 @@ public static int getIrritant(int problemID) {

case IProblem.ComparingIdentical:
return CompilerOptions.ComparingIdentical;
case IProblem.ComparingWrapper:
return CompilerOptions.ComparingWrapper;

case IProblem.MissingSynchronizedModifierInInheritedMethod:
return CompilerOptions.MissingSynchronizedModifierInInheritedMethod;
Expand Down Expand Up @@ -1709,6 +1711,17 @@ public void comparingIdenticalExpressions(Expression comparison){
comparison.sourceStart,
comparison.sourceEnd);
}
public void comparingWrapperExpressions(EqualExpression comparison) {
int severity = computeSeverity(IProblem.ComparingWrapper);
if (severity == ProblemSeverities.Ignore) return;
this.handle(
IProblem.ComparingWrapper,
NoArgument,
NoArgument,
severity,
comparison.sourceStart,
comparison.sourceEnd);
}
/*
* Given the current configuration, answers which category the problem
* falls into:
Expand Down Expand Up @@ -12590,4 +12603,5 @@ public boolean scheduleProblemForContext(Runnable problemComputation) {
public void close() {
this.referenceContext = null;
}

}
3 changes: 2 additions & 1 deletion ...jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/problem/messages.properties
View file
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 @@ -1174,6 +1173,8 @@
2000 = As of release 21, '_' is only allowed to declare unnamed patterns, local variables, exception parameters or lambda parameters
2001 = Unnamed variables must have an initializer

2011 = Comparing primitive wrapper types or Strings using '=='

### ELABORATIONS
## Access restrictions
78592 = The type ''{1}'' is not API (restriction on classpath entry ''{0}'')
Expand Down
62 changes: 62 additions & 0 deletions ...ore.tests.compiler/src/org/eclipse/jdt/core/tests/compiler/regression/AnnotationTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9976,6 +9976,7 @@ public void test297() {
runner.customOptions.put(CompilerOptions.OPTION_SuppressOptionalErrors, CompilerOptions.ENABLED);
runner.customOptions.put(CompilerOptions.OPTION_ReportUnusedLocal, CompilerOptions.WARNING);
runner.customOptions.put(CompilerOptions.OPTION_ReportComparingIdentical, CompilerOptions.ERROR);
runner.customOptions.put(CompilerOptions.OPTION_ReportComparingWrapper, CompilerOptions.ERROR);
runner.customOptions.put(CompilerOptions.OPTION_ReportUncheckedTypeOperation, CompilerOptions.ERROR);

runner.expectedCompilerLog =
Expand Down Expand Up @@ -10023,6 +10024,67 @@ public void test297() {
runner.javacTestOptions = JavacTestOptions.Excuse.EclipseWarningConfiguredAsError;
runner.runNegativeTest();
}
// https://github.com/eclipse-jdt/eclipse.jdt.core/issues/2176
public void testGH2167() {
Runner runner = new Runner();
runner.customOptions = getCompilerOptions();
runner.customOptions.put(CompilerOptions.OPTION_SuppressWarnings, CompilerOptions.ENABLED);
runner.customOptions.put(CompilerOptions.OPTION_ReportUnhandledWarningToken, CompilerOptions.WARNING);
runner.customOptions.put(CompilerOptions.OPTION_SuppressOptionalErrors, CompilerOptions.ENABLED);
runner.customOptions.put(CompilerOptions.OPTION_ReportComparingIdentical, CompilerOptions.ERROR);
runner.customOptions.put(CompilerOptions.OPTION_ReportComparingWrapper, CompilerOptions.ERROR);

runner.expectedCompilerLog =
"----------\n" +
"1. ERROR in A.java (at line 5)\n" +
" return x1 == x2;\n" +
" ^^^^^^^^\n" +
"Comparing primitive wrapper types or Strings using '=='\n" +
"----------\n" +
"2. ERROR in A.java (at line 10)\n" +
" return x1 == x2;\n" +
" ^^^^^^^^\n" +
"Comparing primitive wrapper types or Strings using '=='\n" +
"----------\n" +
"3. ERROR in A.java (at line 15)\n" +
" return x1 == x2;\n" +
" ^^^^^^^^\n" +
"Comparing primitive wrapper types or Strings using '=='\n" +
"----------\n" +
"4. ERROR in A.java (at line 20)\n" +
" return x1 == x2;\n" +
" ^^^^^^^^\n" +
"Comparing primitive wrapper types or Strings using '=='\n" +
"----------\n";

runner.testFiles = new String[] {
"A.java",
"public class A {\n" +
" public boolean a1() {\n" +
" Long x1 = 12345L;\n" +
" Long x2 = 67890L;\n" +
" return x1 == x2;\n" +
" }\n" +
" public boolean a2() {\n" +
" String x1 = \"12345\";\n" +
" String x2 = \"67890\";\n" +
" return x1 == x2;\n" +
" }\n" +
" public boolean a3() {\n" +
" Byte x1 = 12;\n" +
" Byte x2 = 34;\n" +
" return x1 == x2;\n" +
" }\n" +
" public boolean a4() {\n" +
" Integer x2 = 456;\n" +
" Integer x1 = 123;\n" +
" return x1 == x2;\n" +
" }\n" +
"}"
};
runner.javacTestOptions = JavacTestOptions.Excuse.EclipseWarningConfiguredAsError;
runner.runNegativeTest();
}
// Bug 366003 - CCE in ASTNode.resolveAnnotations(ASTNode.java:639)
// many syntax errors fixed, does not trigger CCE
public void testBug366003() {
Expand Down

0 comments on commit 7ee8ffe

Please sign in to comment.