Skip to content
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

Merged
merged 10 commits into from
Nov 1, 2019
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,7 @@ Safe Logging can be found at [github.com/palantir/safe-logging](https://github.c
- `ThrowError`: Prefer throwing a RuntimeException rather than Error.
- `ReverseDnsLookup`: Calling address.getHostName may result in an unexpected DNS lookup.
- `ReadReturnValueIgnored`: The result of a read call must be checked to know if EOF has been reached or the expected number of bytes have been consumed.
- `FinalClass`: A class should be declared final if all of its constructors are private.

### Programmatic Application

Expand Down
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,
Copy link
Contributor Author

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.

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 {
Copy link
Contributor

Choose a reason for hiding this comment

The 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

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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 final from methods of classes that have been converted. That way we avoid having to run the fix twice to get to a good state and can catch other issues like public on interface methods

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Implemented the fix, four hits in large internal ;-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bug: We can't remove the final modifier from methods annotated with java.lang.SafeVarargs


@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;
}
}
Loading