From 0b4910b2dbcb9467a6807eb5118855aa0b4763d7 Mon Sep 17 00:00:00 2001 From: Haifeng Shi Date: Tue, 14 Jun 2022 17:21:04 -0400 Subject: [PATCH 01/14] add downcast to cf --- .../common/basetype/BaseTypeVisitor.java | 37 +++++++++++++------ 1 file changed, 25 insertions(+), 12 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 0ffca954748..e74a9650fdc 100644 --- a/framework/src/main/java/org/checkerframework/common/basetype/BaseTypeVisitor.java +++ b/framework/src/main/java/org/checkerframework/common/basetype/BaseTypeVisitor.java @@ -2333,6 +2333,18 @@ protected void checkTypecastSafety(TypeCastTree typeCastTree) { } } + /** + * Allow no incomparable cast by default. Other type systems may override this method to allow + * certain incomparable casts. + * + * @param castType castType + * @param exprType exprType + * @return always false in BaseTypeVisitor + */ + protected boolean isIncomparableCastSafe(AnnotatedTypeMirror castType, AnnotatedTypeMirror exprType) { + return false; + } + /** * Returns true if the cast is safe. * @@ -2346,21 +2358,22 @@ protected void checkTypecastSafety(TypeCastTree typeCastTree) { protected boolean isTypeCastSafe(AnnotatedTypeMirror castType, AnnotatedTypeMirror exprType) { final TypeKind castTypeKind = castType.getKind(); - if (castTypeKind == TypeKind.DECLARED) { - // Don't issue an error if the annotations are equivalent to the qualifier upper bound - // of the type. - AnnotatedDeclaredType castDeclared = (AnnotatedDeclaredType) castType; - Set bounds = - atypeFactory.getTypeDeclarationBounds(castDeclared.getUnderlyingType()); - - if (AnnotationUtils.areSame(castDeclared.getAnnotations(), bounds)) { - return true; + QualifierHierarchy qualifierHierarchy = atypeFactory.getQualifierHierarchy(); + Set castAnnos; + + if (castTypeKind == TypeKind.DECLARED) { // Check if the downcast is valid + TypeMirror castJavaType = castType.getUnderlyingType(); + TypeMirror exprJavaType = exprType.getUnderlyingType(); + if (exprJavaType.getClass().isAssignableFrom(castJavaType.getClass())) { + if (qualifierHierarchy.isSubtype(castType.getAnnotations(), exprType.getAnnotations()) + || qualifierHierarchy.isSubtype(exprType.getAnnotations(), castType.getAnnotations())) { + return true; + } else { + return isIncomparableCastSafe(castType, exprType); + } } } - QualifierHierarchy qualifierHierarchy = atypeFactory.getQualifierHierarchy(); - - Set castAnnos; if (!checker.hasOption("checkCastElementType")) { // checkCastElementType option wasn't specified, so only check effective annotations. castAnnos = castType.getEffectiveAnnotations(); From f765a98f7a2fe17fee9126f969f644b33a3b3283 Mon Sep 17 00:00:00 2001 From: Haifeng Shi Date: Wed, 15 Jun 2022 09:57:20 -0400 Subject: [PATCH 02/14] add downcast to cf --- .../common/basetype/BaseTypeVisitor.java | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 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 e74a9650fdc..e76476936ea 100644 --- a/framework/src/main/java/org/checkerframework/common/basetype/BaseTypeVisitor.java +++ b/framework/src/main/java/org/checkerframework/common/basetype/BaseTypeVisitor.java @@ -2341,7 +2341,8 @@ protected void checkTypecastSafety(TypeCastTree typeCastTree) { * @param exprType exprType * @return always false in BaseTypeVisitor */ - protected boolean isIncomparableCastSafe(AnnotatedTypeMirror castType, AnnotatedTypeMirror exprType) { + protected boolean isIncomparableCastSafe( + AnnotatedTypeMirror castType, AnnotatedTypeMirror exprType) { return false; } @@ -2359,14 +2360,15 @@ protected boolean isTypeCastSafe(AnnotatedTypeMirror castType, AnnotatedTypeMirr final TypeKind castTypeKind = castType.getKind(); QualifierHierarchy qualifierHierarchy = atypeFactory.getQualifierHierarchy(); - Set castAnnos; if (castTypeKind == TypeKind.DECLARED) { // Check if the downcast is valid TypeMirror castJavaType = castType.getUnderlyingType(); TypeMirror exprJavaType = exprType.getUnderlyingType(); - if (exprJavaType.getClass().isAssignableFrom(castJavaType.getClass())) { - if (qualifierHierarchy.isSubtype(castType.getAnnotations(), exprType.getAnnotations()) - || qualifierHierarchy.isSubtype(exprType.getAnnotations(), castType.getAnnotations())) { + if (TypesUtils.isErasedSubtype(castJavaType, exprJavaType, types)) { + Set castAnnos = castType.getAnnotations(); + Set exprAnnos = exprType.getAnnotations(); + if (qualifierHierarchy.isSubtype(castAnnos, exprAnnos) + || qualifierHierarchy.isSubtype(exprAnnos, castAnnos)) { return true; } else { return isIncomparableCastSafe(castType, exprType); @@ -2374,6 +2376,7 @@ protected boolean isTypeCastSafe(AnnotatedTypeMirror castType, AnnotatedTypeMirr } } + Set castAnnos; if (!checker.hasOption("checkCastElementType")) { // checkCastElementType option wasn't specified, so only check effective annotations. castAnnos = castType.getEffectiveAnnotations(); From 5fbe6e2c35e7ee1e5e297040a4a468ffbdf241fa Mon Sep 17 00:00:00 2001 From: Haifeng Shi Date: Fri, 17 Jun 2022 13:44:54 -0400 Subject: [PATCH 03/14] add downcast/incomparable cast to cf --- .../index/inequality/LessThanVisitor.java | 3 +- .../checker/interning/InterningVisitor.java | 5 +- .../common/basetype/BaseTypeVisitor.java | 168 ++++++++++++------ 3 files changed, 121 insertions(+), 55 deletions(-) diff --git a/checker/src/main/java/org/checkerframework/checker/index/inequality/LessThanVisitor.java b/checker/src/main/java/org/checkerframework/checker/index/inequality/LessThanVisitor.java index 60fc1572c4c..7bb11ef7719 100644 --- a/checker/src/main/java/org/checkerframework/checker/index/inequality/LessThanVisitor.java +++ b/checker/src/main/java/org/checkerframework/checker/index/inequality/LessThanVisitor.java @@ -113,7 +113,8 @@ protected void commonAssignmentCheck( } @Override - protected boolean isTypeCastSafe(AnnotatedTypeMirror castType, AnnotatedTypeMirror exprType) { + protected CastSafeKind isTypeCastSafe( + AnnotatedTypeMirror castType, AnnotatedTypeMirror exprType) { AnnotationMirror exprLTAnno = exprType.getEffectiveAnnotationInHierarchy(atypeFactory.LESS_THAN_UNKNOWN); diff --git a/checker/src/main/java/org/checkerframework/checker/interning/InterningVisitor.java b/checker/src/main/java/org/checkerframework/checker/interning/InterningVisitor.java index 44f0dbb4d51..479075f0757 100644 --- a/checker/src/main/java/org/checkerframework/checker/interning/InterningVisitor.java +++ b/checker/src/main/java/org/checkerframework/checker/interning/InterningVisitor.java @@ -977,9 +977,10 @@ DeclaredType typeToCheck() { } @Override - protected boolean isTypeCastSafe(AnnotatedTypeMirror castType, AnnotatedTypeMirror exprType) { + protected CastSafeKind isTypeCastSafe( + AnnotatedTypeMirror castType, AnnotatedTypeMirror exprType) { if (castType.getKind().isPrimitive()) { - return true; + return CastSafeKind.WARNING; } return super.isTypeCastSafe(castType, exprType); } 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 e76476936ea..5289d8e7a08 100644 --- a/framework/src/main/java/org/checkerframework/common/basetype/BaseTypeVisitor.java +++ b/framework/src/main/java/org/checkerframework/common/basetype/BaseTypeVisitor.java @@ -2327,57 +2327,42 @@ protected void checkTypecastSafety(TypeCastTree typeCastTree) { } // We cannot do a simple test of casting, as isSubtypeOf requires // the input types to be subtypes according to Java. - if (!calledOnce && !isTypeCastSafe(castType, exprType)) { - checker.reportWarning( - typeCastTree, "cast.unsafe", exprType.toString(true), castType.toString(true)); + if (!calledOnce) { + CastSafeKind castResult = isTypeCastSafe(castType, exprType); + + if (castResult == CastSafeKind.WARNING + || castResult == CastSafeKind.ERROR) { // TODO: refine the error report + checker.reportWarning( + typeCastTree, + "cast.unsafe", + exprType.toString(true), + castType.toString(true)); + } } } - /** - * Allow no incomparable cast by default. Other type systems may override this method to allow - * certain incomparable casts. - * - * @param castType castType - * @param exprType exprType - * @return always false in BaseTypeVisitor - */ - protected boolean isIncomparableCastSafe( - AnnotatedTypeMirror castType, AnnotatedTypeMirror exprType) { - return false; + /** CastSafeKind */ + protected enum CastSafeKind { + SAFE, + ERROR, + WARNING, + NOT_UPCAST, + NOT_DOWNCAST } /** - * Returns true if the cast is safe. + * isUpcast * - *

Only primary qualifiers are checked unless the command line option "checkCastElementType" - * is supplied. - * - * @param castType annotated type of the cast - * @param exprType annotated type of the casted expression - * @return true if the type cast is safe, false otherwise + * @param castType castType + * @param exprType exprType + * @return CastSafeKind */ - protected boolean isTypeCastSafe(AnnotatedTypeMirror castType, AnnotatedTypeMirror exprType) { - - final TypeKind castTypeKind = castType.getKind(); + protected CastSafeKind isUpcast(AnnotatedTypeMirror castType, AnnotatedTypeMirror exprType) { QualifierHierarchy qualifierHierarchy = atypeFactory.getQualifierHierarchy(); - - if (castTypeKind == TypeKind.DECLARED) { // Check if the downcast is valid - TypeMirror castJavaType = castType.getUnderlyingType(); - TypeMirror exprJavaType = exprType.getUnderlyingType(); - if (TypesUtils.isErasedSubtype(castJavaType, exprJavaType, types)) { - Set castAnnos = castType.getAnnotations(); - Set exprAnnos = exprType.getAnnotations(); - if (qualifierHierarchy.isSubtype(castAnnos, exprAnnos) - || qualifierHierarchy.isSubtype(exprAnnos, castAnnos)) { - return true; - } else { - return isIncomparableCastSafe(castType, exprType); - } - } - } - Set castAnnos; - if (!checker.hasOption("checkCastElementType")) { + TypeKind castTypeKind = castType.getKind(); + boolean flagEnabled = checker.hasOption("checkCastElementType"); + if (!flagEnabled) { // checkCastElementType option wasn't specified, so only check effective annotations. castAnnos = castType.getEffectiveAnnotations(); } else { @@ -2395,13 +2380,13 @@ protected boolean isTypeCastSafe(AnnotatedTypeMirror castType, AnnotatedTypeMirr } if (!atypeFactory.getTypeHierarchy().isSubtype(newExprType, newCastType)) { - return false; + return CastSafeKind.ERROR; } if (newCastType.getKind() == TypeKind.ARRAY && newExprType.getKind() != TypeKind.ARRAY) { // Always warn if the cast contains an array, but the expression // doesn't, as in "(Object[]) o" where o is of type Object - return false; + return CastSafeKind.ERROR; } else if (newCastType.getKind() == TypeKind.DECLARED && newExprType.getKind() == TypeKind.DECLARED) { int castSize = ((AnnotatedDeclaredType) newCastType).getTypeArguments().size(); @@ -2411,7 +2396,7 @@ protected boolean isTypeCastSafe(AnnotatedTypeMirror castType, AnnotatedTypeMirr // Always warn if the cast and expression contain a different number of type // arguments, e.g. to catch a cast from "Object" to "List<@NonNull Object>". // TODO: the same number of arguments actually doesn't guarantee anything. - return false; + return CastSafeKind.ERROR; } } else if (castTypeKind == TypeKind.TYPEVAR && exprType.getKind() == TypeKind.TYPEVAR) { // If both the cast type and the casted expression are type variables, then check @@ -2422,11 +2407,13 @@ protected boolean isTypeCastSafe(AnnotatedTypeMirror castType, AnnotatedTypeMirr Set lowerBoundAnnotationsExpr = AnnotatedTypes.findEffectiveLowerBoundAnnotations( qualifierHierarchy, exprType); - return qualifierHierarchy.isSubtype( - lowerBoundAnnotationsExpr, lowerBoundAnnotationsCast) - && qualifierHierarchy.isSubtype( - exprType.getEffectiveAnnotations(), - castType.getEffectiveAnnotations()); + boolean result = + qualifierHierarchy.isSubtype( + lowerBoundAnnotationsExpr, lowerBoundAnnotationsCast) + && qualifierHierarchy.isSubtype( + exprType.getEffectiveAnnotations(), + castType.getEffectiveAnnotations()); + return result ? CastSafeKind.SAFE : CastSafeKind.ERROR; } if (castTypeKind == TypeKind.TYPEVAR) { // If the cast type is a type var, but the expression is not, then check that the @@ -2440,7 +2427,84 @@ protected boolean isTypeCastSafe(AnnotatedTypeMirror castType, AnnotatedTypeMirr } AnnotatedTypeMirror exprTypeWidened = atypeFactory.getWidenedType(exprType, castType); - return qualifierHierarchy.isSubtype(exprTypeWidened.getEffectiveAnnotations(), castAnnos); + boolean result = + qualifierHierarchy.isSubtype(exprTypeWidened.getEffectiveAnnotations(), castAnnos); + if (result) { + return CastSafeKind.SAFE; + } else if (flagEnabled) { // when the flag is enabled and it is not an upcast, return an + // error + return CastSafeKind.ERROR; + } else { + return CastSafeKind.NOT_UPCAST; + } + } + + /** + * isSafeDowncast + * + * @param castType castType + * @param exprType exprType + * @return CastSafeKind + */ + protected CastSafeKind isSafeDowncast( + AnnotatedTypeMirror castType, AnnotatedTypeMirror exprType) { + Set castAnnos = castType.getAnnotations(); + Set exprAnnos = exprType.getAnnotations(); + QualifierHierarchy qualifierHierarchy = atypeFactory.getQualifierHierarchy(); + + if (!qualifierHierarchy.isSubtype(castAnnos, exprAnnos)) { // not a downcast + return CastSafeKind.NOT_DOWNCAST; + } + + // check if the downcast can be statically verified + final TypeKind castTypeKind = castType.getKind(); + + if (castTypeKind == TypeKind.DECLARED) { + // Don't issue an error if the annotations are equivalent to the qualifier upper bound + // of the type. + AnnotatedDeclaredType castDeclared = (AnnotatedDeclaredType) castType; + Set bounds = + atypeFactory.getTypeDeclarationBounds(castDeclared.getUnderlyingType()); + + if (AnnotationUtils.areSame(castDeclared.getAnnotations(), bounds)) { + return CastSafeKind.SAFE; + } + } + return CastSafeKind.WARNING; + } + + protected CastSafeKind isSafeIncomparableCast( + AnnotatedTypeMirror castType, AnnotatedTypeMirror exprType) { + return CastSafeKind.ERROR; + } + + /** + * Returns true if the cast is safe. + * + *

Only primary qualifiers are checked unless the command line option "checkCastElementType" + * is supplied. + * + * @param castType annotated type of the cast + * @param exprType annotated type of the casted expression + * @return CastSafeKind if the type cast is safe, false otherwise + */ + protected CastSafeKind isTypeCastSafe( + AnnotatedTypeMirror castType, AnnotatedTypeMirror exprType) { + CastSafeKind castResult = isUpcast(castType, exprType); + + if (castResult + == CastSafeKind.NOT_UPCAST) { // not upcast, do downcast and incomparable cast check + castResult = isSafeDowncast(castType, exprType); + + if (castResult == CastSafeKind.NOT_DOWNCAST) { // fall to incomparable cast + return isSafeIncomparableCast(castType, exprType); + } else { + return castResult; + } + + } else { + return castResult; + } } /** @@ -2453,7 +2517,7 @@ protected boolean isTypeCastSafe(AnnotatedTypeMirror castType, AnnotatedTypeMirr */ private boolean isInvariantTypeCastSafe( AnnotatedTypeMirror castType, AnnotatedTypeMirror exprType, AnnotationMirror top) { - if (!isTypeCastSafe(castType, exprType)) { + if (isTypeCastSafe(castType, exprType) != CastSafeKind.SAFE) { return false; } AnnotationMirror castTypeAnno = castType.getEffectiveAnnotationInHierarchy(top); @@ -2505,7 +2569,7 @@ public Void visitInstanceOf(InstanceOfTree tree, Void p) { if (variableTree.getModifiers() != null) { AnnotatedTypeMirror variableType = atypeFactory.getAnnotatedType(variableTree); AnnotatedTypeMirror expType = atypeFactory.getAnnotatedType(tree.getExpression()); - if (!isTypeCastSafe(variableType, expType)) { + if (isTypeCastSafe(variableType, expType) != CastSafeKind.SAFE) { checker.reportWarning(tree, "instanceof.pattern.unsafe", expType, variableTree); } } From 3e7383bc87f19ffa95160a475bd7a0453ddabf2e Mon Sep 17 00:00:00 2001 From: Haifeng Shi Date: Sat, 18 Jun 2022 14:37:56 -0400 Subject: [PATCH 04/14] use getEffectiveAnnotation --- .../checker/interning/InterningVisitor.java | 2 +- .../common/basetype/BaseTypeVisitor.java | 17 +++++++++++++++-- .../checkerframework/javacutil/TypesUtils.java | 14 +++++++++++++- 3 files changed, 29 insertions(+), 4 deletions(-) diff --git a/checker/src/main/java/org/checkerframework/checker/interning/InterningVisitor.java b/checker/src/main/java/org/checkerframework/checker/interning/InterningVisitor.java index 479075f0757..ecee97630d1 100644 --- a/checker/src/main/java/org/checkerframework/checker/interning/InterningVisitor.java +++ b/checker/src/main/java/org/checkerframework/checker/interning/InterningVisitor.java @@ -980,7 +980,7 @@ DeclaredType typeToCheck() { protected CastSafeKind isTypeCastSafe( AnnotatedTypeMirror castType, AnnotatedTypeMirror exprType) { if (castType.getKind().isPrimitive()) { - return CastSafeKind.WARNING; + return CastSafeKind.SAFE; } return super.isTypeCastSafe(castType, exprType); } 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 5289d8e7a08..f748b0e984a 100644 --- a/framework/src/main/java/org/checkerframework/common/basetype/BaseTypeVisitor.java +++ b/framework/src/main/java/org/checkerframework/common/basetype/BaseTypeVisitor.java @@ -2426,9 +2426,22 @@ protected CastSafeKind isUpcast(AnnotatedTypeMirror castType, AnnotatedTypeMirro } } + // // Return a warning in this case, as compiler will not report the unchecked + // warning in + // // this case, and we cannot statically verify the subtype relation of the + // annotations. + // if (castTypeKind == TypeKind.TYPEVAR && exprType.getKind() == TypeKind.TYPEVAR) { + // TypeMirror castJavaType = castType.getUnderlyingType(); + // TypeMirror exprJavaType = exprType.getUnderlyingType(); + // if (TypesUtils.areSameTypeVariables(castJavaType, exprJavaType)) { + // return CastSafeKind.WARNING; + // } + // } + AnnotatedTypeMirror exprTypeWidened = atypeFactory.getWidenedType(exprType, castType); boolean result = qualifierHierarchy.isSubtype(exprTypeWidened.getEffectiveAnnotations(), castAnnos); + if (result) { return CastSafeKind.SAFE; } else if (flagEnabled) { // when the flag is enabled and it is not an upcast, return an @@ -2448,8 +2461,8 @@ protected CastSafeKind isUpcast(AnnotatedTypeMirror castType, AnnotatedTypeMirro */ protected CastSafeKind isSafeDowncast( AnnotatedTypeMirror castType, AnnotatedTypeMirror exprType) { - Set castAnnos = castType.getAnnotations(); - Set exprAnnos = exprType.getAnnotations(); + Set castAnnos = castType.getEffectiveAnnotations(); + Set exprAnnos = exprType.getEffectiveAnnotations(); QualifierHierarchy qualifierHierarchy = atypeFactory.getQualifierHierarchy(); if (!qualifierHierarchy.isSubtype(castAnnos, exprAnnos)) { // not a downcast diff --git a/javacutil/src/main/java/org/checkerframework/javacutil/TypesUtils.java b/javacutil/src/main/java/org/checkerframework/javacutil/TypesUtils.java index 907d8399ed1..5e8e5c9704e 100644 --- a/javacutil/src/main/java/org/checkerframework/javacutil/TypesUtils.java +++ b/javacutil/src/main/java/org/checkerframework/javacutil/TypesUtils.java @@ -275,7 +275,7 @@ public static boolean areSameDeclaredTypes(Type.ClassType t1, Type.ClassType t2) if (t1.tsym.name != t2.tsym.name) { return false; } - return t1.toString().equals(t1.toString()); + return t1.toString().equals(t2.toString()); } /** @@ -293,6 +293,18 @@ public static boolean areSamePrimitiveTypes(TypeMirror left, TypeMirror right) { return (left.getKind() == right.getKind()); } + /** + * Returns true iff the arguments are both the same type variables. + * + * @param t1 a type + * @param t2 a type + * @return whether the arguments are the same type variables + */ + public static boolean areSameTypeVariables( + TypeMirror t1, TypeMirror t2) { // TODO: a cheaper way of doing this? + return t1.toString().equals(t2.toString()); + } + /// Predicates /** From 2febfb984dacdc288bf217490157fb8c462f07a6 Mon Sep 17 00:00:00 2001 From: Haifeng Shi Date: Sat, 18 Jun 2022 15:51:57 -0400 Subject: [PATCH 05/14] check the java type down cast --- .../checkerframework/common/basetype/BaseTypeVisitor.java | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) 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 f748b0e984a..fd4c7a54252 100644 --- a/framework/src/main/java/org/checkerframework/common/basetype/BaseTypeVisitor.java +++ b/framework/src/main/java/org/checkerframework/common/basetype/BaseTypeVisitor.java @@ -2480,7 +2480,11 @@ protected CastSafeKind isSafeDowncast( atypeFactory.getTypeDeclarationBounds(castDeclared.getUnderlyingType()); if (AnnotationUtils.areSame(castDeclared.getAnnotations(), bounds)) { - return CastSafeKind.SAFE; + TypeMirror castJavaType = castType.getUnderlyingType(); + TypeMirror exprJavaType = exprType.getUnderlyingType(); + if (TypesUtils.isErasedSubtype(castJavaType, exprJavaType, types)) { + return CastSafeKind.SAFE; + } } } return CastSafeKind.WARNING; From 67a2ed2e4b0ef1391423a41282fa8fd31a74d180 Mon Sep 17 00:00:00 2001 From: Haifeng Shi Date: Sat, 18 Jun 2022 21:55:28 -0400 Subject: [PATCH 06/14] revert typo fix, fix in another pr --- .../main/java/org/checkerframework/javacutil/TypesUtils.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/javacutil/src/main/java/org/checkerframework/javacutil/TypesUtils.java b/javacutil/src/main/java/org/checkerframework/javacutil/TypesUtils.java index 5e8e5c9704e..587977eb357 100644 --- a/javacutil/src/main/java/org/checkerframework/javacutil/TypesUtils.java +++ b/javacutil/src/main/java/org/checkerframework/javacutil/TypesUtils.java @@ -275,7 +275,7 @@ public static boolean areSameDeclaredTypes(Type.ClassType t1, Type.ClassType t2) if (t1.tsym.name != t2.tsym.name) { return false; } - return t1.toString().equals(t2.toString()); + return t1.toString().equals(t1.toString()); } /** From b52fb4310011b97ed2dda7290028895a71fd4cef Mon Sep 17 00:00:00 2001 From: Haifeng Shi Date: Wed, 22 Jun 2022 18:04:08 -0400 Subject: [PATCH 07/14] add incomparable cast message key --- checker/tests/lock/Strings.java | 2 ++ checker/tests/signedness/CharToFloat.java | 2 ++ .../common/basetype/BaseTypeVisitor.java | 31 +++++++++++-------- .../common/basetype/messages.properties | 1 + .../javacutil/TypesUtils.java | 9 +++--- 5 files changed, 27 insertions(+), 18 deletions(-) diff --git a/checker/tests/lock/Strings.java b/checker/tests/lock/Strings.java index c29200e1d47..d788ebef87a 100644 --- a/checker/tests/lock/Strings.java +++ b/checker/tests/lock/Strings.java @@ -13,7 +13,9 @@ void StringIsGBnothing( @GuardSatisfied Object o3, @GuardedByBottom Object o4) { String s1 = (String) o1; + // :: error: (cast.incompatible) String s2 = (String) o2; + // :: error: (cast.incompatible) String s3 = (String) o3; String s4 = (String) o4; // OK } diff --git a/checker/tests/signedness/CharToFloat.java b/checker/tests/signedness/CharToFloat.java index 1caaea9f8f0..44874da7161 100644 --- a/checker/tests/signedness/CharToFloat.java +++ b/checker/tests/signedness/CharToFloat.java @@ -2,7 +2,9 @@ public class CharToFloat { void castCharacter(Object o) { + // :: error: (cast.incompatible) floatParameter((Character) o); + // :: error: (cast.incompatible) doubleParameter((Character) o); } 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 fd4c7a54252..2a71c1322da 100644 --- a/framework/src/main/java/org/checkerframework/common/basetype/BaseTypeVisitor.java +++ b/framework/src/main/java/org/checkerframework/common/basetype/BaseTypeVisitor.java @@ -142,6 +142,7 @@ import javax.lang.model.type.DeclaredType; import javax.lang.model.type.TypeKind; import javax.lang.model.type.TypeMirror; +import javax.lang.model.type.TypeVariable; import javax.lang.model.util.ElementFilter; import javax.tools.Diagnostic.Kind; @@ -2330,13 +2331,18 @@ protected void checkTypecastSafety(TypeCastTree typeCastTree) { if (!calledOnce) { CastSafeKind castResult = isTypeCastSafe(castType, exprType); - if (castResult == CastSafeKind.WARNING - || castResult == CastSafeKind.ERROR) { // TODO: refine the error report + if (castResult == CastSafeKind.WARNING) { checker.reportWarning( typeCastTree, "cast.unsafe", exprType.toString(true), castType.toString(true)); + } else if (castResult == CastSafeKind.ERROR) { + checker.reportError( + typeCastTree, + "cast.incompatible", + exprType.toString(true), + castType.toString(true)); } } } @@ -2426,17 +2432,16 @@ protected CastSafeKind isUpcast(AnnotatedTypeMirror castType, AnnotatedTypeMirro } } - // // Return a warning in this case, as compiler will not report the unchecked - // warning in - // // this case, and we cannot statically verify the subtype relation of the - // annotations. - // if (castTypeKind == TypeKind.TYPEVAR && exprType.getKind() == TypeKind.TYPEVAR) { - // TypeMirror castJavaType = castType.getUnderlyingType(); - // TypeMirror exprJavaType = exprType.getUnderlyingType(); - // if (TypesUtils.areSameTypeVariables(castJavaType, exprJavaType)) { - // return CastSafeKind.WARNING; - // } - // } + // Return a warning in this case, as compiler will not report the unchecked warning in + // this case, and we cannot statically verify the subtype relation of the annotations. + if (castTypeKind == TypeKind.TYPEVAR && exprType.getKind() == TypeKind.TYPEVAR) { + TypeMirror castJavaType = castType.getUnderlyingType(); + TypeMirror exprJavaType = exprType.getUnderlyingType(); + if (TypesUtils.areSameTypeVariables( + (TypeVariable) castJavaType, (TypeVariable) exprJavaType)) { + return CastSafeKind.WARNING; + } + } AnnotatedTypeMirror exprTypeWidened = atypeFactory.getWidenedType(exprType, castType); boolean result = diff --git a/framework/src/main/java/org/checkerframework/common/basetype/messages.properties b/framework/src/main/java/org/checkerframework/common/basetype/messages.properties index 89a38bd6735..55772bf918c 100644 --- a/framework/src/main/java/org/checkerframework/common/basetype/messages.properties +++ b/framework/src/main/java/org/checkerframework/common/basetype/messages.properties @@ -26,6 +26,7 @@ type.invalid.too.few.annotations=invalid type: missing annotations %s in type "% type.invalid.annotations.on.use=invalid type: annotations %s conflict with declaration of type %s type.invalid.super.wildcard=bounds must have the same annotations.%nsuper bound : %s%nextends bound: %s cast.unsafe=cast from "%s" to "%s" cannot be statically verified +cast.incompatible=type cast incompatible from "%s" to "%s" invariant.cast.unsafe=cannot cast from "%s" to "%s" cast.unsafe.constructor.invocation=constructor invocation cast from "%s" to "%s" cannot be statically verified exception.parameter.invalid=invalid type in catch argument.%nfound : %s%nrequired: %s diff --git a/javacutil/src/main/java/org/checkerframework/javacutil/TypesUtils.java b/javacutil/src/main/java/org/checkerframework/javacutil/TypesUtils.java index 587977eb357..23fbde90faa 100644 --- a/javacutil/src/main/java/org/checkerframework/javacutil/TypesUtils.java +++ b/javacutil/src/main/java/org/checkerframework/javacutil/TypesUtils.java @@ -296,13 +296,12 @@ public static boolean areSamePrimitiveTypes(TypeMirror left, TypeMirror right) { /** * Returns true iff the arguments are both the same type variables. * - * @param t1 a type - * @param t2 a type + * @param v1 a type variable + * @param v2 a type variable * @return whether the arguments are the same type variables */ - public static boolean areSameTypeVariables( - TypeMirror t1, TypeMirror t2) { // TODO: a cheaper way of doing this? - return t1.toString().equals(t2.toString()); + public static boolean areSameTypeVariables(TypeVariable v1, TypeVariable v2) { + return v1.asElement().getSimpleName() == v2.asElement().getSimpleName(); } /// Predicates From 9de934dae692ea0be732b694c97e0b44a55e84af Mon Sep 17 00:00:00 2001 From: Haifeng Shi Date: Wed, 22 Jun 2022 21:23:31 -0400 Subject: [PATCH 08/14] replace some warnings with incompatible cast errors --- checker/tests/lock/ChapterExamples.java | 4 ++-- .../nullness-checkcastelementtype/Issue1315.java | 2 +- checker/tests/nullness/CastTypeVariable.java | 1 + checker/tests/nullness/CastsNullness.java | 4 ++-- checker/tests/nullness/java8inference/Issue1032.java | 2 +- checker/tests/signedness/CharCast.java | 4 ++-- checker/tests/signedness/PrimitiveCasts.java | 2 +- checker/tests/signedness/WideningConversion.java | 12 ++++++------ .../common/basetype/messages.properties | 2 +- framework/tests/value/Basics.java | 2 +- framework/tests/value/Issue2367.java | 10 +++++----- framework/tests/value/Overflows.java | 6 +++--- framework/tests/value/TypeCast.java | 4 ++-- framework/tests/value/Underflows.java | 6 +++--- 14 files changed, 31 insertions(+), 30 deletions(-) diff --git a/checker/tests/lock/ChapterExamples.java b/checker/tests/lock/ChapterExamples.java index f2c76716ed4..c571c36b532 100644 --- a/checker/tests/lock/ChapterExamples.java +++ b/checker/tests/lock/ChapterExamples.java @@ -318,7 +318,7 @@ void someMethod() { o2 = o1; // {"lock"} and {} are not identical sets. } - @SuppressWarnings("lock:cast.unsafe") + @SuppressWarnings("lock:cast.incompatible") void someMethod2() { // A cast can be used if the user knows it is safe to do so. // However, the @SuppressWarnings must be added. @@ -564,7 +564,7 @@ public boolean compare(T[] a1, T[] a2) { private static final Object NULL_KEY = new Object(); // A guardsatisfied.location.disallowed error is issued for the cast. - @SuppressWarnings({"cast.unsafe", "guardsatisfied.location.disallowed"}) + @SuppressWarnings({"cast.incompatible", "guardsatisfied.location.disallowed"}) private static @GuardSatisfied(1) Object maskNull(@GuardSatisfied(1) Object key) { return (key == null ? (@GuardSatisfied(1) Object) NULL_KEY : key); } diff --git a/checker/tests/nullness-checkcastelementtype/Issue1315.java b/checker/tests/nullness-checkcastelementtype/Issue1315.java index 3fb1afe1d52..76fd8fb342e 100644 --- a/checker/tests/nullness-checkcastelementtype/Issue1315.java +++ b/checker/tests/nullness-checkcastelementtype/Issue1315.java @@ -13,7 +13,7 @@ static class Box { @SuppressWarnings("unchecked") T test1(@Nullable Object p) { - // :: warning: (cast.unsafe) + // :: error: (cast.incompatible) return (T) p; } // The Nullness Checker should not issue a cast.unsafe warning, diff --git a/checker/tests/nullness/CastTypeVariable.java b/checker/tests/nullness/CastTypeVariable.java index 323ed8c0487..5364cdec637 100644 --- a/checker/tests/nullness/CastTypeVariable.java +++ b/checker/tests/nullness/CastTypeVariable.java @@ -9,6 +9,7 @@ class MyAnnotatedTypeVariable extends MyAnnotatedTypeMirror {} public class CastTypeVariable { public static V mapGetHelper( Map mappings, MyAnnotatedTypeVariable key) { + // :: warning: (cast.unsafe) V possValue = (V) mappings.get(key); // :: error: (dereference.of.nullable) possValue.addAnnotations(); diff --git a/checker/tests/nullness/CastsNullness.java b/checker/tests/nullness/CastsNullness.java index cb5deb34eec..7c468a9a711 100644 --- a/checker/tests/nullness/CastsNullness.java +++ b/checker/tests/nullness/CastsNullness.java @@ -88,9 +88,9 @@ void m() { // :: error: (assignment.type.incompatible) t = (@Nullable T) null; nt = (@Nullable T) null; - // :: warning: (cast.unsafe) + // :: error: (cast.incompatible) t = (T) null; - // :: warning: (cast.unsafe) + // :: error: (cast.incompatible) nt = (T) null; } } diff --git a/checker/tests/nullness/java8inference/Issue1032.java b/checker/tests/nullness/java8inference/Issue1032.java index b09a59413fd..273dd2779fa 100644 --- a/checker/tests/nullness/java8inference/Issue1032.java +++ b/checker/tests/nullness/java8inference/Issue1032.java @@ -16,7 +16,7 @@ public class Issue1032 { return arg.map(Issue1032::castStringToNonNull); } - @SuppressWarnings("nullness") + @SuppressWarnings("nullness", "cast.unsafe") static @NonNull T castTToNonNull(@Nullable T arg) { return (@NonNull T) arg; } diff --git a/checker/tests/signedness/CharCast.java b/checker/tests/signedness/CharCast.java index 788eb5b19ce..324a385f0fe 100644 --- a/checker/tests/signedness/CharCast.java +++ b/checker/tests/signedness/CharCast.java @@ -8,13 +8,13 @@ void m(@SignedPositive int i) { void m1(short s) { int x = s; - // :: warning: (cast.unsafe) + // :: error: (cast.incompatible) char c = (char) x; } void m2(int i) { int x = (short) i; - // :: warning: (cast.unsafe) + // :: error: (cast.incompatible) char c = (char) x; } diff --git a/checker/tests/signedness/PrimitiveCasts.java b/checker/tests/signedness/PrimitiveCasts.java index 132d430d023..62ef25e3076 100644 --- a/checker/tests/signedness/PrimitiveCasts.java +++ b/checker/tests/signedness/PrimitiveCasts.java @@ -3,7 +3,7 @@ public class PrimitiveCasts { void shortToChar1(short s) { - // :: warning: (cast.unsafe) + // :: error: (cast.incompatible) char c = (char) s; } diff --git a/checker/tests/signedness/WideningConversion.java b/checker/tests/signedness/WideningConversion.java index 68d2ae776ab..f294b6da05c 100644 --- a/checker/tests/signedness/WideningConversion.java +++ b/checker/tests/signedness/WideningConversion.java @@ -74,19 +74,19 @@ void plus() { char c; c = (char) (c1 + c2); - // :: warning: (cast.unsafe) + // :: error: (cast.incompatible) c = (char) (c1 + i2); - // :: warning: (cast.unsafe) + // :: error: (cast.incompatible) c = (char) (i1 + c2); - // :: warning: (cast.unsafe) + // :: error: (cast.incompatible) c = (char) (i1 + i2); c = (char) (c1 + c2); - // :: warning: (cast.unsafe) + // :: error: (cast.incompatible) c = (char) (c1 + si2); - // :: warning: (cast.unsafe) + // :: error: (cast.incompatible) c = (char) (si1 + c2); - // :: warning: (cast.unsafe) + // :: error: (cast.incompatible) c = (char) (si1 + si2); c = (char) (c1 + c2); diff --git a/framework/src/main/java/org/checkerframework/common/basetype/messages.properties b/framework/src/main/java/org/checkerframework/common/basetype/messages.properties index 55772bf918c..0160bb082a2 100644 --- a/framework/src/main/java/org/checkerframework/common/basetype/messages.properties +++ b/framework/src/main/java/org/checkerframework/common/basetype/messages.properties @@ -26,7 +26,7 @@ type.invalid.too.few.annotations=invalid type: missing annotations %s in type "% type.invalid.annotations.on.use=invalid type: annotations %s conflict with declaration of type %s type.invalid.super.wildcard=bounds must have the same annotations.%nsuper bound : %s%nextends bound: %s cast.unsafe=cast from "%s" to "%s" cannot be statically verified -cast.incompatible=type cast incompatible from "%s" to "%s" +cast.incompatible=incompatible typecast from "%s" to "%s" invariant.cast.unsafe=cannot cast from "%s" to "%s" cast.unsafe.constructor.invocation=constructor invocation cast from "%s" to "%s" cannot be statically verified exception.parameter.invalid=invalid type in catch argument.%nfound : %s%nrequired: %s diff --git a/framework/tests/value/Basics.java b/framework/tests/value/Basics.java index 05787674e97..a0926304f39 100644 --- a/framework/tests/value/Basics.java +++ b/framework/tests/value/Basics.java @@ -170,7 +170,7 @@ public void intCastTest(@IntVal({0, 1}) int input) { @IntVal({0, 1, 2}) int sc = (@IntVal({0, 1, 2}) int) input; // :: warning: (cast.unsafe) @IntVal({1}) int uc = (@IntVal({1}) int) input; - // :: warning: (cast.unsafe) + // :: error: (cast.incompatible) @IntVal({2}) int bc = (@IntVal({2}) int) input; } diff --git a/framework/tests/value/Issue2367.java b/framework/tests/value/Issue2367.java index f2b9e9126e8..42a4b441c18 100644 --- a/framework/tests/value/Issue2367.java +++ b/framework/tests/value/Issue2367.java @@ -12,13 +12,13 @@ public class Issue2367 { // Without the `(byte)` cast, all of these produce the following javac error: // error: incompatible types: possible lossy conversion from int to byte - // The Value Checker's `cast.unsafe` error is analogous and is desirable. + // The Value Checker's `cast.incompatible` error is analogous and is desirable. - // :: warning: (cast.unsafe) + // :: error: (cast.incompatible) byte b4 = (byte) 139; // b4 == -117 - // :: warning: (cast.unsafe) + // :: error: (cast.incompatible) byte b5 = (byte) -240; - // :: warning: (cast.unsafe) + // :: error: (cast.incompatible) byte b6 = (byte) 251; // Outside the signed byte range, but written as a hexadecimal literal. @@ -29,6 +29,6 @@ public class Issue2367 { // The program element "(byte) 0x8B" has already been converted to "(byte)139" by javac before // the Checker Framework gets access to it. - // :: warning: (cast.unsafe) + // :: error: (cast.incompatible) byte b7 = (byte) 0x8B; // 0x8B == 137, and b4 == -117 } diff --git a/framework/tests/value/Overflows.java b/framework/tests/value/Overflows.java index 72b1806bca7..36a2c0a0611 100644 --- a/framework/tests/value/Overflows.java +++ b/framework/tests/value/Overflows.java @@ -4,19 +4,19 @@ public class Overflows { static void bytes() { byte max = Byte.MAX_VALUE; - // :: warning: (cast.unsafe) + // :: error: (cast.incompatible) @IntVal(-128) byte maxPlus1 = (byte) (max + 1); } static void chars() { char max = Character.MAX_VALUE; - // :: warning: (cast.unsafe) + // :: error: (cast.incompatible) @IntVal(0) char maxPlus1 = (char) (max + 1); } static void shorts() { short max = Short.MAX_VALUE; - // :: warning: (cast.unsafe) + // :: error: (cast.incompatible) @IntVal(-32768) short maxPlus1 = (short) (max + 1); } diff --git a/framework/tests/value/TypeCast.java b/framework/tests/value/TypeCast.java index 959bb4d7c78..089b121efc9 100644 --- a/framework/tests/value/TypeCast.java +++ b/framework/tests/value/TypeCast.java @@ -45,12 +45,12 @@ void otherCast() { void rangeCast(@IntRange(from = 127, to = 128) int a, @IntRange(from = 128, to = 129) int b) { @IntRange(from = 0, to = 128) - // :: error: (assignment.type.incompatible) :: warning: (cast.unsafe) + // :: error: (assignment.type.incompatible) :: error: (cast.incompatible) byte c = (byte) a; // (byte) a is @IntRange(from = -128, to = 127) because of casting @IntRange(from = -128, to = -127) - // :: warning: (cast.unsafe) + // :: error: (cast.incompatible) byte d = (byte) b; } } diff --git a/framework/tests/value/Underflows.java b/framework/tests/value/Underflows.java index 3c51eedad78..8204eec61e5 100644 --- a/framework/tests/value/Underflows.java +++ b/framework/tests/value/Underflows.java @@ -3,19 +3,19 @@ public class Underflows { static void bytes() { byte min = Byte.MIN_VALUE; - // :: warning: (cast.unsafe) + // :: error: (cast.incompatible) @IntVal(127) byte maxPlus1 = (byte) (min - 1); } static void chars() { char min = Character.MIN_VALUE; - // :: warning: (cast.unsafe) + // :: error: (cast.incompatible) @IntVal(65535) char maxPlus1 = (char) (min - 1); } static void shorts() { short min = Short.MIN_VALUE; - // :: warning: (cast.unsafe) + // :: error: (cast.incompatible) @IntVal(32767) short maxPlus1 = (short) (min - 1); } From 78ecdebf414643a9fbc594877ab3afbe208807ad Mon Sep 17 00:00:00 2001 From: Haifeng Shi Date: Sat, 25 Jun 2022 09:55:16 -0400 Subject: [PATCH 09/14] refined the approach when casting T to T --- .../Issue1315.java | 2 +- checker/tests/nullness/CastsNullness.java | 4 +- .../nullness/java8inference/Issue1032.java | 2 +- .../common/basetype/BaseTypeVisitor.java | 44 +++++++++++-------- .../framework/type/QualifierHierarchy.java | 27 ++++++++++++ 5 files changed, 56 insertions(+), 23 deletions(-) diff --git a/checker/tests/nullness-checkcastelementtype/Issue1315.java b/checker/tests/nullness-checkcastelementtype/Issue1315.java index 76fd8fb342e..3fb1afe1d52 100644 --- a/checker/tests/nullness-checkcastelementtype/Issue1315.java +++ b/checker/tests/nullness-checkcastelementtype/Issue1315.java @@ -13,7 +13,7 @@ static class Box { @SuppressWarnings("unchecked") T test1(@Nullable Object p) { - // :: error: (cast.incompatible) + // :: warning: (cast.unsafe) return (T) p; } // The Nullness Checker should not issue a cast.unsafe warning, diff --git a/checker/tests/nullness/CastsNullness.java b/checker/tests/nullness/CastsNullness.java index 7c468a9a711..cb5deb34eec 100644 --- a/checker/tests/nullness/CastsNullness.java +++ b/checker/tests/nullness/CastsNullness.java @@ -88,9 +88,9 @@ void m() { // :: error: (assignment.type.incompatible) t = (@Nullable T) null; nt = (@Nullable T) null; - // :: error: (cast.incompatible) + // :: warning: (cast.unsafe) t = (T) null; - // :: error: (cast.incompatible) + // :: warning: (cast.unsafe) nt = (T) null; } } diff --git a/checker/tests/nullness/java8inference/Issue1032.java b/checker/tests/nullness/java8inference/Issue1032.java index 273dd2779fa..b09a59413fd 100644 --- a/checker/tests/nullness/java8inference/Issue1032.java +++ b/checker/tests/nullness/java8inference/Issue1032.java @@ -16,7 +16,7 @@ public class Issue1032 { return arg.map(Issue1032::castStringToNonNull); } - @SuppressWarnings("nullness", "cast.unsafe") + @SuppressWarnings("nullness") static @NonNull T castTToNonNull(@Nullable T arg) { return (@NonNull T) arg; } 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 2a71c1322da..9b75a3a456b 100644 --- a/framework/src/main/java/org/checkerframework/common/basetype/BaseTypeVisitor.java +++ b/framework/src/main/java/org/checkerframework/common/basetype/BaseTypeVisitor.java @@ -2367,8 +2367,8 @@ protected CastSafeKind isUpcast(AnnotatedTypeMirror castType, AnnotatedTypeMirro QualifierHierarchy qualifierHierarchy = atypeFactory.getQualifierHierarchy(); Set castAnnos; TypeKind castTypeKind = castType.getKind(); - boolean flagEnabled = checker.hasOption("checkCastElementType"); - if (!flagEnabled) { + boolean checkCastElementType = checker.hasOption("checkCastElementType"); + if (!checkCastElementType) { // checkCastElementType option wasn't specified, so only check effective annotations. castAnnos = castType.getEffectiveAnnotations(); } else { @@ -2386,13 +2386,13 @@ protected CastSafeKind isUpcast(AnnotatedTypeMirror castType, AnnotatedTypeMirro } if (!atypeFactory.getTypeHierarchy().isSubtype(newExprType, newCastType)) { - return CastSafeKind.ERROR; + return CastSafeKind.WARNING; } if (newCastType.getKind() == TypeKind.ARRAY && newExprType.getKind() != TypeKind.ARRAY) { // Always warn if the cast contains an array, but the expression // doesn't, as in "(Object[]) o" where o is of type Object - return CastSafeKind.ERROR; + return CastSafeKind.WARNING; } else if (newCastType.getKind() == TypeKind.DECLARED && newExprType.getKind() == TypeKind.DECLARED) { int castSize = ((AnnotatedDeclaredType) newCastType).getTypeArguments().size(); @@ -2402,7 +2402,7 @@ protected CastSafeKind isUpcast(AnnotatedTypeMirror castType, AnnotatedTypeMirro // Always warn if the cast and expression contain a different number of type // arguments, e.g. to catch a cast from "Object" to "List<@NonNull Object>". // TODO: the same number of arguments actually doesn't guarantee anything. - return CastSafeKind.ERROR; + return CastSafeKind.WARNING; } } else if (castTypeKind == TypeKind.TYPEVAR && exprType.getKind() == TypeKind.TYPEVAR) { // If both the cast type and the casted expression are type variables, then check @@ -2419,7 +2419,7 @@ protected CastSafeKind isUpcast(AnnotatedTypeMirror castType, AnnotatedTypeMirro && qualifierHierarchy.isSubtype( exprType.getEffectiveAnnotations(), castType.getEffectiveAnnotations()); - return result ? CastSafeKind.SAFE : CastSafeKind.ERROR; + return result ? CastSafeKind.SAFE : CastSafeKind.WARNING; } if (castTypeKind == TypeKind.TYPEVAR) { // If the cast type is a type var, but the expression is not, then check that the @@ -2435,11 +2435,19 @@ protected CastSafeKind isUpcast(AnnotatedTypeMirror castType, AnnotatedTypeMirro // Return a warning in this case, as compiler will not report the unchecked warning in // this case, and we cannot statically verify the subtype relation of the annotations. if (castTypeKind == TypeKind.TYPEVAR && exprType.getKind() == TypeKind.TYPEVAR) { - TypeMirror castJavaType = castType.getUnderlyingType(); - TypeMirror exprJavaType = exprType.getUnderlyingType(); - if (TypesUtils.areSameTypeVariables( - (TypeVariable) castJavaType, (TypeVariable) exprJavaType)) { - return CastSafeKind.WARNING; + TypeVariable castTV = (TypeVariable) castType.getUnderlyingType(); + TypeVariable exprTV = (TypeVariable) exprType.getUnderlyingType(); + if (TypesUtils.areSameTypeVariables(castTV, exprTV)) { + Set t1 = + AnnotatedTypes.findEffectiveLowerBoundAnnotations( + qualifierHierarchy, castType); + Set t2 = + AnnotatedTypes.findEffectiveAnnotations(qualifierHierarchy, exprType); + + if (!qualifierHierarchy.isSubtype(t2, t1)) { + return CastSafeKind.WARNING; + } + return CastSafeKind.SAFE; } } @@ -2449,9 +2457,10 @@ protected CastSafeKind isUpcast(AnnotatedTypeMirror castType, AnnotatedTypeMirro if (result) { return CastSafeKind.SAFE; - } else if (flagEnabled) { // when the flag is enabled and it is not an upcast, return an + } else if (checkCastElementType) { // when the flag is enabled and it is not an upcast, + // return an // error - return CastSafeKind.ERROR; + return CastSafeKind.WARNING; } else { return CastSafeKind.NOT_UPCAST; } @@ -2470,7 +2479,8 @@ protected CastSafeKind isSafeDowncast( Set exprAnnos = exprType.getEffectiveAnnotations(); QualifierHierarchy qualifierHierarchy = atypeFactory.getQualifierHierarchy(); - if (!qualifierHierarchy.isSubtype(castAnnos, exprAnnos)) { // not a downcast + for (int i = 0; i < castAnnos.size(); i++) {} + if (!qualifierHierarchy.isComparable(castAnnos, exprAnnos)) { // exists an incomparable cast return CastSafeKind.NOT_DOWNCAST; } @@ -2485,11 +2495,7 @@ protected CastSafeKind isSafeDowncast( atypeFactory.getTypeDeclarationBounds(castDeclared.getUnderlyingType()); if (AnnotationUtils.areSame(castDeclared.getAnnotations(), bounds)) { - TypeMirror castJavaType = castType.getUnderlyingType(); - TypeMirror exprJavaType = exprType.getUnderlyingType(); - if (TypesUtils.isErasedSubtype(castJavaType, exprJavaType, types)) { - return CastSafeKind.SAFE; - } + return CastSafeKind.SAFE; } } return CastSafeKind.WARNING; diff --git a/framework/src/main/java/org/checkerframework/framework/type/QualifierHierarchy.java b/framework/src/main/java/org/checkerframework/framework/type/QualifierHierarchy.java index 1fb69df13cb..fd9f1067f5a 100644 --- a/framework/src/main/java/org/checkerframework/framework/type/QualifierHierarchy.java +++ b/framework/src/main/java/org/checkerframework/framework/type/QualifierHierarchy.java @@ -149,6 +149,33 @@ default boolean isSubtype( return true; } + /** + * Tests whether all qualifiers in {@code qualifiers1} are comparable with the qualifier in the + * same hierarchy in {@code qualifiers2}. + * + * @param qualifiers1 set of qualifiers; exactly one per hierarchy + * @param qualifiers2 set of qualifiers; exactly one per hierarchy + * @return true iff all qualifiers in {@code qualifiers1} are comparable with qualifiers in + * {@code qualifiers2} + */ + default boolean isComparable( + Collection qualifiers1, + Collection qualifiers2) { + assertSameSize(qualifiers1, qualifiers2); + for (AnnotationMirror subQual : qualifiers1) { + AnnotationMirror superQual = findAnnotationInSameHierarchy(qualifiers2, subQual); + if (superQual == null) { + throw new BugInCF( + "QualifierHierarchy: missing annotation in hierarchy %s. found: %s", + subQual, StringsPlume.join(",", qualifiers2)); + } + if (!isSubtype(subQual, superQual) && !isSubtype(superQual, subQual)) { + return false; + } + } + return true; + } + /** * Returns the least upper bound (LUB) of the qualifiers {@code qualifier1} and {@code * qualifier2}. Returns {@code null} if the qualifiers are not from the same qualifier From 87d3d7462ec6a7c387963d41e8ef60b00888256e Mon Sep 17 00:00:00 2001 From: Haifeng Shi Date: Sat, 25 Jun 2022 11:40:40 -0400 Subject: [PATCH 10/14] suppress some warnings --- .../checkerframework/checker/nullness/util/NullnessUtil.java | 2 ++ checker/tests/nullness/CastTypeVariable.java | 1 - 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/checker-util/src/main/java/org/checkerframework/checker/nullness/util/NullnessUtil.java b/checker-util/src/main/java/org/checkerframework/checker/nullness/util/NullnessUtil.java index d7877d31455..ae79c99afbf 100644 --- a/checker-util/src/main/java/org/checkerframework/checker/nullness/util/NullnessUtil.java +++ b/checker-util/src/main/java/org/checkerframework/checker/nullness/util/NullnessUtil.java @@ -67,6 +67,7 @@ private NullnessUtil() { * @return the argument, casted to have the type qualifier @NonNull */ @EnsuresNonNull("#1") + @SuppressWarnings("cast.unsafe") public static @NonNull T castNonNull(@Nullable T ref) { assert ref != null : "Misuse of castNonNull: called with a null argument"; return (@NonNull T) ref; @@ -82,6 +83,7 @@ private NullnessUtil() { * @param message text to include if this method is misused * @return the argument, casted to have the type qualifier @NonNull */ + @SuppressWarnings("cast.unsafe") public static @EnsuresNonNull("#1") @NonNull T castNonNull( @Nullable T ref, String message) { assert ref != null : "Misuse of castNonNull: called with a null argument: " + message; diff --git a/checker/tests/nullness/CastTypeVariable.java b/checker/tests/nullness/CastTypeVariable.java index 5364cdec637..323ed8c0487 100644 --- a/checker/tests/nullness/CastTypeVariable.java +++ b/checker/tests/nullness/CastTypeVariable.java @@ -9,7 +9,6 @@ class MyAnnotatedTypeVariable extends MyAnnotatedTypeMirror {} public class CastTypeVariable { public static V mapGetHelper( Map mappings, MyAnnotatedTypeVariable key) { - // :: warning: (cast.unsafe) V possValue = (V) mappings.get(key); // :: error: (dereference.of.nullable) possValue.addAnnotations(); From 53a732daf2734eb9da2712251ef16f46d053c0a5 Mon Sep 17 00:00:00 2001 From: Haifeng Shi Date: Sun, 26 Jun 2022 11:10:38 -0400 Subject: [PATCH 11/14] refine the logic to cast from T to T --- .../checker/nullness/util/NullnessUtil.java | 2 -- .../common/basetype/BaseTypeVisitor.java | 26 +++++++++++++------ 2 files changed, 18 insertions(+), 10 deletions(-) diff --git a/checker-util/src/main/java/org/checkerframework/checker/nullness/util/NullnessUtil.java b/checker-util/src/main/java/org/checkerframework/checker/nullness/util/NullnessUtil.java index ae79c99afbf..d7877d31455 100644 --- a/checker-util/src/main/java/org/checkerframework/checker/nullness/util/NullnessUtil.java +++ b/checker-util/src/main/java/org/checkerframework/checker/nullness/util/NullnessUtil.java @@ -67,7 +67,6 @@ private NullnessUtil() { * @return the argument, casted to have the type qualifier @NonNull */ @EnsuresNonNull("#1") - @SuppressWarnings("cast.unsafe") public static @NonNull T castNonNull(@Nullable T ref) { assert ref != null : "Misuse of castNonNull: called with a null argument"; return (@NonNull T) ref; @@ -83,7 +82,6 @@ private NullnessUtil() { * @param message text to include if this method is misused * @return the argument, casted to have the type qualifier @NonNull */ - @SuppressWarnings("cast.unsafe") public static @EnsuresNonNull("#1") @NonNull T castNonNull( @Nullable T ref, String message) { assert ref != null : "Misuse of castNonNull: called with a null argument: " + message; 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 9b75a3a456b..1efa2b64302 100644 --- a/framework/src/main/java/org/checkerframework/common/basetype/BaseTypeVisitor.java +++ b/framework/src/main/java/org/checkerframework/common/basetype/BaseTypeVisitor.java @@ -2349,10 +2349,15 @@ protected void checkTypecastSafety(TypeCastTree typeCastTree) { /** CastSafeKind */ protected enum CastSafeKind { + /** The cast is safe */ SAFE, + /** The cast is illegal */ ERROR, + /** Cannot statically verify the cast, report a warning */ WARNING, + /** It's not an upcast */ NOT_UPCAST, + /** It's not a downcast */ NOT_DOWNCAST } @@ -2438,16 +2443,22 @@ protected CastSafeKind isUpcast(AnnotatedTypeMirror castType, AnnotatedTypeMirro TypeVariable castTV = (TypeVariable) castType.getUnderlyingType(); TypeVariable exprTV = (TypeVariable) exprType.getUnderlyingType(); if (TypesUtils.areSameTypeVariables(castTV, exprTV)) { - Set t1 = + Set castLower = AnnotatedTypes.findEffectiveLowerBoundAnnotations( qualifierHierarchy, castType); - Set t2 = + Set exprLower = + AnnotatedTypes.findEffectiveLowerBoundAnnotations( + qualifierHierarchy, exprType); + Set castUpper = + AnnotatedTypes.findEffectiveAnnotations(qualifierHierarchy, castType); + Set exprUpper = AnnotatedTypes.findEffectiveAnnotations(qualifierHierarchy, exprType); - if (!qualifierHierarchy.isSubtype(t2, t1)) { - return CastSafeKind.WARNING; + if (qualifierHierarchy.isSubtype(exprLower, castLower) + && qualifierHierarchy.isSubtype(exprUpper, castUpper)) { + return CastSafeKind.SAFE; } - return CastSafeKind.SAFE; + return CastSafeKind.WARNING; } } @@ -2457,9 +2468,8 @@ protected CastSafeKind isUpcast(AnnotatedTypeMirror castType, AnnotatedTypeMirro if (result) { return CastSafeKind.SAFE; - } else if (checkCastElementType) { // when the flag is enabled and it is not an upcast, - // return an - // error + } else if (checkCastElementType) { + // when the flag is enabled, and it is not an upcast, return an error return CastSafeKind.WARNING; } else { return CastSafeKind.NOT_UPCAST; From d31ef8f3d5f56403b73482c7f6562eb70e20eda5 Mon Sep 17 00:00:00 2001 From: Haifeng Shi Date: Sun, 26 Jun 2022 19:24:07 -0400 Subject: [PATCH 12/14] add method documentation --- .../common/basetype/BaseTypeVisitor.java | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 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 1efa2b64302..74b9244e865 100644 --- a/framework/src/main/java/org/checkerframework/common/basetype/BaseTypeVisitor.java +++ b/framework/src/main/java/org/checkerframework/common/basetype/BaseTypeVisitor.java @@ -2362,11 +2362,11 @@ protected enum CastSafeKind { } /** - * isUpcast + * Return true if it's an upcast (from the view of the qualifiers) and it's safe. * * @param castType castType * @param exprType exprType - * @return CastSafeKind + * @return Can return NOT_UPCAST, WARNING or SAFE CastSafeKind. */ protected CastSafeKind isUpcast(AnnotatedTypeMirror castType, AnnotatedTypeMirror exprType) { QualifierHierarchy qualifierHierarchy = atypeFactory.getQualifierHierarchy(); @@ -2477,11 +2477,11 @@ protected CastSafeKind isUpcast(AnnotatedTypeMirror castType, AnnotatedTypeMirro } /** - * isSafeDowncast + * Return true if it's a downcast (from the view of the qualifiers) and the cast is safe. * * @param castType castType * @param exprType exprType - * @return CastSafeKind + * @return Can return SAFE, NOT_DOWNCAST or WARNING. */ protected CastSafeKind isSafeDowncast( AnnotatedTypeMirror castType, AnnotatedTypeMirror exprType) { @@ -2511,6 +2511,14 @@ protected CastSafeKind isSafeDowncast( return CastSafeKind.WARNING; } + /** + * If it's an incomparable cast in terms of qualifiers, return ERROR by default. Subchecker can + * override this method to allow certain incomparable casts. + * + * @param castType castType + * @param exprType exprType + * @return CastSafeKind.ERROR if it's an incomparable cast. + */ protected CastSafeKind isSafeIncomparableCast( AnnotatedTypeMirror castType, AnnotatedTypeMirror exprType) { return CastSafeKind.ERROR; From 0e2fac308af24880d2e574294fa8a1dbe17605b7 Mon Sep 17 00:00:00 2001 From: Haifeng Shi Date: Sun, 7 Aug 2022 21:44:18 -0400 Subject: [PATCH 13/14] remove useless for loop --- .../org/checkerframework/common/basetype/BaseTypeVisitor.java | 1 - 1 file changed, 1 deletion(-) 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 74b9244e865..6f011f2a8c7 100644 --- a/framework/src/main/java/org/checkerframework/common/basetype/BaseTypeVisitor.java +++ b/framework/src/main/java/org/checkerframework/common/basetype/BaseTypeVisitor.java @@ -2489,7 +2489,6 @@ protected CastSafeKind isSafeDowncast( Set exprAnnos = exprType.getEffectiveAnnotations(); QualifierHierarchy qualifierHierarchy = atypeFactory.getQualifierHierarchy(); - for (int i = 0; i < castAnnos.size(); i++) {} if (!qualifierHierarchy.isComparable(castAnnos, exprAnnos)) { // exists an incomparable cast return CastSafeKind.NOT_DOWNCAST; } From 3aad9022909a12088a1e184aed01135d25fe4739 Mon Sep 17 00:00:00 2001 From: Haifeng Shi Date: Mon, 12 Sep 2022 10:41:17 -0400 Subject: [PATCH 14/14] update mustcallvisitor, signedness visitor --- .../checkerframework/checker/mustcall/MustCallVisitor.java | 5 +++-- .../checker/signedness/SignednessVisitor.java | 5 +++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/checker/src/main/java/org/checkerframework/checker/mustcall/MustCallVisitor.java b/checker/src/main/java/org/checkerframework/checker/mustcall/MustCallVisitor.java index c35de186ba5..22d18b6d707 100644 --- a/checker/src/main/java/org/checkerframework/checker/mustcall/MustCallVisitor.java +++ b/checker/src/main/java/org/checkerframework/checker/mustcall/MustCallVisitor.java @@ -366,9 +366,10 @@ private boolean noMustCallObligation(AnnotatedTypeMirror atm) { } @Override - protected boolean isTypeCastSafe(AnnotatedTypeMirror castType, AnnotatedTypeMirror exprType) { + protected CastSafeKind isTypeCastSafe( + AnnotatedTypeMirror castType, AnnotatedTypeMirror exprType) { if (noMustCallObligation(castType) || noMustCallObligation(exprType)) { - return true; + return CastSafeKind.SAFE; } return super.isTypeCastSafe(castType, exprType); diff --git a/checker/src/main/java/org/checkerframework/checker/signedness/SignednessVisitor.java b/checker/src/main/java/org/checkerframework/checker/signedness/SignednessVisitor.java index 4cc3ac4a965..411428ad3ec 100644 --- a/checker/src/main/java/org/checkerframework/checker/signedness/SignednessVisitor.java +++ b/checker/src/main/java/org/checkerframework/checker/signedness/SignednessVisitor.java @@ -384,10 +384,11 @@ public Void visitCompoundAssignment(CompoundAssignmentTree node, Void p) { } @Override - protected boolean isTypeCastSafe(AnnotatedTypeMirror castType, AnnotatedTypeMirror exprType) { + protected CastSafeKind isTypeCastSafe( + AnnotatedTypeMirror castType, AnnotatedTypeMirror exprType) { if (!atypeFactory.maybeIntegral(castType)) { // If the cast is not a number or a char, then it is legal. - return true; + return CastSafeKind.SAFE; } return super.isTypeCastSafe(castType, exprType); }