Prohibit protected methods without @OverRide in final classes

Closes gh-466

Author: wilkinsona

spring-javaformat/spring-javaformat-checkstyle/src/main/java/io/spring/javaformat/checkstyle/check/SpringMethodVisibilityCheck.java

@@ -20,10 +20,17 @@
 import com.puppycrawl.tools.checkstyle.api.TokenTypes;
 
 /**
- * Checks that protected, package-private and private classes to not have public methods
- * unless they are also annotated with {@link Override @Override}.
+ * Check for compliance with Spring-style method visibility. Checks that:
+ *
+ * <ul>
+ * <li>package-private and private classes do not have public methods
+ * unless they are also annotated with {@link Override @Override}
+ * <li>final classes do not have protected methods unless that are also
+ * annotated with {@link Override @Override}
+ * </ul>
  *
  * @author Phillip Webb
+ * @author Andy Wilkinson
  */
 public class SpringMethodVisibilityCheck extends AbstractSpringCheck {
 
@@ -38,6 +45,9 @@ public void visitToken(DetailAST ast) {
 		if (modifiers.findFirstToken(TokenTypes.LITERAL_PUBLIC) != null) {
 			visitPublicMethod(modifiers, ast);
 		}
+		else if (modifiers.findFirstToken(TokenTypes.LITERAL_PROTECTED) != null) {
+			visitProtectedMethod(modifiers, ast);
+		}
 	}
 
 	private void visitPublicMethod(DetailAST modifiers, DetailAST method) {
@@ -56,6 +66,18 @@ private void visitPublicMethod(DetailAST modifiers, DetailAST method) {
 		log(ident.getLineNo(), ident.getColumnNo(), "methodvisibility.publicMethod", ident.getText());
 	}
 
+	private void visitProtectedMethod(DetailAST modifiers, DetailAST method) {
+		if (hasOverrideAnnotation(modifiers)) {
+			return;
+		}
+		DetailAST classDef = getClassDef(method.getParent());
+		if (classDef == null || !isFinal(classDef)) {
+			return;
+		}
+		DetailAST ident = method.findFirstToken(TokenTypes.IDENT);
+		log(ident.getLineNo(), ident.getColumnNo(), "methodvisibility.protectedMethodInFinalClass", ident.getText());
+	}
+
 	private boolean hasOverrideAnnotation(DetailAST modifiers) {
 		DetailAST candidate = modifiers.getFirstChild();
 		while (candidate != null) {
@@ -98,4 +120,12 @@ private boolean isPublicOrProtected(DetailAST ast) {
 				|| modifiers.findFirstToken(TokenTypes.LITERAL_PROTECTED) != null;
 	}
 
+	private boolean isFinal(DetailAST ast) {
+		DetailAST modifiers = ast.findFirstToken(TokenTypes.MODIFIERS);
+		if (modifiers == null) {
+			return false;
+		}
+		return modifiers.findFirstToken(TokenTypes.FINAL) != null;
+	}
+
 }

spring-javaformat/spring-javaformat-checkstyle/src/main/resources/io/spring/javaformat/checkstyle/check/messages.properties

@@ -31,6 +31,7 @@ lambda.unnecessaryBlock=Lambda block is unnecessary.
 lambda.unnecessaryParen=Lambda argument has unnecessary parentheses.
 methodorder.outOfOrder=Method ''{0}'' is out of order, expected {1}.
 methodvisibility.publicMethod=Method ''{0}'' in private class should not be public.
+methodvisibility.protectedMethodInFinalClass=Method ''{0}'' in final class should be package-private rather than protected.
 nothis.unexpected=Reference to instance variable ''{0}'' should not use \"this.\".
 nullability.bannedImport=Nullability should be expressed using JSpecify. Replace ''{0}'' with ''{1}''.
 nullability.annotationLocation=''{0}'' should only be used immediately before a type.

spring-javaformat/spring-javaformat-checkstyle/src/test/resources/check/MethodVisibilityFinalClassWithOverride.txt

@@ -0,0 +1 @@
++0 errors

spring-javaformat/spring-javaformat-checkstyle/src/test/resources/check/MethodVisibilityFinalClassWithProtectedMethod.txt

@@ -0,0 +1,2 @@
++Method 'bad' in final class should be package-private rather than protected.
++Method 'badStatic' in final class should be package-private rather than protected.

spring-javaformat/spring-javaformat-checkstyle/src/test/resources/check/MethodVisibilityProtectedWithPublicMethod.txt

@@ -0,0 +1 @@
++0 errors

spring-javaformat/spring-javaformat-checkstyle/src/test/resources/source/MethodVisibilityFinalClassWithOverride.java

@@ -0,0 +1,33 @@
+/*
+ * Copyright 2017-present the original author or authors.
+ *
+ * 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
+ *
+ *      https://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.
+ */
+
+/**
+ * Good visibility because, while class is final, protected methods are
+ * annotated with {@code @Override}.
+ *
+ * @author Andy Wilkinson
+ */
+public final class MethodVisibilityFinalClassWithOverride {
+
+	@Override
+	protected void good() {
+	}
+
+	@Override
+	protected static void goodStatic() {
+	}
+
+}

spring-javaformat/spring-javaformat-checkstyle/src/test/resources/source/MethodVisibilityFinalClassWithProtectedMethod.java

@@ -0,0 +1,30 @@
+/*
+ * Copyright 2017-present the original author or authors.
+ *
+ * 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
+ *
+ *      https://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.
+ */
+
+/**
+ * Bad visibility because class is final.
+ *
+ * @author Andy Wilkinson
+ */
+public final class MethodVisibilityFinalClassWithProtectedMethod {
+
+	protected void bad() {
+	}
+
+	protected static void badStatic() {
+	}
+
+}

spring-javaformat/spring-javaformat-checkstyle/src/test/resources/source/MethodVisibilityProtectedWithPublicMethod.java

@@ -0,0 +1,33 @@
+/*
+ * Copyright 2017-present the original author or authors.
+ *
+ * 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
+ *
+ *      https://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.
+ */
+
+/**
+ * Good visibility because class is protected.
+ *
+ * @author Phillip Webb
+ */
+protected class MethodVisibilityProtectedWithPublicMethod {
+
+	MethodVisibilityPackagePrivateWithPublicMethod() {
+	}
+
+	public void bad() {
+	}
+
+	public static void badStatic() {
+	}
+
+}

spring-javaformat/spring-javaformat-checkstyle/src/test/resources/source/MethodVisibilityWithOverride.java

@@ -15,7 +15,7 @@
  */
 
 /**
- * Bad visibility because of public method.
+ * Good visibility because of {@code @Override} annotation.
  *
  * @author Phillip Webb
  */