Skip to content

Conversation

@opencirclesolutions-wove
Copy link

@opencirclesolutions-wove opencirclesolutions-wove commented Mar 21, 2025

What's changed?

Added deeper scanning into RemoveUnusedImports

What's your motivation?

I’ve forked the class and made some improvements for them and wanted to provide feedback of those improvements.
These improvements were made because the recipe was throwing away a lot of imports that were being used for our use case.

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

I’m fairly new to OpenRewrite so I’m not 100% sure that my changes are in line with the best practices. But I tried. Feel free to improve on my code.

Have you considered any alternatives or workarounds?

Use my own cloned class?

Any additional context

I’ve disabled one unit test because it gave a parser error. The same unit test in my project using an older rewrite version works fine. It may help down track where the error may come from.

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

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
timtebeek and others added 3 commits March 26, 2025 13:40
…sedImportsTest.java

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@timtebeek timtebeek added bug Something isn't working enhancement New feature or request labels Apr 18, 2025
Comment on lines +369 to +384
private static Set<J.Annotation> findAllAnnotations(J.CompilationUnit cu) {
Set<J.Annotation> annotations = new HashSet<>();
cu.getClasses().stream()
.map(cl -> cl.getLeadingAnnotations())
.flatMap(List::stream)
.forEach(annotations::add);
cu.getTypesInUse().getTypesInUse().stream().forEach(type -> {
if (type instanceof JavaType.Class) {
JavaType.Class aClass = (JavaType.Class) type;
if (aClass.getKind() == JavaType.FullyQualified.Kind.Annotation) {
FindAnnotations.find(cu, aClass.getFullyQualifiedName()).forEach(annotations::add);
}
}
});
return annotations;
}
Copy link
Member

Choose a reason for hiding this comment

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

I'd have expected something like the following, instead of all the scan methods below; would that be possible here?

Set<J.Annotation> annotations = new JavaIsoVisitor<Set<J.Annotation> >() {
    public J.Annotation visitAnnotations(J.Annotations ann, Set<J.Annotation> set) {
        set.add(ann);
        return ann;
    }
}.reduce(cu, new HashSet<>());

@timtebeek
Copy link
Member

Fixed a couple issues with the recipe yesterday

I've now reran the tests you added here without any of the recipe changes, and that means now only this test fails:

    @Test
    void lombokValInLambda() {
        rewriteRun(
          java(
            """
            import lombok.val;
            import java.util.List;
            import java.util.ArrayList;
            import java.util.stream.Collectors;
            
            public class Foo {
                List<String> names = new ArrayList<>();
                public String method() {
                    return names.stream()
                    .map(n -> {
                        val name = "";
                        return n + name;
                    })
                    .collect(Collectors.joining(""));
                }
            }
            """
          )
        );
    }

I don't think we'd need all of the changes you've added here just to support that case, and given the conflicts we see here I'm going to close this PR for now. You're welcome to propose a separate fix for that one failing case still. Thanks for helping out!

@timtebeek timtebeek closed this Jul 27, 2025
@github-project-automation github-project-automation bot moved this from In Progress to Done in OpenRewrite Jul 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working enhancement New feature or request

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

2 participants