Don't remove referenced annotations from record parameters

[mvitz]

What's changed?

What's your motivation?

• This fixes RemoveUnusedImports removes used imports for annotated method parameters #4473

Anything in particular you'd like reviewers to focus on?

Anyone you would like to review specifically?

Have you considered any alternatives or workarounds?

Any additional context

Checklist

• I've added unit tests to cover both positive and negative cases
• I've read and applied the recipe conventions and best practices
• I've used the IntelliJ IDEA auto-formatter on affected files

[mvitz]

Currently, the test fails with:

    java.lang.AssertionError: Source file was parsed into an LST that contains non-whitespace characters in its whitespace. This is indicative of a bug in the parser.
    import java.beans.BeanProperty;
    record A(~~(non-whitespace)~~>@BeanProperty <~~String a) {}
        at org.openrewrite.test.RewriteTest.rewriteRun(RewriteTest.java:323)
        at org.openrewrite.test.RewriteTest.rewriteRun(RewriteTest.java:131)
        at org.openrewrite.test.RewriteTest.rewriteRun(RewriteTest.java:126)
        at org.openrewrite.java.RemoveUnusedImportsTest.doesNotRemoveReferencedAnnotationFromRecordParameter(RemoveUnusedImportsTest.java:1821)```

[timtebeek]

Thanks for getting this reduced and started @mvitz ! Looks like an oversight in the parser after the addition of records, which would then cause the annotation to not be parsed as such, but as "whitespace". The fix is then likely in ReloadableJava17ParserVisitor.

[mvitz]

Thanks for the hint.
When debugging I see that visitClass is called but there the record is already presented as a class with a constructor.
Unfortunately, at that point the annotation of the parameter is gone, and I didn't find any other visitor callback to see the original record.

[timtebeek]

Hmm; thanks for diving in! It looks like the annotation is still present at this point in the parsing:

rewrite/rewrite-java-17/src/main/java/org/openrewrite/java/isolated/ReloadableJava17Parser.java

Lines 193 to 194 in a98d83d

	JCTree.JCCompilationUnit jcCompilationUnit = compiler.parse(new ReloadableJava17ParserInputFileObject(input1, ctx));
	cus.put(input1, jcCompilationUnit);

But it's gone when we exit that org.openrewrite.java.isolated.ReloadableJava17Parser#parseInputsToCompilerAst 🤔

[mvitz]

Sorry, my fault. I added a more accurate testcase where the annotation contains PARAMETER and RECORD_COMPONENT as @Target and now the annotation is preserved on the generated constructor parameter and can be seen from ReloadableJava17ParserVisitor.

[mvitz]

Note to myself, if I change Lines 411 to 415 into

                if (member instanceof JCMethodDecl m) {
                    if (hasFlag(m.getModifiers(), Flags.GENERATEDCONSTR)) {
                        for (Tree param : m.getParameters()) {
                            stateVector.add(param);
                        }

The assertion failure is still there, but now marks another non-whitespace:

    java.lang.AssertionError: Source file was parsed into an LST that contains non-whitespace characters in its whitespace. This is indicative of a bug in the parser. 
    package b;
    import a.A;
    record B(~~(non-whitespace)~~>@A <~~~~(non-whitespace)~~>String <~~a) {}

Within private <J2 extends J> J2 convert(Tree t) { (Line 1667) the call to ((JCTree) t).getStartPosition() returns another value than cursor for this parameter and therefore the cursor is advanced, and the Annotation is skipped when the call to scan is done.
For Methods and their parameters (see visitMethod) the startPosition is equal to the cursor and therefore that annotations are preserved. However both are of type JCVariableDecl but the record constructor parameters contain an additional new line (\n) between the annotation and the type.

After thinking during the night and checking some examples today annotations on records opens a rabbit hole. All the following are valid annotations for a record component:

@Target({METHOD})
@Retention(RUNTIME)
public @interface MethodAnnotation {
}

@Target({PARAMETER})
@Retention(RUNTIME)
public @interface ParameterAnnotation {
}

@Target({FIELD})
@Retention(RUNTIME)
public @interface FieldAnnotation {
}

However, in the generated class (after compilation) they appear on different locations:

• MethodAnnotation appears at the generated accessor method.
• ParameterAnnotation appears at the corresponding argument within the generated record constructor.
• FieldAnnotation appears at the corresponding generated private final field.

Therefore, the first initial example with record B(@BeanProperty String a) is a valid case and the import for that annotation should not be removed. However, that would mean that this annotation needs to be detected by looking at the generated accessor method...

[timtebeek]

Thanks for the detailed look! Problems sure have a way of looking deceptively simple don't they? 🙃
Let me tag @knutwannheden here as he might have some ideas too, even if he's caught up with other work at the moment.