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

Is shorting extension methods the right choice? #392

Closed
munificent opened this issue Jun 6, 2019 · 4 comments
Closed

Is shorting extension methods the right choice? #392

munificent opened this issue Jun 6, 2019 · 4 comments
Labels
extension-methods nnbd NNBD related issues

Comments

@munificent
Copy link
Member

Extension methods and NNBD interact in interesting ways around null-aware short-circuiting. We do allow defining extension methods on nullable types, which means we the language tells users "We think it's useful to let you define and call methods on things that may be null." But then the current behavior for null-aware short-circuiting shorts all method calls, extension or not, which means the language tells users "We never think it's useful to call methods on things that may be null."

That feels inconsistent. It also rules out potentially useful features, like:

extension MaybeBool on bool? {
  bool get orFalse => this ?? false;
}

class Button {
  bool isEnabled;
}

test(Button? button) {
  if (button?.enabled.orFalse) print("Got enabled button.");
}

The model we have for extension methods is that they are simply a nice syntax for passing an argument to a function. The above is semantically very much like:

class MaybeBool {
  static bool orFalse(bool? self) => self ?? false;
}

class Button {
  bool isEnabled;
}

test(Button? button) {
  if (MaybeBool.orFalse(button?.enabled)) print("Got enabled button.");
}

We do not null-short function calls if an argument is null, so this model implies we should not null-short extension method calls. So, there's an argument that not null-shorting extension method calls is the principled thing to do, and that it's the practical thing to do (for some extension methods).

However, this adds complexity. It means we don't know the control flow of a method chain until we've resolved invocations to see which are extensions and which aren't.

If we never null-short extension methods, then it is anti-useful in other cases:

extension NumStuff on num {
  num get half => this / 2;
}

class Point {
  num x, y;
}

test(Point? point) {
  print(point?.x.half);
}

If we don't short the .half, then this code always produces a static error. The user is forced to turn the last . into ?. which feels tedious and obscures the fact that .x itself never produces null. In other words, it fails for exactly the reasons we are adding null-shorting in the first place.

(One argument for never shorting is that extension methods basically overload the . to mean "statically call this function". The syntax directly maps to a completely static behavior, and users are expected to understand that. If you want a different completely static behavior, you should use a different syntax and we would thus also overload ?. to mean "statically call this function if the first argument is not null".)

The third option is to null-short extension methods on non-nullable types, since not shorting those is a static error. And then don't null-short extension methods on nullable types, since — by the method's own declaration — it's safe to pass null to it. That sounds nice.

The problem is potentially nullable extension methods:

extension Wat<T extends Object?> on T {
  weird() => print("!");
}

Do calls to weird() get shorted or not? We might be able to answer based on the substituted type:

class A {
  int? maybe;
  int definitely;
}

main() {
  A? a;
  a?.maybe.weird(); // Don't short.
  a?.definitely.weird(); // Short.
}

That feels pretty complex and scary. In particular:

generic<T extends Object?>(T thing) {
  List<T>? stuff = [thing];
  stuff?.first.weird(); // ?
}

I assume we would choose based on the type bound, but we are pretty far away from something typical users can reason about.

@eernstg
Copy link
Member

eernstg commented Jun 7, 2019

It is certainly a source of complexity for the reader of a call site if null-shorting treats extension methods differently from instance method, even though it might seem convenient for cases like button?.enabled.orFalse. So my first take on this is that we should preserve the syntactic nature of the null-shorting rules and treat extension methods and instance methods the same.

That feels pretty complex and scary.

Exactly. ;-)

Btw, this might imply that it is a code smell to even have a static extension whose on type is nullable, and it would at least be helpful if the Dart folklore (maybe via 'CONSIDER' entries in the style guide) around nullable on types will at least nudge developers away from overusing it.

@lrhn
Copy link
Member

lrhn commented Jun 7, 2019

Short circuiting null operations are not just about avoiding invocations on null. It's that too, but it's more than that. It's also about promoting the value to non-null, so the remainder of the chain, the continuation, can work normally, without having to check for spurious null values all the way down.

As mentioned in #393, there is an easy way to escape from shortening: (a?.maybe).weird, but no way to get it back if the language takes it away.
That reason alone is enough for me to say that we short everything, whether it would apply to null or not. So, a hard no to always making extension methods break the short-circuiting.

It must also be easy for the reader to understand, and extension methods are made to look like normal methods, so they should work like normal methods. The fact that they desugar to something else is not important to readability. We are not calling a static method the normal way, we are providing a different shorthand for the pipe operator (#43).

It makes the type reasoning easier too to treat everything the same.
Assume:

extension E1 on int? {
  int? randomUpTo() => this == null ? null : Random().nextInt(this);
}
extension E2 on int {
  int randomUpTo() => Random().nextInt(this);
}
List? l = ...;
print(l?.length.randomUpTo().toRadixString(16));

Which extension applies?

If we allow extension methods to break the short-circuiting when they are nullable, then we get unpredictability.

If we say that a nullable extension can apply and escape the short-circuiting, then it must be because we check against int? as the receiver. Then E1 is the only one to match, and calling toRadixString(16) on an int? is a compile-time error.

If we check extensions against int as a receiver type, then both E1 and E2 applies, and E2 is more specific, so it wins, and .toRadixString(16) is fine.

(Or, in other words, this code is fine, and if we break it, we are probably doing something wrong).

@leafpetersen
Copy link
Member

Am I correct in remembering that we resolved this in favor of doing null-shorting uniformly?

@lrhn
Copy link
Member

lrhn commented Aug 15, 2019

Yes.

We no longer have the ? modifier on on-types, but the readability arguments are still valid.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
extension-methods nnbd NNBD related issues
Projects
None yet
Development

No branches or pull requests

4 participants