rewrite-static-analysis icon indicating copy to clipboard operation
rewrite-static-analysis copied to clipboard

RemoveRedundantTypeCast removes too much

Open koppor opened this issue 2 years ago • 10 comments

We have a code base soon 20 years old - and not all code was modernized.

When from 6.4.0 to 6.5.4 and BOM to 2.5.0 - https://github.com/JabRef/jabref/pull/10650, OpenRewrite changed too much:

        private static final List<Character> DISALLOWED_CHARACTERS = Arrays.asList('{', '}', '(', ')', ',', '=', '\\', '"', '#', '%', '~', '\'');

        String newKey = key.chars()
                           .filter(c -> unwantedCharacters.indexOf(c) == -1)
-                           .filter(c -> !DISALLOWED_CHARACTERS.contains((char) c))
+                           .filter(c -> !DISALLOWED_CHARACTERS.contains(c))
                           .collect(StringBuilder::new,
                                   StringBuilder::appendCodePoint, StringBuilder::append)
                           .toString();
         private final Deque<Object> stack;

-        stack.push((int) s.charAt(0));
+        stack.push(s.charAt(0));

Tests fail after this improvement. Not sure whether it's just a hint to modernize the code also at these places. I just wanted to report it to make it more googable. No need to resolve it.

My workaround is to disable the recipe.

koppor avatar Nov 20 '23 23:11 koppor

I know the issue is a bit old. Just came up with a unit test case for the second example given. And the good news is that the test passes, which I assume is thanks to some other changes in the code base:

    @Test
    void primitives() {
        rewriteRun(
          //language=java
          java(
            """
              import java.util.Deque;

              class Test {
                  Deque<Object> stack = null;

                  void method(String s) {
                      stack.push((int) s.charAt(10));
                  }
              }
              """
          )
        );
    }

greg-at-moderne avatar Mar 25 '25 16:03 greg-at-moderne

@koppor, it seems like this issue is no longer affecting the https://github.com/JabRef/jabref codebase. So I thought I would contribute back by raising https://github.com/JabRef/jabref/pull/12827, but it seems like I don't have all the setup needed, etc. Maybe you can take it from there?

greg-at-moderne avatar Mar 26 '25 10:03 greg-at-moderne

Closing the issue as it seems the problem no longer occurs, although I haven't been able to pinpoint the exact change which fixed that.

greg-at-moderne avatar Mar 26 '25 10:03 greg-at-moderne

No

> Task :compileJava
C:\git-repositories\JabRef\src\main\java\org\jabref\gui\errorconsole\ErrorConsoleView.java:74: error: reference to addListener is ambiguous
        viewModel.allMessagesDataProperty().addListener((change -> {
diff --git a/rewrite.yml b/rewrite.yml
index 527118397a..34faa836c1 100644
--- a/rewrite.yml
+++ b/rewrite.yml
@@ -167,7 +167,7 @@ recipeList:
   - org.openrewrite.staticanalysis.RemoveExtraSemicolons
   - org.openrewrite.staticanalysis.RemoveJavaDocAuthorTag
   - org.openrewrite.staticanalysis.RemoveHashCodeCallsFromArrayInstances
-#  - org.openrewrite.staticanalysis.RemoveRedundantTypeCast
+  - org.openrewrite.staticanalysis.RemoveRedundantTypeCast
   - org.openrewrite.staticanalysis.RemoveToStringCallsFromArrayInstances
   - org.openrewrite.staticanalysis.RemoveUnneededAssertion
   - org.openrewrite.staticanalysis.RemoveUnneededBlock
diff --git a/src/main/java/org/jabref/gui/errorconsole/ErrorConsoleView.java b/src/main/java/org/jabref/gui/errorconsole/ErrorConsoleView.java
index 30a276468b..237e00055d 100644
--- a/src/main/java/org/jabref/gui/errorconsole/ErrorConsoleView.java
+++ b/src/main/java/org/jabref/gui/errorconsole/ErrorConsoleView.java
@@ -1,6 +1,5 @@
 package org.jabref.gui.errorconsole;

-import javafx.collections.ListChangeListener;
 import javafx.collections.ObservableList;
 import javafx.fxml.FXML;
 import javafx.scene.Node;
@@ -72,7 +71,7 @@ public class ErrorConsoleView extends BaseDialog<Void> {
         messagesListView.itemsProperty().bind(viewModel.allMessagesDataProperty());
         messagesListView.scrollTo(viewModel.allMessagesDataProperty().getSize() - 1);
         messagesListView.getSelectionModel().setSelectionMode(SelectionMode.MULTIPLE);
-        viewModel.allMessagesDataProperty().addListener((ListChangeListener<LogEventViewModel>) (change -> {
+        viewModel.allMessagesDataProperty().addListener((change -> {
             int size = viewModel.allMessagesDataProperty().size();
             if (size > 0) {
                 messagesListView.scrollTo(size - 1);
diff --git a/src/test/java/org/jabref/model/database/BibDatabaseTest.java b/src/test/java/org/jabref/model/database/BibDatabaseTest.java
index b3ce97fb44..759a4736b3 100644
--- a/src/test/java/org/jabref/model/database/BibDatabaseTest.java
+++ b/src/test/java/org/jabref/model/database/BibDatabaseTest.java
@@ -423,7 +423,7 @@ class BibDatabaseTest {
         database.addString(tripleB);
         database.insertEntry(entry);

-        List<BibtexString> usedStrings = (List<BibtexString>) database.getUsedStrings(Collections.singletonList(entry));
+        List<BibtexString> usedStrings = database.getUsedStrings(Collections.singletonList(entry));
         assertEquals(strings, usedStrings);
     }

koppor avatar Mar 26 '25 10:03 koppor

With version 3.2.0; I cannot update to 3.4.0 due to https://github.com/openrewrite/rewrite-static-analysis/commit/3a05d8787da33ff4914139548b7010b0bccbf6e3

I will wait for a next release and either open a new or be silent ^^

koppor avatar Mar 26 '25 10:03 koppor

Thanks for checking.

I cannot update to 3.4.0 due to https://github.com/openrewrite/rewrite-static-analysis/commit/3a05d8787da33ff4914139548b7010b0bccbf6e3

What's wrong with this? I am not sure what is the impact on your project.

greg-at-moderne avatar Mar 26 '25 10:03 greg-at-moderne

I cannot update to 3.4.0 due to 3a05d87

What's wrong with this? I am not sure what is the impact on your project.

I think, the recipe was updated from 3.2.0 onwards.

Slack post: https://rewriteoss.slack.com/archives/C01A843MWG5/p1742247722746019

PR trying to update from 3.2.0 to 3.4.0: https://github.com/JabRef/jabref/pull/12766


Itis very OK for me to wait for a new release.

koppor avatar Mar 26 '25 10:03 koppor

I tried today.

Same diff.

> Task :compileJava
C:\git-repositories\JabRef\src\main\java\org\jabref\gui\errorconsole\ErrorConsoleView.java:74: error: reference to addListener is ambiguous
        viewModel.allMessagesDataProperty().addListener((change -> {
                                           ^
  both method addListener(InvalidationListener) in Observable and method addListener(ListChangeListener<? super E>) in ObservableList match
  where E is a type-variable:
    E extends Object declared in interface ObservableList
1 error

To reproduce:

  1. Checkout https://github.com/JabRef/jabref/pull/12827
  2. Merge upstream/main
  3. Execute ./gradlew run
  4. Maybe: git reset --hard upstream/main
  5. Re-enable - org.openrewrite.staticanalysis.RemoveRedundantTypeCast
  6. Execute ./gradlew rewriterun
  7. Execute ./gradlew run

I tried to execute it on the moderne platform --> https://app.moderne.io/recipes/org.openrewrite.staticanalysis.RemoveRedundantTypeCast?organizationId=ZjhkMzFkYmMtOTE5Yi00ODgwLThjMDItMDkwMThmYzhmODMz

However, there is no ./gradlew assemble task executed, is it?

koppor avatar Apr 01 '25 11:04 koppor

Still an error with
id 'org.openrewrite.rewrite' version '7.3.0'
and org.openrewrite.recipe:rewrite-recipe-bom:3.5.0

https://github.com/JabRef/jabref/pull/12828


ErrorConsoleView.java:74: error: reference to addListener is ambiguous
        viewModel.allMessagesDataProperty().addListener((change -> {
                                           ^
  both method addListener(InvalidationListener) in Observable and method addListener(ListChangeListener<? super E>) in ObservableList match
  where E is a type-variable:
    E extends Object declared in interface ObservableList

Siedlerchr avatar Apr 09 '25 19:04 Siedlerchr

Re-opening based on the feedback above. Thanks for providing the fresh input.

greg-at-moderne avatar Apr 10 '25 04:04 greg-at-moderne