Fix unsound handling of catch parameters and throws clauses #436
Conversation
Ao-senXiong
force-pushed
the
add-tests-throw-initialization
branch
from
0abca68
to
0fd1c05
Compare
…work into add-tests-throw-initialization
…nto add-tests-throw-initialization
…into add-tests-throw-initialization
…nto add-tests-throw-initialization
Ao-senXiong
changed the title
| @@ -17,15 +17,15 @@ class Inner {} | |||
| // Type var test | |||
| <E extends @UI PolyUIException> void throwTypeVarUI1(E ex1, @UI E ex2) throws PolyUIException { | |||
| if (flag) { | |||
| // :: error: (throw.type.invalid) | |||
| // :: error: (throw.type.incompatible) | |||
There was a problem hiding this comment.
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.
I have opened a new PR here #618.
There was a problem hiding this comment.
So you should undo these changes in this PR, so that we can merge this PR independently of #618.
| import org.checkerframework.checker.initialization.qual.NotOnlyInitialized; | ||
| import org.checkerframework.checker.initialization.qual.UnknownInitialization; | ||
|
|
||
| class MyException extends Exception { |
There was a problem hiding this comment.
Try having just one top-level class in tests or include something, like 363a in all top-level class names.
Otherwise, it is very easy for two different tests to define a generic class name like MyException and strange behavior can result.
| this.cause = cause; | ||
| } | ||
|
|
||
| MyException(@Initialized EISOPIssue363a cause, String dummy) { |
There was a problem hiding this comment.
This just makes the @Initialized qualifier explicit. Is that the intention?
Should there be a separate constructor that takes an @UnderInitialization parameter, to test all possibilities?
| throw new MyException(this, 0); | ||
| } | ||
|
|
||
| EISOPIssue363a(char dummy) throws MyException { |
There was a problem hiding this comment.
It's rather misleading that this constructor, with a char parameter, invokes the MyException constructor with a String argument. The following constructor with a String parameter, invokes a different constructor. Maybe these can be aligned a bit better?
| public static void test3(String[] args) { | ||
| try { | ||
| EISOPIssue363a obj = new EISOPIssue363a(); | ||
| } catch (@Initialized MyException ex) { |
There was a problem hiding this comment.
This is the same as test1, just with an explicit qualifier. Put them next to each other and add a comment?
framework/src/main/java/org/checkerframework/common/basetype/BaseTypeVisitor.java
Outdated
Show resolved
Hide resolved
framework/src/main/java/org/checkerframework/common/basetype/BaseTypeVisitor.java
Outdated
Show resolved
Hide resolved
framework/src/main/java/org/checkerframework/common/basetype/BaseTypeVisitor.java
Outdated
Show resolved
Hide resolved
framework/src/main/java/org/checkerframework/common/basetype/BaseTypeVisitor.java
Outdated
Show resolved
Hide resolved
framework/src/main/java/org/checkerframework/common/basetype/BaseTypeVisitor.java
Outdated
Show resolved
Hide resolved
| getExceptionParameterLowerBoundAnnotationsCached(); | ||
| VariableTree excParamTree = tree.getParameter(); | ||
| AnnotatedTypeMirror excParamType = atypeFactory.getAnnotatedType(excParamTree); | ||
|
|
||
| // List<AnnotatedTypeMirror> exceptionList = | ||
| // atypeFactory.getAnnotatedType(methodTree).getThrownTypes(); |
There was a problem hiding this comment.
@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?
|
@Ao-senXiong Can you look into wrapping up this change, without the renaming that is done in #618? Let's first fix the soundness issue and then improve the naming. |
Fixes #363. This PR revealed the need for #494 and depends on it being merged first.