Skip to content

Support separate defaults for wildcard bounds and type variable bounds and uses#708

Merged
wmdietl merged 8 commits intomasterfrom
wildcard-defaults
Feb 28, 2024
Merged

Support separate defaults for wildcard bounds and type variable bounds and uses#708
wmdietl merged 8 commits intomasterfrom
wildcard-defaults

Conversation

@wmdietl
Copy link
Copy Markdown
Member

@wmdietl wmdietl commented Feb 8, 2024

No description provided.

IMPLICIT_UPPER_BOUND,

/**
* Apply default annotations to unannotated wildcard upper bounds: {@code <?>} or {@code <?
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

JSpecify treats those two cases differently in null-unmarked code:

  • <?> has an unspecified-nullness upper bound.
  • <? super C> has a nullable upper bound.

I want to say that @kevinb9n and I had a discussion about how it feels weird that <?> behaves more like <? extends ...> than like <? super ...> but that it ultimately makes sense (or at least works out best)? But I'm blanking on details.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Hmm. Let me think how to split this up into even finer configurations.

I don't remember that JSpecify discussion, but find it a bit surprising.
I've tried to write this example:

import org.jspecify.annotations.Nullable;
import org.jspecify.annotations.NullMarked;

@NullMarked
interface Box<T extends @Nullable Object> {
  T get();
}

interface Producer {
  Box<?> p1();
  Box<? super Object> p2();
}

@NullMarked
class Use {
  void use(Object o) {}
  void foo(Producer p1) {
    use(p1.p1().get());
  }

  void bar(Producer p2) {
    use(p2.p2().get());
  }
}

Using demo on this example produces no errors.
If the upper bound of the return type of p2 is @Nullable, shouldn't there be an error in use(p2.p2().get())? Is that distinction not implemented in the reference checker?
Is my combination of marked and unmarked code incorrect?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

JSpecify treats those two cases differently in null-unmarked code:

  • <?> has an unspecified-nullness upper bound.
  • <? super C> has a nullable upper bound.

(Assuming this type parameter is not non-null-bounded.)

Yeah, it's weird, but in my view the reason the first is unspecified is because when that code gets null-marked the user might either leave it (making it effectively <? extends @Nullable Foo>) or change it to <? extends Foo>. With the second bullet it's nullable no matter what.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

(Thanks, @kevinb9n!)

@wmdietl: Ah, I think you have hit on the case that I couldn't quite remember in jspecify/jspecify-reference-checker#162!

I agree that there should be an error there. But producing an error is tricky because of how javac handles this case.

Let's remove nullness from the picture for the moment:

interface SuperObject {
  interface Box<T> {}

  Box<? super Object> makeBox();

  default void useBox() {
    Box<Object> b = makeBox();
  }
}

As I read the JLS, that shouldn't compile: makeBox() should return Box<C>, where C is a capture variable with upper bound Object and lower bound Object. However, javac reasonably concludes that a capture variable there would be obfuscatory, and it simplifies to the one plain-Java type that could fulfill that type: Box<Object>. (Maybe the JLS actually makes accommodations for this somewhere. It's not like I've read the whole thing :))

From a nullness perspective (or a broader pluggable-type-system perspective), that's trouble: We really need something like Box<? extends Object super Object> so that we can fill in Box<? extends @Nullable Object super @NonNull Object>. Our inability to express that is what causes us not to produce an error for your code above, which I believe is similar to the lack of error in https://github.com/jspecify/jspecify/blob/8a7029daa220e64058fd6b90a8bc345841dba271/samples/ContainmentSuperVsExtendsSameType.java#L22.

(To partially address the issue, I think I had vague intentions to at least produce a type of Box<@Unspec Object> instead of what we produce now, which is apparently Box<@NonNull Object>. Maybe someday I will. But I have been happy not worrying about it for these past 2 years :))

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Thanks for the detailed explanations and motivation for this, @kevinb9n and @cpovirk!

One point about the gradual evolution: if we ever support annotations on wildcards, one could write Box<@NonNull ? super Object> p2(); to make the upper bound explicitly restricted.
The current default of always making the upper bound of super-bounded wildcards @Nullable shouldn't prevent us from in the future allowing the explicit annotation to set a different upper bound.

In a soon to come commit, I'll allow having different defaults for these cases.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Noting for the record: I still see that not as just a "feature we might add later", but as something that fundamentally doesn't make sense in our model and that I hope we would never try to hack in. But we don't have to delve into that discussion if nothing's making it important right now.

@wmdietl wmdietl changed the title Support separate defaults for wildcard and type variable upper bounds Support separate defaults for wildcard bounds and type variable bounds and uses Feb 28, 2024
@wmdietl wmdietl merged commit 5028c60 into master Feb 28, 2024
@wmdietl wmdietl deleted the wildcard-defaults branch February 28, 2024 15:33
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.

3 participants