openrewrite / rewrite-testing-frameworks

OpenRewrite recipes that perform common Java testing migration tasks.
Apache License 2.0
68 stars 60 forks source link

RemoveTryCatchFailBlocks leaves unused import #421

Closed vlsi closed 8 months ago

vlsi commented 8 months ago

What version of OpenRewrite are you using?

rewrite-recipe-bom:2.4.1

How are you running OpenRewrite?

./gradlew rewriteRun

See https://github.com/pgjdbc/pgjdbc/pull/2979

What is the smallest, simplest way to reproduce the problem?

Here's a problematic class: https://github.com/pgjdbc/pgjdbc/blob/9cf9f36a1d3a1edd9286721f9c0b9cfa9e8422e3/pgjdbc/src/test/java/org/postgresql/test/jdbc4/ClientInfoTest.java#L87

import java.sql.SQLClientInfoException;
...
  @Test
  public void testWarningOnUnknownName() throws SQLException {
    try {
      con.setClientInfo("NonexistentClientInfoName", "NoValue");
    } catch (SQLClientInfoException e) {
      fail("Trying to set a nonexistent name must not throw an exception (spec)");
    }
    assertNotNull(con.getWarnings());
  }

What did you expect to see?

import SQLClientInfoException should be removed

What did you see instead?

import java.sql.SQLClientInfoException; // <-- this is left intact
...
  @Test
  void testWarningOnUnknownName() throws SQLException {
    assertDoesNotThrow(() -> {
      con.setClientInfo("NonexistentClientInfoName", "NoValue");
    }, "Trying to set a nonexistent name must not throw an exception (spec)");
    assertNotNull(con.getWarnings());
  }

Are you interested in contributing a fix to OpenRewrite?

I'm not sure it is a misconfiguration or a bug in OpenRewrite

timtebeek commented 8 months ago

Seems like a good improvement indeed to potentially remove that exception class import. We have a method we can call from the recipe called maybeRemoveImport just for this, which will only remove the import if it's no longer used. Could be a good first contribution for anyone interested to add that.

knutwannheden commented 8 months ago

@timtebeek Missed the good first issue label you just added.

timtebeek commented 8 months ago

@timtebeek Missed the good first issue label you just added.

All good; glad to see it was indeed that simple. :)

knutwannheden commented 8 months ago

All good; glad to see it was indeed that simple. :)

I think the JavaType.MultiCatch would have been easy to miss.