Improve cast warnings/errors logic#1049
Conversation
Co-authored-by: Werner Dietl <wdietl@gmail.com>
wmdietl
left a comment
wmdietl
left a comment
There was a problem hiding this comment.
Thanks for picking up this PR!
| public CastInit() { | ||
| @UnknownInitialization CastInit t1 = (@UnknownInitialization CastInit) this; | ||
| // :: warning: (cast.unsafe) | ||
| // :: error: (cast.incomparable) |
There was a problem hiding this comment.
Does this change make sense to you? @UnknownInitialization is the top of this hierarchy and @Initialized is a subtype. So why isn't this still cast.unsafe?
There was a problem hiding this comment.
I think this error message is for @Initialized CastInit t2 = (@Initialized CastInit) this; and as this is in the constructor, so this is @UnderInitialization and is casting to @Initialized, so the cast is incomparable.
| @GuardSatisfied Object o3, | ||
| @GuardedByBottom Object o4) { | ||
| String s1 = (String) o1; | ||
| // :: error: (cast.incomparable) |
There was a problem hiding this comment.
This is incompatible with the comment on line 9 - there should be no errors in this method.
| @Signed Integer x = s.bar(local); | ||
| } | ||
|
|
||
| class Inner<T extends @UnknownSignedness Object> { |
There was a problem hiding this comment.
Can you rename the type parameter T to make the code easier to read.
| "cast.unsafe", | ||
| exprType.toString(true), | ||
| castType.toString(true)); | ||
| } else if (castResult == TypecastKind.ERROR) { |
There was a problem hiding this comment.
Why does this only handle WARNING and ERROR and not also at least NOT_UPCAST?
It seems a bit odd to have the TypecastKind with five values if isTypeCastSafe only returns three of the options.
Can you think of cleaning up the logic and the enum to make this easier to follow? Maybe there should be two separate enums, one for isTypeCastSafe and one for isUpcast?
| AnnotatedDeclaredType castDeclared = (AnnotatedDeclaredType) castType; | ||
| AnnotationMirrorSet bounds = | ||
| atypeFactory.getTypeDeclarationBounds(castDeclared.getUnderlyingType()); | ||
| protected TypecastKind isTypeCastSafe( |
There was a problem hiding this comment.
A method that starts with is should usually return a boolean. Can you think of a more appropriate name?
We are already breaking all existing clients, so we might as well pick a better name.
| @IntVal({0, 1}) int ac = (@IntVal({0, 1}) int) input; | ||
| @IntVal({0, 1, 2}) int sc = (@IntVal({0, 1, 2}) int) input; | ||
| // :: warning: (cast.unsafe) | ||
| // :: error: (cast.incomparable) |
There was a problem hiding this comment.
Isn't this a cast from "0 or 1" to "definitely 1", so a downcast? Why is this incomparable now?
| // 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.incomparable` error is analogous and is desirable. |
There was a problem hiding this comment.
Also doesn't seem like this should be incomparable...
| * @return whether the arguments are the same type variables | ||
| */ | ||
| public static boolean areSameTypeVariables(TypeVariable v1, TypeVariable v2) { | ||
| return v1.asElement().getSimpleName() == v2.asElement().getSimpleName(); |
There was a problem hiding this comment.
This seems really undesirable. Two different type variable that have the same simple name "T" are not the same. Can you think of better logic for the single use of this function?
3 participants
Cherry-pick #337
Merge with eisop/guava#10