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

Null-aware operators on not-non-nullable extension types #3456

Open
lrhn opened this issue Nov 9, 2023 · 1 comment
Open

Null-aware operators on not-non-nullable extension types #3456

lrhn opened this issue Nov 9, 2023 · 1 comment

Comments

@lrhn
Copy link
Member

lrhn commented Nov 9, 2023

The analyzer gives a warning if you write int x = ...; x?.toString() or x ?? 42, an "unnecessary null-aware" warning because the receiver cannot be null.

For a not-non-nullable extension type with a non-nullable representation type, like:

extension type NeverNull(Object _) implements Object {
  String get foo => "foo";
}
extension type NotNull(Object _) {
  String get foo => "foo";
}
extension type MaybeNull(Object? _) {
  String get foo => "foo";
}
void main() {
  NeverNull vn = NeverNull(1);
  NotNull nn = NotNull(1);
  MaybeNull mnn = MaybeNull(1);
  MaybeNull mny = MaybeNull(null);
 
  print(vn?.foo);  // Should probably warn!
  print(nn?.foo);  // ?
  print(mnn?.foo); // ?
  print(mny?.foo); // ?.
}

Should we warn about or even disallow using ?. (or other null-aware operators like ?? and ??=) in any of these cases, or not?

NeverNull

The vn?.foo should warn. The receiver is definitely non-nullable, and can never be null. Same as today, nothing new or interesting, it's just some non-nullable type.

NotNull

The nn?.foo is unnecessary, today, but if someone changes NotNull's representation type to Object? tomorrow, then the ?. will start doing something.

That change may be breaking, or it may not (if the author has documented that you should not assume anything about the representation type, then your code getting broken by making assumptions is just your own fault.)

Which means it's probaby safest to treat it the same as MaybeNull.

MaybeNull

For mmn?.foo and mny?.foo, the ?. is unnecessary for calling foo, you can do .foo directly.
But it does change the behavior, since mny?.foo is null and mnn?.foo is "foo".

If we think of ?. as an operator on T? to conditionally call members on T, then it's unnecessary of MaybeNull. And we should possibly warn about it being unnecessary.

But if we think of v?.foo as just a shorthand for v == null ? null : v.foo, with no special intent, then it works as intended here.

I think assuming "no intent" is a mistake.
The use here may not be desired since the ?. is not really working on the MaybeNull type, but on the underlying representation value, effectively breaching abstraction.
So maybe we do want a warning.

But then, we do allow, without any warning, x?.foo when x has a type variable type X with a nullable bound. That's a not-non-nullable, not-nullable type, just like MaybeNull, so if we want to warn about MaybeNull here, it's not based solely on the nullability of the receiver.
It's really based on whether we are trying to remove a ? (whether successful or not).

In that case we'd need to define some predicate on types, whether ?. is intended to be used, maybe:

  • allowNullAware(T?) = true
  • allowNullAware(X extends B) = allowNullAware(B)
  • allowNullAware(X & B) = allowNullAware(B)
  • allowNullAware(FutureOr<T>) = allowNullAware(T)
  • allowNullAware(T) = (Null <: T) // otherwise. Which is going to be false for all current types other than Null.

That function tries to traverse structural types to get down to a type that is either T? or not, using the bound or promotion of a static-type type variable as the type to look at, and taking the T branch of FutureOr<T>, since Future<T> is not nullable. Then it says it's OK to use null-aware operators if that type is T?.

WDYT?
Should this be how we warn about null-aware operators on extension types?

Or should we just do what we'd do with the current rules, and not warn about extension types which do not implement Object? (Since they are not non-nullable, therefore potentially nullable, and should be assumed to potentially be null).

Or should we do what we do for await and disallow using null-aware operators on extension types that are not nullable, or even only those that are non-nullable. (Which won't break any existing code, since there are no existing extension types.)

The only thing I'm certain of is that we should not be looking at the (transitive) representation type when deciding what to do.

@munificent
Copy link
Member

NeverNull

The vn?.foo should warn.

Agreed. I see a warning today in analyzer when I try it today.

NotNull

The nn?.foo is unnecessary, today, but if someone changes NotNull's representation type to Object? tomorrow, then the ?. will start doing something.

Agreed. The author of the representation type is trying to encapsulate the representation as well as extension types allow them to, so we should respect that.

MaybeNull

For mmn?.foo and mny?.foo, the ?. is unnecessary for calling foo, you can do .foo directly. But it does change the behavior, since mny?.foo is null and mnn?.foo is "foo".

If we think of ?. as an operator on T? to conditionally call members on T, then it's unnecessary of MaybeNull. And we should possibly warn about it being unnecessary.

But if we think of v?.foo as just a shorthand for v == null ? null : v.foo, with no special intent, then it works as intended here.

I think we should follow extension member dispatch. If I write:

extension on int? {
  String get foo => "foo";
}

void main() {
  int? i = null;
  i.foo;
  i?.foo;
}

I don't get any warnings on either call to .foo. As you say, the latter semantically is just a shorthand, and possibly one the user wants. So I wouldn't warn.

That's also how analyzer behaves today on MaybeNull.

So, I think the implementations are doing what I would want them to do in all cases here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants