Remove fully qualified JavaDoc references from TypesInUse for RemoveUnusedImports#5738
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
|
seems consistent: |
@Nested
@Issue("https://github.com/openrewrite/rewrite/pull/5738")
class removeUnusedImportsDoc {
@Test
void usedInDoc() {
rewriteRun(
java(
"""
import java.util.Date;
import java.util.List;
/**
* referencing {@link Date} only in doc
*/
class Test {
List list;
}
"""
)
);
}
@Test
void usedInDocFullQualified() {
rewriteRun(
java(
"""
import java.util.Date;
import java.util.List;
/**
* referencing {@link java.util.Date} only in doc
*/
class Test2 {
List list;
}
""",
"""
import java.util.List;
/**
* referencing {@link java.util.Date} only in doc
*/
class Test2 {
List list;
}
"""
)
);
}
}still the case seem to be fixed, but i came across another issue, not able to fix the impl. Feel free to incorporate. |
TypesInUse for RemoveUnusedImports
| // Fully qualified Javadoc references are _using_ those types as much as they are just references; | ||
| // TypesInUse also determines what imports are retained, and for fully qualified these can be dropped | ||
| return cursor.getValue() instanceof J.FieldAccess && | ||
| cursor.getPathAsStream().anyMatch(o -> o instanceof Javadoc.Reference); |
There was a problem hiding this comment.
Since this code is a bit performance critical, I think we may want to optimize this, so that we also have other parent types, which break the cursor walking. Can we perhaps also break on J.Block?
Also, what if we have a reference to a nested type, like Outer.Inner? It seems like the Outer import would still be required.
There was a problem hiding this comment.
Good point, thanks! Added a first loop for the J.Block; haven't yet explored the Outer.Inner, but worst case there we would not remove a stray import, which I'd consider to be fairly niche and perhaps not worth handling separately here.
|
no going to finish this, feel free to continue. Thanks. |
| Iterator<Object> path = cursor.getPath(); | ||
| while (path.hasNext()) { |
There was a problem hiding this comment.
Capturing a note from Slack: some performance concerns here due to the use of cursor.getPath().
There was a problem hiding this comment.
Discussed again; consensus was it looks ok now with the early return for blocks.
`TypesInUse` deliberately excludes fully qualified Javadoc references from its import-retention set (#5738), since such references need no import. `ChangePackage` therefore carried its own LST walk to rediscover exactly those references for its precondition — duplicating the "is this a fully qualified Javadoc reference" predicate, which is how the regression fixed in #7284 originally crept in. Capture the packages of those references in a separate transient `javadocPackages` bucket during the single `FindTypesInUse` walk, and have `ChangePackage` query it via `hasJavadocReferenceInPackage`. Only the package is retained, since that is all package-renaming recipes ask. The import-retention set is unchanged, so `RemoveUnusedImports`/`UsesType`/the FQN trie behave exactly as before. The bucket is not serialized; `TypesInUse.of` leaves it empty.
`TypesInUse` deliberately excludes fully qualified documentation-comment
references (Javadoc `{@link ...}`, and the equivalent in other languages
that share the `JavaType` attribution) from its import-retention set
(#5738), since such references need no import. `ChangePackage` therefore
carried its own LST walk to rediscover exactly those references for its
precondition — duplicating the "is this a fully qualified doc reference"
predicate, which is how the regression fixed in #7284 originally crept in.
Capture the packages of those references in a separate transient
`docReferencePackages` bucket during the single `FindTypesInUse` walk, and
have `ChangePackage` query it via `hasDocReferenceInPackage`. Only the
package is retained, since that is all package-renaming recipes ask. The
import-retention set is unchanged, so `RemoveUnusedImports`/`UsesType`/the
FQN trie behave exactly as before. The bucket is not serialized
(`TypesInUse.of` leaves it empty) and is queried only through
`hasDocReferenceInPackage`; the raw set is not exposed.
`TypesInUse` deliberately excludes fully qualified documentation-comment
references (Javadoc `{@link ...}`, and the equivalent in other languages
that share the `JavaType` attribution) from its import-retention set
(#5738), since such references need no import. `ChangePackage` therefore
carried its own LST walk to rediscover exactly those references for its
precondition — duplicating the "is this a fully qualified doc reference"
predicate, which is how the regression fixed in #7284 originally crept in.
Capture the packages of those references in a separate transient
`docReferencePackages` bucket during the single `FindTypesInUse` walk, and
have `ChangePackage` query it via `hasDocReferenceInPackage`. Only the
package is retained, since that is all package-renaming recipes ask. The
import-retention set is unchanged, so `RemoveUnusedImports`/`UsesType`/the
FQN trie behave exactly as before. The bucket is not serialized
(`TypesInUse.of` leaves it empty) and is queried only through
`hasDocReferenceInPackage`; the raw set is not exposed.
What's changed?
rewritecapability junit-team/junit-framework#4708What's your motivation?
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