-
Notifications
You must be signed in to change notification settings - Fork 19
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
Fix unsound handling of catch parameters and throws clauses #436
base: master
Are you sure you want to change the base?
Changes from 65 commits
0fd1c05
bdc0984
15d3a06
8d800e3
a36ccd8
be0944e
19d0e41
11fd903
7fca586
d411e71
7552ae8
227b661
4fe7623
fd5d966
a09bb74
d53d96c
814926d
bd13421
d951cba
eb1421f
6b61eb9
edd6b1f
34af1c9
f8fa5af
fdf508c
78004bc
b6ba932
ba2cca8
afaf923
8b21d1c
b261c0c
820e9b6
8916dac
bf76d9c
51edde5
8c913a6
53122d7
f23d832
14d6d46
0fe31da
eeb0f52
dafe559
41e5bd3
cc72ed7
a7586e4
3ca4ac1
e832320
5beec9b
b54e4e3
593e77d
ef042d1
be00a27
0e42c58
dc0303e
0b5cea6
b422422
e65a00a
33f822d
2689059
f213fd7
bf039c3
b4af2b9
4f0f7a8
2cc9262
8565088
fdc8f56
770135b
3363ffd
2ccc562
4862beb
8869606
7678ddc
9a7140b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,154 @@ | ||
// Test case for issue 363: | ||
// https://github.com/eisop/checker-framework/issues/363 | ||
|
||
import org.checkerframework.checker.initialization.qual.NotOnlyInitialized; | ||
import org.checkerframework.checker.initialization.qual.UnderInitialization; | ||
import org.checkerframework.checker.initialization.qual.UnknownInitialization; | ||
|
||
public class EISOPIssue363a { | ||
class MyException extends Exception { | ||
@NotOnlyInitialized EISOPIssue363a cause; | ||
|
||
MyException(EISOPIssue363a cause) { | ||
this.cause = cause; | ||
} | ||
|
||
MyException(@UnderInitialization EISOPIssue363a cause, String dummy) { | ||
this.cause = cause; | ||
} | ||
|
||
MyException(@UnknownInitialization EISOPIssue363a cause, int dummy) { | ||
this.cause = cause; | ||
} | ||
} | ||
|
||
Object field; | ||
|
||
EISOPIssue363a() throws MyException { | ||
// :: error: (throw.type.incompatible) | ||
// :: error: (argument.type.incompatible) | ||
throw new MyException(this); | ||
} | ||
|
||
EISOPIssue363a(boolean dummy1, boolean dummy2) throws MyException { | ||
// :: error: (throw.type.incompatible) | ||
throw new MyException(this, 0); | ||
} | ||
|
||
EISOPIssue363a(boolean dummy1, boolean dummy2, boolean dummy3) throws MyException { | ||
// :: error: (throw.type.incompatible) | ||
throw new MyException(this, "UnderInitialization"); | ||
} | ||
|
||
EISOPIssue363a(int dummy) throws @UnknownInitialization MyException { | ||
throw new MyException(this, 0); | ||
} | ||
|
||
EISOPIssue363a(int dummy1, int dummy2) throws @UnknownInitialization MyException { | ||
throw new MyException(this, "UnderInitialization"); | ||
} | ||
|
||
EISOPIssue363a(int dummy1, int dummy2, int dummy3) throws @UnknownInitialization MyException { | ||
// :: error: (argument.type.incompatible) | ||
throw new MyException(this); | ||
} | ||
|
||
EISOPIssue363a(String dummy) throws @UnderInitialization MyException { | ||
throw new MyException(this, "UnderInitialization"); | ||
} | ||
|
||
EISOPIssue363a(String dummy1, String dummy2) throws @UnderInitialization MyException { | ||
throw new MyException(this, 0); | ||
} | ||
|
||
EISOPIssue363a(String dummy1, String dummy2, String dummy3) | ||
throws @UnderInitialization MyException { | ||
// :: error: (argument.type.incompatible) | ||
throw new MyException(this); | ||
} | ||
|
||
void test1() { | ||
try { | ||
EISOPIssue363a obj = new EISOPIssue363a(1); | ||
} catch (MyException ex) { | ||
ex.cause.field.toString(); | ||
} | ||
} | ||
|
||
void test2() { | ||
try { | ||
EISOPIssue363a obj = new EISOPIssue363a(); | ||
} catch (@UnknownInitialization MyException ex) { | ||
// :: error: (dereference.of.nullable) | ||
ex.cause.field.toString(); | ||
} | ||
} | ||
|
||
void test3() { | ||
try { | ||
EISOPIssue363a obj = new EISOPIssue363a(); | ||
} catch (@UnderInitialization MyException ex) { | ||
// :: error: (dereference.of.nullable) | ||
ex.cause.field.toString(); | ||
} | ||
} | ||
|
||
void test4() { | ||
try { | ||
EISOPIssue363a obj = new EISOPIssue363a(1); | ||
} catch (MyException ex) { | ||
ex.cause.field.toString(); | ||
} | ||
} | ||
|
||
void test5() { | ||
try { | ||
EISOPIssue363a obj = new EISOPIssue363a(1); | ||
} catch (@UnknownInitialization MyException ex) { | ||
// :: error: (dereference.of.nullable) | ||
ex.cause.field.toString(); | ||
} | ||
} | ||
|
||
void test6() { | ||
try { | ||
EISOPIssue363a obj = new EISOPIssue363a(1); | ||
} catch (@UnderInitialization MyException ex) { | ||
// :: error: (dereference.of.nullable) | ||
ex.cause.field.toString(); | ||
} | ||
} | ||
|
||
void test7() { | ||
try { | ||
EISOPIssue363a obj = new EISOPIssue363a("UnderInitialization"); | ||
} catch (MyException ex) { | ||
ex.cause.field.toString(); | ||
} | ||
} | ||
|
||
void test8() { | ||
try { | ||
EISOPIssue363a obj = new EISOPIssue363a("UnderInitialization"); | ||
} catch (@UnknownInitialization MyException ex) { | ||
// :: error: (dereference.of.nullable) | ||
ex.cause.field.toString(); | ||
} | ||
} | ||
|
||
void test9() { | ||
try { | ||
EISOPIssue363a obj = new EISOPIssue363a("UnderInitialization"); | ||
} catch (@UnderInitialization MyException ex) { | ||
// :: error: (dereference.of.nullable) | ||
ex.cause.field.toString(); | ||
} | ||
} | ||
|
||
<X extends Throwable> void throwIfInstanceOf(Throwable throwable, Class<X> declaredType) | ||
throws X { | ||
if (declaredType.isInstance(throwable)) { | ||
throw declaredType.cast(throwable); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,94 @@ | ||
// Test multiple throw excetion in method signature for issue 363: | ||
// https://github.com/eisop/checker-framework/issues/363 | ||
|
||
import org.checkerframework.checker.initialization.qual.NotOnlyInitialized; | ||
import org.checkerframework.checker.initialization.qual.UnderInitialization; | ||
import org.checkerframework.checker.initialization.qual.UnknownInitialization; | ||
|
||
public class EISOPIssue363b { | ||
class MyException1 extends Exception { | ||
@NotOnlyInitialized EISOPIssue363b cause; | ||
|
||
MyException1(EISOPIssue363b cause) { | ||
this.cause = cause; | ||
} | ||
|
||
MyException1(@UnderInitialization EISOPIssue363b cause, String dummy) { | ||
this.cause = cause; | ||
} | ||
|
||
MyException1(@UnknownInitialization EISOPIssue363b cause, int dummy) { | ||
this.cause = cause; | ||
} | ||
} | ||
|
||
class MyException2 extends Exception { | ||
@NotOnlyInitialized EISOPIssue363b cause; | ||
|
||
MyException2(EISOPIssue363b cause) { | ||
this.cause = cause; | ||
} | ||
|
||
MyException2(@UnderInitialization EISOPIssue363b cause, String dummy) { | ||
this.cause = cause; | ||
} | ||
|
||
MyException2(@UnknownInitialization EISOPIssue363b cause, int dummy) { | ||
this.cause = cause; | ||
} | ||
} | ||
|
||
Object field; | ||
|
||
EISOPIssue363b(int dummy) throws MyException1, MyException2 { | ||
// :: error: (throw.type.incompatible) | ||
throw new MyException1(this, 0); | ||
} | ||
|
||
EISOPIssue363b(int dummy1, int dummy2) throws MyException1, MyException2 { | ||
// :: error: (throw.type.incompatible) | ||
throw new MyException2(this, 0); | ||
} | ||
|
||
EISOPIssue363b(boolean dummy) throws @UnknownInitialization MyException1, MyException2 { | ||
throw new MyException1(this, 0); | ||
} | ||
|
||
EISOPIssue363b(boolean dummy1, boolean dummy2) | ||
throws @UnknownInitialization MyException1, MyException2 { | ||
// :: error: (throw.type.incompatible) | ||
throw new MyException2(this, 0); | ||
} | ||
|
||
EISOPIssue363b(boolean dummy1, boolean dummy2, boolean dummy3) | ||
throws MyException1, @UnknownInitialization MyException2 { | ||
// :: error: (throw.type.incompatible) | ||
throw new MyException1(this, 0); | ||
} | ||
|
||
EISOPIssue363b(boolean dummy1, boolean dummy2, boolean dummy3, boolean dummy4) | ||
throws MyException1, @UnknownInitialization MyException2 { | ||
throw new MyException2(this, 0); | ||
} | ||
|
||
EISOPIssue363b(String dummy) throws @UnderInitialization MyException1, MyException2 { | ||
throw new MyException1(this, "UnderInitialization"); | ||
} | ||
|
||
EISOPIssue363b(String dummy1, String dummy2) | ||
throws @UnderInitialization MyException1, MyException2 { | ||
// :: error: (throw.type.incompatible) | ||
throw new MyException2(this, "UnderInitialization"); | ||
} | ||
|
||
EISOPIssue363b(String dummy1, String dummy2, String dummy3) | ||
throws MyException1, @UnderInitialization MyException2 { | ||
// :: error: (throw.type.incompatible) | ||
throw new MyException1(this, "UnderInitialization"); | ||
} | ||
|
||
EISOPIssue363b(String dummy1, String dummy2, String dummy3, String dummy4) | ||
throws MyException1, @UnderInitialization MyException2 { | ||
throw new MyException2(this, "UnderInitialization"); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -153,6 +153,7 @@ | |
import javax.lang.model.type.TypeKind; | ||
import javax.lang.model.type.TypeMirror; | ||
import javax.lang.model.util.ElementFilter; | ||
import javax.lang.model.util.Types; | ||
|
||
/* NO-AFU | ||
import org.checkerframework.common.wholeprograminference.WholeProgramInference; | ||
|
@@ -3052,11 +3053,23 @@ private AnnotationMirrorSet getExceptionParameterLowerBoundAnnotationsCached() { | |
* @param tree a CatchTree to check | ||
*/ | ||
protected void checkExceptionParameter(CatchTree tree) { | ||
AnnotationMirrorSet requiredAnnotations = | ||
Set<? extends AnnotationMirror> requiredAnnotations = | ||
getExceptionParameterLowerBoundAnnotationsCached(); | ||
VariableTree excParamTree = tree.getParameter(); | ||
AnnotatedTypeMirror excParamType = atypeFactory.getAnnotatedType(excParamTree); | ||
|
||
// List<AnnotatedTypeMirror> exceptionList = | ||
// atypeFactory.getAnnotatedType(methodTree).getThrownTypes(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @wmdietl I try to compare the type of exception that method body throw and the type of catch parameter. Do you have any suggestion on how to get what exception the method body throws? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @wmdietl I think we should allow the following code as the exception may not be catched like how Java handle this:
|
||
// if (methodTree != null && exceptionList != null) { | ||
// for (AnnotatedTypeMirror exception : exceptionList) { | ||
// Types typesUtil = atypeFactory.getProcessingEnv().getTypeUtils(); | ||
// if (typesUtil.isSubtype( | ||
// excParamType.getUnderlyingType(), exception.getUnderlyingType())) | ||
// { | ||
// requiredAnnotations = excParamType.getEffectiveAnnotations(); | ||
// break; | ||
// } | ||
// } | ||
// } | ||
for (AnnotationMirror required : requiredAnnotations) { | ||
AnnotationMirror found = excParamType.getAnnotationInHierarchy(required); | ||
assert found != null; | ||
|
@@ -3116,15 +3129,35 @@ protected void checkThrownExpression(ThrowTree tree) { | |
AnnotatedTypeMirror throwType = atypeFactory.getAnnotatedType(tree.getExpression()); | ||
TypeMirror throwTM = throwType.getUnderlyingType(); | ||
Set<? extends AnnotationMirror> required = getThrowUpperBoundAnnotations(); | ||
if (methodTree != null) { | ||
List<AnnotatedTypeMirror> exceptionList = | ||
atypeFactory.getAnnotatedType(methodTree).getThrownTypes(); | ||
if (exceptionList != null) { | ||
for (AnnotatedTypeMirror exception : exceptionList) { | ||
Types typesUtil = atypeFactory.getProcessingEnv().getTypeUtils(); | ||
if (typesUtil.isSubtype( | ||
exception.getUnderlyingType(), throwType.getUnderlyingType())) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is the logic for this subtype test? Shouldn't it be the other way around? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The purpose here is to update The logic is: we should only update
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. add test case for methodinvocation. |
||
// use the type of the exception in the method signature if this exception | ||
// is a subtype of it, otherwise use top to permit runtime exceptions not | ||
// declared. | ||
required = exception.getEffectiveAnnotations(); | ||
break; | ||
} | ||
} | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should there be a warning or error if no specific required type is found? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I am think about because there is not always a throw clause in the method signature. Consider the following example. If there is not match, there should not be warning or error.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You didn't come back to this comment. Doesn't this mean that from a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @wmdietl Do you mean code like following shoule issue error for throw type and catch clause mismatch?
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think in that case we would need a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not sure why it is not shown in the first page of this PR directly but I do reply the comment here yesterday. |
||
switch (throwType.getKind()) { | ||
case NULL: | ||
case DECLARED: | ||
case TYPEVAR: | ||
case WILDCARD: | ||
if (!typeHierarchy.isSubtypeShallowEffective(throwType, required)) { | ||
AnnotationMirrorSet found = throwType.getEffectiveAnnotations(); | ||
AnnotationMirrorSet foundEffective = throwType.getEffectiveAnnotations(); | ||
checker.reportError( | ||
tree.getExpression(), "throw.type.invalid", found, required); | ||
tree.getExpression(), | ||
"throw.type.incompatible", | ||
foundEffective, | ||
required); | ||
} | ||
break; | ||
|
||
|
@@ -3133,15 +3166,18 @@ protected void checkThrownExpression(ThrowTree tree) { | |
AnnotationMirrorSet foundPrimary = unionType.getAnnotations(); | ||
if (!qualHierarchy.isSubtypeShallow(foundPrimary, required, throwTM)) { | ||
checker.reportError( | ||
tree.getExpression(), "throw.type.invalid", foundPrimary, required); | ||
tree.getExpression(), | ||
"throw.type.incompatible", | ||
foundPrimary, | ||
required); | ||
} | ||
for (AnnotatedTypeMirror altern : unionType.getAlternatives()) { | ||
TypeMirror alternTM = altern.getUnderlyingType(); | ||
if (!qualHierarchy.isSubtypeShallow( | ||
altern.getAnnotations(), required, alternTM)) { | ||
checker.reportError( | ||
tree.getExpression(), | ||
"throw.type.invalid", | ||
"throw.type.incompatible", | ||
altern.getAnnotations(), | ||
required); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changing the error key also requires a change to this file:
https://github.com/eisop/checker-framework/blob/e604d8d10a38d09f7c3e94c4d614b2d729f48085/framework/src/main/java/org/checkerframework/common/basetype/messages.properties#L35C12-L35C19
incompatible
is the better term, as there is a comparison between two types.In a previous discussion we said we do this renaming in a separate PR. Was there a change of mind?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have opened a new PR here #618.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So you should undo these changes in this PR, so that we can merge this PR independently of #618.