feat: issue optional warning using '==' for wrapper types #2188

dashorst · 2024-03-20T12:27:28Z

What it does

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.

Long x1 = 12345L;
Long x2 = 67890L;
if(x1 == x2)
    System.out.println("Equal");
else
    System.out.println("Not equal");

Fixes #2176

How to test

Use offending code comparing two Long references or two Strings using '==', and set the optional problem to a level like information, warning or error. The compiler should flag the offending line with the message Comparing primitive wrapper types or Strings using '=='

Author checklist

I have thoroughly tested my changes
The change is following the coding conventions
I have signed the Eclipse Contributor Agreement (ECA)

iloveeclipse · 2024-03-31T10:58:26Z

@dashorst : thanks for the contribution. Please undo merge commit and rebase your fix commit on top of the latest master. We disallow merge commits for regular PR's to keep the history linear.

iloveeclipse · 2024-03-31T11:00:05Z

@stephan-herrmann : any objections to the proposed warning & any chance you could review this PR?

stephan-herrmann · 2024-04-01T17:32:47Z

stephan-herrmann force-pushed the issue_2176 branch from b8f1cae to 014ee74

Simply rebase of the one feature commit on top of current master.

This should make iloveeclipse happy, and serve as a baseline during the review -- please no further rebase or merge for the time being. Thanks.

stephan-herrmann

Thanks for the PR which indeed pushes a lot of the correct buttons already!

Please have a look at my detailed remarks.

Once this is merged, we will also need a matching change to provide configuration of the new option in the compiler preferences UI (JDT/UI). Will you be available to provide a PR also for that part? (need to wait until the new constant in JavaCore is available).

When making further changes please keep them as separate commits, to allow me to inspect the incremental progress during review, thanks.

org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/core/compiler/IProblem.java

...lipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ast/EqualExpression.java

...ipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/batch/messages.properties

...e.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/problem/ProblemReporter.java

...se.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/problem/messages.properties

...t.core.tests.compiler/src/org/eclipse/jdt/core/tests/compiler/regression/AnnotationTest.java

org.eclipse.jdt.core/model/org/eclipse/jdt/core/JavaCore.java

...t.core.tests.compiler/src/org/eclipse/jdt/core/tests/compiler/regression/AnnotationTest.java

stephan-herrmann · 2024-04-01T20:50:57Z

One more thing: when the new option remains unset, we don't see any problems. I suggest to set the default to 'info', so that users will be informed about the new analysis, without it getting in their way, e.g., when their build is configured to fail on warnings etc. WDYT?

Once we have the UI for this option, the problem hover will have affordance to directly go into the configuration UI for this particular irritant, which is a good way to ask the user if they want this analysis. If ecj keeps silent, they don't have this option :)

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

dashorst · 2024-04-02T08:12:56Z

I very much appreciate the feedback @stephan-herrmann Will look into it.

stephan-herrmann · 2024-04-02T12:18:07Z

dashorst force-pushed the issue_2176 branch from 014ee74 to be4d139

What is this push?

Actually, 014ee74 was "more up-to-date" than this newer push.

Please note that every force push disrupts the incremental review process. That's why I asked:

please no further rebase or merge for the time being. Thanks.

and:

When making further changes please keep them as separate commits, to allow me to inspect the incremental progress during review, thanks.

dashorst · 2024-04-04T11:26:39Z

Please note that every force push disrupts the incremental review process. That's why I asked:

I did it because @iloveeclipse asked for it (merges are not accepted) I didn't see your push. My sincere apologies.

- rename ComparingWrapper to UnlikelyEqualExpressionArgumentType - move IProblem constant value to 1202 - add `SuppressWarnings("unlikely-arg-type") support - move test to `ProgrammingProblemsTest` - add test cases - add documentation

dashorst · 2024-04-06T22:09:44Z

@stephan-herrmann Many thanks for your comments and feedback. I've done my best to incorporate your review and have updated the PR.

dashorst · 2024-04-06T22:25:48Z

One more thing: when the new option remains unset, we don't see any problems. I suggest to set the default to 'info', so that users will be informed about the new analysis, without it getting in their way, e.g., when their build is configured to fail on warnings etc. WDYT?

I've set it to info.

Once we have the UI for this option, the problem hover will have affordance to directly go into the configuration UI for this particular irritant, which is a good way to ask the user if they want this analysis. If ecj keeps silent, they don't have this option :)

Great and sounds logical

...e.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/problem/ProblemReporter.java

…arison

e.g. detecting Number == Long, and Number == Serializable comparisons.

stephan-herrmann · 2024-04-23T16:26:37Z

should we stick to the habit of grouping similar problems to one irritant and one suppress token, or is more fine grained control a good idea?

My preference is to group similar ones together wherever possible.

what do you think of adopting the suppress tokens from IDEA: "NumberEquality", "ObjectEquality", "StringEquality"

I am for it for the simple reason that this saves us time, going with a tried and tested feature. That it will find more takers and more people will feel at home is a big bonus.

These two answers seem to contradict each other, one token, or the three introduced by IDEA?

So let's do a quick vote with only two alternatives for the tokens used with @SuppressWarnings:

"reference-comparison" to suppress all such problems (here I don't see a need to specify unlikely or dubious ...), or
"NumberEquality", "ObjectEquality", "StringEquality" plus "BooleanEquality", each suppressing one specific flavor of the problem

Please have your say! :)

For the CLI option I could warm for -warn:dubiousReferenceComparison

In the Errors/Warnings UI we could make it a two step configuration:

Dubious reference comparison [Error/Warning/Info/Ignore]
** [x] boxed numerical values [ ] boxed Boolean [x] strings
** [ ] other objects: "*", or list of fully qualified type names

Internal names can then follow these decisions in a round-about manner.

iloveeclipse · 2024-04-23T16:31:47Z

I like "dubious reference comparison" wording and I assume a simple "reference-comparison" for SuppressWarnings should be enough. Usually there aren't that many various dubious comparisons in same code.

jarthana · 2024-04-24T04:44:42Z

I like "dubious reference comparison" wording and I assume a simple "reference-comparison" for SuppressWarnings should be enough. Usually there aren't that many various dubious comparisons in same code.

+1

stephan-herrmann · 2024-05-05T14:41:40Z

Looks like I forgot to save when I tried to call out that we seem to have this consensus (yea 😄 ):

@SuppressWarnings("reference-comparison")
CLI option: -warn:dubiousReferenceComparison
Internal names can then follow these decisions in a round-about manner.

@dashorst do you want to update your change accordingly? I hope these will be small changes only.

Please also revisit the warning message, if it speaks the same language as the above.

- rename all unlikely to dubious - add suppression for this warning 'reference-comparison' - rename compiler option to dubiousReferenceComparison

dashorst · 2024-05-09T21:13:39Z

I have updated the PR with the new 'requirements'. The build fails with the newly introduced warning. Before I add suppression to all those locations, I'd like to get feedback if the current state is correct.

stephan-herrmann

We are getting very close!
@dashorst please don't let the number of fresh review comments intimidate you.
Those are only minor issues that should not require further discussion, right?
Everything else looks great to me.

...s.compiler/src/org/eclipse/jdt/core/tests/compiler/regression/AbstractBatchCompilerTest.java

...e.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/problem/ProblemReporter.java

stephan-herrmann · 2024-05-12T13:55:41Z

...lipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ast/EqualExpression.java

+				((leftType.isBoxedPrimitiveType() && rightType.isBoxedPrimitiveType()) ||
+				(!leftType.isPrimitiveType() && (rightType.isBoxedPrimitiveType() || rightTypeIsNumber)) ||
+				((leftType.isBoxedPrimitiveType() || leftTypeIsNumber) && !rightType.isPrimitiveType()) ||
+				(leftType.id == rightType.id && (leftType.id == TypeIds.T_JavaLangString || leftTypeIsNumber))))


Here || leftTypeIsNumber seems unnecessary: the case Number == Number is already covered above. Remove it and test method a8() still triggers the error.

The same even holds for (leftType.isBoxedPrimitiveType() && rightType.isBoxedPrimitiveType()) ||

Detecting "boxed primitive or Number vs. anything non-primitive" covers all wrapper comparisons, doesn't it? 😄

...ipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/batch/messages.properties

...ipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/impl/CompilerOptions.java

stephan-herrmann · 2024-05-12T14:17:02Z

org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/impl/IrritantSet.java

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


~~warnings~~ -> infos

stephan-herrmann · 2024-05-12T14:18:32Z

...ipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/impl/CompilerOptions.java

@@ -2367,6 +2380,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 equal expression: ").append(getSeverityString(DubiousReferenceComparison)); //$NON-NLS-1$


~~equal expression~~ -> reference comparison

dashorst · 2024-05-12T16:01:27Z

We are getting very close! @dashorst please don't let the number of fresh review comments intimidate you. Those are only minor issues that should not require further discussion, right? Everything else looks great to me.

I really don't mind. I know I'm not very well versed in this code base, so I rely on your expertise on how I should implement things. I'll take a look soon and implement the necessary changes. Thank you so much for your time and energy.

- modified compiler CLI option - implemented tests for CLI option

stephan-herrmann · 2024-05-20T10:31:12Z

Thanks for your updates. We'll miss the 2024-06 release, given that M3 is completed. I'll set 2024-09 M1 as the milestone when it becomes available.

This gives us a little time to discuss the final step: UI for the new option. @dashorst do feel prepared to propose such change in jdt.ui as well? I'd suggest looking at recent changes in ProblemSeveritiesConfigurationBlock as your starting point.

stephan-herrmann

@dashorst the master branch has been re-opened, and this PR really is on the finish line.

I marked final polish items, but there's one more surprise: the jenkins build (already applying the modified compiler) signals dubious reference comparison also for various array types. What's going on there?

stephan-herrmann · 2024-06-06T09:26:20Z

org.eclipse.jdt.core/model/org/eclipse/jdt/core/JavaCore.java

+	 * Compiler option ID: Reporting an equal comparison with dubious reference types, i.e. wrapper types (Integer, Long) for primitives (int, resp. long) and Strings.
+	 * <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>


where is the wrapper in the example?

stephan-herrmann · 2024-06-06T09:28:07Z

org.eclipse.jdt.core/model/org/eclipse/jdt/core/JavaCore.java

+	 * <dt>Possible values:</dt><dd><code>{ "error", "warning", "info", "ignore" }</code></dd>
+	 * <dt>Default:</dt><dd><code>"info"</code></dd>
+	 * </dl>
+	 * @since 3.38


When all code changes are done, we'll have to bump up bundles to 3.39.0 (currently 3.38.100) and mention that version in @since tags.
I can take care of that in the end.

stephan-herrmann · 2024-06-06T09:47:50Z

...e.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/problem/ProblemReporter.java

+			new String[] {operator, new String(leftType.shortReadableName()), new String(rightType.shortReadableName())},
+			new String[] {operator, new String(leftType.readableName()), new String(rightType.readableName())},


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.

stephan-herrmann · 2024-06-06T10:14:16Z

Merge branch 'master' into dashorst:issue_2176

I resolved merge conflicts.
I will of course make sure that the undesired merged commit will not show up in master (that's handled by squash and merge) 😄

stephan-herrmann · 2024-07-02T10:52:01Z

@dashorst the master branch has been re-opened, and this PR really is on the finish line.

I marked final polish items, but there's one more surprise: the jenkins build (already applying the modified compiler) signals dubious reference comparison also for various array types. What's going on there?

@dashorst a friendly reminder ..

dashorst · 2024-07-02T14:42:14Z

Unfortunately I have lost 60% of vision in my right eye: murky eye juice scrambles my vision such that letters become unreadable. This issue strains my eyes during the work day, limiting what I can do during the evenings (which is my volunteer time). While I have the will to continue pushing this further, I can't guarantee any progress in the short term. I would not mind if anyone else picks this up, but I can also imagine this being stalled for a while.

stephan-herrmann · 2024-07-02T20:47:51Z

Unfortunately I have lost 60% of vision in my right eye [...]

Oh no! I'm very sorry.
And thanks for letting us know.

While I have the will to continue pushing this further, I can't guarantee any progress in the short term. I would not mind if anyone else picks this up, but I can also imagine this being stalled for a while.

It would be a pity if all that effort would go stale. I'll keep it on my radar and will try to take it to completion, unless you beat me to it, but don't feel urged to further stress your eyes.

dashorst force-pushed the issue_2176 branch 3 times, most recently from 7ee8ffe to cfade8f Compare March 20, 2024 12:46

dashorst mentioned this pull request Mar 20, 2024

RFE: issue (optional) warning when '==' is used on wrapper types #2176

Open

dashorst force-pushed the issue_2176 branch 2 times, most recently from 4d8d0c7 to b8f1cae Compare March 27, 2024 13:41

stephan-herrmann self-requested a review March 31, 2024 11:48

stephan-herrmann force-pushed the issue_2176 branch from b8f1cae to 014ee74 Compare April 1, 2024 17:30

stephan-herrmann requested changes Apr 1, 2024

View reviewed changes

dashorst force-pushed the issue_2176 branch from 014ee74 to be4d139 Compare April 2, 2024 08:11

dashorst added 5 commits April 6, 2024 23:50

refactor: ComparingWrapper constant -> 1202

5d9a86e

docs: clarify ComparingWrapper detection of boxed types

33b3448

refactor: use already available severity computation

f4e2db9

fix: also rename compiler option and document it

dcd55f0

stephan-herrmann reviewed Apr 7, 2024

View reviewed changes

...e.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/problem/ProblemReporter.java Outdated Show resolved Hide resolved

dashorst added 4 commits April 8, 2024 10:37

refactor: UnlikelyEqualExpressionArgumenType -> UnlikelyReferenceComp…

0784fef

…arison

refactor: add operator to unlikelyReferenceComparison message

389bdfb

fix: CompilerInvocationTests

b3140c8

feat: add support for more reference comparison detections

4b3c272

e.g. detecting Number == Long, and Number == Serializable comparisons.

dashorst added 4 commits May 9, 2024 19:01

refactor: unlikelyReferenceComparison -> dubiousReferenceComparison

b3cf7e2

- rename all unlikely to dubious - add suppression for this warning 'reference-comparison' - rename compiler option to dubiousReferenceComparison

fix: test runner gave errors on non-normalized paths

d4eb747

fix: give better test failure messages

df2ef7e

Update warning message from Unlikely -> Dubious

d6393d1

stephan-herrmann requested changes May 12, 2024

View reviewed changes

dashorst added 3 commits May 12, 2024 23:03

test: Normalise path differences in assertion

3c0d8ac

refactor: implement review comments

2ec884d

- modified compiler CLI option - implemented tests for CLI option

clean: remove duplicate entry in CompilerOptions

bf2fec3

stephan-herrmann added this to the MilestoneNxt milestone May 20, 2024

mpalat modified the milestones: MilestoneNxt, 4.33 Jun 6, 2024

stephan-herrmann modified the milestones: 4.33, 4.33 M1 Jun 6, 2024

stephan-herrmann requested changes Jun 6, 2024

View reviewed changes

Merge branch 'master' into dashorst:issue_2176

5d54c68

stephan-herrmann modified the milestones: 4.33 M1, 4.33 M3 Jul 2, 2024

stephan-herrmann modified the milestones: 4.33 M3, MilestoneNxt Aug 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: issue optional warning using '==' for wrapper types #2188

feat: issue optional warning using '==' for wrapper types #2188

dashorst commented Mar 20, 2024 •

edited

Loading

iloveeclipse commented Mar 31, 2024

iloveeclipse commented Mar 31, 2024

stephan-herrmann commented Apr 1, 2024

stephan-herrmann left a comment

stephan-herrmann commented Apr 1, 2024

dashorst commented Apr 2, 2024

stephan-herrmann commented Apr 2, 2024

dashorst commented Apr 4, 2024

dashorst commented Apr 6, 2024

dashorst commented Apr 6, 2024

stephan-herrmann commented Apr 23, 2024

iloveeclipse commented Apr 23, 2024

jarthana commented Apr 24, 2024

stephan-herrmann commented May 5, 2024

dashorst commented May 9, 2024

stephan-herrmann left a comment

stephan-herrmann May 12, 2024

stephan-herrmann May 12, 2024

stephan-herrmann May 12, 2024

dashorst commented May 12, 2024

stephan-herrmann commented May 20, 2024

stephan-herrmann left a comment

stephan-herrmann Jun 6, 2024

stephan-herrmann Jun 6, 2024

stephan-herrmann Jun 6, 2024

stephan-herrmann commented Jun 6, 2024

stephan-herrmann commented Jul 2, 2024

dashorst commented Jul 2, 2024

stephan-herrmann commented Jul 2, 2024

		new String[] {operator, new String(leftType.shortReadableName()), new String(rightType.shortReadableName())},
		new String[] {operator, new String(leftType.readableName()), new String(rightType.readableName())},

feat: issue optional warning using '==' for wrapper types #2188

Are you sure you want to change the base?

feat: issue optional warning using '==' for wrapper types #2188

Conversation

dashorst commented Mar 20, 2024 • edited Loading

What it does

How to test

Author checklist

iloveeclipse commented Mar 31, 2024

iloveeclipse commented Mar 31, 2024

stephan-herrmann commented Apr 1, 2024

stephan-herrmann left a comment

Choose a reason for hiding this comment

stephan-herrmann commented Apr 1, 2024

dashorst commented Apr 2, 2024

stephan-herrmann commented Apr 2, 2024

dashorst commented Apr 4, 2024

dashorst commented Apr 6, 2024

dashorst commented Apr 6, 2024

stephan-herrmann commented Apr 23, 2024

iloveeclipse commented Apr 23, 2024

jarthana commented Apr 24, 2024

stephan-herrmann commented May 5, 2024

dashorst commented May 9, 2024

stephan-herrmann left a comment

Choose a reason for hiding this comment

stephan-herrmann May 12, 2024

Choose a reason for hiding this comment

stephan-herrmann May 12, 2024

Choose a reason for hiding this comment

stephan-herrmann May 12, 2024

Choose a reason for hiding this comment

dashorst commented May 12, 2024

stephan-herrmann commented May 20, 2024

stephan-herrmann left a comment

Choose a reason for hiding this comment

stephan-herrmann Jun 6, 2024

Choose a reason for hiding this comment

stephan-herrmann Jun 6, 2024

Choose a reason for hiding this comment

stephan-herrmann Jun 6, 2024

Choose a reason for hiding this comment

stephan-herrmann commented Jun 6, 2024

stephan-herrmann commented Jul 2, 2024

dashorst commented Jul 2, 2024

stephan-herrmann commented Jul 2, 2024

dashorst commented Mar 20, 2024 •

edited

Loading