From cd03501ac40e4e08ce5cef4e80d80794df76da8a Mon Sep 17 00:00:00 2001 From: Haifeng Shi Date: Tue, 3 Oct 2023 12:48:46 -0400 Subject: [PATCH 1/4] improve comments and readability --- .../common/basetype/BaseTypeVisitor.java | 38 +++++++++++-------- 1 file changed, 23 insertions(+), 15 deletions(-) diff --git a/framework/src/main/java/org/checkerframework/common/basetype/BaseTypeVisitor.java b/framework/src/main/java/org/checkerframework/common/basetype/BaseTypeVisitor.java index f7f7b81fde4..c5f2bccf248 100644 --- a/framework/src/main/java/org/checkerframework/common/basetype/BaseTypeVisitor.java +++ b/framework/src/main/java/org/checkerframework/common/basetype/BaseTypeVisitor.java @@ -1608,15 +1608,20 @@ && getCurrentPath().getParentPath().getLeaf().getKind() } /** - * Validate if the annotations on the VariableTree are at the right locations, which is - * specified by the meta-annotation @TargetLocations. The difference of this method between - * {@link BaseTypeVisitor#validateTargetLocation(Tree, AnnotatedTypeMirror, TypeUseLocation)} is - * that this one is only used in {@link BaseTypeVisitor#visitVariable(VariableTree, Void)} - * - * @param tree annotations on this VariableTree will be validated + * Validate if the qualifiers on the VariableTree are at the right locations, which is specified + * by the meta-annotation @TargetLocations. The difference of this method between {@link + * BaseTypeVisitor#validateTargetLocation(Tree, AnnotatedTypeMirror, TypeUseLocation)} is that + * this one is only used in {@link BaseTypeVisitor#visitVariable(VariableTree, Void)}. The three + * different validate methods in the visitor and validator file achieve the same goal and the + * need for having three is due to implementation reasons. We perform checks for types in + * VariableTree at {@link BaseTypeVisitor#visitVariable(VariableTree, Void)}; checks for types + * in wildcards at {@link BaseTypeValidator#visitWildcard(AnnotatedWildcardType, Tree)}; the + * rest done in the {@link BaseTypeVisitor#validateTypeOf(Tree)}. + * + * @param tree qualifiers on this VariableTree will be validated * @param type the type of the tree */ - protected void validateVariablesTargetLocation(Tree tree, AnnotatedTypeMirror type) { + protected void validateVariablesTargetLocation(VariableTree tree, AnnotatedTypeMirror type) { if (ignoreTargetLocations) { return; } @@ -1634,14 +1639,17 @@ protected void validateVariablesTargetLocation(Tree tree, AnnotatedTypeMirror ty boolean issueError = true; switch (elemKind) { case LOCAL_VARIABLE: - if (locations.contains(TypeUseLocation.LOCAL_VARIABLE)) issueError = false; + if (locations.contains(TypeUseLocation.LOCAL_VARIABLE)) { + issueError = false; + } break; case EXCEPTION_PARAMETER: - if (locations.contains(TypeUseLocation.EXCEPTION_PARAMETER)) + if (locations.contains(TypeUseLocation.EXCEPTION_PARAMETER)) { issueError = false; + } break; case PARAMETER: - if (((VariableTree) tree).getName().contentEquals("this")) { + if (tree.getName().contentEquals("this")) { if (locations.contains(TypeUseLocation.RECEIVER)) { issueError = false; } @@ -1682,13 +1690,13 @@ protected void validateVariablesTargetLocation(Tree tree, AnnotatedTypeMirror ty } /** - * Validate if the annotations on the tree are at the right locations, which are specified by - * the meta-annotation @TargetLocations. + * Validate if the qualifiers on the tree are at the right locations, which are specified by the + * meta-annotation @TargetLocations. * - * @param tree annotations on this VariableTree will be validated + * @param tree qualifiers on this VariableTree will be validated * @param type the type of the tree - * @param required if all of the TypeUseLocations in {@code required} are not present in the - * specification of the annotation (@TargetLocations), issue an error. + * @param required if all the TypeUseLocations in {@code required} are not present in the + * specification of the meta-annotation (@TargetLocations), issue an error. */ protected void validateTargetLocation( Tree tree, AnnotatedTypeMirror type, TypeUseLocation required) { From 9fd08045cfbb3421d00aea6a598c9fe660693801 Mon Sep 17 00:00:00 2001 From: Haifeng Shi Date: Tue, 3 Oct 2023 15:56:02 -0400 Subject: [PATCH 2/4] improve comments and readability --- .../common/basetype/BaseTypeVisitor.java | 110 +++++++++--------- 1 file changed, 54 insertions(+), 56 deletions(-) diff --git a/framework/src/main/java/org/checkerframework/common/basetype/BaseTypeVisitor.java b/framework/src/main/java/org/checkerframework/common/basetype/BaseTypeVisitor.java index c5f2bccf248..f6b868e758c 100644 --- a/framework/src/main/java/org/checkerframework/common/basetype/BaseTypeVisitor.java +++ b/framework/src/main/java/org/checkerframework/common/basetype/BaseTypeVisitor.java @@ -1625,66 +1625,64 @@ protected void validateVariablesTargetLocation(VariableTree tree, AnnotatedTypeM if (ignoreTargetLocations) { return; } - Element element = TreeUtils.elementFromTree(tree); - - if (element != null) { - ElementKind elemKind = element.getKind(); - // TypeUseLocation.java doesn't have ENUM type use location right now. - for (AnnotationMirror am : type.getAnnotations()) { - List locations = - qualAllowedLocations.get(AnnotationUtils.annotationName(am)); - if (locations == null || locations.contains(TypeUseLocation.ALL)) { - continue; - } - boolean issueError = true; - switch (elemKind) { - case LOCAL_VARIABLE: - if (locations.contains(TypeUseLocation.LOCAL_VARIABLE)) { - issueError = false; - } - break; - case EXCEPTION_PARAMETER: - if (locations.contains(TypeUseLocation.EXCEPTION_PARAMETER)) { - issueError = false; - } - break; - case PARAMETER: - if (tree.getName().contentEquals("this")) { - if (locations.contains(TypeUseLocation.RECEIVER)) { - issueError = false; - } - } else { - if (locations.contains(TypeUseLocation.PARAMETER)) { - issueError = false; - } - } - break; - case RESOURCE_VARIABLE: - if (locations.contains(TypeUseLocation.RESOURCE_VARIABLE)) { - issueError = false; - } - break; - case FIELD: - if (locations.contains(TypeUseLocation.FIELD)) { + Element element = TreeUtils.elementFromDeclaration(tree); + + ElementKind elemKind = element.getKind(); + // TypeUseLocation.java doesn't have ENUM type use location right now. + for (AnnotationMirror am : type.getAnnotations()) { + List locations = + qualAllowedLocations.get(AnnotationUtils.annotationName(am)); + if (locations == null || locations.contains(TypeUseLocation.ALL)) { + continue; + } + boolean issueError = true; + switch (elemKind) { + case LOCAL_VARIABLE: + if (locations.contains(TypeUseLocation.LOCAL_VARIABLE)) { + issueError = false; + } + break; + case EXCEPTION_PARAMETER: + if (locations.contains(TypeUseLocation.EXCEPTION_PARAMETER)) { + issueError = false; + } + break; + case PARAMETER: + if (tree.getName().contentEquals("this")) { + if (locations.contains(TypeUseLocation.RECEIVER)) { issueError = false; } - break; - case ENUM_CONSTANT: - if (locations.contains(TypeUseLocation.FIELD) - || locations.contains(TypeUseLocation.CONSTRUCTOR_RESULT)) { + } else { + if (locations.contains(TypeUseLocation.PARAMETER)) { issueError = false; } - break; - default: - throw new BugInCF("Location not matched"); - } - if (issueError) { - checker.reportError( - tree, - "type.invalid.annotations.on.location", - am.toString(), - element.getKind().name()); - } + } + break; + case RESOURCE_VARIABLE: + if (locations.contains(TypeUseLocation.RESOURCE_VARIABLE)) { + issueError = false; + } + break; + case FIELD: + if (locations.contains(TypeUseLocation.FIELD)) { + issueError = false; + } + break; + case ENUM_CONSTANT: + if (locations.contains(TypeUseLocation.FIELD) + || locations.contains(TypeUseLocation.CONSTRUCTOR_RESULT)) { + issueError = false; + } + break; + default: + throw new BugInCF("Location not matched"); + } + if (issueError) { + checker.reportError( + tree, + "type.invalid.annotations.on.location", + am.toString(), + element.getKind().name()); } } } From 0c8b600c7bdd019063281df6052e7c062333536a Mon Sep 17 00:00:00 2001 From: Haifeng Shi Date: Mon, 23 Oct 2023 15:28:41 -0400 Subject: [PATCH 3/4] Apply suggestions from code review Co-authored-by: Werner Dietl --- .../org/checkerframework/common/basetype/BaseTypeVisitor.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/framework/src/main/java/org/checkerframework/common/basetype/BaseTypeVisitor.java b/framework/src/main/java/org/checkerframework/common/basetype/BaseTypeVisitor.java index e705d6a82e8..3fa1d9045dd 100644 --- a/framework/src/main/java/org/checkerframework/common/basetype/BaseTypeVisitor.java +++ b/framework/src/main/java/org/checkerframework/common/basetype/BaseTypeVisitor.java @@ -1642,14 +1642,14 @@ && getCurrentPath().getParentPath().getLeaf().getKind() /** * Validate if the qualifiers on the VariableTree are at the right locations, which is specified - * by the meta-annotation @TargetLocations. The difference of this method between {@link + * by the meta-annotation @TargetLocations. The difference between this method and {@link * BaseTypeVisitor#validateTargetLocation(Tree, AnnotatedTypeMirror, TypeUseLocation)} is that * this one is only used in {@link BaseTypeVisitor#visitVariable(VariableTree, Void)}. The three * different validate methods in the visitor and validator file achieve the same goal and the * need for having three is due to implementation reasons. We perform checks for types in * VariableTree at {@link BaseTypeVisitor#visitVariable(VariableTree, Void)}; checks for types * in wildcards at {@link BaseTypeValidator#visitWildcard(AnnotatedWildcardType, Tree)}; the - * rest done in the {@link BaseTypeVisitor#validateTypeOf(Tree)}. + * rest is done in the {@link BaseTypeVisitor#validateTypeOf(Tree)}. * * @param tree qualifiers on this VariableTree will be validated * @param type the type of the tree From 39e78397a17dc0eaa1b71c026592cb6db458ad4f Mon Sep 17 00:00:00 2001 From: Haifeng Shi Date: Mon, 23 Oct 2023 16:54:15 -0400 Subject: [PATCH 4/4] clarify the implementation details of having three validating methods --- .../common/basetype/BaseTypeValidator.java | 14 +++++-- .../common/basetype/BaseTypeVisitor.java | 38 ++++++++++++------- 2 files changed, 36 insertions(+), 16 deletions(-) diff --git a/framework/src/main/java/org/checkerframework/common/basetype/BaseTypeValidator.java b/framework/src/main/java/org/checkerframework/common/basetype/BaseTypeValidator.java index 24e2524dc1b..ea4a3a04a58 100644 --- a/framework/src/main/java/org/checkerframework/common/basetype/BaseTypeValidator.java +++ b/framework/src/main/java/org/checkerframework/common/basetype/BaseTypeValidator.java @@ -718,9 +718,17 @@ public boolean areBoundsValid(AnnotatedTypeMirror upperBound, AnnotatedTypeMirro } /** - * Validate if qualifiers on wildcard are permitted by {@link - * org.checkerframework.framework.qual.TargetLocations}. Report an error if the actual use of - * this annotation is not listed in the declared TypeUseLocations in this meta-annotation. + * Validate if the qualifiers on the tree are at the right type-use locations, which is + * specified by the meta-annotation {@link org.checkerframework.framework.qual.TargetLocations}. + * + *

More specifically, this method only checks qualifiers on the WildcardTree and thus checks + * for these following type-use locations: (EXPLICIT/IMPLICIT) LOWER_BOUND and + * (EXPLICIT/IMPLICIT) UPPER_BOUND. + * + *

The other two validate methods achieve the same goal but perform checks on different trees + * and different type-use locations. See {@link + * BaseTypeVisitor#validateVariablesTargetLocation(VariableTree, AnnotatedTypeMirror)} and + * {@link BaseTypeVisitor#validateTargetLocation(Tree, AnnotatedTypeMirror, TypeUseLocation)}. * * @param type the type to check * @param tree the tree of this type diff --git a/framework/src/main/java/org/checkerframework/common/basetype/BaseTypeVisitor.java b/framework/src/main/java/org/checkerframework/common/basetype/BaseTypeVisitor.java index 3fa1d9045dd..5b8e7993161 100644 --- a/framework/src/main/java/org/checkerframework/common/basetype/BaseTypeVisitor.java +++ b/framework/src/main/java/org/checkerframework/common/basetype/BaseTypeVisitor.java @@ -1641,15 +1641,17 @@ && getCurrentPath().getParentPath().getLeaf().getKind() } /** - * Validate if the qualifiers on the VariableTree are at the right locations, which is specified - * by the meta-annotation @TargetLocations. The difference between this method and {@link - * BaseTypeVisitor#validateTargetLocation(Tree, AnnotatedTypeMirror, TypeUseLocation)} is that - * this one is only used in {@link BaseTypeVisitor#visitVariable(VariableTree, Void)}. The three - * different validate methods in the visitor and validator file achieve the same goal and the - * need for having three is due to implementation reasons. We perform checks for types in - * VariableTree at {@link BaseTypeVisitor#visitVariable(VariableTree, Void)}; checks for types - * in wildcards at {@link BaseTypeValidator#visitWildcard(AnnotatedWildcardType, Tree)}; the - * rest is done in the {@link BaseTypeVisitor#validateTypeOf(Tree)}. + * Validate if the qualifiers on the tree are at the right type-use locations, which is + * specified by the meta-annotation {@link org.checkerframework.framework.qual.TargetLocations}. + * + *

More specifically, this method only checks qualifiers on the VariableTree and thus checks + * for these following type-use locations: FIELD, LOCAL_VARIABLE, RESOURCE_VARIABLE, + * EXCEPTION_PARAMETER, PARAMETER, RECEIVER and CONSTRUCTOR_RESULT. + * + *

The other two validate methods achieve the same goal but perform checks on different trees + * and different type-use locations. See {@link BaseTypeVisitor#validateTargetLocation(Tree, + * AnnotatedTypeMirror, TypeUseLocation)} and {@link + * BaseTypeValidator#validateWildCardTargetLocation(AnnotatedWildcardType, Tree)}. * * @param tree qualifiers on this VariableTree will be validated * @param type the type of the tree @@ -1721,13 +1723,23 @@ protected void validateVariablesTargetLocation(VariableTree tree, AnnotatedTypeM } /** - * Validate if the qualifiers on the tree are at the right locations, which are specified by the - * meta-annotation @TargetLocations. + * Validate if the qualifiers on the tree are at the right type-use locations, which is + * specified by the meta-annotation {@link org.checkerframework.framework.qual.TargetLocations}. * - * @param tree qualifiers on this VariableTree will be validated + *

More specifically, this method only checks qualifiers on the TypeParameter and Method tree + * and thus checks for these following type-use locations: LOWER_BOUND, UPPER_BOUND, + * CONSTRUCTOR_RESULT and RETURN. + * + *

The other two validate methods achieve the same goal but perform checks on different trees + * and different type-use locations. See {@link + * BaseTypeVisitor#validateVariablesTargetLocation(VariableTree, AnnotatedTypeMirror)} and + * {@link BaseTypeValidator#validateWildCardTargetLocation(AnnotatedWildcardType, Tree)}. + * + * @param tree qualifiers on the tree will be validated * @param type the type of the tree * @param required if all the TypeUseLocations in {@code required} are not present in the - * specification of the meta-annotation (@TargetLocations), issue an error. + * specification of the meta-annotation ({@link + * org.checkerframework.framework.qual.TargetLocations}), issue an error. */ protected void validateTargetLocation( Tree tree, AnnotatedTypeMirror type, TypeUseLocation required) {