From cfade8f421d82524b73dddecc7739807d6adb0c4 Mon Sep 17 00:00:00 2001 From: Martijn Dashorst Date: Wed, 20 Mar 2024 13:18:43 +0100 Subject: [PATCH] feat: issue optional warning using '==' for wrapper types 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 --- .../eclipse/jdt/core/compiler/IProblem.java | 5 ++ .../compiler/ast/EqualExpression.java | 7 +++ .../jdt/internal/compiler/batch/Main.java | 3 + .../compiler/batch/messages.properties | 1 + .../compiler/impl/CompilerOptions.java | 7 +++ .../compiler/problem/ProblemReporter.java | 13 ++++ .../compiler/problem/messages.properties | 3 +- .../compiler/regression/AnnotationTest.java | 62 +++++++++++++++++++ 8 files changed, 100 insertions(+), 1 deletion(-) diff --git a/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/core/compiler/IProblem.java b/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/core/compiler/IProblem.java index ec8532f60ec..b6cb0dd4b13 100644 --- a/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/core/compiler/IProblem.java +++ b/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/core/compiler/IProblem.java @@ -2601,4 +2601,9 @@ public interface IProblem { * @noreference preview feature */ int UnnamedVariableMustHaveInitializer = PreviewRelated + 2001; + + /** + * @since 3.38 + */ + int ComparingWrapper = Internal + 2011; } diff --git a/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ast/EqualExpression.java b/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ast/EqualExpression.java index 2231ddf745b..85511fe95e3 100644 --- a/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ast/EqualExpression.java +++ b/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ast/EqualExpression.java @@ -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; diff --git a/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/batch/Main.java b/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/batch/Main.java index 8c0387ed683..17b3c419295 100644 --- a/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/batch/Main.java +++ b/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/batch/Main.java @@ -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; diff --git a/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/batch/messages.properties b/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/batch/messages.properties index 4a2fa61d0bb..a39d905fb21 100644 --- a/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/batch/messages.properties +++ b/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/batch/messages.properties @@ -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\ diff --git a/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/impl/CompilerOptions.java b/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/impl/CompilerOptions.java index 85b9d926fbf..994599eb2a3 100644 --- a/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/impl/CompilerOptions.java +++ b/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/impl/CompilerOptions.java @@ -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$ @@ -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 @@ -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 : @@ -976,6 +980,7 @@ public static String[] warningOptionNames() { OPTION_ReportAssertIdentifier, OPTION_ReportAutoboxing, OPTION_ReportComparingIdentical, + OPTION_ReportComparingWrapper, OPTION_ReportDeadCode, OPTION_ReportDeadCodeInTrivialIfStatement, OPTION_ReportDeprecation, @@ -1409,6 +1414,7 @@ public Map 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)); @@ -1965,6 +1971,7 @@ public void set(Map 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); diff --git a/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/problem/ProblemReporter.java b/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/problem/ProblemReporter.java index 5f1c0389055..3c02c43813e 100644 --- a/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/problem/ProblemReporter.java +++ b/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/problem/ProblemReporter.java @@ -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; @@ -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: diff --git a/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/problem/messages.properties b/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/problem/messages.properties index 778775209c8..e5db89bf959 100644 --- a/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/problem/messages.properties +++ b/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/problem/messages.properties @@ -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 @@ -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}'') diff --git a/org.eclipse.jdt.core.tests.compiler/src/org/eclipse/jdt/core/tests/compiler/regression/AnnotationTest.java b/org.eclipse.jdt.core.tests.compiler/src/org/eclipse/jdt/core/tests/compiler/regression/AnnotationTest.java index 54a76520fee..c9241778a39 100644 --- a/org.eclipse.jdt.core.tests.compiler/src/org/eclipse/jdt/core/tests/compiler/regression/AnnotationTest.java +++ b/org.eclipse.jdt.core.tests.compiler/src/org/eclipse/jdt/core/tests/compiler/regression/AnnotationTest.java @@ -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 = @@ -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() {