`org.openrewrite.staticanalysis.FinalClass` is wrong for nested sub classes

[Bananeweizen]

org.openrewrite.staticanalysis.FinalClass works on wrong assumptions. A non-final class with no public constructor can have nested static sub classes, and in that case it cannot be made final.

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

public class Reproducer {

    private Reproducer(final String name) {
        //...
    }

    public static final class Sub extends Reproducer {

        public Sub() {
            super("subclass");
        }
    }
}

What did you expect to see?

no change

What did you see instead?

final added to the top level class

Are you interested in contributing a fix to OpenRewrite?

Maybe. Have to look for how to get the information that there are nested sub classes.

[timtebeek]

Thanks for reporting this variant! To detect sub classes you can either expand this loop https://github.com/openrewrite/rewrite-static-analysis/blob/2163f0f05dd6fe7f4762e3486c8f16a9a5787036/src/main/java/org/openrewrite/staticanalysis/FinalClassVisitor.java#L72-L83

Or add a new JavaIsoVisitor<>() { ... } that overrides visitClassDeclaration, and visit on the outer classDeclaration.getBody() and set an AtomicBoolean if any matching nested class declarations are found.

[github-actions[bot]]

This issue is stale because it has not had any activity for 60 days. Remove question label or comment or this will be closed in two weeks. Issues may be reopened when there is renewed interest.

[protocol7]

We're running into this with:

• org.openrewrite:rewrite-core:8.62.4
• org.openrewrite.recipe:rewrite-static-analysis:2.18.0

Here's a test case:

  @Test
  void dontFinalClassWithNestedInheritedClasses() {
    rewriteRun(
        spec -> spec.recipe(new FinalClass()),
        // language=java
        java(
            """
             public class Foo<T> {
                  private Foo() {}
            
                  public static final class Bar extends Foo<String> {}
              }
            """));
  }

org.opentest4j.AssertionFailedError: [Expected recipe to complete in 0 cycles, but took at least one more cycle. Between the last two executed cycles there were changes to "Foo.java"]
expected:
  "public class Foo<T> {
       private Foo() {}

       public static final class Bar extends Foo<String> {}
   }"
 but was:
  "public final class Foo<T> {
       private Foo() {}

       public static final class Bar extends Foo<String> {}
   }"
	at org.openrewrite.test.LargeSourceSetCheckingExpectedCycles.afterCycle(LargeSourceSetCheckingExpectedCycles.java:97)
	at org.openrewrite.RecipeScheduler.runRecipeCycles(RecipeScheduler.java:95)
	at org.openrewrite.RecipeScheduler.scheduleRun(RecipeScheduler.java:41)
	at org.openrewrite.Recipe.run(Recipe.java:442)
	at org.openrewrite.test.RewriteTest.rewriteRun(RewriteTest.java:382)
	at org.openrewrite.test.RewriteTest.rewriteRun(RewriteTest.java:130)

[timtebeek]

Thanks both! Fixed just now in

• #742

[protocol7]

Thanks!

[protocol7]

Just verified that we are still seeing this issue, with the above test case. This is with:

• org.openrewrite:rewrite-core:8.63.3
• org.openrewrite.recipe:rewrite-static-analysis:2.19.0

Has this not yet made it into a release, or is there something off with the fix?

[timtebeek]

The associated fix made it into 2.19.0, with the test added in https://github.com/openrewrite/rewrite-static-analysis/commit/6d6fe5e495330d851e1c8257270263337b994a02 Is there any change you can make to the test to reproduce the issue you're seeing?

[protocol7]

I did some testing and it comes down to a difference in the tests. In my test above, the subclass is final, which still triggers the bug, in your test case it is not.

[timtebeek]

That helps, thanks! I've pushed up another change in https://github.com/openrewrite/rewrite-static-analysis/commit/c195a4ff79d01c7b147eab35f49fbf211c00b3a3