Add a lint when defining default values for optional parameters

[bwilkerson]

Default values for optional parameters

• break forwarding in ugly ways,
• pollute interfaces in confusing ways,
• become part of the signature of the method so that it affects subclasses, and
• don't play nice with non-nullability.

Therefore, some people choose to not use them. It would be nice to have a lint to catch places where they are accidentally used.

[kevmoo]

I believe @munificent and @nex3 have thoughts on this – only for bool args, if I remember correctly.

It's documented somewhere...

[nex3]

That's right. The default value of a boolean parameter is inherently part of a method's public API in a way that it's not for other types, so it makes sense for it to be reified in the signature.

[munificent]

The default value of a boolean parameter is inherently part of a method's public API in a way that it's not for other types

I recall that this is true, but I don't remember why bools are special. Can you remind me?

[nex3]

Because there are only two options, the caller always has to make an explicit decision about whether they want the flag to be enabled or disabled, regardless of whether they explicitly pass it or not. This means that there's no reason for the caller to want to explicitly pass "default" rather than true or false; they inherently have to make the decision in terms of enabling or disabling the flag.

[munificent]

What about cases where a boolean parameter is being forwarded?

foo({bool param}) => bar(param: param);
bar({bool param}) => ...

Here, using a default value in bar would break foo's forwarding and force it to have a default value too.

[nex3]

foo() is still explicitly deciding what the default behavior will be for its call to bar(). It makes sense for that explicitness to be reified in its signature as well.

[munificent]

foo() is still explicitly deciding what the default behavior will be for its call to bar().

How? Note that bar() doesn't have a default value so the null will be handled in the body of bar(). If you were to change bar() from:

bar({bool param}) {
  param ??= true;
  ...
}

to:

bar({bool param}) {
  param ??= false;
  ...
}

Wouldn't that change the effective default value without having to touch foo()?

[nex3]

That's actually a really good example of why it's good to use explicit defaults. In your example, when bar()'s default changes, not only is that a breaking change to the API of bar(), it transitively becomes a breaking change to the API of foo() as well. If foo() used an explicit default, it would be insulated from that breaking change.

Booleans are special here because the difference between true and false is so substantial—changing from one to the other makes whatever behavior the boolean controls do the complete opposite thing. Other types' defaults usually mean stuff like "apply to the whole thing" or "use an empty instance", but booleans inherently represent decisions that users make, and making those decisions explicit promotes clarity and stability.

[eernstg]

I think that the default value of a named parameter is a significant part of the interface of the enclosing method no matter which type it has: Actual arguments are used to select a specific variant of the range of behaviors that a given method may exhibit, and using a default value is just as significant as giving that value explicitly. So default values should be part of the documentation.

But the tricky part is that default values are method implementation specific, i.e., an overriding method implementation can choose different default values.

You could try to fix this and say "the effect of choosing the default value is a significant part of the interface", and then require that the different default values chosen in a set of implementations of the same method in a subclass hierarchy must have "the same semantics". But if you need to know which class is the runtime type of x before you can decide which argument value e you have to use in x.foo(arg: e) in order to get a specific effect then we have completely destroyed the encapsulation of the implementation that subtype polymorphism is supposed to give us. So that's not acceptable.

In other words, the default values of parameters should always be part of the interface, and they should never be allowed to change in a subclass or subtype.

We can always choose a less strict set of rules, say, to make it a hint if the default value for a given optional parameter diverges in a subtype graph, but I believe that the strict point of view that "it's an interface matter" is a good place to start.

[eernstg]

About the 'breaks forwarding' problem: Don't we just need mandatory named arguments for that?

[munificent]

In other words, the default values of parameters should always be part of the interface, and they should never be allowed to change in a subclass or subtype.

Exactly right. And in Dart, that means you have to repeat the default value in the subclass, which I think violates the basic DRY principle.

About the 'breaks forwarding' problem: Don't we just need mandatory named arguments for that?

No, those would be nice, but that's orthogonal. Let's say there's some function like:

int increment(int n, [int by = 1]) {
  return n + by;
}

This code is directly being called and we're happy with its API, including the fact that by is optional. But I also want to write my own function that forwards to it:

int loggedIncrement(int n, [int by]) {
  var result = increment(n, by);
  print("incremented to $result");
  return result;
}

This doesn't work. (In fact, it will throw an exception because increment() assumes by will not be null.) Instead, you should define increment() like so:

int increment(int n, [int by]) {
  by ??= 1;
  return n + by;
}

Or you have to jump through weird hoops in loggedIncrement():

int loggedIncrement(int n, [int by]) {
  var result;
  if (by == null) {
    result = increment(n);
  } else {
    result = increment(n, by);
  }
  print("incremented to $result");
  return result;
}

That doesn't scale well to multiple optional parameters—you end up needing the combinatorial explosion of different calls.

The core problem is that Dart makes a semantic distinction between "passed null" and "did not pass a value", but the latter has no first-class representation in the language. Where JS has undefined, we have nothing. Getting rid of undefined is great, but our optional parameter semantics still have the same behavior, you just can't abstract over it any more.

[lrhn]

This doesn't work. (In fact, it will throw an exception because increment() assumes by will not be null.)

And that's wrong already. Nothing prevents you from passing null as the actual by parameter - it's assignable to an int parameter after all. The fact that another method does the calling doesn't change that - your original loggedIncrement method has undefined behavior when by is null.

What we have here is a function that's subtly broken because it uses a default value instead of doing by ??= 1; (which is exactly your point, repeated for emphasis).

Default behavior for omitted optional parameters is part of a function's contract. That doesn't necessarily mean that it's part of the function's signature - much of a contract isn't part of the signature, like "index value must be in the range 0 .. list.length" where the signature is just int.

You can break a contract in a sub-class in many ways, and we can't stop you from that. I personally don't think default values belong in a function signature. The slight added security it gives you to get warnings if a subclass changes the value doesn't outweigh the complexity it introduces - changing the internal representation of the default value, or changing it to using a different code path if the parameter is omitted (often happens for function parameters) becomes a breaking API change when it doesn't need to be.

I don't think named boolean parameters are special. Any problem they have also goes for any other optional parameters - the user needs to know the behavior when you omit the parameter. That's part of the contract - but it's only the behavior that matters, not the actual value ending up inside the function.

If we have non-nullable types, you can make the named boolean parameter have type bool! and then you have to pass the parameter, it can't be omitted. (I guess optional positional parameters need to have nullable types or default values).

So, I'd prefer to remove default values from function signatures. I don't mind if we keep the default values as part of function declarations - it's one way to implement the desired behavior, even if it isn't also the way to document it - but I'd prefer if it also worked when the parameter is passed as null explicitly.

[munificent]

So, I'd prefer to remove default values from function signatures.

+1!

I'm curious to see how many optional parameters people would still use if Dart supported overloading. My hunch is that the latter would cover many cases where users have to resort to optional params today.

[zoechi]

@munificent I think overloading works fine for simple cases where a single parameter at the end is optional, but what if there are 3 optional parameters? That would require lots of overloads already. non-nullable types for parameters with default values seems the best option to me.

[lrhn]

non-nullable types for parameters with default values seems the best option to me.

I have no problem with having that option, as long as the default value isn't part of the function signature - it's just a shorthand for having a nullable formal parameter with an implicit initialization, giving a non-nullable variable in the function body.

I still want the formal parameter to be nullable when it's optional, and let null represent the missing parameter, so that you can forward parameters in a generic way. There must not be a difference between not providing a parameter and passing a parameter from another function that wasn't provided there. That means that null and non-provided parameters should be the same. That's not currently the case because actively passing null means not getting the default value.

That means that a default parameter is an implementation detail, not an externally visible part of the function signature, just like being async is. I think we can make that work reasonably well.

Then we can even change the requirements on the default value to be an initialization expression that is executed every time the function gets a null argument, like bool! localFlag = parameterFlag ?? initializerExpression;. If it's part of the function implementation, it doesn't have to be constant (but it obviously still can be, it just makes initialization quicker).

[eernstg]

.. you have to repeat the default value in the subclass .. [violating DRY]

Indeed; it would be a breaking change, but we could say that the default value for a given parameter is implicitly preserved in overriding versions of the enclosing method. (So we'd have a notion of a default default ;-). We could have an explicit default default marker (maybe [int by = super]) in order to avoid the breaking change.

With the increment and loggedIncrement example, we could also get better support for correct forwarding:

int increment(int n, [int by = 1]) {
  return n + by;
}

@forward(#increment)
int loggedIncrement(int n, [int by]) {
  var result = increment(n, by); // Hints!
  print("incremented to $result");
  return result;
}

The hints given at the invocation of increment are based on the information that all invocations of increment inside loggedIncrement are intended to be forwarding invocations, i.e., they should always pass all arguments, and the default should match up (ah, maybe we can have [int by = forward] as well ;-). So there wouldn't be any omitted arguments (based on the static knowledge about increment, which should be sufficient exactly because of this type of rules). So we'll have a safe forwarding invocation when the tools are happy.

OK, @forward(#increment) doesn't work well for invocations on different receivers (which would be an important case), so there's a need to come up with a more general idea than just "specify the selector of the forwardee". Conversely, for non-forwarding invocations it is not reasonable to assume that all parameters must always be passed (they wouldn't need to be optional), so we do need some kind of special flag to get this special kind of check. But that's a separate subproblem.

This definition was mentioned as well. It is interesting for a reason that I haven't seen anyone mention explicitly (forgive me if I missed it):

int increment(int n, [int by]) {
  by ??= 1;
  return n + by;
}

This idea makes null a universal "no value given" marker (requiring us to take care that it doesn't mean other things as well). That makes it easy to pass "no value given" around from one optional argument to the next one, implicitly or explicitly, and get the default value using ??= at the end. This is especially convenient when we wish to give a subset of a list of optional positional arguments (where others have tried things like foo(1, , 3) to indicate that the 2nd argument should have the default value, but we can now use foo(1, null, 3)). So this idea has several interesting properties.

But still, we could have explicit syntax for the default value (foo(1, default, 3) might work), and we could maintain the default parameter value mechanism as a part of the signature of the associated method and get safe forwarding as well as informative signatures.

Oh, and static overloading?: Just say no!! ;-)

[lrhn]

This idea makes null a universal "no value given" marker (requiring us to take care that it doesn't mean other things as well).

I think that would be awesome. Assigning a meaning to null apart from "optional value not available" is usually a hack. A nullable variable of type Foo should mean "zero or one of Foo". If it means something more complicated than that, it's going to be harder to maintain and likely error-prone.

That matches optional parameters exactly.

I am not convinced that maps should allow null as key or value :)

[eernstg]

There is one little quirk here, though: If a null valued parameter means "this value was not provided" then we get a conflict in the situation where the parameter itself uses null to mean "please put the message "this value was not provided" into your configuration/database/whatever". In other words, it's not hard to come up with situations where this "not provided" signal can be sent to multiple targets.

[floitschG]

We have that already now, and most of the time we accept this. The only exception is new List(null) where we go through some workarounds to make this throw. We can't set the optional value to an integer there since that would make the list fixed-length.

It turns out that with constructor forwarding we had a warning-free way to do this correctly in Dart, but it is a hack (and I wouldn't be surprised if we can't do it in strong-mode anymore). I actually think that we just treat the list-constructor specially now in the dart2js and probably the VM.

[nex3]

The test package does use the ability to tell the difference between passing no arguments and passing null at the moment to wrap functions transparently. However, if we were guaranteed that no functions anywhere differed between those two cases, we could also get away with not telling the difference.

[srawlins]

I've just transferred this to the linter package; AFAIK there is still not a lint for this, and with the Null Safe type system and required keyword in play, I wonder if this deserves fresh discussion.