Introduce a NullSpecTypeValidator to ensure unspecified type variables are ok
#178
Conversation
|
The code change looks reasonable. Assuming that I did the testing with eisop/checker-framework#746 correctly, however, I'm now seeing at least a few dozen new errors. It's mostly errors like this: And some of this similar kind of error: I also see a tiny bit of this: (As usual, the line numbers there might not exactly line up with the public version.) One small note: It looks like I had been testing past PRs with the most recent version of eisop/checker-framework that we'd imported already. That version doesn't include any changes from April. That might not matter, but I point it out to say that it's at least possible that any problems "with this PR" actually come from some recent eisop/checker-framework change that I only now imported. However, I'm not sure what that would be other than eisop/checker-framework@1d2bd6c. And when I make that change in my previous test client (which had a combination of #171 and #175), I don't see any errors. So I think that the errors really do result from this PR or the eisop/checker-framework PR. |
|
@cpovirk Thanks for testing! I've merged eisop/checker-framework#746 now. Locally, the conformance tests for this PR pass for me. Adding the On the highest level, for the The biggest blocker are the 180 unexpected diagnostics in strict mode. Do you see a pattern do those? Picking one of the lenient failures: In lenient mode, should T* be a subtype of Object? Currently, it is not, even in lenient mode. Is that a problem with the hierarchy or the test case? I don't see a reason why the bounds of T should matter, as there is an explicit "unspecified" annotation on the use of T. Another question: Looking at Should the Sorry for the long message. I made one long comment, but it might be easier to split up replies into individual comments. Or whatever works best for you :-) |
|
Assuming that I did the testing right, I'm still seeing the errors from last time even after importing eisop/checker-framework#746 :( More on the rest in a bit. |
…eference-checker into issue-177
…eference-checker into issue-177
|
Oops, I replied about the |
|
Correction: As I just noted in the other thread: The I'll see about finally actually getting you a command to reproduce them. |
|
OK, that wasn't actually hard at all, sorry. In contrast, if I create a fresh clone and build from the |
|
@cpovirk Thanks for looking into this! You seem to be testing against |
|
🤦 I managed to notice that just before posting and then still not post it :) import java.util.List;
import java.util.function.Predicate;
import org.jspecify.annotations.NullMarked;
import org.jspecify.annotations.Nullable;
@NullMarked
interface ListSubclass<Z extends @Nullable Object> extends List<Z> {
boolean removeIf(Predicate<? super Z> filter);
} |
|
I see the same error when building with this PR (f2c3679). |
|
If you clone the Currently:
I wonder we could be so lucky that the problem would go away once I enact the proposal from jspecify/jspecify#248..... |
Somehow I ended up with a Whatever was going on, I no longer see the 2 errors in a new clone. |
There was a problem hiding this comment.
Yes, that sounds good. After re-reading some things, my impression is that I was treating the problem as "caused by" this PR because this PR required eisop/checker-framework#746—which is the true cause of the problem.
…hecker into issue-177
wmdietl
changed the title
NullSpecTypeValidator to ensure unspecified type variables are ok
|
Sorry, I am confused again: It turns out that some of the new errors appear with eisop/checker-framework#746 and others appear only after we also include this PR. (I probably missed the second batch (at least sometimes) because the first batch was preventing the build from continuing. I have made that mistake before....) The ones that need this PR look like they're all similar to the As you say, this PR should be purely a fix, so something is already wrong in the checker, and this PR is probably just (correctly) producing situations that exercise that bug more. And in fact, I have a lead, which is an "unrelated" problem I had a couple weeks back when the checker wouldn't let me override I'm not entirely sure whether that fix will address the smaller number of problems given in my initial post in #193. But given that that's a small number of problems (and that it's possibly related to type inference), I probably won't be worried much even if that problem remains. |
--- a/NullSpecAnnotatedTypeFactory.java 2024-07-26 10:56:46.000000000 -0400
+++ b/NullSpecAnnotatedTypeFactory.java 2024-07-26 11:37:39.000000000 -0400
@@ -762,6 +762,9 @@
if (subtype.hasAnnotation(minusNull)) {
return true;
}
+ if (!isLeastConvenientWorld && subtype.hasAnnotation(nullnessOperatorUnspecified)) {
+ return true;
+ }
if (isUnionNullOrEquivalent(subtype)) {
return false;
|
|
Sorry, I didn't mean to post that yet :) Anyway: The above changes fixes some of the problems but not all. That makes sense, since it merely updates one usage of |
|
Also, I should be posting on #193.... |
|
I just had one thought on the original change: Perhaps we should be allowing the unspecified-unspecified bounds pair only in lenient mode. But I have forgotten the full context here. I know there's some weirdness around combining |
Fixes #177.
Depends on eisop/checker-framework#746 to also correctly default type variable lower bounds.
@cpovirk If it is easy, could you see whether that EISOP PR introduces any new issues?
With both of these changes, the conformance tests should pass again.