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 capture conversion in lenient mode. #197

Merged
merged 2 commits into from
Aug 14, 2024

Conversation

cpovirk
Copy link
Collaborator

@cpovirk cpovirk commented Aug 8, 2024

...by always performing capture conversion in strict mode, where there
is a better-defined hierarchy (non-null < unspecified < nullable, with
no "but also unspecified < non-null if you want).

Fixes #193

...by always performing capture conversion in _strict_ mode, where there
is a better-defined hierarchy (non-null < unspecified < nullable, with
no "but also unspecified < non-null if you want).

Fixes jspecify#193
@cpovirk cpovirk requested a review from wmdietl August 8, 2024 19:47
Copy link
Collaborator

@wmdietl wmdietl left a comment

Choose a reason for hiding this comment

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

Thanks for the detailed discussion of the fix in #193!
The fix makes sense. I'm wondering whether to add the test from #193 (comment) either as a regression test here or as a conformance test.

@cpovirk
Copy link
Collaborator Author

cpovirk commented Aug 14, 2024

Good point. I'm adding a regression test for now to match what we've done before, but the regression directory should obviously be one source of possible conformance tests in the future. (There will be an interesting discussion to be had about which kinds of bugs justify per-checker tests vs. those which justify conformance tests. Maybe the right outcome will be that everything becomes a conformance test but that the borderline cases get thrown into a junk-drawer directory where no one expects organization? Maybe that wouldn't even be part of "the conformance tests" per se, just a part of the same repo for convenient integration? Or maybe that's all a bad idea.)

I've confirmed that the new test passes with JAVA_HOME=/usr/lib/jvm/java-17-openjdk-amd64 ./gradlew test --include-build ../jspecify --include-build ../checker-framework. (And I "intentionally" verified that it can fail by checking yet again whether I could remove the wonkyness workaround :) Yet again, I found that I can't.)

@cpovirk cpovirk merged commit fc545a8 into jspecify:main-eisop Aug 14, 2024
1 of 2 checks passed
@cpovirk cpovirk deleted the land-lubber branch August 14, 2024 17:44
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.

2 participants