Skip to content

Omit migration to @Setter annotated field of other annotations are present #672

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

Merged
merged 4 commits into from
Feb 12, 2025

Conversation

MBoegers
Copy link
Contributor

@MBoegers MBoegers commented Feb 11, 2025

What's changed?

What's your motivation?

prevent breaking changes

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

Comment on lines 160 to 162
List<J.Annotation> annotations = method.getLeadingAnnotations();
return !annotations.isEmpty() &&
!(annotations.size() == 1 && overwriteMatcher.matches(annotations.get(0)));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Best to use the annotation service here as there might also be annotations on the return type for instance. Leading annotations only has a limited view on annotations.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't have a cursor here, it is a util outside a recipe.
2 options I see:

  1. accept and note for final fix
  2. move the check into recipe as additional check

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As LombokUtils is private it should be possible to add a Cursor parameter, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactored it in, I don't know if it is the right thing to do considering it's just a fix not a final solution ;)

@MBoegers MBoegers self-assigned this Feb 12, 2025
@MBoegers
Copy link
Contributor Author

@timtebeek @knutwannheden The CI is failing because of missing type information at @AllArgsConstructor annotation. May this be a symtom of the changes at Lombok support?

MigrateCollectionsSingletonListTest > lombokAllArgsConstructor() FAILED
    java.lang.IllegalStateException: LST contains missing or invalid type information
    NewClass->EnumValue->EnumValueSet->Block->ClassDeclaration->Block->ClassDeclaration->CompilationUnit
    /*~~(NewClass type is missing or malformed)~~>*/new(singletonList(FOO))

@timtebeek
Copy link
Contributor

Indeed that's due to Lombok changes in openrewrite/rewrite. I hadn't yet made adjustments here as it seems we're close to a fix upstream. 🙏🏻

Copy link
Contributor

@timtebeek timtebeek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great to see, thanks! I've added some slight polish: we tend to retrieve the AnnotationService through a service method defined on the visitor, to prevent repeated instantiation and to allow for instance the Kotlin visitor to swap in an alternative implementation.

@timtebeek timtebeek merged commit b741117 into main Feb 12, 2025
2 checks passed
@timtebeek timtebeek deleted the lombok-breakes-setter branch February 12, 2025 11:59
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
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants