Skip to content

NNBD, non-nullable named parameters with defaults, and wrapping. #577

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

Open
jakemac53 opened this issue Sep 13, 2019 · 15 comments
Open

NNBD, non-nullable named parameters with defaults, and wrapping. #577

jakemac53 opened this issue Sep 13, 2019 · 15 comments
Labels
feature Proposed language feature that solves one or more problems

Comments

@jakemac53
Copy link
Contributor

jakemac53 commented Sep 13, 2019

Problem statement

With NNBD we will now have static checking that non-nullable named parameters can never contain null, which is great generally 🎉.

This does present a problem however when trying to wrap a function with non-nullable optional named parameters that have default values, especially if you want those same named parameters in your own function.

Here are a couple examples of how you could do this with NNBD as it stands now:

void original({int a: 0, int b: 1}) {}

void wrappedWithExplicitChecks({int? a, int? b}) {
  if (a != null) {
    if (b != null) {
      return original(a: a, b: b);
    } else {
      return original(a: a);
    }
  } else if (b != null) {
    return original(b: b);
  } else {
    original(a: a, b: b);
  }
}

void wrappedWithCopiedDefaults({int a: 0, int b: 1}) => original(a: a, b: b);

Neither of these are satisfactory:

  • in the first example the number of conditions quickly explodes to the point that it is unreasonable even with just 2 arguments.
  • in the second example you are forced to copy the defaults which is slightly annoying, but more importantly it could lead to accidental breakage if the underlying default changes.

Proposed solution

Credit to @jodinathan for the original idea here (from gitter).

Allow default as a value for named arguments. This would be compile time syntactic sugar only and would translate to copying the value of the default from the underlying named parameter. If the default value is not statically known then it would be a static error to use default.

default is already a reserved word (so it can be used in switch statements) so there should be no concern with using that name.

Example usage:

void original({int a: 0, int b: 1}) {}

void wrappedWithDefault({int? a, int? b}) => original(a: a ?? default, b: b ?? default);

Pros:

  • scales linearly based on the number of named arguments
  • relatively minimal boilerplate involved
  • relatively minimal complexity overhead
  • can likely be implemented only in the CFE
  • low risk

Cons:

  • you still end up having to make all your named arguments nullable, and do explicit null checks
  • it doesn't work for all cases, only ones where the default is statically known
@jakemac53 jakemac53 added feature Proposed language feature that solves one or more problems nnbd NNBD related issues labels Sep 13, 2019
@leafpetersen
Copy link
Member

leafpetersen commented Sep 13, 2019

There are three additional arguments that I see against this approach.

First, it makes removing the default value on the original function a breaking change.

Second, it forces you to make the wrapping function arguments nullable.

Third, if the original function arguments were nullable, then the wrapping function behaves differently from the original function.

original({int? a : 0}) {
  print(a);
}
wrapper({int? a}) => original(a: a ?? default);

void main() {
  original(a: null); // prints "null"
  wrapper(a: null); // prints "0"
}

@jakemac53
Copy link
Contributor Author

jakemac53 commented Sep 13, 2019

First, it makes removing the default value on the original function a breaking change.

Fair point, that would usually be breaking anyways but it would move to a compile time error instead of a runtime one. That could be good or bad, depending who you ask :D.

Second, it forces you to make the wrapping function arguments nullable.

Yep - this was the con I listed :). A more in depth feature could clean this up, but within the context of the wrapper function it really is nullable so I think it actually makes sense. Otherwise the defaulting would have to actually happen when you call the wrapper function and not when it calls the original function.

Third, if the original function arguments were nullable, then the wrapping function behaves differently from the original function.

Good point - it is a weird edge case but totally valid.

@munificent
Copy link
Member

I agree that this is an annoying problem with the language. It was a problem before NNBD and it continues to be a problem with NNBD.

Third, if the original function arguments were nullable, then the wrapping function behaves differently from the original function.

Yeah, to me, this is one of the real problems with the proposed solution. I think the cleaner solution is to just keep using the solution you use before NNBD: assign the default value in the body:

void original({int? a, int? b}) {
  a ??= 0;
  b ??= 1;
}

void wrappedWithDefault({int? a, int? b}) => original(a: a, b: b);

This does mean that the parameters are nullable in original(), which means a user can do: original(a: null, b: null). Maybe that's not what you want because you only intend the parameter to be forward-able, not nullable. But in practice, original() does actually gracefully handle null — it gives you the default value — so I think there's an argument that making the parameters nullable is fine.

@jakemac53
Copy link
Contributor Author

jakemac53 commented Sep 13, 2019

Yes, I agree that barring any changes here nullable named parameters are probably all I will ever use (when I have a default at least). That matches how we do this always today, and this will help enforce the defaulting so it would be a small win still.

It is a little bit sad to end up there, but acceptable.

@lrhn
Copy link
Member

lrhn commented Sep 17, 2019

There is no reason to restrict this feature to named parameters, it should work for any parameter with a default value.
Since function types do not carry default value information, it's a new thing to require knowledge about default values at call sites. It can only work with methods, not functions, unless we change function types too (which is very likely undesirable).

This feature even allows you to leak the default value of a function argument:

foo([Object x = const _SecretToken()]) { ... };
...
Object leaker() {
  Object o;
  try {
    foo(((o = default) == null && false) || throw o);
  } on Object { }
  return o;
}

Maybe default should only be allowed in non-cascade tail position of the argument expression.

There have been earlier proposals about have suggested a new "special value" which, when uses as an argment, is equivalent to not passing an argument. Then it would be up to the receiver to fill in the default value when called instead of letting the call-site see the value. That's probably safer, but has some interesting edge cases like foo(x, default, y).
(And I usually say that we should just let null be that value, and always treat omitted arguments s implicitly passing null, and then converting null to the default value in the function, just as the behavior Bob recommended anyway).

@jakemac53
Copy link
Contributor Author

There is no reason to restrict this feature to named parameters, it should work for any parameter with a default value.

Correct, this should also be extended to support positional parameters with defaults.

Maybe default should only be allowed in non-cascade tail position of the argument expression.

Ya, this likely removes a lot of potential unintended uses of this feature, I would be perfectly fine with that.

(And I usually say that we should just let null be that value, and always treat omitted arguments s implicitly passing null, and then converting null to the default value in the function, just as the behavior Bob recommended anyway).

Right, that is a little bit less flexible but I would be in favor of it, I don't think apis that treat Null as a meaningful value are idiomatic Dart anyways.

@derolf
Copy link

derolf commented Oct 30, 2019

What about introducing an ‘undefined’ value?

@eernstg
Copy link
Member

eernstg commented Oct 31, 2019

Here's the proposal that I usually mention when this comes up. ;-)

If we wish to enable consistent and maintainable propagation of default values for formal parameters in forwarding functions, those default values should be denotable.

Let's assume the syntax f.default for getting access to default parameters (default is a reserved word in Dart, so that cannot be a conflict with the name of any declaration), where f must denote a declaration of a function (so it can be a top-level function, a static method, an instance method, a local function, but it cannot be a function type, and it cannot denote a variable of any kind, not even when that variable has a type which is a function type). For instance, C.f.default could be used for a static method or an instance method declared in C, and f.default would be sufficient in a subtype of C when it is an instance method.

Then we could use f.default[0] to denote the default value of the first optional positional parameter of f, f.default[1] the second one, etc. Similarly, f.default[#p] would denote the default value of the named parameter with name p, etc. These terms would be compile-time constants, so there would not be support for anything like f.default[mySymbol] where mySymbol is a dynamic value.

(We don't need f.default[#op] for any operator symbol op, and we might never introduce any optional parameters to any operators, so we could also use a plain identifier like f.default[p]. However, that would be a significant anomaly because the p is not looked up in the current scope.)

It is a compile-time error to refer to a default value that does not exist (in particular, when there is no named parameter with that name, or when the parameter exists but has a non-nullable type and no declared default value).

The original example would then be expressed as follows:

void original({int a: 0, int b: 1}) {}

void wrappedWithCopiedDefaults(
    {int a: original.default[#a], int b: original.default[#b]}) =>
    original(a: a, b: b);

It's a bit verbose, and it would break if the named parameters are renamed, but that's breaking anyway; or if a default value is removed, but that would require all call sites to be updated if they omit said parameter, so why wouldn't the forwarding call site need to be revisited as well?

Besides, we could let default be a shorthand for f.default/C.f.default in a function declaration with a body that contains exactly one function invocation, calling f respectively e.f where C is the static type of e. And f.default could be a shorthand for f.default[k] when it occurs in a parameter denoted by k in a default expression. Which yields the following:

void wrappedWithCopiedDefaults({int a: default, int b: default}) =>
    original(a: a, b: b);

This mechanism doesn't force the parameter types to have any particular properties (they can be nullable or non-nullable as you wish, as long as the forwarding call can pass the type checks).

Next, this design avoids the clash between "null as a dynamic request for a default value, and null as a proper argument", which always comes up in these discussions. Here is the example from @leafpetersen, rewritten to use explicitly denotable default values:

original({int? a : 0}) {
  print(a);
}
wrapper({int? a = default}) => original(a: a);

void main() {
  original(a: null); // prints "null".
  wrapper(a: null); // prints "null", so the discrepancy is gone.
}

With respect to the original goals, we have the following:

Pros:

  • Scales linearly based on the number of arguments with a default (true for ...default[...], too).
  • Relatively minimal boilerplate involved (ok, slightly longer than default, but more flexible as well).
  • Relatively minimal complexity overhead (zero run-time overhead, these are constant expressions).
  • Can likely be implemented only in the CFE (I'd say surely ;-).
  • Low risk

For the cons, the situation differs:

  • There is no requirement to may any parameter type nullable, or non-nullable, just use the type you want.
  • It doesn't work for all cases, only ones where the default is statically known: That is certainly also true for the mechanism using f.default[...]. On the other hand, using a purely static approach has benefits as well.

Finally there was the issue about leaking default values. We can already leak defaults of instance methods:

// LIBRARY 'widely_used_stuff.dart'.

// This is the argument type, available to the public.
class A {}

class _Secret implements A {
  const _Secret();
}
const _secret = _Secret();

class B {
  void f([A a = _secret]) {
    // Here, we want to use `a == _secret` as evidence that the
    // actual argument for `a` was omitted.
  }
}

// LIBRARY 'my_program.dart'.

// Let's get access to `B.f.default[0]`.
class Leaker implements B {
  noSuchMethod(Invocation i) => i.positionalArguments[0];
}

main() {
  A mySecret = Leaker().f() as dynamic;
  print(mySecret.runtimeType); // Prints '_Secret'.
}

However, I'm not convinced that leaking a secret object which is used as a default in order to indicate that "this argument was omitted" is a huge problem. You can pass it explicitly in order to cheat and pretend that the argument was omitted, and you can forward it (implicitly, if you receive it in an invocation of the forwarder). But where's the exploit?

@xvrh
Copy link

xvrh commented Oct 31, 2019

If we look only at the "non-nullable with default value" case.
Can someone explain why this simple proposal is not acceptable?

// User write:
void original({int a = 0, int b = 1}) {
  //...
}

// Automatically becomes:
void original({int? a, int? b}) {
  int a = a ?? 0; // shadows the parameter
  int b = b ?? 1;

  //...
}

The parameter becomes nullable from the outside and null means "use the default value".

Since the parameter is intentionally declared as non-nullable, there is never a conflict that null could be a "proper argument".

It changes the signature of the function but as a developer it seems totally understandable to me.

@eernstg
Copy link
Member

eernstg commented Oct 31, 2019

I think the desugaring to parameter ?? defaultValue in the body works fine in that special case, but it may not be obvious how to extend this approach consistently to other cases.

In particular, we couldn't use an approach that requires the parameter type to be non-nullable if the parameter type is a type variable X whose bound is nullable (e.g., if there is no bound at all): That type variable is a 'potentially nullable' type, and there is no guarantee that X? is different from X, in which case we may be capturing the value null to mean "this parameter was omitted", which is a breaking change if null was already used to mean something else (e.g., if the frobnicator argument is omitted then we use the default frobnicator, but if null is passed then we use the slower algorithm that doesn't need a frobnicator at all).

@xvrh
Copy link

xvrh commented Oct 31, 2019

Is it possible for a parameter to be of a type variable without bound but have a default value?

void original<T>({T frobnicator = ???}) {}

@eernstg
Copy link
Member

eernstg commented Oct 31, 2019

That's right, we cannot construct a constant expression with type X when X is a type variable. So the example where the parameter types are explicitly nullable is still the most obvious case where the approach you mentioned here is a breaking change, cf. @leafpetersen's example here.

However, we might want to allow abstract methods to omit such an "impossible" default value (cf. #655), which would allow for abstract instance method declarations like void original({X frobnicator}); even when X is a potentially non-nullable type variable declared by the enclosing class, and even though frobnicator is not required.

@leafpetersen leafpetersen removed the nnbd NNBD related issues label Dec 10, 2019
@leafpetersen
Copy link
Member

We won't have this for the NNBD release, but leaving it open for future consideration.

@jodinathan
Copy link

will this be in Dart 3? :-)

@lrhn
Copy link
Member

lrhn commented Jan 6, 2023

No current plans to change this for Dart 3. (Read: Not a chance, we're too late in the 3.0 process to add more features.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Proposed language feature that solves one or more problems
Projects
None yet
Development

No branches or pull requests

8 participants