Add a new fix to BoxedIdentityComparison #314

ghost · 2017-12-02T21:35:50Z

In cases where the LHS or RHS is definitely not null, the provided fix
should rewrite the == or != expression to a call to equals() on the
definitely-not-null side (negated if the original op is !=). This allows
a fix to be offered when the source level is 6 or earlier.

Also, NPECheck is enhanced with a "nullability of fields" user-configurable
preference which allows the user to customize whether certain fields
(specified by a fully-qualified name) are to be considered null, not
null, or possibly null by NPECheck. The BASE_NULLABILITY_OF_FIELDS map
can store such configuration for fields in the standard java.** and
javax.** packages.

This PR depends on #320 being merged first.

sdedic · 2017-12-05T07:32:20Z

java.hints/src/org/netbeans/modules/java/hints/bugs/BoxedIdentityComparison.java

+            this.lhsIsReceiver = lhsIsReceiver;
+            BinaryTree bt = (BinaryTree) comparisonExprPath.getLeaf();
+            ExpressionTree receiver = (lhsIsReceiver ? bt.getLeftOperand() : bt.getRightOperand());
+            @SuppressWarnings("LocalVariableHidesMemberVariable")


Nitpick: unnecessary hide, makes the code less readable despite the @SuppressWarnings

Changed the local var name to baseReceiverString.

sdedic · 2017-12-05T07:34:16Z

java.hints/src/org/netbeans/modules/java/hints/bugs/BoxedIdentityComparison.java

+            }
+            ExpressionTree ms = mk.MemberSelect(receiver, "equals");
+            ExpressionTree replace = mk.MethodInvocation(Collections.emptyList(), ms, Arrays.asList(lhsIsReceiver ? bt.getRightOperand() : bt.getLeftOperand()));
+            if (bt.getKind() == Tree.Kind.NOT_EQUAL_TO) {


this.invert ?

sdedic · 2017-12-05T07:37:46Z

java.hints/src/org/netbeans/modules/java/hints/bugs/BoxedIdentityComparison.java

+            if (!JavaFixUtilities.isPrimary(receiver)) {
+                receiver = mk.Parenthesized(receiver);
+            }
+            ExpressionTree ms = mk.MemberSelect(receiver, "equals");


// NOI18N mark comment missing

sdedic · 2017-12-05T07:38:17Z

java.hints/src/org/netbeans/modules/java/hints/bugs/BoxedIdentityComparison.java

+            ExpressionTree receiver = (lhsIsReceiver ? bt.getLeftOperand() : bt.getRightOperand());
+            @SuppressWarnings("LocalVariableHidesMemberVariable")
+            String receiverString = receiver.toString();
+            this.receiverString = (JavaFixUtilities.isPrimary(receiver) ? receiverString : "(" + receiverString + ")");


add // NOI18N

sdedic · 2017-12-05T08:19:04Z

spi.java.hints/src/org/netbeans/spi/java/hints/JavaFixUtilities.java

+     * @return {@code true} iff {@code tree} can be used where a Primary is
+     * required.
+     */
+    public static boolean isPrimary(@NonNull Tree tree) {


API is being changed, so I think that old good API change procedure should happen. See http://wiki.netbeans.org/API_Development; http://wiki.netbeans.org/VersioningPolicy
You need to bump up Specification version of the spi.java.hints, write relevant apichanges.xml entry. Add proper "since" javadoc tag to this added method.
You need to increase the required spi.java.hints spec version In module java.hints which uses this new API.

It's often better to separate API change(s) into their own PR, which should come first

I created PR #320 for the API change.

sdedic · 2017-12-05T08:37:56Z

java.hints/src/org/netbeans/modules/java/hints/bugs/NPECheck.java

+                    // This is specifically to make sure that the state of
+                    // TRUE and FALSE static imports from java.lang.Boolean
+                    // is NOT_NULL.
+                    if (modifiers != null && modifiers.contains(Modifier.STATIC) && modifiers.contains(Modifier.FINAL)) {


I would be cautious to add rather broad-scope heuristics like this. It is rather necessary for the fix to work OK, but consider if some more restrictive condition would not be acceptable - e.g. just static final fields of primitive wrappers, or java.lang.* package.

With such rather broad scope, I'd like to see an UI option introduced - something like list of REs the FQN is matched against.

@jlahoda may have an opinion on this, too

jtulach · 2018-01-10T07:01:41Z

spi.java.hints/apichanges.xml

+        <change id="JavaFixUtilities.isPrimary">
+            <api name="JavaHintsSPI"/>
+            <summary>Added JavaFixUtilities.isPrimary() utility</summary>
+            <version major="1" minor="29"/>


Nice to see the API change here! Shouldn't you guys also increase version in manifest.mf and reference it here? E.g. use 1.30?

Hello @jtulach. Thank you for your review.

I was a little confused by this; I figured that I needed to bump some sort of version number, but I found that the "spec.version.base" in spi.java.hints/nbproject/project.properties was already set to "1.30.0" in the project properties:

https://github.com/apache/incubator-netbeans/blob/dd5eb868ec5e8037c77118dbd5e2c53103f1c30e/spi.java.hints/nbproject/project.properties#L20

By the way, I have opened PR #320 to track this API change separately from the "Add a new fix to BoxedIdentityComparison" changes.

vieiro · 2018-01-13T08:23:05Z

If these changes require #320, why not add these to #320? Having different interdependent PRs makes reviewing changes more complicated.

ghost · 2018-01-13T16:56:23Z

Hello @vieiro,

The "Code cleanup" label is not appropriate for this PR. This PR implements an enhancement to the BoxedIdentityComparison hint. Is there an "enhancement" label that could be used instead?

ghost · 2018-01-16T16:28:41Z

I have significantly overhauled the modifications to NPECheck, based on @sdedic's concern about the broad-scope heuristic that I was using in the previous iteration of this patch.

There is now a user-configurable "nullability of fields" preference, which allows the user to customize which fields (identified by their fully-qualified name) would be considered null, non-null, or possibly null by NPECheck. This mechanism also allows for a set of base mappings, stored in BASE_NULLABILITY_OF_FIELDS, that are referred to if the user has not specifically configured a field. Right now, I have only added mappings for java.lang.Boolean.TRUE and java.lang.Boolean.FALSE, but I will add a lot more if you all like this new approach.

I was also thinking about "nullability of method return values" which would allow the user to customize whether specifically-identified methods return null, non-null, or possibly null values. This will be slightly more complex to support than "nullability of fields", so I have not done this, but if you all like the "nullability of fields" idea, then I will certainly work on this extension.

ghost · 2018-01-16T17:07:30Z

By the way, to see the new UI for editing the "nullability of fields" preference, go to NetBeans > Preferences…. Click on the Editor tab, and then the Hints subtab. In the tree on the left, navigate to and click Probable Bugs > Null Pointer Dereference:

This utility method determines whether a Tree can be used in places where a Primary is required (for instance, as the receiver expression of a method invocation).

In cases where the LHS or RHS is definitely not null, the provided fix should rewrite the == or != expression to a call to equals() on the definitely-not-null side (negated if the original op is !=). This allows a fix to be offered when the source level is 6 or earlier. Also, NPECheck is enhanced with a "nullability of fields" user-configurable preference which allows the user to customize whether certain fields (specified by a fully-qualified name) are to be considered null, not null, or possibly null by NPECheck. The BASE_NULLABILITY_OF_FIELDS map can store such configuration for fields in the standard java.** and javax.** packages.

vieiro · 2018-01-19T18:21:00Z

Hi @dtrebbien is this ready for review?

ghost · 2018-01-19T18:30:46Z

Yes

cowwoc · 2018-03-16T16:18:52Z

@geertjanw Ready for review.

PS: Can you please look into granting everyone permission to request a review? Right now only a small number of people have permissions.

matthiasblaesing · 2019-02-22T20:26:23Z

Closing this, as the requesting account was removed and the PR is stalled.

sdedic reviewed Dec 5, 2017

View reviewed changes

sdedic added the API Change [ci] enable extra API related tests label Dec 5, 2017

jtulach reviewed Jan 10, 2018

View reviewed changes

sdedic removed the API Change [ci] enable extra API related tests label Jan 12, 2018

vieiro added work-in-progress Code cleanup labels Jan 13, 2018

sdedic added enhancement and removed Code cleanup labels Jan 15, 2018

ghost mentioned this pull request Jan 15, 2018

Add the JavaFixUtilities.isPrimary() utility API #320

Merged

dtrebbien added 2 commits January 17, 2018 23:30

Add the JavaFixUtilities.isPrimary() utility API

e854b9e

This utility method determines whether a Tree can be used in places where a Primary is required (for instance, as the receiver expression of a method invocation).

vieiro removed the work-in-progress label Jan 19, 2018

matthiasblaesing closed this Feb 22, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a new fix to BoxedIdentityComparison #314

Add a new fix to BoxedIdentityComparison #314

ghost commented Dec 2, 2017 •

edited by ghost

sdedic Dec 5, 2017

ghost Dec 5, 2017

sdedic Dec 5, 2017

ghost Dec 5, 2017

sdedic Dec 5, 2017

ghost Dec 5, 2017

sdedic Dec 5, 2017

ghost Dec 5, 2017

sdedic Dec 5, 2017

ghost Dec 5, 2017

sdedic Dec 5, 2017

jtulach Jan 10, 2018 •

edited

ghost Jan 11, 2018

vieiro commented Jan 13, 2018

ghost commented Jan 13, 2018 •

edited by ghost

ghost commented Jan 16, 2018

ghost commented Jan 16, 2018

vieiro commented Jan 19, 2018

ghost commented Jan 19, 2018

cowwoc commented Mar 16, 2018

matthiasblaesing commented Feb 22, 2019

Add a new fix to BoxedIdentityComparison #314

Add a new fix to BoxedIdentityComparison #314

Conversation

ghost commented Dec 2, 2017 • edited by ghost

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtulach Jan 10, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vieiro commented Jan 13, 2018

ghost commented Jan 13, 2018 • edited by ghost

ghost commented Jan 16, 2018

ghost commented Jan 16, 2018

vieiro commented Jan 19, 2018

ghost commented Jan 19, 2018

cowwoc commented Mar 16, 2018

matthiasblaesing commented Feb 22, 2019

ghost commented Dec 2, 2017 •

edited by ghost

jtulach Jan 10, 2018 •

edited

ghost commented Jan 13, 2018 •

edited by ghost