Add method visibility checkstyle rule

closes gh-128

Author: philwebb

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

@@ -0,0 +1,85 @@
+/*
+ * Copyright 2017-2019 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.
+ */
+
+package io.spring.javaformat.checkstyle.check;
+
+import com.puppycrawl.tools.checkstyle.api.DetailAST;
+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}.
+ *
+ * @author Phillip Webb
+ */
+public class SpringMethodVisibilityCheck extends AbstractSpringCheck {
+
+	@Override
+	public int[] getAcceptableTokens() {
+		return new int[] { TokenTypes.METHOD_DEF };
+	}
+
+	@Override
+	public void visitToken(DetailAST ast) {
+		DetailAST modifiers = ast.findFirstToken(TokenTypes.MODIFIERS);
+		if (modifiers.findFirstToken(TokenTypes.LITERAL_PUBLIC) != null) {
+			visitPublicMethod(modifiers, ast);
+		}
+	}
+
+	private void visitPublicMethod(DetailAST modifiers, DetailAST method) {
+		if (hasOverrideAnnotation(modifiers)) {
+			return;
+		}
+		DetailAST classDef = getClassDef(method.getParent());
+		if (classDef == null) {
+			return;
+		}
+		DetailAST classModifiers = classDef.findFirstToken(TokenTypes.MODIFIERS);
+		if (classModifiers.findFirstToken(TokenTypes.LITERAL_PUBLIC) != null
+				|| classModifiers.findFirstToken(TokenTypes.LITERAL_PROTECTED) != null) {
+			return;
+		}
+		DetailAST ident = method.findFirstToken(TokenTypes.IDENT);
+		log(ident.getLineNo(), ident.getColumnNo(), "methodvisibility.publicMethod", ident.getText());
+	}
+
+	private boolean hasOverrideAnnotation(DetailAST modifiers) {
+		DetailAST candidate = modifiers.getFirstChild();
+		while (candidate != null) {
+			if (candidate.getType() == TokenTypes.ANNOTATION) {
+				DetailAST dot = candidate.findFirstToken(TokenTypes.DOT);
+				String name = (dot != null ? dot : candidate).findFirstToken(TokenTypes.IDENT).getText();
+				if ("Override".equals(name)) {
+					return true;
+				}
+			}
+			candidate = candidate.getNextSibling();
+		}
+		return false;
+	}
+
+	private DetailAST getClassDef(DetailAST ast) {
+		while (ast != null) {
+			if (ast.getType() == TokenTypes.CLASS_DEF) {
+				return ast;
+			}
+			ast = ast.getParent();
+		}
+		return null;
+	}
+
+}

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

@@ -14,3 +14,4 @@ junit5.lifecyclePublicMethod="Lifecycle method ''{0}'' should not be public."
 junit5.lifecyclePrivateMethod="Lifecycle method ''{0}'' should not be private."
 junit5.testPublicMethod="Test method ''{0}'' should not be public."
 junit5.testPrivateMethod="Test method ''{0}'' should not be private."
+methodvisibility.publicMethod="Method ''{0}'' in private class should not be public."

spring-javaformat/spring-javaformat-checkstyle/src/main/resources/io/spring/javaformat/checkstyle/spring-checkstyle.xml

@@ -169,5 +169,6 @@
 		<module name="io.spring.javaformat.checkstyle.check.SpringCatchCheck" />
 		<module name="io.spring.javaformat.checkstyle.check.SpringJavadocCheck" />
 		<module name="io.spring.javaformat.checkstyle.check.SpringMethodOrderCheck" />
+		<module name="io.spring.javaformat.checkstyle.check.SpringMethodVisibilityCheck" />
  	</module>
 </module>

spring-javaformat/spring-javaformat-checkstyle/src/test/java/io/spring/javaformat/checkstyle/SpringChecksTests.java

@@ -117,7 +117,7 @@ private void printDebugInfo(File file) throws CheckstyleException {
 
 	@Parameters(name = "{0}")
 	public static Collection<Parameter> files() throws IOException {
-		return Arrays.stream(SOURCES_DIR.list((dir, name) -> !name.startsWith("."))).map(Parameter::new)
+		return Arrays.stream(SOURCES_DIR.list((dir, name) -> !name.startsWith("."))).sorted().map(Parameter::new)
 				.collect(Collectors.toList());
 	}
 

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

@@ -0,0 +1,3 @@
++Method 'badPrivateInner' in private class should not be public.
++Method 'badDefaultInner' in private class should not be public.
++2 errors

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

@@ -0,0 +1,3 @@
++Method 'bad' in private class should not be public.
++Method 'badStatic' in private class should not be public.
++2 errors

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

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

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

@@ -0,0 +1,51 @@
+/*
+ * Copyright 2017-2019 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 of public method.
+ *
+ * @author Phillip Webb
+ */
+public class MethodVisibilityInnerClassesWithPublicMethod {
+
+	private static class PrivateInnerClass {
+
+		public void badPrivateInner() {
+		}
+
+	}
+
+	protected static class ProtectedInnerClass {
+
+		public void okProtectedInner() {
+		}
+
+	}
+
+	static class DefaultInnerClass {
+
+		public void badDefaultInner() {
+		}
+
+	}
+
+	public static class PublicInnerClass {
+
+		public void okPublicInner() {
+		}
+
+	}
+}

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

@@ -0,0 +1,33 @@
+/*
+ * Copyright 2017-2019 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 of public method.
+ *
+ * @author Phillip Webb
+ */
+class MethodVisibilityPackagePrivateWithPublicMethod {
+
+	MethodVisibilityPackagePrivateWithPublicMethod() {
+	}
+
+	public void bad() {
+	}
+
+	public static void badStatic() {
+	}
+
+}

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

@@ -0,0 +1,48 @@
+/*
+ * Copyright 2017-2019 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 of public method.
+ *
+ * @author Phillip Webb
+ */
+public class MethodVisibilityWithOverride {
+
+	private static class PrivateInnerClass {
+
+		@Override
+		public void okPrivateInner() {
+		}
+
+	}
+
+	protected static class ProtectedInnerClass {
+
+		@Override
+		public void okProtectedInner() {
+		}
+
+	}
+
+	static class DefaultInnerClass {
+
+		@Override
+		public void okDefaultInner() {
+		}
+
+	}
+
+}