-
Notifications
You must be signed in to change notification settings - Fork 135
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
Implement FinalClass
error prone check, replacing the checkstyle implementation
#1008
Changes from all commits
46463aa
84986ed
c04eadb
ef89c4b
a618f10
0097104
c28a3ac
38a99cf
3dc12c1
d8242ee
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,138 @@ | ||
/* | ||
* (c) Copyright 2019 Palantir Technologies Inc. All rights reserved. | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); | ||
* you may not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ | ||
|
||
package com.palantir.baseline.errorprone; | ||
|
||
import com.google.auto.service.AutoService; | ||
import com.google.errorprone.BugPattern; | ||
import com.google.errorprone.VisitorState; | ||
import com.google.errorprone.bugpatterns.BugChecker; | ||
import com.google.errorprone.fixes.SuggestedFix; | ||
import com.google.errorprone.fixes.SuggestedFixes; | ||
import com.google.errorprone.matchers.Description; | ||
import com.google.errorprone.util.ASTHelpers; | ||
import com.sun.source.tree.ClassTree; | ||
import com.sun.source.tree.MethodTree; | ||
import com.sun.source.tree.NewClassTree; | ||
import com.sun.source.tree.Tree; | ||
import com.sun.source.util.TreeScanner; | ||
import java.util.List; | ||
import java.util.Optional; | ||
import java.util.Set; | ||
import javax.lang.model.element.Modifier; | ||
|
||
@AutoService(BugChecker.class) | ||
@BugPattern( | ||
name = "FinalClass", | ||
// Support legacy suppressions from checkstyle | ||
altNames = {"checkstyle:finalclass", "checkstyle:FinalClass"}, | ||
link = "https://github.com/palantir/gradle-baseline#baseline-error-prone-checks", | ||
linkType = BugPattern.LinkType.CUSTOM, | ||
providesFix = BugPattern.ProvidesFix.REQUIRES_HUMAN_ATTENTION, | ||
severity = BugPattern.SeverityLevel.ERROR, | ||
summary = "A class should be declared final if all of its constructors are private. Utility classes -- " | ||
+ "i.e., classes all of whose methods and fields are static -- have a private, empty, " | ||
+ "zero-argument constructor.\n" | ||
+ "https://github.com/palantir/gradle-baseline/tree/develop/docs/best-practices/" | ||
+ "java-coding-guidelines#private-constructors") | ||
public final class FinalClass extends BugChecker implements BugChecker.ClassTreeMatcher { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we may want to consider checking/cleaning up all of the methods in the class to avoid inadvertently causing method modifiers to become redundant There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like that, remove final from methods that inherit finality from the class. Error prone doesn't have an unnecessary modifier check afaik, thoughts on implementing that separately? Counterargument is that it must be applied in another round after this check succeeds, but I like the separation of validation, and the ability to fix existing instances that aren't created by this check. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would be nice to have both. A generic redundant modifier check and then a special cleanup on this check to specifically remove There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Implemented the fix, four hits in large internal ;-) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: We can't remove the |
||
|
||
@Override | ||
public Description matchClass(ClassTree tree, VisitorState state) { | ||
if (tree.getKind() != Tree.Kind.CLASS) { | ||
// Don't apply to out interfaces and enums | ||
return Description.NO_MATCH; | ||
} | ||
Set<Modifier> classModifiers = tree.getModifiers().getFlags(); | ||
if (classModifiers.contains(Modifier.FINAL) | ||
|| classModifiers.contains(Modifier.ABSTRACT)) { | ||
// Already final, nothing to check | ||
return Description.NO_MATCH; | ||
} | ||
List<MethodTree> constructors = ASTHelpers.getConstructors(tree); | ||
if (constructors.isEmpty()) { | ||
return Description.NO_MATCH; | ||
} | ||
for (MethodTree constructor : constructors) { | ||
if (!constructor.getModifiers().getFlags().contains(Modifier.PRIVATE)) { | ||
return Description.NO_MATCH; | ||
} | ||
} | ||
if (isClassExtendedInternally(tree, state)) { | ||
return Description.NO_MATCH; | ||
} | ||
return buildDescription(tree) | ||
.addFix(buildFix(tree, state)) | ||
.build(); | ||
} | ||
|
||
private static Optional<SuggestedFix> buildFix(ClassTree tree, VisitorState state) { | ||
return SuggestedFixes.addModifiers(tree, state, Modifier.FINAL) | ||
.map(fix -> { | ||
// Remove redundant 'final' methods modifers that are no longer necessary. | ||
SuggestedFix.Builder builder = SuggestedFix.builder().merge(fix); | ||
tree.getMembers().stream() | ||
.filter(member -> member instanceof MethodTree) | ||
.map(MethodTree.class::cast) | ||
.filter(methodTree -> methodTree.getModifiers().getFlags().contains(Modifier.FINAL) | ||
// static final is redundant, however outside of the scope of this check | ||
// to modify. | ||
&& !methodTree.getModifiers().getFlags().contains(Modifier.STATIC)) | ||
.forEach(methodTree -> SuggestedFixes.removeModifiers(methodTree, state, Modifier.FINAL) | ||
.ifPresent(builder::merge)); | ||
return builder.build(); | ||
}); | ||
} | ||
|
||
private static boolean isClassExtendedInternally(ClassTree tree, VisitorState state) { | ||
// Encapsulated classes can be extended by other nested classes, even if they have private constructors. | ||
// In these cases we mustn't fail validation or suggest a final modifier. | ||
for (Tree typeDeclaration : state.getPath().getCompilationUnit().getTypeDecls()) { | ||
Boolean maybeResult = typeDeclaration.accept(new TreeScanner<Boolean, Void>() { | ||
|
||
@Override | ||
public Boolean reduce(Boolean lhs, Boolean rhs) { | ||
// Fail if any class extends 'tree' | ||
return Boolean.TRUE.equals(lhs) || Boolean.TRUE.equals(rhs); | ||
} | ||
|
||
@Override | ||
public Boolean visitClass(ClassTree classTree, Void attachment) { | ||
Tree extendsClause = classTree.getExtendsClause(); | ||
if (extendsClause != null && ASTHelpers.isSameType( | ||
ASTHelpers.getType(tree), ASTHelpers.getType(extendsClause), state)) { | ||
return true; | ||
} | ||
return super.visitClass(classTree, attachment); | ||
} | ||
|
||
@Override | ||
public Boolean visitNewClass(NewClassTree newClassTree, Void attachment) { | ||
if (newClassTree.getClassBody() != null && ASTHelpers.isSameType( | ||
ASTHelpers.getType(tree), ASTHelpers.getType(newClassTree.getIdentifier()), state)) { | ||
return true; | ||
} | ||
return super.visitNewClass(newClassTree, attachment); | ||
} | ||
}, null); | ||
// Unfortunately TreeScanner doesn't provide a way to set a default value, so we must account for null. | ||
if (Boolean.TRUE.equals(maybeResult)) { | ||
return true; | ||
} | ||
} | ||
return false; | ||
} | ||
} |
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 used ERROR here to match the severity of the checkstyle rule. We can reduce the severity if we decide to keep the checkstyle rule around.