JavaVisitors should visit modifiers

[AlexanderSkrock]

What problem are you trying to solve?

While solving #2838 I realized that modifiers are currently not properly visited. In consequence I can neither react to approached modifiers by marking nor can I discover annotations on these modifiers as we stop visiting on modifier-level.

To further illustrate, the following test case shows how modifiers are not visited properly. A testcase exactly for "not visiting" modifiers was not possible as the method does not exist yet, but here we try to visit the annotation which is a sub element of our modifier.

  @Test
  @ExpectedToFail("https://github.com/openrewrite/rewrite/issues/3502")
  void shouldVisitAnnotationWhenBoundToModifier() {
      rewriteRun(
        spec -> spec
          .executionContext(new InMemoryExecutionContext())
          .recipe(toRecipe(() -> new JavaIsoVisitor<>() {
              @Override
              public J.Annotation visitAnnotation(final J.Annotation a, final ExecutionContext executionContext) {
                  J.Annotation annotation = super.visitAnnotation(a, executionContext);
                  if (annotation.getComments().isEmpty()) {
                      annotation = annotation.withComments(List.of(new TextComment(true, "I was visited!", "", Markers.EMPTY)));
                  }
                  return annotation;
              }
          })),
        java(
          """
            public @Deprecated final class A {
            }
            """,
          """
            public /*I was visited!*/@Deprecated final class A {
            }
            """
        )
      );
  }

Describe the solution you'd like

1. We should extend the JavaVisitor with a method public J visitModifier(J.Modifier modifier, P p). Also, this should delegate two visit child elements such as annotations.

public J visitModifier(J.Modifier modifier, P p) {
    J.Modifier m = modifier;
    m = m.withPrefix(visitSpace(m.getPrefix(), Space.Location.MODIFIER_PREFIX, p));
    m = m.withMarkers(visitMarkers(m.getMarkers(), p));
    m = m.withAnnotations(ListUtils.map(m.getAnnotations(), a -> visitAndCast(a, p)));
    return m;
}

2. Then any other visit function that works with modifiers, hopefully already calls visit, but we should check.

Have you considered any alternatives or workarounds?

They are translated into J.Modifier but as they are not visited separately there is no work around.

Additional context

This one is necessary to solve the last few issues with annotations on modifiers in #2838

[timtebeek]

Good to see you're back @AlexanderSkrock ; hope all is well. I'll tag @knutwannheden for review; typically we're very careful adding in new API surface, but we can discuss what you need and how best to approach that.

[AlexanderSkrock]

Thank you for that! I totally understand.

I added both a test case and a possible default implementation for visiting modifiers. Looking forward to hear your thoughts!

[knutwannheden]

We recently closed a very similar PR because we concluded that we didn't need it:

• Change J.Modifier to be visitable by JavaVisitor #3467

@sambsnyd What are your thoughts on the use case described here?

[AlexanderSkrock]

I would like to add that visiting modifiers would be an enhancement but the fact, we do not deeply visit child elements is rather a bug as we simply do not detect and visit occurences of annotations. For various recipes this could end in invalid results or non-compilable code. Though, I have to admit that usage of annotation on modifiers is rarely seen.

[traceyyoshima]

Quick note:
J.Modifiers are technically visited already, so it is possible to update a recipe to visit the annotations like so:

@Override
public @Nullable J visit(@Nullable Tree tree, ExecutionContext executionContext) {
    if (tree instanceof J.Modifier modifier) {
        return modifier.withAnnotations(ListUtils.map(modifier.getAnnotations(), it -> visitAndCast(it, executionContext)));
    }
    return super.visit(tree, executionContext);
}

[Tarmean]

I ended up adding

	public J.MethodDeclaration visitMethodDeclaration(J.MethodDeclaration method, ExecutionContext context) {
		method = method.withModifiers(ListUtils.map(method.getModifiers(), mods -> mods.withAnnotations(ListUtils.map(mods.getAnnotations(),anno -> visitAnnotation(anno, context)))));
		return super.visitMethodDeclaration(method, context);
	}

to a visitor to make sure the annotations get visited. Any chance a similar step could be added to the the visitMethod and visitClass definition in JavaVisitor, or as above the visit method?

[timtebeek]

• Fixed in Add visitModifier() to base JavaVisitor #4562

Thanks all for the helpful context, suggestions and patience!