Skip to content

RemoveRedundantTypeCast removes too much #221

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
koppor opened this issue Nov 20, 2023 · 10 comments
Open

RemoveRedundantTypeCast removes too much #221

koppor opened this issue Nov 20, 2023 · 10 comments
Labels
bug Something isn't working

Comments

@koppor
Copy link
Contributor

koppor commented Nov 20, 2023

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 - JabRef/jabref#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 koppor added the bug Something isn't working label Nov 20, 2023
@greg-at-moderne
Copy link
Contributor

greg-at-moderne commented Mar 25, 2025

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
Copy link
Contributor

@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 JabRef/jabref#12827, but it seems like I don't have all the setup needed, etc. Maybe you can take it from there?

@greg-at-moderne
Copy link
Contributor

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.

@koppor
Copy link
Contributor Author

koppor commented Mar 26, 2025

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
Copy link
Contributor Author

koppor commented Mar 26, 2025

With version 3.2.0; I cannot update to 3.4.0 due to 3a05d87

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

@greg-at-moderne
Copy link
Contributor

Thanks for checking.

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.

@koppor
Copy link
Contributor Author

koppor commented Mar 26, 2025

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: JabRef/jabref#12766


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

@koppor
Copy link
Contributor Author

koppor commented Apr 1, 2025

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 Bring back remove redundant type cast check from OpenRewrite JabRef/jabref#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?

@Siedlerchr
Copy link

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

JabRef/jabref#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

@greg-at-moderne
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants