Forbid annotations on supertypes - #1068
Conversation
|
@wmdietl Should we also forbid annotation on supertype's type parameter? For example nvm, looks like there is a reason we should allow annotation on supertype's type parameter. https://eisop.github.io/cf/manual/manual.html#annotations-are-a-contract |
wmdietl
left a comment
wmdietl
left a comment
There was a problem hiding this comment.
Adding a new checkSupertypeAnnotations in addition to the existing checkExtendsOrImplements seems very confusing. They both basically look at the same thing and the name does not make clear when to use which method.
There is no cross-reference between the methods that would help the user decide.
With the current code, checkSupertypeAnnotations forbids annotations in a location, but then checkExtendsOrImplements checks for a subtyping relation, which also is tricky to follow. Then TaintingVisitor overrides checkSupertypeAnnotations and says do nothing, which is confusing, as this now enables the check in checkExtendsOrImplements.
So try your other suggestion: have one checkExtendsOrImplements that has a boolean whether lenient or strict checks are desired. (If there is enough common code. Maybe you can just have two separate method.)
Then the current checkExtendsOrImplements will call the method with a boolean (or the strict method). And TaintingVisitor can override the method and change the behavior.
…sses framework/tests/viewpointtest/SubclassFieldInheritance.java was added by eisop#1900, after this branch was opened. It declares `SubA extends @A SuperClass` and `SubB extends @b SuperClass`, which the new default annotation.on.supertype check reports, so :framework:test failed. Record the two errors as expected diagnostics. Whether a receiver-dependent type system should be exempt from the check, the way the Tainting Checker is, is a separate question; recording the diagnostics keeps the new behavior visible rather than hiding it behind an override in the test checker. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The closed-issues entry for this change was written as a second `eisop#1015` rather than `eisop#1059`, and rewrapping the paragraph left a trailing space on two lines, which :docs:spotlessMiscCheck rejects. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
checker/tests/nullness/AnonymousClassTypeAnnotation.java suppressed nullness.on.supertype, a message key this branch deletes. The suppression is now dead: the replacement check runs from checkExtendsAndImplements, which returns early for an anonymous class, so nothing is reported on `new @nullable Object() {}` for the checker to suppress. NullnessTest passes either way. Note that this is a loss of coverage relative to the deleted check, which ran from processClassTree and did report on an anonymous class's supertype. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ype key" This reverts commit 6b733422eaa6f4b2ba7d38e1acb3f0e12f2b0f14. Restoring the suppression leaves checker/tests/nullness/ AnonymousClassTypeAnnotation.java unchanged from master, keeping this branch to the CI fixes. The suppression is inert here, since the replacement check runs from checkExtendsAndImplements, which returns early for an anonymous class; NullnessTest passes with or without it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
SubclassFieldInheritance expects annotation.on.supertype on two classes whose subject is field inheritance, which makes it unclear why those classes annotate their extends clause at all. Test the check on its own instead: an annotation on an extends clause and on an implements clause is rejected, and an annotation on a supertype's type argument is still allowed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…itance The extends clauses were annotated only to make the supertype agree with the class declaration bound. That is unrelated to what the file tests, and with this branch it also produces an annotation.on.supertype error, so the file asserted a diagnostic about supertype annotations while its subject is field inheritance. Leave the extends clauses unannotated. The supertype is not adapted to the class bound, so each class now reports declaration.inconsistent.with.extends.clause instead; record that. Every other expected diagnostic in the file is unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
wmdietl
changed the title
Fixes #1059.
BaseTypeVisitornow reportsannotation.on.supertypewhen a supportedqualifier is written on a supertype in an
extends/implementsclause. Only qualifiersfrom the active checker are flagged, and at most one error is reported per clause.
The check lives in a new protected helper
checkAnnotationOnSupertype(Tree), calledfrom
checkExtendsOrImplements. Checkers that allow annotations on supertypes(currently only the Tainting Checker) override this helper with an empty body.
Earlier iterations used a separate
checkSupertypeAnnotationsmethod, then a booleanflag on
checkExtendsOrImplements. Both conflated the annotation check with thesubtyping check; the dedicated helper keeps the two concerns cleanly separated.