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 eclipse-jdt#2176
  • Loading branch information
@dashorst @stephan-herrmann
dashorst authored and stephan-herrmann committed Apr 1, 2024
1 parent 2e89a1d commit 014ee74
Show file tree
Hide file tree
Showing 11 changed files with 120 additions and 4 deletions.
6 changes: 5 additions & 1 deletion 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 @@ -2610,6 +2610,11 @@ public interface IProblem {
*/
int UnnamedVariableMustHaveInitializer = PreviewRelated + 2001;

/**
* @since 3.38
*/
int ComparingWrapper = Internal + 2011;

/**
* @since 3.38
* @noreference preview feature
Expand All @@ -2621,5 +2626,4 @@ public interface IProblem {
* @noreference preview feature
*/
int DisallowedStatementInPrologue = PreviewRelated + 2023;

}
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 @@ -3946,6 +3946,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 @@ -393,6 +393,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
8 changes: 8 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 Expand Up @@ -2346,6 +2353,7 @@ public String toString() {
buf.append("\n\t- unused type arguments for method/constructor invocation: ").append(getSeverityString(UnusedTypeArguments)); //$NON-NLS-1$
buf.append("\n\t- redundant superinterface: ").append(getSeverityString(RedundantSuperinterface)); //$NON-NLS-1$
buf.append("\n\t- comparing identical expr: ").append(getSeverityString(ComparingIdentical)); //$NON-NLS-1$
buf.append("\n\t- comparing wrapper expr: ").append(getSeverityString(ComparingWrapper)); //$NON-NLS-1$
buf.append("\n\t- missing synchronized on inherited method: ").append(getSeverityString(MissingSynchronizedModifierInInheritedMethod)); //$NON-NLS-1$
buf.append("\n\t- should implement hashCode() method: ").append(getSeverityString(ShouldImplementHashcode)); //$NON-NLS-1$
buf.append("\n\t- dead code: ").append(getSeverityString(DeadCode)); //$NON-NLS-1$
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 @@ -781,6 +783,7 @@ public static int getProblemCategory(int severity, int problemID) {
case CompilerOptions.FallthroughCase :
case CompilerOptions.OverridingMethodWithoutSuperInvocation :
case CompilerOptions.ComparingIdentical :
case CompilerOptions.ComparingWrapper :
case CompilerOptions.MissingSynchronizedModifierInInheritedMethod :
case CompilerOptions.ShouldImplementHashcode :
case CompilerOptions.DeadCode :
Expand Down Expand Up @@ -1709,6 +1712,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
6 changes: 3 additions & 3 deletions ...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 @@ -1176,11 +1175,12 @@
2000 = As of release 22, '_' 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 '=='

# 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
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
2 changes: 2 additions & 0 deletions ....tests.compiler/src/org/eclipse/jdt/core/tests/compiler/regression/BatchCompilerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -843,6 +843,7 @@ public void test012b(){
" 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 Expand Up @@ -1061,6 +1062,7 @@ public void test013() {
" <option key=\"org.eclipse.jdt.core.compiler.problem.assertIdentifier\" value=\"warning\"/>\n" +
" <option key=\"org.eclipse.jdt.core.compiler.problem.autoboxing\" value=\"ignore\"/>\n" +
" <option key=\"org.eclipse.jdt.core.compiler.problem.comparingIdentical\" value=\"warning\"/>\n" +
" <option key=\"org.eclipse.jdt.core.compiler.problem.comparingWrapper\" value=\"ignore\"/>\n" +
" <option key=\"org.eclipse.jdt.core.compiler.problem.deadCode\" value=\"warning\"/>\n" +
" <option key=\"org.eclipse.jdt.core.compiler.problem.deadCodeInTrivialIfStatement\" value=\"disabled\"/>\n" +
" <option key=\"org.eclipse.jdt.core.compiler.problem.deprecation\" value=\"warning\"/>\n" +
Expand Down
2 changes: 2 additions & 0 deletions ....compiler/src/org/eclipse/jdt/core/tests/compiler/regression/CompilerInvocationTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -424,6 +424,7 @@ class ProblemAttributes {
expectedProblemAttributes.put("ContradictoryNullAnnotationsInferred", new ProblemAttributes(CategorizedProblem.CAT_POTENTIAL_PROGRAMMING_PROBLEM));
expectedProblemAttributes.put("ContradictoryNullAnnotationsInferredFunctionType", new ProblemAttributes(CategorizedProblem.CAT_POTENTIAL_PROGRAMMING_PROBLEM));
expectedProblemAttributes.put("ComparingIdentical", new ProblemAttributes(CategorizedProblem.CAT_POTENTIAL_PROGRAMMING_PROBLEM));
expectedProblemAttributes.put("ComparingWrapper", new ProblemAttributes(CategorizedProblem.CAT_POTENTIAL_PROGRAMMING_PROBLEM));
expectedProblemAttributes.put("ConflictingImport", new ProblemAttributes(CategorizedProblem.CAT_IMPORT));
expectedProblemAttributes.put("ConflictingNullAnnotations", new ProblemAttributes(CategorizedProblem.CAT_POTENTIAL_PROGRAMMING_PROBLEM));
expectedProblemAttributes.put("ConstructedArrayIncompatible", new ProblemAttributes(CategorizedProblem.CAT_MEMBER));
Expand Down Expand Up @@ -1532,6 +1533,7 @@ class ProblemAttributes {
expectedProblemAttributes.put("CodeSnippetMissingClass", SKIP);
expectedProblemAttributes.put("CodeSnippetMissingMethod", SKIP);
expectedProblemAttributes.put("ComparingIdentical", new ProblemAttributes(JavaCore.COMPILER_PB_COMPARING_IDENTICAL));
expectedProblemAttributes.put("ComparingWrapper", new ProblemAttributes(JavaCore.COMPILER_PB_COMPARING_WRAPPER));
expectedProblemAttributes.put("ConstNonNullFieldComparisonYieldsFalse", new ProblemAttributes(JavaCore.COMPILER_PB_REDUNDANT_NULL_CHECK));
expectedProblemAttributes.put("ConflictingImport", SKIP);
expectedProblemAttributes.put("ConflictingNullAnnotations", new ProblemAttributes(JavaCore.COMPILER_PB_NULL_SPECIFICATION_VIOLATION));
Expand Down
13 changes: 13 additions & 0 deletions org.eclipse.jdt.core/model/org/eclipse/jdt/core/JavaCore.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2507,6 +2507,19 @@ public final class JavaCore extends Plugin {
* @category CompilerOptionID
*/
public static final String COMPILER_PB_COMPARING_IDENTICAL = PLUGIN_ID + ".compiler.problem.comparingIdentical"; //$NON-NLS-1$
/**
* Compiler option ID: Reporting Comparison of Wrapper Expressions.
* <p>When enabled, the compiler will issue an error or a warning if a comparison
* is involving wrapper type (or String) operands (e.g <code>'1234L == 5678L'</code>).</p>
* <dl>
* <dt>Option id:</dt><dd><code>"org.eclipse.jdt.core.compiler.problem.comparingWrapper"</code></dd>
* <dt>Possible values:</dt><dd><code>{ "error", "warning", "info", "ignore" }</code></dd>
* <dt>Default:</dt><dd><code>"ignore"</code></dd>
* </dl>
* @since 3.38
* @category CompilerOptionID
*/
public static final String COMPILER_PB_COMPARING_WRAPPER = PLUGIN_ID + ".compiler.problem.comparingWrapper"; //$NON-NLS-1$
/**
* Compiler option ID: Reporting Missing Synchronized Modifier On Inherited Method.
* <p>When enabled, the compiler will issue an error or a warning if a method
Expand Down

0 comments on commit 014ee74

Please sign in to comment.