address: LiteralsFirstInComparisons PMD rule #368

punkratz312 · 2024-10-14T14:38:34Z

tryout: Fix PMD rule LiteralsFirstInComparisons for compareTo* and contentEquals #362

…isitorTest.java Co-authored-by: Tim te Beek <timtebeek@gmail.com>

…incomparisons_extend

// instanceof check

…iteralsfirstincomparisons_extend' into EqualsAvoidsNullVisitor-extend-literalsfirstincomparisons_extend

…isitorTest.java Co-authored-by: Tim te Beek <timtebeek@gmail.com>

This reverts commit 2c2b33a.

timtebeek · 2024-10-16T20:47:40Z

Thanks again @punkratz312 ! I've got an early flight tomorrow and Friday (mostly) off, so it might be Monday before I get to a review and merge.

punkratz312 · 2024-10-16T22:50:26Z

Have a happy flight and enjoy your trip! Thank you for your guidance and support.

…isitor.java Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

punkratz312 · 2024-10-17T16:53:58Z

src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNullVisitor.java

+ * <a href="https://pmd.github.io/pmd/pmd_rules_java_bestpractices.html#LiteralsFirstInComparisons">Literals First in Comparisons</a>
+ *
+ * @param <P> The type of the parent context used for visiting the AST.
+ */
 @Value
 @EqualsAndHashCode(callSuper = false)
 public class EqualsAvoidsNullVisitor<P> extends JavaVisitor<P> {


Perhaps LiteralsFirstInComparisonsVisitor would be a better fit to align with the corresponding PMD rule name already in place, as it's no longer just about equality; both equality and comparison are now merely implementation details.

Suggested change

public class EqualsAvoidsNullVisitor<P> extends JavaVisitor<P> {

public class LiteralsFirstInComparisonsVisitor<P> extends JavaVisitor<P> {

I'd perhaps hold off on renaming this just yet. While the visitor only appears to be used in this module, the recipe name is well established as part of our Common static analysis issues recipe, which folks have created custom copies off that might break if we similarly rename the recipe. I'll think this over as I go through the review.

Yes thanks. Its just an idea and of course would be a new PR topic.

github-actions

Some suggestions could not be made:

src/main/java/org/openrewrite/staticanalysis/ChainStringBuilderAppendCalls.java
- lines 36-38

src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNullVisitor.java

github-actions

Some suggestions could not be made:

src/main/java/org/openrewrite/staticanalysis/ChainStringBuilderAppendCalls.java
- lines 36-38

github-actions

Some suggestions could not be made:

src/main/java/org/openrewrite/staticanalysis/ChainStringBuilderAppendCalls.java
- lines 36-38

timtebeek

I like how you've expanded the existing visitor to cover more cases, and by extension the recipe to do more similar replacements without the need for users to adopt any new recipe!

I also appreciate you're looking to apply best practices to the visitor itself, even if those diverge from what's common elsewhere throughout our code base. It'll take some getting used to on both side on what to (not) pick up as part of your work on recipes, as it does lead to more work to review, and at times discussions as to what's the best way forward.

Typically we view recipes and visitors as one complete unit; there's little need to optimize within such a unit, and at times patterns like the four-fold requireNonNull(m.getSelect()) introduced here are a symptom of branching off separate methods, where the nested structure would have no such need. Going forward lets try to minimize such changes, or perhaps propose them first as part of a self-review such that there's no conflicts arising late in review.

I've left a final question about a removed earlier optimization; once that's answered we can merge this work; thanks again!

…iteralsfirstincomparisons_extend' into EqualsAvoidsNullVisitor-extend-literalsfirstincomparisons_extend

punkratz312 · 2024-10-22T05:03:36Z

src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNullVisitor.java

@@ -97,8 +92,8 @@ private static J.Binary literalsFirstInComparisonsNull(J.MethodInvocation m, Exp
                m.getPrefix(),
                Markers.EMPTY,
                requireNonNull(m.getSelect()),
-                build(J.Binary.Type.Equal).withBefore(SINGLE_SPACE),
-                firstArgument.withPrefix(SINGLE_SPACE),
+                JLeftPadded.build(J.Binary.Type.Equal).withBefore(Space.SINGLE_SPACE),


Build maybe okay, but the Space is DRY Space. SPACE

If i need unwanted context i move my mouse and not clutter the code with redundancy, but its a style where most dev´s use full qualified imports.

Often classes use a mix of the two constants in Space, only one of which has _SPACE in the name for the repetition. I'd prefer then to be consistent with prior use, and i/fwhen we dedice to change that, do so through our enforced patterns in rewrite-recommendations. I hope that context helps!

I suspect the idea with the name was to emphasize that the object represents a single space character. The class Space represents any sequence of whitespace and comments, so Space.SINGLE would be a bit ambiguous (a single space, tab, newline, or comment?), so Space.SINGLE_SPACE uses "space" in two different meanings.

src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNullVisitor.java

punkratz312 · 2024-10-22T05:05:15Z

Thanks for the feedback and correction. I think we might got it now 👍

github-actions

Some suggestions could not be made:

src/main/java/org/openrewrite/staticanalysis/ChainStringBuilderAppendCalls.java
- lines 36-38

timtebeek · 2024-10-22T08:08:59Z

Thanks again @punkratz312 !

punkratz312 · 2024-10-23T18:05:33Z

Thank you, @timtebeek, it was an honor and a great learning experience. I appreciate the support, guidance, and patience.

timtebeek · 2024-10-23T18:07:52Z

You're more than welcome! Your change has just landed in the latest release:
https://github.com/openrewrite/rewrite-static-analysis/releases/tag/v1.19.0

I753089 and others added 30 commits October 14, 2024 11:23

wip

78eb2e7

wip

db907e9

wip BAR_CONTENT_EQUALS

44432a0

add compareToIgnoreCase

6bbb603

wip

3b95e60

undo

b6f48c8

fix name for direct matching

b187593

EQUALS_IGNORE_CASE

735ca39

add CONTENT_EQUALS

111baaa

add compareToInverted

709a488

tmp fix

983b7f5

wip

d9f83f6

fix working

5b30904

wiß

13d7057

wip

cc2c022

wip

e94ff44

wip

8addad3

wip

21f37a4

finalize

e424b6f

sort

9470cd4

add doc

35ed8cf

undo

a519472

Update src/test/java/org/openrewrite/staticanalysis/EqualsAvoidsNullV…

2f0e9b8

…isitorTest.java Co-authored-by: Tim te Beek <timtebeek@gmail.com>

Merge branch 'main' into EqualsAvoidsNullVisitor-extend-literalsfirst…

931755a

…incomparisons_extend

fix Cast after

2c2b33a

// instanceof check

Merge remote-tracking branch 'origin/EqualsAvoidsNullVisitor-extend-l…

7e813dc

…iteralsfirstincomparisons_extend' into EqualsAvoidsNullVisitor-extend-literalsfirstincomparisons_extend

Update src/test/java/org/openrewrite/staticanalysis/EqualsAvoidsNullV…

241262f

…isitorTest.java Co-authored-by: Tim te Beek <timtebeek@gmail.com>

Revert "fix Cast after"

4d78d78

This reverts commit 2c2b33a.

wip: error: pattern matching in instanceof is not supported in -source 8

ca9d3d7

cast

55b8299

Update src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNullV…

4ec7e3b

…isitor.java Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

punkratz312 commented Oct 17, 2024

View reviewed changes

Fix earlier incorrect bot suggestion

d279932

This comment was marked as outdated.

Sign in to view

Place binary operators on same line; remove nested ternaries

3e715de

github-actions bot reviewed Oct 21, 2024

View reviewed changes

timtebeek reviewed Oct 21, 2024

View reviewed changes

src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNullVisitor.java Outdated Show resolved Hide resolved

Restore more deliberate naming of potentialNullCheck

a2c3c1f

github-actions bot reviewed Oct 21, 2024

View reviewed changes

timtebeek added 2 commits October 21, 2024 23:31

Collapse imports as per what's common elsewhere

dd458ab

Make the method matchers static final again

9fed309

github-actions bot reviewed Oct 21, 2024

View reviewed changes

timtebeek reviewed Oct 21, 2024

View reviewed changes

I753089 added 2 commits October 22, 2024 06:59

add leftover

2a582a1

Merge remote-tracking branch 'origin/EqualsAvoidsNullVisitor-extend-l…

dadb102

…iteralsfirstincomparisons_extend' into EqualsAvoidsNullVisitor-extend-literalsfirstincomparisons_extend

punkratz312 commented Oct 22, 2024

View reviewed changes

src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNullVisitor.java Outdated Show resolved Hide resolved

punkratz312 requested a review from timtebeek October 22, 2024 05:04

Use JSpecify nullable annotations

726c5b0

github-actions bot reviewed Oct 22, 2024

View reviewed changes

timtebeek merged commit 07686d2 into openrewrite:main Oct 22, 2024
2 checks passed

timtebeek mentioned this pull request Oct 22, 2024

Fix PMD rule LiteralsFirstInComparisons for compareTo* and contentEquals #362

Open

punkratz312 deleted the EqualsAvoidsNullVisitor-extend-literalsfirstincomparisons_extend branch October 23, 2024 18:05

timtebeek mentioned this pull request Jan 13, 2025

EqualsAvoidsNull wrongly inverts a.compareTo(b) #442

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

address: LiteralsFirstInComparisons PMD rule #368

address: LiteralsFirstInComparisons PMD rule #368

punkratz312 commented Oct 14, 2024 •

edited

Loading

timtebeek commented Oct 16, 2024

punkratz312 commented Oct 16, 2024

punkratz312 Oct 17, 2024 •

edited

Loading

timtebeek Oct 21, 2024 •

edited

Loading

punkratz312 Oct 22, 2024

This comment was marked as outdated.

github-actions bot left a comment

github-actions bot left a comment

github-actions bot left a comment

timtebeek left a comment

punkratz312 Oct 22, 2024

timtebeek Oct 22, 2024

knutwannheden Oct 22, 2024

punkratz312 commented Oct 22, 2024

github-actions bot left a comment

timtebeek commented Oct 22, 2024

punkratz312 commented Oct 23, 2024

timtebeek commented Oct 23, 2024

	public class EqualsAvoidsNullVisitor<P> extends JavaVisitor<P> {
	public class LiteralsFirstInComparisonsVisitor<P> extends JavaVisitor<P> {

address: LiteralsFirstInComparisons PMD rule #368

address: LiteralsFirstInComparisons PMD rule #368

Conversation

punkratz312 commented Oct 14, 2024 • edited Loading

timtebeek commented Oct 16, 2024

punkratz312 commented Oct 16, 2024

punkratz312 Oct 17, 2024 • edited Loading

Choose a reason for hiding this comment

timtebeek Oct 21, 2024 • edited Loading

Choose a reason for hiding this comment

punkratz312 Oct 22, 2024

Choose a reason for hiding this comment

This comment was marked as outdated.

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

timtebeek left a comment

Choose a reason for hiding this comment

punkratz312 Oct 22, 2024

Choose a reason for hiding this comment

timtebeek Oct 22, 2024

Choose a reason for hiding this comment

knutwannheden Oct 22, 2024

Choose a reason for hiding this comment

punkratz312 commented Oct 22, 2024

github-actions bot left a comment

Choose a reason for hiding this comment

timtebeek commented Oct 22, 2024

punkratz312 commented Oct 23, 2024

timtebeek commented Oct 23, 2024

punkratz312 commented Oct 14, 2024 •

edited

Loading

punkratz312 Oct 17, 2024 •

edited

Loading

timtebeek Oct 21, 2024 •

edited

Loading