-
Notifications
You must be signed in to change notification settings - Fork 380
Add Delegation Checker #6609
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
base: master
Are you sure you want to change the base?
Add Delegation Checker #6609
Conversation
…mework into yoo/nonempty-checker
…mework into yoo/nonempty-checker
…mework into yoo/nonempty-checker
…ansfer-nonempty-information
…ransfer-nonempty-information
* <li>A class overrides <i>all</i> methods declared in its superclass. | ||
* </ul> | ||
*/ | ||
public class DelegationChecker extends BaseTypeChecker {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to extend SourceChecker
.
…elegation-checker
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements a new Delegation Checker that enforces syntactic checks for the @Delegate
annotation. The checker validates that classes using delegation follow specific patterns: having at most one delegate field, properly delegating method calls, and overriding required methods.
- Adds a complete delegation checker framework with visitor, type factory, and checker classes
- Implements comprehensive test suite covering various delegation scenarios
- Includes proper error message handling and validation logic
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 6 comments.
Show a summary per file
File | Description |
---|---|
DelegationChecker.java | Main checker class that extends BaseTypeChecker |
DelegationVisitor.java | Core visitor implementing delegation validation logic |
DelegationAnnotatedTypeFactory.java | Type factory for handling delegation annotations |
Delegate.java | Annotation marking fields as delegates |
DelegatorMustOverride.java | Annotation for methods that must be overridden by delegating classes |
messages.properties | Error message definitions for the delegation checker |
DelegationTest.java | JUnit test runner for the delegation checker |
Various test files | Test cases covering different delegation scenarios |
AnnotatedTypes.java | Minor comment improvements for clarity |
Comments suppressed due to low confidence (1)
framework/src/main/java/org/checkerframework/common/delegation/DelegationVisitor.java:42
- [nitpick] The constant name MAX_NUM_DELEGATE_FIELDS should follow Java naming conventions and be more descriptive. Consider renaming it to MAX_DELEGATE_FIELDS_PER_CLASS.
private final int MAX_NUM_DELEGATE_FIELDS = 1;
framework/src/main/java/org/checkerframework/common/delegation/DelegationChecker.java
Outdated
Show resolved
Hide resolved
framework/src/main/java/org/checkerframework/common/delegation/DelegationChecker.java
Outdated
Show resolved
Hide resolved
framework/src/main/java/org/checkerframework/common/delegation/DelegationChecker.java
Outdated
Show resolved
Hide resolved
framework/src/main/java/org/checkerframework/common/delegation/DelegationVisitor.java
Show resolved
Hide resolved
Set<Name> overriddenMethods = | ||
getOverriddenMethods(tree).stream() | ||
.map(ExecutableElement::getSimpleName) | ||
.collect(Collectors.toSet()); | ||
Set<ExecutableElement> methodsDeclaredInSuperClass = | ||
new HashSet<>(ElementFilter.methodsIn(superClassMirror.asElement().getEnclosedElements())); | ||
Set<Name> methodsThatMustBeOverriden = | ||
methodsDeclaredInSuperClass.stream() | ||
.filter(e -> atypeFactory.getDeclAnnotation(e, DelegatorMustOverride.class) != null) | ||
.map(ExecutableElement::getSimpleName) | ||
.collect(Collectors.toSet()); | ||
|
||
// TODO: comparing a set of names isn't ideal, what about overloading? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The TODO comment identifies a significant limitation: the current implementation doesn't handle method overloading correctly. Consider using ExecutableElement comparison or method signatures instead of just names.
Set<Name> overriddenMethods = | |
getOverriddenMethods(tree).stream() | |
.map(ExecutableElement::getSimpleName) | |
.collect(Collectors.toSet()); | |
Set<ExecutableElement> methodsDeclaredInSuperClass = | |
new HashSet<>(ElementFilter.methodsIn(superClassMirror.asElement().getEnclosedElements())); | |
Set<Name> methodsThatMustBeOverriden = | |
methodsDeclaredInSuperClass.stream() | |
.filter(e -> atypeFactory.getDeclAnnotation(e, DelegatorMustOverride.class) != null) | |
.map(ExecutableElement::getSimpleName) | |
.collect(Collectors.toSet()); | |
// TODO: comparing a set of names isn't ideal, what about overloading? | |
Set<String> overriddenMethods = | |
getOverriddenMethods(tree).stream() | |
.map(ExecutableElement::toString) | |
.collect(Collectors.toSet()); | |
Set<ExecutableElement> methodsDeclaredInSuperClass = | |
new HashSet<>(ElementFilter.methodsIn(superClassMirror.asElement().getEnclosedElements())); | |
Set<String> methodsThatMustBeOverriden = | |
methodsDeclaredInSuperClass.stream() | |
.filter(e -> atypeFactory.getDeclAnnotation(e, DelegatorMustOverride.class) != null) | |
.map(ExecutableElement::toString) | |
.collect(Collectors.toSet()); | |
// Compare full method signatures to handle overloading |
Copilot uses AI. Check for mistakes.
No description provided.