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

Array subtyping and generics #180

Open
msridhar opened this issue May 14, 2024 · 5 comments
Open

Array subtyping and generics #180

msridhar opened this issue May 14, 2024 · 5 comments

Comments

@msridhar
Copy link

Example:

import org.jspecify.annotations.NullMarked;
import org.jspecify.annotations.Nullable;
import java.util.List;

@NullMarked
class ArraysAndGenerics {

  void foo(List<@Nullable Integer[]> l) {}

  void foo2(List<Integer[]> p) { foo(p); }
}

The above passes the reference checker without error. I would have expected that just like List<String[]> is not a subtype of List<Object[]>, List<Integer[]> would not be a subtype of List<@Nullable Integer[]>, even though JSpecify allows for covariant array subtyping. But maybe I have misunderstood the spec again?

@cpovirk
Copy link
Collaborator

cpovirk commented May 14, 2024

That one does look like a bug, thanks :) I'm not too surprised that there would be a bug at the intersection of arrays and type arguments. I don't know that we'd normally investigate anytime soon, but if it is causing you trouble that isn't resolved by my confirmation that it's a bug, let me know, and I might be able to at least try sometime.

@msridhar
Copy link
Author

It doesn't cause me any trouble. I'm just trying to figure out what to implement for NullAway 🙂 Even though not reporting an error here is a little weird, I don't immediately see how it could lead to an NPE in a different way than what is already possible with covariant subtyping on arrays. So I wanted to confirm that this case should in fact be an error according to the spec.

@cpovirk
Copy link
Collaborator

cpovirk commented May 14, 2024

Yes, I think it should be an error.

Well, I mean, that takes us back to the whose discussion of what precisely we specify :) But from the perspective of a tool that wants to apply JSpecify's rules for subtyping, etc. in all the same places that javac would apply the JLS rules for subtyping, etc., then yes, it should be an error.

I hadn't considered your point that tools could check arrays even more leniently than that, given that arrays are already a type-system hole. I certainly wouldn't complain if you were to decide to treat array handling as a lower priority than most other type-checking. (I would love to claim that the reference-checker bug is a result of exactly that kind of conscious decision on my part, but I suspect that that would be giving me too much credit :)) And if you end up thinking it over and deciding that you want to be more lenient here, then I'd be happy to hear about it.

@msridhar
Copy link
Author

Emitting an error for this case requires a small bit of extra implementation effort in NullAway, but not too much I think. I'll give doing "the right thing" a shot and report back if it ends up being much harder than I expected.

@msridhar
Copy link
Author

Reporting back, I implemented an initial version of array assignment checking in uber/NullAway#956, and reporting an error for the case from this issue while still allowing covariant array subtyping at the top level was not a huge deal.

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

No branches or pull requests

2 participants