Don't invent a default value when no well-defined value is available

[eernstg]

Cf. #2269 (comment).

Consider the following example (a variant of the example in the issue mentioned above):

class C {
  void foo([int x]);
  noSuchMethod(i) => print(i.positionalArguments[0]);
}

void main() => C().foo();

This program has no compile-time errors (that's fine, none were expected), but the run-time behavior depends on the configuration: dart runs the program and prints 'null', but DartPad and dart compile js generate code where the execution stops before the print expression because null is not a type correct actual argument when the parameter has type int.

First, please make a clear distinction: C.foo.x has no default value. If the declared type had been int? then it would have had the default value null. If it had been declared with an explicit default value d then it would have had the default value d. In other words, an implicit null is just as much a default value as an explicitly declared default value, but it is possible for an optional parameter (of an abstract declaration) to have no default value at all.

I'd recommend that we specify a dynamic error as follows:

Assume that a class C has an implicitly induced noSuchMethod forwarder D for a method named m (because it is concrete and has a non-trivial noSuchMethod implementation). Such noSuchMethod forwarders can be inherited, too, but we only need to specify the case where the forwarder is introduced. We could also have a noSuchMethod thrower, but they are not relevant for this issue.

Assume that D is invoked, and no actual argument is provided for a given optional formal parameter p (which could be positional or named). A dynamic error will then occur in the case where the default value of p is underspecified:

• p has no default value because C contains an abstract declaration of m where no default value is specified for p, or m is in the interface of C because one or more direct superinterfaces have a member named m, and the most specific underlying declaration does not declare a default value for p for any of them.
• p has an ambiguous default value because multiple direct superinterfaces of C have a member named m whose most specific underlying declaration(s) have multiple non-identical default values for p.

The default value can always be made available and unambiguous by putting an abstract declaration of m into C, specifying the desired default value. This implies, for example, that it is possible to resolve missing or ambiguous default values in superinterfaces without writing an actual method implementation. This could be useful in the declaration of a mock class.

This approach could be described as "fail late". We could also make it a compile-time error in every case where a noSuchmethod forwarder is being implicitly induced, and one or more optional parameters do not have a well-defined default value; that could be described as "fail early".

In both cases the change would be at least somewhat breaking: The current behavior in the VM allows noSuchMethod to be invoked with null as an actual argument, even in cases where that would be a soundness violation in an explicit declaration of the forwarded method, but with this change it would become a dynamic error, or even a compile-time error.

With an ambiguous value, the VM currently makes a choice (perhaps using the value from the textually first superinterface that specifies a default value), which may or may not be the "right" choice. It is again a breaking change to start raising a dynamic error in the ambiguous case.

@dart-lang/language-team, WDYT? Do we need to change the current behavior? Fail early or late?

[lrhn]

I'd just not do anything.

It's weird, and technically a way to distinguish whether a function is invoked with or without an argument (since they cannot pass null explicitly), but it also requires a non-nullable optional parameter, an abstract method, and a custom noSuchMethod to get there
And even then, it's not unsound.

If we could fix dart2js to not throw, then this is adequate behavior.

I'm more worried about the ability to extract the declared default value of an interface method, when there is one.
If we could use null for every omitted argument in the Invocation object, instead of using the inherited default value, I'd prefer that.
Interfaces and abstract methods don't need to have default values, and that's what you're working with when using noSuchMethod forwarders. It's a little off to have behavior that expected there to be a default value

[eernstg]

I'd just not do anything.

That's definitely a less breaking way ahead. But do you really think we're helping anybody by adopting such a just-keep-on-truckin' approach, effectively pretending that the invocation of foo was unsound?

Sure, it is not technically unsound because the typing of Invocation must be flexible enough to allow any type/shape of actual argument list. So we can just call noSuchMethod directly and violate any and all expectations.

But for all invocations of noSuchMethod that are going via a noSuchMethod forwarder, we will have the property that the actual argument list is one of those that the actual member signature requires (except for exactly the situation where null is passed, even though that's a violation of the parameter type of the forwarder).

If we could fix dart2js to not throw, then this is adequate behavior.

You could also say that since dart2js throws, the breakage caused by raising an error is already known to be limited. ;-)

we could use null for every omitted argument

Presumably that would be massively breaking, because lots of existing tests rely on mock objects with default values as declared for the mocked signatures.

abstract methods don't need to have default values

True, but they can have default values, and I actually think it's a pretty rare case where the default values are omitted. After all, it wasn't even possible a few years ago, and I've heard comments like "Can you do that?!" many times.

Anyway, it would make a lot of sense to have those default values if every implementation of said abstract declaration uses that same default value, and it might be necessary in order to get a useful behavior from mocks.

[lrhn]

Apparently, this has never been a problem for anyone. Either they all have default values where they need it, or they don't, and they don't mind the null.

We have to define the signature and behavior of the nSM forwarder in a consistent way.

Currently it probably says "same signature as the interface member", which isn't necessarily valid for a concrete method.
That doesn't mean we can't say it, it's just not something you can declare using Dart source, it's not just a plain desugaring. It can be a signature, or function type, with a non-nullable optional parameter. Whether it has a default value is ... an implementation detail, and we get to define the implementation any way we want to, because we do it in spec prose.

Let's be precise about what we have today.
We say:

A noSuchMethod forwarder is a concrete member of $C$
with the signature taken from the interface of $C$,
and with the same default value for each optional parameter.

Method signatures can carry default values. We derive a method member signature from a method declaration by erasing some things, and possibly inserting an explicit covariant, but we don't mention erasing default values.
The combined member signature takes the first among possible candidates, which is biased towards the superclass.

So we are saying the in this example, C has a concrete foo method with the signature void foo([int x]).
That's not normally possible, and therefore not covered by existing semantics, so we at least has to say what happens if it's invoked.
The method is currently said to invoke this.noSuchMethod with an invocation corresponding to the method, and the values of its parameters. (And not the actual argument list.) And that's not enough.

We can say anything we want here, and it becomes the law.
The options, when an optional parameter is non-nullable, has no default value, and the argument list does not provide a value for it, include:

• It causes a run-time error (current dart2js behavior). We can make it a noSuchMethodError instead of a type error if we want to.
• Instead of the parameter's value, we use null as the corresponding value in the Invocation object. (Current VM behavior)
• If possible, we omit the value in the Invocation (always possible for named parameters, only possible for positional if all trailing positional parameters are also omitted). If not possible, either of the previous options.
• Something else?

The first two are both consistent and matches current behavior.

I'm wary about causing runtime errors in noSuchMethod. It's a dynamic feature.
In a dynamic call, you can already pass too many or too few arguments, and still have noSuchMethod invoked, but not passing an optional argument is where we draw the line? Feels inconsistent.

Passing null instead is workable, doesn't throw, requires you to be ready for it. But if you're the one who wrote a noSuchMethod and is aware of an abstract method signature that you want to handle, you should probably know whether it has a default value or not. (Although someone could remove the default value it in a subclass.)

We could also just say that default values are not part of method signatures, since this is all they're used for.
That would make more nSM-forwarders need to handle this case.
Then I would go with passing null for omitted arguments, or omitting values in the Invocation for omitted arguments entirely, even those that are nullable, which is then always possible.

[eernstg]

Apparently, this has never been a problem for anyone.

"OK, we have a bug? It was never reported? Very good, let's keep it". 😄

Seriously, I do recognize that it could be a breaking bug fix to do as I suggested, because the cases where a default value is passed as null even though that's a violation of the parameter type could occur in a test that appears to be working today.

However, it would only be working on the VM, not with dart2js. Also note that the VM throws in the case where null is passed explicitly at the call site. In other words, this behavior allows the caller to determine whether or not an actual argument was passed: If it's null, the actual argument was definitely omitted; if it's not null, the argument was definitely provided.

Even in the case where we wish to support the ability to test whether a given optional parameter was passed or not passed, we probably don't want to start by making that feature available via noSuchMethod, thus forcing developers to use that.

We have to define the signature and behavior of the nSM forwarder in a consistent way.

Certainly!

Currently it probably says "same signature as the interface member"

This is true for the current language specification and also for the version (not yet landed, hint hint ;-) which has been updated to include null safety. As you mention:

A noSuchMethod forwarder is a concrete member of C with the signature taken from the interface of C, and with the same default value for each optional parameter.

The interface of C does have such a member signature because it's a compile-time error for C to have a signature conflict and not resolve it.

but we don't mention erasing default values.

The null safety update does specify that the default values are omitted from member signatures.

Method signatures can carry default values

This is true for the current language specification. It is not true for the null safety update. This change was required because it is not an error to have multiple superinterfaces containing a member signature with the same name and specifying different default values for optional formal parameters that correspond to each other.

(Bad style, but not an error.)

So method signatures can't carry default values any more.

Both the current language specification and the language specification with null safety have no references to default values in the section about combined member signatures.

The combined member signature takes the first among possible candidates

With null safety, it does not take the first signature, it uses topMerge.

This change was introduced specifically in order to avoid the accidental nature of a conflict resolution which relied on the textual order of superinterface declarations.

The options, when a an optional parameter is non-nullable, has no default value, and the argument list does not provide a value for it, include: ... [raise a run-time error (like dart2js); use null (like the vm)]

Agreed, we can do either of those two things, I just think it's a bug to use a value which (1) has no relation to the associated declarations, it's invented completely out of the blue, and (2) it would be a violation of soundness to pass null as the actual argument because the parameter type doesn't include null.

In a dynamic call, you can already pass too many or too few arguments,

This thread is concerned with invocations of noSuchMethod forwarders. A dynamic call does not invoke a noSuchMethod forwarder with too few or too many arguments. Just like a dynamic invocation of methodThatDoesNotExist(), such calls will invoke noSuchMethod directly, and the Invocation will specify the actual arguments, no more no less.

I don't see how this could be relevant to a regular (non-dynamic) method invocation on a mock object (or whatever it is that has a non-trivial noSuchMethod).

We could also just say that default values are not part of method signatures,

This is already true for the null safety spec. However, I've referred to the underlying declaration all the time exactly because default values aren't part of the member signature.

This treatment is required in order to allow default values to be declared inconsistently, e.g., different superinterfaces specify different values. I'd very much prefer to have a lint against that, but the language says that it must be possible to do it.

So my proposal is that we determine what the default value is, when defined consistently, and we detect ambiguities and handle that case separately (multiple declarations, not the same value), and we also detect the case where there is no default value at all and handle that case separately as well.

Then I would go with passing null for omitted arguments, or omitting values in the Invocation for omitted arguments entirely, even those that are nullable, which is then always possible.

I think this would be a huge deterioration in the quality of service delivered to users of mocks. Why would we do that?

[lrhn]

OK, we have several options. The current specification is under-specified - it says "use default value", but doesn't cover case where there is no default value. That makes the current behavior accidental, and we should decide which behavior we want.

The current behavior (dart2js and VM both, don't know why I thought VM did something else):

• If optional parameter is non-nullable, does not have a default value, and no value is passed, a runtime error is thrown ("Null is not assignable to X").

It's a possible behavior, but not one I'd prefer, because it's a runtime error from a well-typed, typed invocation, which happens before the user noSuchMethod has a chance to interact with the invocation.
Generally, if you use noSuchMethod, it's because you want to intercept invocations. This was a valid invocation. The error occurs internally in the forwarding of that invocation to noSuchMethod, it's not caused by the user, and only caused by the noSuchMethod invocation not noticing a missing default value.

The options I see are:

1. Keep the current throwing behavior. A runtime error occurs if an nSM forwarder has no default value for a non-nullable optional parameter, and no argument is passed.
2. Change nSM-forwarders to forward only actual arguments, not all parameters. If no argument was passed, no argument occurs in the Invocation.positionalArguments list.
3. Change nSM-forwarders to successfully pass null values for omitted arguments with no default value.
4. Change nSM-forwarders to successfully pass null values for omitted arguments, always.
5. Make it a compile-time error to have an unimplemented interface member and a non-default noSuchMethod implementation, where we'd currently insert a nSM-forwarder, if the member signature has a non-nullable optional parameter with no default value.

All of these will give a specified behavior.

The fact that nSM forwarders today throw if called, suggests that they are not actually called like that. The current behavior is not important, so changing it should be fine.

Switching to number 3 (pass null instead of throwing) is non-breaking by definition. The only case where behavior would change is where we throw an error today.

Number 2 may confuse existing code which assumes that if memberName is #foo, there's always, say, two positional arguments.
Number 4 may confuse existing code which assumes that if memberName is #foo, there's always two positional arguments, and the argument with a default value is never null.
Number 5 may break existing code which has nSM-forwarders for non-nullable parameters with no default value, but where they always pass the optional argument. But that's not a stable state, it'd be easy to somewhere not pass such an argument, and that works until some test passes in a mock. Maybe even in the integration testing of another package. With this compile-time error, the creator of the mock would be alerted that they need to add a signature with a default value (or which makes the parameter nullable).

I think one of the reasons this hasn't affected mocks yet, is that generated mocks tend to make parameters nullable.

The things about any of these changes, except number 5, that can worry me, is that they'll allow a nSM implementation to distinguish passing an argument from not passing an argument. For number 3, only for parameters with no default value.

It's not much, you can get a similar effect by having a private subclass which changes the parameter to nullable, and then only expose it at the superclass type, where the parameter is not nullable. People will have to do dynamic invocations with null to trick that.
And the way we're current talking about, maybe, possibly, allowing people to detect and control whether arguments are passed or not, I'm not so worried about this slight allowance. I don't see someone writing all their code using noSuchMethod, just to distinguish an unpassed integer argument from a default value.

(Heck, even allowing you to widen the type of the local variable introduced by the parameter would allow that, like foo([int x as int? = null]) which makes int? x the type of the local variable that the int x argument is stored into.
There are so many options around parameters, and almost all of them end up potentially exposing whether and argument was passed or not.)

My vote is on option 3.

[eernstg]

That's a great analysis, @lrhn!

As usual, I'd prefer to detect and report problems at compile time whenever possible, that is, option 5. It could be a lint, if we expect that a compile-time error would break a lot of code that currently seems to work.

Option 1, raise a run-time error if the missing default value is needed, does report the situation. So we could use that as a fallback if we implement option 5 as a lint.

I think option 2 and 4 will break a lot of code (tests, presumably), because existing code expects to receive the actual default values in a lot of simple cases (just one default value declared, and that's what we get): With option 2 the existing code could break at run time because the list positionalArguments is shorter than expected, or the given name is missing as a key in the namedArguments map; with option 4 we would get a value, but it would be null rather than the actual default value.

Option 3, use null if the missing default value is needed, is the behavior that I can see with the current VM (but not with other backends).

I think this is confusing because it violates a rule that we are otherwise maintaining: If noSuchMethod is invoked as a result of a statically checked instance method invocation then it occurs by calling an automatically generated forwarding method with the same signature.

If we adopt option 3 then we need to say something like "... and that forwarding method doesn't just forward what it gets: it will pass null in the Invocation to noSuchMethod for each optional formal parameter that does not have a default value and whose type is non-nullable". It's more complex than just "it's a forwarder", and it invents a parameter value out of thin air, even a value which would be a run-time error if passed by the caller. Is that actually helping anyone?

[eernstg]

Note that forwarding functions would make the situation described in this issue a non-problem: The default values of the forwardee are allowed to have types that could not be used for the corresponding default values in the forwarder.