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
Show file tree
Hide file tree
Changes from 48 commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,8 @@ protected void commonAssignmentCheck(
}

@Override
protected boolean isTypeCastSafe(AnnotatedTypeMirror castType, AnnotatedTypeMirror exprType) {
protected TypecastKind isTypeCastSafe(
AndrewShf marked this conversation as resolved.
Show resolved Hide resolved
AnnotatedTypeMirror castType, AnnotatedTypeMirror exprType) {

AnnotationMirror exprLTAnno =
exprType.getEffectiveAnnotationInHierarchy(atypeFactory.LESS_THAN_UNKNOWN);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -980,9 +980,10 @@ DeclaredType typeToCheck() {
}

@Override
protected boolean isTypeCastSafe(AnnotatedTypeMirror castType, AnnotatedTypeMirror exprType) {
protected TypecastKind isTypeCastSafe(
AndrewShf marked this conversation as resolved.
Show resolved Hide resolved
AnnotatedTypeMirror castType, AnnotatedTypeMirror exprType) {
if (castType.getKind().isPrimitive()) {
return true;
return TypecastKind.SAFE;
AndrewShf marked this conversation as resolved.
Show resolved Hide resolved
}
return super.isTypeCastSafe(castType, exprType);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -374,9 +374,10 @@ private boolean noMustCallObligation(AnnotatedTypeMirror atm) {
}

@Override
protected boolean isTypeCastSafe(AnnotatedTypeMirror castType, AnnotatedTypeMirror exprType) {
protected TypecastKind isTypeCastSafe(
AndrewShf marked this conversation as resolved.
Show resolved Hide resolved
AnnotatedTypeMirror castType, AnnotatedTypeMirror exprType) {
if (noMustCallObligation(castType) || noMustCallObligation(exprType)) {
return true;
return TypecastKind.SAFE;
AndrewShf marked this conversation as resolved.
Show resolved Hide resolved
}

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

@Override
protected boolean isTypeCastSafe(AnnotatedTypeMirror castType, AnnotatedTypeMirror exprType) {
protected TypecastKind isTypeCastSafe(
AndrewShf marked this conversation as resolved.
Show resolved Hide resolved
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 TypecastKind.SAFE;
AndrewShf marked this conversation as resolved.
Show resolved Hide resolved
}
return super.isTypeCastSafe(castType, exprType);
}
Expand Down
4 changes: 2 additions & 2 deletions checker/tests/lock/ChapterExamples.java
Original file line number Diff line number Diff line change
Expand Up @@ -319,7 +319,7 @@ void someMethod() {
o2 = o1; // {"lock"} and {} are not identical sets.
}

@SuppressWarnings("lock:cast.unsafe")
@SuppressWarnings("lock:cast.incomparable")
void someMethod2() {
// A cast can be used if the user knows it is safe to do so.
// However, the @SuppressWarnings must be added.
Expand Down Expand Up @@ -567,7 +567,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.incomparable", "guardsatisfied.location.disallowed"})
private static @GuardSatisfied(1) Object maskNull(@GuardSatisfied(1) Object key) {
return (key == null ? (@GuardSatisfied(1) Object) NULL_KEY : key);
}
Expand Down
2 changes: 2 additions & 0 deletions checker/tests/lock/Strings.java
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,9 @@ void StringIsGBnothing(
@GuardSatisfied Object o3,
@GuardedByBottom Object o4) {
String s1 = (String) o1;
// :: error: (cast.incomparable)
String s2 = (String) o2;
// :: error: (cast.incomparable)
String s3 = (String) o3;
String s4 = (String) o4; // OK
}
Expand Down
35 changes: 35 additions & 0 deletions checker/tests/signedness/CastFromTtoT.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
import org.checkerframework.checker.signedness.qual.*;

import java.util.*;

class CastFromTtoT<T extends @UnknownSignedness Object> {
@SuppressWarnings("unchecked")
T bar(@UnknownSignedness Object p) {
T x = (T) p;
return x;
}

// Seems to have no cast, but in the instantiation, it's a downcast.
AndrewShf marked this conversation as resolved.
Show resolved Hide resolved
// if we use getEffectiveAnnotations, we'll see it as no cast (qualifier aspect)
AndrewShf marked this conversation as resolved.
Show resolved Hide resolved
// But it's fine, as compiler will warn about casting object to 'T' [unchecked warning]
AndrewShf marked this conversation as resolved.
Show resolved Hide resolved
void foo(CastFromTtoT<@Signed Integer> s, @UnknownSignedness Object local) {
@Signed Integer x = s.bar(local);
}

class Inner<T extends @UnknownSignedness Object> {
T bar2(@Signed T p) {
// without a warning from the compiler. The T in the casting expression may have
Copy link
Member

Choose a reason for hiding this comment

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

Can you change your comments to full sentences, with initial uppercase letter and a period at the end.

Copy link
Member Author

Choose a reason for hiding this comment

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

I re-addressed all the comments in this test-file, I think they are better now!

// different signedness annotation with the arugment p.
// :: warning: (cast.unsafe)
T x = (T) p;
return x;
}

// 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
AndrewShf marked this conversation as resolved.
Show resolved Hide resolved
// 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);
}
}
}
4 changes: 2 additions & 2 deletions checker/tests/signedness/CharCast.java
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,13 @@ void m(@SignedPositive int i) {

void m1(short s) {
int x = s;
// :: warning: (cast.unsafe)
// :: error: (cast.incomparable)
char c = (char) x;
}

void m2(int i) {
int x = (short) i;
// :: warning: (cast.unsafe)
// :: error: (cast.incomparable)
char c = (char) x;
}

Expand Down
2 changes: 2 additions & 0 deletions checker/tests/signedness/CharToFloat.java
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,9 @@

public class CharToFloat {
void castCharacter(Object o) {
// :: error: (cast.incomparable)
floatParameter((Character) o);
// :: error: (cast.incomparable)
doubleParameter((Character) o);
}

Expand Down
8 changes: 4 additions & 4 deletions checker/tests/signedness/LiteralCast.java
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ void m() {
requireSigned((@Unsigned int) 2);
requireSigned((int) 2);
requireSigned((@m int) 2);
// :: warning: (cast.unsafe)
// :: error: (cast.incomparable)
requireSigned((@Signed int) u);
// :: error: (argument.type.incompatible)
requireSigned((@Unsigned int) u);
Expand All @@ -33,7 +33,7 @@ void m() {
// :: error: (argument.type.incompatible)
requireSigned((@m int) u);
requireSigned((@Signed int) s);
// :: error: (argument.type.incompatible) :: warning: (cast.unsafe)
// :: error: (argument.type.incompatible) :: error: (cast.incomparable)
requireSigned((@Unsigned int) s);
requireSigned((int) s);
requireSigned((@m int) s);
Expand All @@ -43,14 +43,14 @@ void m() {
requireUnsigned((@Unsigned int) 2);
requireUnsigned((int) 2);
requireUnsigned((@m int) 2);
// :: error: (argument.type.incompatible) :: warning: (cast.unsafe)
// :: error: (argument.type.incompatible) :: error: (cast.incomparable)
requireUnsigned((@Signed int) u);
requireUnsigned((@Unsigned int) u);
requireUnsigned((int) u);
requireUnsigned((@m int) u);
// :: error: (argument.type.incompatible)
requireUnsigned((@Signed int) s);
// :: warning: (cast.unsafe)
// :: error: (cast.incomparable)
requireUnsigned((@Unsigned int) s);
// :: error: (argument.type.incompatible)
requireUnsigned((int) s);
Expand Down
2 changes: 1 addition & 1 deletion checker/tests/signedness/PrimitiveCasts.java
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
public class PrimitiveCasts {

void shortToChar1(short s) {
// :: warning: (cast.unsafe)
// :: error: (cast.incomparable)
char c = (char) s;
}

Expand Down
12 changes: 6 additions & 6 deletions checker/tests/signedness/WideningConversion.java
Original file line number Diff line number Diff line change
Expand Up @@ -74,19 +74,19 @@ void plus() {

char c;
c = (char) (c1 + c2);
// :: warning: (cast.unsafe)
// :: error: (cast.incomparable)
c = (char) (c1 + i2);
// :: warning: (cast.unsafe)
// :: error: (cast.incomparable)
c = (char) (i1 + c2);
// :: warning: (cast.unsafe)
// :: error: (cast.incomparable)
c = (char) (i1 + i2);

c = (char) (c1 + c2);
// :: warning: (cast.unsafe)
// :: error: (cast.incomparable)
c = (char) (c1 + si2);
// :: warning: (cast.unsafe)
// :: error: (cast.incomparable)
c = (char) (si1 + c2);
// :: warning: (cast.unsafe)
// :: error: (cast.incomparable)
c = (char) (si1 + si2);

c = (char) (c1 + c2);
Expand Down
10 changes: 9 additions & 1 deletion docs/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,10 @@ Fixed a bug in the Nullness Checker where an instance receiver is incorrectly ma
a static method or field access. This could lead to new nullness errors. The static access should be
changed to be through a class name.

A new error message `cast.incomparable` will be raised if casting an expression to one target type which
AndrewShf marked this conversation as resolved.
Show resolved Hide resolved
does not share a subtype relationship with the expression type in the lattice. No longer issue errors
for statically verifiable downcast.

**Implementation details:**

Corrected the arguments to an `ObjectCreationNode` when the node refers to an
Expand All @@ -36,9 +40,13 @@ Changed the return types of
- `AnalysisResult#getFinalLocalValues()` to `Map<VariableElement, V>`, and
- `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
AndrewShf marked this conversation as resolved.
Show resolved Hide resolved
is statically verifiable or not.

**Closed issues:**

eisop#297, eisop#376, eisop#400, eisop#519, eisop#532, eisop#533, typetools#1590.
eisop#155, eisop#297, eisop#376, eisop#400, eisop#519, eisop#532, eisop#533, typetools#1590.


Version 3.34.0-eisop1 (May 9, 2023)
Expand Down
Loading