openrewrite / rewrite-static-analysis

OpenRewrite recipes for identifying and fixing static analysis issues.
Apache License 2.0
27 stars 43 forks source link

Refine documentation on isemptycalloncollections #168

Closed koppor closed 7 months ago

koppor commented 9 months ago

I am reading https://docs.openrewrite.org/recipes/staticanalysis/isemptycalloncollections

I thought that following diff would be created:

-            if ((entry.getFiles().size() > 0) && stateManager.getActiveDatabase().isPresent()) {
+           if ((!entry.getFiles().isEmpty()) && stateManager.getActiveDatabase().isPresent()) {

Source: https://github.com/JabRef/jabref/blob/3583b4e1fdc38cd5927a6425b9bec98aaf65ea72/src/main/java/org/jabref/gui/actions/ActionHelper.java#L70

Did I do some misconfiguration in my project (https://github.com/JabRef/jabref/blob/70e9eb37b5ea919c2afd1efb522d9c2e2920c60e/rewrite.yml#L140) - or does is the rule simply not triggered in a compound expression?

timtebeek commented 9 months ago

Hmm; we do have some tests that use compound expressions, so I'd imagine this should trigger. https://github.com/openrewrite/rewrite-static-analysis/blob/451eef891203adca363141971edcc5df3782aa84/src/test/java/org/openrewrite/staticanalysis/IsEmptyCallOnCollectionsTest.java#L87-L94

I also don't see anything in the recipe that would have it not trigger in the recipe https://github.com/openrewrite/rewrite-static-analysis/blob/451eef891203adca363141971edcc5df3782aa84/src/main/java/org/openrewrite/staticanalysis/IsEmptyCallOnCollections.java#L76-L81

In your code there's no obvious issues either that I can see; no use of lombok or anything. There were two earlier issues relating to lambdas, but I don't think that factors in here at first glance:

I'll log this as an issue and place it on the backlog; anyone welcome to explore it further.

koppor commented 8 months ago

Today, I saw following diff in a PR. Think, this rule maybe doesn't match lambdas yet?

-                list -> list.size() > 0,
+                list -> !list.isEmpty(),

JabRef/jabref@032e29a (#10527)

koppor commented 8 months ago

Doesn't seem to work with JSONArray:

            JSONArray result = response.getBody()
                                       .getObject()
                                       .getJSONArray("results");

Manual change needed

-            if (result.length() > 0) {
+            if (!result.isEmpty()) {
timtebeek commented 8 months ago

Are you using the latest snapshot? That recipe has seen some recent changes: https://github.com/openrewrite/rewrite-static-analysis/compare/v1.0.8...main Not quite sure if your issue has been fixed, but it might help us pin down if we need more changes.

koppor commented 8 months ago

Are you using the latest snapshot?

Thank you for the hint! Now, I do. However, seems not to match https://github.com/JabRef/jabref/blob/main/src/main/java/org/jabref/logic/importer/fetcher/ZbMATH.java

timtebeek commented 8 months ago

Doesn't seem to work with JSONArray:

            JSONArray result = response.getBody()
                                       .getObject()
                                       .getJSONArray("results");

The recipe is implemented for Collections, which JSONArray does not implement. If you want to same results for JSONArray we'd need to either add a new matcher in the existing recipe, or more likely add a new recipe with a similar effect, perhaps using Refaster templates.

timtebeek commented 7 months ago

I see the above line that started this issue has since been changed, and there have been quite some improvements to the recipe since it was opened. The JSONArray case would need a separate recipe, but that's likely best tracked & developed separately, as it's not an immediate fit here. Hope you don't mind me closing this issue for now; we can revisit if it's easier to reproduce now with the latest versions.