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

Fix crash in ajava-based WPI related to captures #5335

Merged
merged 10 commits into from Sep 28, 2022

Conversation

kelloggm
Copy link
Contributor

I'm fairly confident that there is a better way to do this than catching the ClassCastException, but I was worried of the performance implications of calling asSuper on every inference of a field's type, since only very rarely will the two AnnotatedTypeMirror not actually have the same actual Java class (causing the exception).

Technically, their kind is the same - they are both typevars - so it cannot be checked that way, even though the underlying Java class of one is AnnotatedDeclaredType. I think it's probably bad that updateAtmWithLub directly casts to AnnotatedTypeVariable, but I'm not sure how else it could work.

@smillst
Copy link
Member

smillst commented Sep 23, 2022

Technically, their kind is the same - they are both typevars - so it cannot be checked that way, even though the underlying Java class of one is AnnotatedDeclaredType. I think it's probably bad that updateAtmWithLub directly casts to AnnotatedTypeVariable, but I'm not sure how else it could work.

Are you saying there's an AnnotatedDelcaredType that returns TYPEVAR when getKind is called? If so, that needs to be fix.

updateAtmWithLub(rhsATM, atmFromStorage);
// It is possible that rhsATM is a type variable but atmFromStorage is not, which
// would cause a ClassCastException in updateAtmWithLub(). This can
// happen for example when a local variable of type ? extends T is assigned to a
Copy link
Member

Choose a reason for hiding this comment

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

A local variable can't have a wildcard type. I think you when an expression has type ? extends T.

@kelloggm
Copy link
Contributor Author

Are you saying there's an AnnotatedDelcaredType that returns TYPEVAR when getKind is called? If so, that needs to be fix.

I think that is what's happening. I've added debug code to the catch block in this PR so that you can see for yourself (by running ./gradlew ainferIndexAjavaTest -Pemit.test.debug). That prints out something like this every time the exception is caught:

    Catching a ClassCastException: java.lang.ClassCastException: class org.checkerframework.framework.type.AnnotatedTypeMirror$AnnotatedDeclaredType cannot be cast to class org.checkerframework.framework.type.AnnotatedTypeMirror$AnnotatedTypeVariable (org.checkerframework.framework.type.AnnotatedTypeMirror$AnnotatedDeclaredType and org.checkerframework.framework.type.AnnotatedTypeMirror$AnnotatedTypeVariable are in unnamed module of loader 'app')
    The cast must be one of the following two AnnotatedTypeMirrors. The type to cast to is decided by the kind of rhsATM.
    rhsATM: capture#40 extends T extends Object
    rhsATM.getKind(): TYPEVAR
    atmFromStorage: T extends Object
    atmFromStorage.getKind(): TYPEVAR

I have no idea where the bad AnnotatedTypeMirror is coming from - I'm not sure even where to start looking to debug that. My goal with this PR was to treat the symptom of this problem so that I can run WPI without a crash 🙃 However, I agree this seems like it breaks one of our invariants for the CF as a whole.

updateAtmWithLub(rhsATM, atmFromStorage);
} catch (ClassCastException c) {
System.out.println("Catching a ClassCastException: " + c);
System.out.println(
Copy link
Member

Choose a reason for hiding this comment

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

The problem isn't a AnnotatedDeclaredType with an underlying type of TYPEVAR, it's when updateAtmWithLub recurs on the upper bounds of the type var, they may not be the same kind of type. This happens with capture conversion, but also two type variables. See the test case I added.

Hopefully using that information you can fix the problem, rather than catching the exception.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You were exactly right - thanks for figuring this out! I'm glad there wasn't a deeper problem, and I've now written a proper fix.

@kelloggm kelloggm assigned smillst and unassigned kelloggm Sep 28, 2022
smillst
smillst previously approved these changes Sep 28, 2022
@smillst smillst enabled auto-merge (squash) September 28, 2022 15:10
@kelloggm
Copy link
Contributor Author

@smillst I'll need you to review this again: the tests for the version you approved earlier today failed for a good reason: the check did the wrong thing if one of the arguments to updateAtmWithLub was a null type. With this improved handling of type variable bounds in updateAtmWithLub, I was also able to remove another workaround for a similar problem that I added a few months ago.

@smillst smillst merged commit df157d3 into typetools:master Sep 28, 2022
@smillst smillst deleted the wpi-classcast-crash-icse branch September 28, 2022 22:49
wmdietl pushed a commit to eisop/checker-framework that referenced this pull request Oct 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants