Skip to content
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

Improve cast warnings/errors logic #337

Closed
wants to merge 63 commits into from
Closed
Changes from 1 commit
Commits
Show all changes
63 commits
Select commit Hold shift + click to select a range
298df8a
add downcast to cf
AndrewShf Jun 14, 2022
c4588df
add downcast to cf
AndrewShf Jun 15, 2022
82740f7
add downcast/incomparable cast to cf
AndrewShf Jun 17, 2022
d71ead6
use getEffectiveAnnotation
AndrewShf Jun 18, 2022
0dec947
check the java type down cast
AndrewShf Jun 18, 2022
f5c5d22
revert typo fix, fix in another pr
AndrewShf Jun 19, 2022
c916493
add incomparable cast message key
AndrewShf Jun 22, 2022
bfa5794
replace some warnings with incompatible cast errors
AndrewShf Jun 23, 2022
93a6a1a
refined the approach when casting T to T
AndrewShf Jun 25, 2022
b04007c
suppress some warnings
AndrewShf Jun 25, 2022
9b116a7
refine the logic to cast from T to T
AndrewShf Jun 26, 2022
b88949f
add method documentation
AndrewShf Jun 26, 2022
361f487
remove useless for loop
AndrewShf Aug 8, 2022
874e594
update mustcallvisitor, signedness visitor
AndrewShf Sep 12, 2022
09904ba
Merge branch 'master' into eisop_typecast
wmdietl Oct 12, 2022
9862649
Merge branch 'master' into eisop_typecast
wmdietl Oct 13, 2022
b8f1549
Merge remote-tracking branch 'eisop/master' into eisop_typecast
AndrewShf Oct 13, 2022
deffd1d
add one test file and improve documentation
AndrewShf Oct 13, 2022
4f476f2
move testfile to another directory
AndrewShf Oct 14, 2022
f82dc77
fix testcase
AndrewShf Oct 14, 2022
efeeea2
fix testcase
AndrewShf Oct 17, 2022
c34285f
improve naming in the error message
AndrewShf Nov 1, 2022
16d69a6
add comments
AndrewShf Nov 2, 2022
4cd7e9a
Merge branch 'master' into eisop_typecast
wmdietl Nov 4, 2022
cb0cb64
Merge branch 'master' into eisop_typecast
wmdietl Feb 22, 2023
d7429e4
solve field shadowing
AndrewShf Feb 24, 2023
8502489
update error messages
AndrewShf Feb 24, 2023
72e074e
simplify branches
AndrewShf Feb 24, 2023
1cfd105
update
AndrewShf Feb 24, 2023
e92f0c6
Merge branch 'master' into eisop_typecast
wmdietl Mar 1, 2023
0658def
Merge branch 'master' into eisop_typecast
wmdietl Apr 25, 2023
7991a8b
merge with eisop master
AndrewShf Apr 27, 2023
4ffe0f1
merge with eisop master
AndrewShf May 8, 2023
fa1439e
merge code in valuevisitor
AndrewShf May 8, 2023
a15d727
change name of the enum
AndrewShf Jul 3, 2023
bfee453
remove daikon checkout id
AndrewShf Jul 5, 2023
20d62af
add daikon checkout back, as it is resolved by another pr
AndrewShf Jul 5, 2023
8e0f908
Merge remote-tracking branch 'eisop/master' into eisop_typecast
AndrewShf Jul 5, 2023
91b22c9
Merge branch 'master' into eisop_typecast
AndrewShf Sep 1, 2023
09cc33d
Merge branch 'master' into eisop_typecast
wmdietl Sep 26, 2023
24105c4
Merge remote-tracking branch 'eisop/master' into eisop_typecast
AndrewShf Sep 26, 2023
c2bea34
Merge remote-tracking branch 'origin/eisop_typecast' into eisop_typecast
AndrewShf Sep 27, 2023
1ec438e
add changelog entry
AndrewShf Sep 27, 2023
a6fb6ce
Merge branch 'master' into eisop_typecast
AndrewShf Sep 27, 2023
724a95e
Merge branch 'master' into eisop_typecast
AndrewShf Sep 29, 2023
d241446
Merge branch 'master' into eisop_typecast
AndrewShf Sep 30, 2023
07eaacb
Merge branch 'master' into eisop_typecast
wmdietl Sep 30, 2023
4229827
Merge branch 'master' into eisop_typecast
wmdietl Oct 1, 2023
fc9ad98
Enum rename; Improve comments
AndrewShf Oct 2, 2023
d06ad1f
rename enum and improve comments in the test file CastFromTtoT.java
AndrewShf Oct 2, 2023
f4ffe07
improve jovadoc
AndrewShf Oct 2, 2023
ac60c2e
move isTypeCastSafe to an earlier position
AndrewShf Oct 3, 2023
0115fbe
Merge remote-tracking branch 'eisop/master' into eisop_typecast
AndrewShf Nov 20, 2023
67ba76b
use new apis for the change
AndrewShf Nov 20, 2023
0b78587
fix error messages on the test files
AndrewShf Nov 20, 2023
1b8f4ab
fix error messages on the test files
AndrewShf Nov 20, 2023
2d969fc
Merge remote-tracking branch 'eisop/master' into eisop_typecast
AndrewShf Nov 22, 2023
1b20546
improve document
AndrewShf Nov 22, 2023
b94311d
Merge branch 'master' into eisop_typecast
AndrewShf Nov 22, 2023
98060e3
Merge branch 'master' into eisop_typecast
wmdietl Jan 3, 2024
f4d4ec1
Merge branch 'master' into eisop_typecast
wmdietl Jan 4, 2024
0831233
Merge branch 'master' into eisop_typecast
wmdietl Jul 14, 2024
9147d86
Merge branch 'master' into eisop_typecast
wmdietl Dec 27, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Enum rename; Improve comments
Co-authored-by: Werner Dietl <wdietl@gmail.com>
  • Loading branch information
AndrewShf and wmdietl authored Oct 2, 2023
commit fc9ad98d57eefc492325c051b3132ffe2b7dc736
Original file line number Diff line number Diff line change
@@ -113,7 +113,7 @@ protected void commonAssignmentCheck(
}

@Override
protected TypecastKind isTypeCastSafe(
protected TypeCastKind isTypeCastSafe(
AnnotatedTypeMirror castType, AnnotatedTypeMirror exprType) {

AnnotationMirror exprLTAnno =
Original file line number Diff line number Diff line change
@@ -980,10 +980,10 @@ DeclaredType typeToCheck() {
}

@Override
protected TypecastKind isTypeCastSafe(
protected TypeCastKind isTypeCastSafe(
AnnotatedTypeMirror castType, AnnotatedTypeMirror exprType) {
if (castType.getKind().isPrimitive()) {
return TypecastKind.SAFE;
return TypeCastKind.SAFE;
}
return super.isTypeCastSafe(castType, exprType);
}
Original file line number Diff line number Diff line change
@@ -374,10 +374,10 @@ private boolean noMustCallObligation(AnnotatedTypeMirror atm) {
}

@Override
protected TypecastKind isTypeCastSafe(
protected TypeCastKind isTypeCastSafe(
AnnotatedTypeMirror castType, AnnotatedTypeMirror exprType) {
if (noMustCallObligation(castType) || noMustCallObligation(exprType)) {
return TypecastKind.SAFE;
return TypeCastKind.SAFE;
}

return super.isTypeCastSafe(castType, exprType);
Original file line number Diff line number Diff line change
@@ -384,11 +384,11 @@ public Void visitCompoundAssignment(CompoundAssignmentTree tree, Void p) {
}

@Override
protected TypecastKind isTypeCastSafe(
protected TypeCastKind isTypeCastSafe(
AnnotatedTypeMirror castType, AnnotatedTypeMirror exprType) {
if (!atypeFactory.maybeIntegral(castType)) {
// If the cast is not a number or a char, then it is legal.
return TypecastKind.SAFE;
return TypeCastKind.SAFE;
}
return super.isTypeCastSafe(castType, exprType);
}
2 changes: 1 addition & 1 deletion checker/tests/signedness/CastFromTtoT.java
Original file line number Diff line number Diff line change
@@ -26,7 +26,7 @@ T bar2(@Signed T p) {
}

// Looks like an upcast, but it's a downcast.
// This time, as arugment p has type T, which is the same as the casting type
// This time, as argument p has type T, which is the same as the casting type
// and the compiler will not issue a warning. So we should give a warning.
void foo2(Inner<@SignednessGlb Integer> s, @Signed Integer local) {
@SignednessGlb Integer x = s.bar2(local);
2 changes: 1 addition & 1 deletion docs/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -41,7 +41,7 @@ Changed the return types of
- `GenericAnnotatedTypeFactory#getFinalLocalValues()` to `Map<VariableElement, Value>`.

Refactored the implementation of `isTypeCastSafe` to categorize the kinds of a typecast, whether
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isTypeCastSafe previously returned a boolean, so the is beginning makes sense. It now returns an enum. Would a different name be better to describe the wider task? computeTypeCastKind? Although Kind isn't particularly specific either... more on that with the enum.

it's an upcast, downcast or incomparable cast. Based on that, further determine if the typecast
it is an upcast, downcast or incomparable cast. Based on that, further determine if the typecast
is statically verifiable or not.

**Closed issues:**
Original file line number Diff line number Diff line change
@@ -2474,7 +2474,7 @@ 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 (!reported) {
TypecastKind castResult = isTypeCastSafe(castType, exprType);
TypeCastKind castResult = isTypeCastSafe(castType, exprType);

if (castResult == TypecastKind.WARNING) {
checker.reportWarning(
@@ -2492,17 +2492,17 @@ protected void checkTypecastSafety(TypeCastTree typeCastTree) {
}
}

/** To represent all possible kinds of typecast */
protected enum TypecastKind {
/** The cast is safe */
/** Represents all possible kinds of typecasts. */
protected enum TypeCastKind {
/** The cast is safe. */
SAFE,
/** The cast is illegal */
/** The cast is illegal. */
ERROR,
/** Cannot statically verify the cast, report a warning */
/** Cannot statically verify the cast, report a warning. */
WARNING,
/** It's not an upcast */
/** It is not an upcast. */
NOT_UPCAST,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think having only three possible values would be better, instead of the two extra NOT_ cases that are only used as the results for the helper methods.

Copy link
Member Author

@AndrewShf AndrewShf Oct 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously, I use NOT_UPCAST to represent a state that needs to be further processed, evaluated. SAFE, ERROR, WARNING all mean that we already get the result so that we could return our results early.

To remove NOT_UPCAST, NOT_DOWNCAST, the only way I can think of is putting null into the enum variable, to represent current isTypecastSafe information is not-yet decided. Or we could use UNKNOWN to replace these two.

/** It's not a downcast */
/** It is not a downcast. */
NOT_DOWNCAST
}

@@ -2516,7 +2516,7 @@ protected enum TypecastKind {
* @param exprType annotated type of the casted expression
* @return return NOT_UPCAST, WARNING or SAFE TypecastKind.
*/
protected TypecastKind isUpcast(AnnotatedTypeMirror castType, AnnotatedTypeMirror exprType) {
protected TypeCastKind isUpcast(AnnotatedTypeMirror castType, AnnotatedTypeMirror exprType) {
QualifierHierarchy qualifierHierarchy = atypeFactory.getQualifierHierarchy();
AnnotationMirrorSet castAnnos;
TypeKind castTypeKind = castType.getKind();
@@ -2538,13 +2538,13 @@ protected TypecastKind isUpcast(AnnotatedTypeMirror castType, AnnotatedTypeMirro
}

if (!atypeFactory.getTypeHierarchy().isSubtype(newExprType, newCastType)) {
return TypecastKind.WARNING;
return TypeCastKind.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 TypecastKind.WARNING;
return TypeCastKind.WARNING;
} else if (newCastType.getKind() == TypeKind.DECLARED
&& newExprType.getKind() == TypeKind.DECLARED) {
int castSize = ((AnnotatedDeclaredType) newCastType).getTypeArguments().size();
@@ -2554,7 +2554,7 @@ protected TypecastKind 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 TypecastKind.WARNING;
return TypeCastKind.WARNING;
}
} else if (castTypeKind == TypeKind.TYPEVAR && exprType.getKind() == TypeKind.TYPEVAR) {
// If both the cast type and the casted expression are type variables, then check
@@ -2571,7 +2571,7 @@ protected TypecastKind isUpcast(AnnotatedTypeMirror castType, AnnotatedTypeMirro
&& qualifierHierarchy.isSubtype(
exprType.getEffectiveAnnotations(),
castType.getEffectiveAnnotations());
return result ? TypecastKind.SAFE : TypecastKind.WARNING;
return result ? TypeCastKind.SAFE : TypeCastKind.WARNING;
}
if (castTypeKind == TypeKind.TYPEVAR) {
// If the cast type is a type var, but the expression is not, then check that the
@@ -2607,9 +2607,9 @@ protected TypecastKind isUpcast(AnnotatedTypeMirror castType, AnnotatedTypeMirro
// return SAFE only it satisfies the below condition
if (qualifierHierarchy.isSubtype(exprLower, castLower)
&& qualifierHierarchy.isSubtype(exprUpper, castUpper)) {
return TypecastKind.SAFE;
return TypeCastKind.SAFE;
}
return TypecastKind.WARNING;
return TypeCastKind.WARNING;
}
}

@@ -2618,12 +2618,12 @@ protected TypecastKind isUpcast(AnnotatedTypeMirror castType, AnnotatedTypeMirro
qualifierHierarchy.isSubtype(exprTypeWidened.getEffectiveAnnotations(), castAnnos);

if (result) {
return TypecastKind.SAFE;
return TypeCastKind.SAFE;
} else if (checkCastElementType) {
// when the flag is enabled, and it is not an upcast, return a warning
return TypecastKind.WARNING;
return TypeCastKind.WARNING;
} else {
return TypecastKind.NOT_UPCAST;
return TypeCastKind.NOT_UPCAST;
}
}

@@ -2635,14 +2635,14 @@ protected TypecastKind isUpcast(AnnotatedTypeMirror castType, AnnotatedTypeMirro
* @param exprType annotated type of the casted expression
* @return Can return SAFE, NOT_DOWNCAST or WARNING.
*/
protected TypecastKind isSafeDowncast(
protected TypeCastKind isSafeDowncast(
AnnotatedTypeMirror castType, AnnotatedTypeMirror exprType) {
AnnotationMirrorSet castAnnos = castType.getEffectiveAnnotations();
AnnotationMirrorSet exprAnnos = exprType.getEffectiveAnnotations();
QualifierHierarchy qualifierHierarchy = atypeFactory.getQualifierHierarchy();

if (!qualifierHierarchy.isComparable(castAnnos, exprAnnos)) { // exists an incomparable cast
return TypecastKind.NOT_DOWNCAST;
return TypeCastKind.NOT_DOWNCAST;
}

// check if the downcast can be statically verified
@@ -2656,14 +2656,14 @@ protected TypecastKind isSafeDowncast(
atypeFactory.getTypeDeclarationBounds(castDeclared.getUnderlyingType());

if (AnnotationUtils.areSame(castDeclared.getAnnotations(), bounds)) {
return TypecastKind.SAFE;
return TypeCastKind.SAFE;
}
}
return TypecastKind.WARNING;
return TypeCastKind.WARNING;
}

/**
* If it's an incomparable cast in terms of qualifiers, return ERROR by default. Subchecker can
* If it is an incomparable cast in terms of qualifiers, return ERROR by default. Subchecker can
* override this method to allow certain incomparable casts.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is returned if it is not ERROR?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

addressed

*
* @param castType annotated type of the cast
@@ -2672,26 +2672,26 @@ protected TypecastKind isSafeDowncast(
*/
protected TypecastKind isSafeIncomparableCast(
AnnotatedTypeMirror castType, AnnotatedTypeMirror exprType) {
return TypecastKind.ERROR;
return TypeCastKind.ERROR;
}

/**
* This method returns TypecastKind.SAFE when the typecast is an upcast or a statically
* This method returns TypeCastKind.SAFE when the typecast is an upcast or a statically
* verifiable downcast. Returns TypecastKind.WARNING and ERROR for those downcasts which cannot
* be statically verified or some incomparable casts.
*
* @param castType annotated type of the cast
* @param exprType annotated type of the casted expression
* @return TypecastKind.SAFE if the typecast is safe, error or warning otherwise
*/
protected TypecastKind isTypeCastSafe(
protected TypeCastKind isTypeCastSafe(
AnnotatedTypeMirror castType, AnnotatedTypeMirror exprType) {
TypecastKind castResult = isUpcast(castType, exprType);
TypeCastKind castResult = isUpcast(castType, exprType);

// not upcast, do downcast and incomparable cast check
if (castResult == TypecastKind.NOT_UPCAST) {
if (castResult == TypeCastKind.NOT_UPCAST) {
castResult = isSafeDowncast(castType, exprType);
if (castResult == TypecastKind.NOT_DOWNCAST) { // fall into incomparable cast
if (castResult == TypeCastKind.NOT_DOWNCAST) { // fall into incomparable cast
return isSafeIncomparableCast(castType, exprType);
}
}
@@ -2708,7 +2708,7 @@ protected TypecastKind isTypeCastSafe(
*/
private boolean isInvariantTypeCastSafe(
AnnotatedTypeMirror castType, AnnotatedTypeMirror exprType, AnnotationMirror top) {
if (isTypeCastSafe(castType, exprType) != TypecastKind.SAFE) {
if (isTypeCastSafe(castType, exprType) != TypeCastKind.SAFE) {
return false;
}
AnnotationMirror castTypeAnno = castType.getEffectiveAnnotationInHierarchy(top);
@@ -2759,7 +2759,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) != TypecastKind.SAFE) {
if (isTypeCastSafe(variableType, expType) != TypeCastKind.SAFE) {
checker.reportWarning(tree, "instanceof.pattern.unsafe", expType, variableTree);
}
}
Original file line number Diff line number Diff line change
@@ -345,7 +345,7 @@ public Void visitTypeCast(TypeCastTree tree, Void p) {
// This method returns true for (@IntVal(-1), @IntVal(255)) if the underlying type is `byte`,
// but not for any other underlying type.
@Override
protected TypecastKind isTypeCastSafe(
protected TypeCastKind isTypeCastSafe(
AnnotatedTypeMirror castType, AnnotatedTypeMirror exprType) {
TypeKind castTypeKind =
TypeKindUtils.primitiveOrBoxedToTypeKind(castType.getUnderlyingType());
@@ -358,7 +358,7 @@ protected TypecastKind isTypeCastSafe(
AnnotationMirrorSet castAnnos = castType.getAnnotations();
AnnotationMirrorSet exprAnnos = exprType.getAnnotations();
if (castAnnos.equals(exprAnnos)) {
return TypecastKind.SAFE;
return TypeCastKind.SAFE;
}
assert castAnnos.size() == 1;
assert exprAnnos.size() == 1;
@@ -374,20 +374,20 @@ protected TypecastKind isTypeCastSafe(
switch (castTypeKind) {
case BYTE:
return castValues.get(0).byteValue() == exprValues.get(0).byteValue()
? TypecastKind.SAFE
: TypecastKind.ERROR;
? TypeCastKind.SAFE
: TypeCastKind.ERROR;
case INT:
return castValues.get(0).intValue() == exprValues.get(0).intValue()
? TypecastKind.SAFE
: TypecastKind.ERROR;
? TypeCastKind.SAFE
: TypeCastKind.ERROR;
case SHORT:
return castValues.get(0).shortValue() == exprValues.get(0).shortValue()
? TypecastKind.SAFE
: TypecastKind.ERROR;
? TypeCastKind.SAFE
: TypeCastKind.ERROR;
default:
return castValues.get(0).longValue() == exprValues.get(0).longValue()
? TypecastKind.SAFE
: TypecastKind.ERROR;
? TypeCastKind.SAFE
: TypeCastKind.ERROR;
}
} else {
switch (castTypeKind) {
@@ -402,8 +402,8 @@ protected TypecastKind isTypeCastSafe(
CollectionsPlume.mapList(
Number::byteValue, exprValues));
return sortedSetContainsAll(castValuesTree, exprValuesTree)
? TypecastKind.SAFE
: TypecastKind.ERROR;
? TypeCastKind.SAFE
: TypeCastKind.ERROR;
}
case INT:
{
@@ -416,8 +416,8 @@ protected TypecastKind isTypeCastSafe(
CollectionsPlume.mapList(
Number::intValue, exprValues));
return sortedSetContainsAll(castValuesTree, exprValuesTree)
? TypecastKind.SAFE
: TypecastKind.ERROR;
? TypeCastKind.SAFE
: TypeCastKind.ERROR;
}
case SHORT:
{
@@ -430,16 +430,16 @@ protected TypecastKind isTypeCastSafe(
CollectionsPlume.mapList(
Number::shortValue, exprValues));
return sortedSetContainsAll(castValuesTree, exprValuesTree)
? TypecastKind.SAFE
: TypecastKind.ERROR;
? TypeCastKind.SAFE
: TypeCastKind.ERROR;
}
default:
{
TreeSet<Long> castValuesTree = new TreeSet<>(castValues);
TreeSet<Long> exprValuesTree = new TreeSet<>(exprValues);
return sortedSetContainsAll(castValuesTree, exprValuesTree)
? TypecastKind.SAFE
: TypecastKind.ERROR;
? TypeCastKind.SAFE
: TypeCastKind.ERROR;
}
}
}
Original file line number Diff line number Diff line change
@@ -297,7 +297,7 @@ public static boolean areSamePrimitiveTypes(TypeMirror left, TypeMirror right) {
}

/**
* Returns true iff the arguments are both the same type variables.
* Returns true iff the arguments are both the same type variable.
*
* @param v1 a type variable
* @param v2 a type variable