-
Notifications
You must be signed in to change notification settings - Fork 2
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
Extend UnnecessarilyFullyQualified to remove java.lang
qualifiers
#2
Conversation
d0f336f
to
f3542a4
Compare
f3542a4
to
cd5be4b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rebased and added a commit. I think we can open an upstream PR for this one. Suggested PR/commit message:
UnnecessarilyFullyQualified: drop `java.lang` qualifiers
@@ -61,6 +61,7 @@ | |||
public final class UnnecessarilyFullyQualified extends BugChecker | |||
implements CompilationUnitTreeMatcher { | |||
|
|||
private static final String JAVA_LANG = "java.lang"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While no such package currently exist, we shouldn't also match a hypothetical java.langSomeSuffix
:
private static final String JAVA_LANG = "java.lang"; | |
private static final String JAVA_LANG = "java.lang."; |
if (!pathsToFix.get(0).getLeaf().toString().contains(JAVA_LANG) | ||
&& pathsToFix.stream() | ||
.anyMatch(path -> findIdent(nameString, state.withPath(path), VAL_TYP) != null)) { | ||
continue; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should to special-case java.lang
here. Instead we should generalize the anyMatch
logic and compare any matching ident again the type symbol under consideration; if they're the same we can proceed.
if (!pathsToFix.get(0).getLeaf().toString().contains(JAVA_LANG) | |
&& pathsToFix.stream() | |
.anyMatch(path -> findIdent(nameString, state.withPath(path), VAL_TYP) != null)) { | |
continue; | |
} | |
if (pathsToFix.stream() | |
.anyMatch( | |
path -> { | |
Symbol ident = findIdent(nameString, state.withPath(path), VAL_TYP); | |
return ident != null && !ident.equals(type); | |
})) { | |
continue; | |
} |
continue; | ||
} | ||
SuggestedFix.Builder fixBuilder = SuggestedFix.builder(); | ||
fixBuilder.addImport(getOnlyElement(types.keySet()).getQualifiedName().toString()); | ||
String newImport = getOnlyElement(types.keySet()).getQualifiedName().toString(); | ||
if (!newImport.contains(JAVA_LANG)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (!newImport.contains(JAVA_LANG)) { | |
if (!newImport.startsWith(JAVA_LANG)) { |
cd5be4b
to
9a06dc8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generalized the fix to cover additional cases. New suggested commit message:
UnnecessarilyFullyQualified: handle `java.lang` and inconsistent import usages
43d7f86
to
b368b93
Compare
b368b93
to
80dc087
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Slightly extended suggested commit message:
UnnecessarilyFullyQualified: handle `java.lang` and inconsistent import usages
This introduces two improvements:
- Where possible `java.lang.SomeType` references are simplified to `SomeType`
without introducing an associated `import` statement.
- References to `some.package.SomeType` are now also replaced with `SomeType`
if said type is already imported and referenced by its simple name elsewhere
in the compilation unit.
d70dd0f
to
8051af2
Compare
Opened PR upstream: google#2776 |
8051af2
to
1dae0db
Compare
Closing this PR as it got merged in: google@c07a7a9 |
No description provided.