Members declared in final classes should not be protected

[quaff]

Since final classes cannot be inherited, marking the method as protected may be confusing. It is better to declare such members as private or package-visible instead.

Reported by IDEA, I tried to write ArchRule to prohibit such usage, but it's difficult to test methods are overriding super class methods.
Feel free to close if it's not worthy.

[nosan]

If I'm not mistaken, the following rules should help automate the process to some extent.

static ArchRule allFieldsThatHaveProtectedModifierMustNotBeDeclaredInFinalClass() {
		return ArchRuleDefinition.noFields()
			.that()
			.areProtected()
			.should()
			.beDeclaredInClassesThat(isFinalClass())
			.allowEmptyShould(true);
	}

	static ArchRule allMethodsThatHaveProtectedModifierMustNotBeDeclaredInFinalClass() {
		return ArchRuleDefinition.noMethods()
			.that()
			.areProtected()
			.and(doesNotOverrideSuperclassMethod())
			.should()
			.beDeclaredInClassesThat(isFinalClass())
			.allowEmptyShould(true);
	}

	static ArchRule allConstructorsThatHaveProtectedModifierMustNotBeDeclaredInFinalClass() {
		return ArchRuleDefinition.noConstructors()
			.that()
			.areProtected()
			.and(areNotDefaultConstructors())
			.should()
			.beDeclaredInClassesThat(isFinalClass())
			.allowEmptyShould(true);
	}

	private static DescribedPredicate<JavaMethod> doesNotOverrideSuperclassMethod() {
		return new DescribedPredicate<>("does not override a superclass method") {
			@Override
			public boolean test(JavaMethod method) {
				return method.getOwner()
					.getAllRawSuperclasses()
					.stream()
					.flatMap((superClass) -> superClass.getMethods().stream())
					.noneMatch((superMethod) -> isMethodOverridden(method, superMethod));
			}
		};
	}

	private static boolean isMethodOverridden(JavaMethod child, JavaMethod parent) {
		if (child.getOwner().equals(parent.getOwner())) {
			return false;
		}
		if (!child.getName().equals(parent.getName())) {
			return false;
		}
		if (!child.getRawParameterTypes().equals(parent.getRawParameterTypes())) {
			return false;
		}
		Set<JavaModifier> childModifiers = child.getModifiers();
		Set<JavaModifier> parentModifiers = parent.getModifiers();
		if (childModifiers.contains(JavaModifier.PRIVATE) || childModifiers.contains(JavaModifier.STATIC)) {
			return false;
		}
		if (parentModifiers.contains(JavaModifier.PRIVATE) || parentModifiers.contains(JavaModifier.STATIC)) {
			return false;
		}
		if (parentModifiers.contains(JavaModifier.PUBLIC)) {
			return childModifiers.contains(JavaModifier.PUBLIC);
		}
		if (parentModifiers.contains(JavaModifier.PROTECTED)) {
			return childModifiers.contains(JavaModifier.PROTECTED) || childModifiers.contains(JavaModifier.PUBLIC);
		}
		return child.getOwner().getPackageName().equals(parent.getOwner().getPackageName());
	}

	private static DescribedPredicate<? super JavaClass> isFinalClass() {
		return DescribedPredicate.describe("have FINAL modifier",
				(javaClass) -> javaClass.getModifiers().contains(JavaModifier.FINAL));
	}

[quaff]

@nosan Thanks, I didn't try your solution yet, but I think child.getRawParameterTypes().equals(parent.getRawParameterTypes()) it's not enough, it doesn't consider generic methods which make me stuck.

class A<T> {
  protected void test(T t);
}

final class B extends A<String> {
  @Override
  protected void test(String str) {
  }
}

[nosan]

Unfortunately, there's no easy way to do this because of type erasure and bridge methods.

I've come up with the following idea:

	static ArchRule allMethodsThatHaveProtectedModifierMustNotBeDeclaredInFinalClass() {
		return ArchRuleDefinition.noMethods()
			.that()
			.areProtected()
			.and(DescribedPredicate.not(isBridgeOrSyntheticMethod()))
			.and(DescribedPredicate.doNot(isOverridingMethodInSuperclass()))
			.should()
			.beDeclaredInClassesThat(isFinalClass())
			.allowEmptyShould(true);
	}

	private static DescribedPredicate<? super JavaMethod> isOverridingMethodInSuperclass() {
		return DescribedPredicate.describe("override method in super class", (method) -> {
			JavaClass owner = method.getOwner();
			for (JavaClass clazz : owner.getAllRawSuperclasses()) {
				if (doesOverrideMethodInClass(method, clazz)) {
					return true;
				}
			}
			return false;
		});
	}

	private static DescribedPredicate<JavaMethod> isBridgeOrSyntheticMethod() {
		return DescribedPredicate.describe("bridge or synthetic",
				(method) -> method.getModifiers().contains(JavaModifier.BRIDGE)
						|| method.getModifiers().contains(JavaModifier.SYNTHETIC));
	}

	private static boolean doesOverrideMethodInClass(JavaMethod candidate, JavaClass clazz) {
		if (candidate.getOwner().equals(clazz)) {
			return false;
		}
		if (candidate.getModifiers().contains(JavaModifier.PRIVATE)
				|| candidate.getModifiers().contains(JavaModifier.STATIC)) {
			return false;
		}
		Optional<JavaMethod> tryGetMethod = clazz.tryGetMethod(candidate.getName(),
				candidate.getRawParameterTypes().stream().map(JavaClass::getName).toArray(String[]::new));
		if (tryGetMethod.isPresent()) {
			JavaMethod method = tryGetMethod.get();
			if (method.getModifiers().contains(JavaModifier.PRIVATE)
					|| method.getModifiers().contains(JavaModifier.STATIC)) {
				return false;
			}
			if (method.getModifiers().contains(JavaModifier.PUBLIC)) {
				return candidate.getModifiers().contains(JavaModifier.PUBLIC);
			}
			if (method.getModifiers().contains(JavaModifier.PROTECTED)) {
				return candidate.getModifiers().contains(JavaModifier.PUBLIC)
						|| candidate.getModifiers().contains(JavaModifier.PROTECTED);
			}
			return candidate.getOwner().getPackageName().equals(clazz.getPackageName());
		}
		for (JavaMethod method : candidate.getOwner().getMethods()) {
			if (isMethodBridgeFor(method, candidate)) {
				return doesOverrideMethodInClass(method, clazz);
			}
		}
		return false;
	}

	private static boolean isMethodBridgeFor(JavaMethod method, JavaMethod candidate) {
		if (!method.getName().equals(candidate.getName())) {
			return false;
		}
		if (!method.getModifiers().contains(JavaModifier.BRIDGE)) {
			return false;
		}
		if (method.getParameters().size() != candidate.getParameters().size()) {
			return false;
		}
		return method.getCallsFromSelf()
			.stream()
			.map(JavaAccess::getTarget)
			.map(AccessTarget::resolveMember)
			.filter(Optional::isPresent)
			.map(Optional::get)
			.anyMatch(candidate::equals);
	}
	

All rules are here

[wilkinsona]

I think we could simplify things by relying on @Override being present. If it's there, the method's allowed to be protected. If it's not, we'd enforce at most package-private visibility.

[nosan]

I first thought about the @Override annotation. However, it has @Retention(RetentionPolicy.SOURCE), which means it is not available in the .class file.