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

Compile the samples files as tests (samples-google-prototype-eisop) #498

Conversation

wmdietl
Copy link
Collaborator

@wmdietl wmdietl commented Mar 22, 2024

This is into the samples-google-prototype-eisop branch. Let me know if I should do something similar for main and/or samples-google-prototype.

@wmdietl wmdietl requested a review from cpovirk March 22, 2024 23:25
@@ -19,8 +19,10 @@
@NullMarked
class ContainmentSuperVsExtendsSameType {
void x() {
// `conflicting_annotations` b/c upper and lower bound of `? super Object` differ.
// :: error: jspecify_nullness_mismatch jspecify_but_expect_nothing
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Separately, as we go through expect_nothing and but_expect_error messages, it would be nice to have a comment why a mismatch is expected.
Here, I expect the problem is that Lib<? super Object> is not a subtype of Lib<? extends Object>, right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, I believe that's right.

And yes, much of the idea of the conformance-test framework is that it lets us replace "error of some kind on this line" with more of an "expected Foo but was Foo?" style.

// :: error: jspecify_nullness_mismatch jspecify_but_expect_nothing
new Check<Lib<? extends Number>, Lib<? super Number>>();
// :: error: jspecify_conflicting_annotations
new Check<Lib<? extends Object>, Lib<? super Object>>();
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

#485 previously changed this, but that change introduced a java compilation failure that went unnoticed.

Copy link
Collaborator

@cpovirk cpovirk left a comment

Choose a reason for hiding this comment

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

This is into the samples-google-prototype-eisop branch. Let me know if I should do something similar for main and/or samples-google-prototype.

I could see doing it for main (and then merging that into samples-google-prototype). Maybe it will help with the conformance-test scores there?

@@ -19,8 +19,10 @@
@NullMarked
class ContainmentSuperVsExtendsSameType {
void x() {
// `conflicting_annotations` b/c upper and lower bound of `? super Object` differ.
// :: error: jspecify_nullness_mismatch jspecify_but_expect_nothing
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, I believe that's right.

And yes, much of the idea of the conformance-test framework is that it lets us replace "error of some kind on this line" with more of an "expected Foo but was Foo?" style.

@wmdietl wmdietl changed the title Compile the samples files as tests Compile the samples files as tests (samples-google-prototype-eisop) Mar 26, 2024
@wmdietl
Copy link
Collaborator Author

wmdietl commented Mar 26, 2024

I could see doing it for main (and then merging that into samples-google-prototype). Maybe it will help with the conformance-test scores there?

Doing it on main fails, because UnknownNullness isn't declared in that branch. Is it worth going through the samples and removing the failing tests? As we're rewriting the samples as conformance tests, I think that's not worth it.

I've created #501 as additional PR, to also compile the samples in the sister-branch samples-google-prototype.

@cpovirk
Copy link
Collaborator

cpovirk commented Mar 26, 2024

Heh, oops. As soon as I saw that this thread had a new message on my phone, I suddenly realized my mistake about main :) I agree that it's not worth doing anything there.

@wmdietl wmdietl merged commit 45d2204 into samples-google-prototype-eisop Mar 26, 2024
11 checks passed
@wmdietl wmdietl deleted the samples-google-prototype-eisop-compile-samples branch March 26, 2024 15:02
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