Optional parameters with potentially non-nullable types can omit default values in abstract methods

[scheglov]

There are new test failures on Issue 40287. Don't report MISSING_DEFAULT_VALUE_FOR_PARAMETER for abstract methods..

The tests

co19/LanguageFeatures/nnbd/override_checking_A02_REQ_ARGS_t10 MissingCompileTimeError (expected Pass)
co19/LanguageFeatures/nnbd/override_checking_A02_REQ_ARGS_t12 MissingCompileTimeError (expected Pass)
co19/LanguageFeatures/nnbd/override_checking_A02_REQ_ARGS_t14 MissingCompileTimeError (expected Pass)

are failing on configurations

analyzer-asserts-strong-linux

The spec says:

It is an error if an optional parameter (named or otherwise) with no default value has a potentially non-nullable type except in the parameter list of an abstract method declaration.

@eernstg @leafpetersen One possible question here however is whether we need to validate the implicit default value (i.e. null) against the parameter type, because if we need, then Null is not assignable to int, and there is still an error.

@eernstg @leafpetersen Also, the spec does not seem to make any exclusion for abstract methods and the requirement that default values for parameters must be identical. So, we need to report a warning here?

I'm going to land my CL, because the NNBD spec is unequivocal about abstract methods, and the language spec is less clear.

[leafpetersen]

There are two questions in play here:

1. Must a parameter have a default value because it is non-nullable?
2. Must a parameter have a default value (and specific default value) because the corresponding parameter in an overridden method has a default value.

The NNBD spec adds a new error for 1, but makes an exception to that error when the method in question is abstract. So in the following, foo is erroneous, and bar is not.

abstract class A {
  int foo([int x]) => 1; // Error, x has no default value and foo is not abstract
  int bar([int x]); // Ok,  x has no default value but bar is abstract
}

For 2, there are no changes specified as part of the NNBD spec, so this requirement remains (@munificent @lrhn @eernstg let me know if you think we should make a change here).

So in the following, the declaration of foo in B is erroneous for reason 2 above (but not for reason 1).

class A {
  int foo([int x = 1]) {}
}

abstract class B extends A {
  int foo([int x]);
}

[eernstg]

@scheglov wrote:

[should we] validate the implicit default value (i.e. null) against the parameter type

No. The motivation for allowing the abstract method to have an optional non-nullable named parameter with no default value is that the dynamic semantics won't need that value, and, for example, a default value for such a parameter of type X where X is a potentially non-nullable type variable simply cannot be specified.

So there is no need to assume that the parameter does have a default value after all, and it's null because it isn't specified. Instead, we should recognize that such a parameter does not have a default value at all, and it doesn't need one. But we know that every implementation of that method will have a default value for that parameter, or the parameter could have been given a nullable type, so any method implementation that actually runs will always have the required information.

Regarding @leafpetersen's item 2 (that an optional named parameter p must have a default value if it corresponds to a parameter in an overridden method that has one, even in an abstract method declaration): I believe that there is no reason to make any changes in this area: When the overridden method has a p with a default value, that value can be used in the overriding declaration, abstract or not. You could say that p could be covariant, so perhaps the overridden default value does not have the required type, but I don't think it's a good idea to introduce special rules in order to enable that scenario.

[scheglov]

@eernstg I'm sorry, but I don't understand.

In the first two paragraphs, where you are talking about abstract methods, do you mean the case when this abstract method does not override another method, does override, or both?

1. NNBD, abstract method, no override.

abstract class A {
  void foo([int a]);
}

2. NNBD, abstract method, override with default.

class A {
  void foo([int a = 42]) {}
}
class B extends A {
  void foo([int a]);
}
abstract class C implements A {
  void foo([int a]);
}

3. NNBD, abstract method, multiple overrides with default.

class A {
  void foo([int a = 1]) {}
}
class B {
  void foo([int a = 2]) {}
}
class C extends A implements B {
  void foo([int a]);
}

The last paragraph (with @leafpetersen's item 2) is even more puzzling to me. Saying "that value can be used" and at the same time "there is no reason to make any changes" seems contradictory. At least in analyzer we don't to such kind of inheritance of default values. Each override is checked that it does specify the identical default value. Is this still an error in legacy and NNBD?

class A {
  void foo([int a = 42]) {}
}
class B extends A {
  void foo([int a]) {}
}

Or do you mean "that value can be used" as "can be used at runtime", so when you override, you must specify the default value in the overriding declaration, abstract or not?

[eernstg]

@scheglov, sorry about the delay, some recent days happily evaporated into a vacation. ;)

I think the main issue here is that the permission to omit the default value of some optional parameters requires the concept of "not having an opinion" about that default value. This implies that there is no conflict with any other declarations about the given value.

If, in contrast, we would take void foo([int a]); to mean that there is a default value and it is null then we would immediately get some errors, because int is a non-nullable type and null is not an admissible value for a. So we cannot possibly treat the situation as equivalent to void foo([int a = null]);. I'll unfold below in more detail what this means.

The two first paragraphs here are concerned with the situation where an abstract method declaration D named m has an optional parameter p whose type annotation is T which is non-nullable, and no default value is specified.

First assume case 1: The declaration D does not override any declarations.

Except for the change that was adopted based on dart-lang/language#655, this would be a compile-time error.

The reason why it is generally an error is that an invocation of such a method could omit p, but the body of the method could refer to p, so it would have to have some value at run time, and we couldn't use null (any more). The reason why it is not an error in the special case where the method declaration is abstract is that there is no method body which could be executed, so there won't be any situations at run time where an expression p is evaluated and p is uninitialized (or has a value which is a violation of the heap invariant, like null).

So there is no default value for p, but we don't need that default value in order to run the program, so that's not a problem.

We could say that the default value is existential: We don't know at D what the default value would be in any implementation of m that might be executed at a call site where D is the statically known declaration, but each such declaration implementing m is required to provide some default value (for example, {T p = myT}, or a contravariantly overridden parameter type could implicitly specify the default value null by adding ?: {T? p}).

The fact that the default value is never needed at run time is the main point here, and that's the reason why it is permissible to omit the specification of the default value. The reason why it's worthwhile to allow it is that it is simply impossible to specify any default value in some cases (like x of A.f here).

However, we also have some rules about consistency, namely that it's a warning to override a default value by some other default value. That's relevant for the remaining cases.

Case 2(a): D overrides D0, and D0 specifies no default value for p.

No new considerations here. D0 must be abstract, but that doesn't actually matter for D. In this case there is no inconsistency, because D0 also doesn't constrain the default value in any way.

Case 2(b): D overrides D0, and D0 specifies default value d for p.

We have not made it an error for default values to differ in an override relationship (it's a warning), and I don't see any changes about this in the nnbd related feature specs. We can already say that it is at most a warning for D to omit the specification of the default value for p.

However, we can maintain the perspective that the default value for p in D is existential (D doesn't have an opinion about the default value for p), so this doesn't conflict with D0. Hence, we can allow D to omit the default value for p even in this case, without a warning.

You could argue that it is possible to specify the default value in this case, because d should work. This may not be true, however, because p could be covariant-by-declaration, so d might not have type T.

I think this justifies the choice to allow D to omit the default value without causing any diagnostics. Of course, developers could still provide the default value if they think that's more readable.

Case 3: D overrides m with multiple distinct default values for p.

class A {
  void foo([int a = 1]) {}
}
class B {
  void foo([int a = 2]) {}
}
class C extends A implements B {
  void foo([int a]);
}

In this case C has an implementation of foo whose a has default value 1, and it's a warning that this implementation overrides B.foo where the default value is 2 (cf. this text).

We can again consider the abstract declaration to have no opinion about the default value of a, so there is no need to emit a warning involving the abstract declaration.

If the two other declarations had agreed on the default value there would not be a conflict.

Last paragraph

The last paragraph (with @leafpetersen's item 2) is even more puzzling
to me. Saying "that value can be used" and at the same time "there is
no reason to make any changes" seems contradictory.

When I said "that value can be used" I meant that a developer could in general specify the default value from the overridden method (with the usual exception about covariant parameters), so I did not see a strong case for changing the rules about the relationship between the default value for a parameter in an overridden and an overriding method declaration.

However, it is in fact helpful to be allowed to omit the default value in the special case where it cannot be specified:

abstract class A {
  void foo([num a = 42]); // Default value could be given or omitted.
}

abstract class B<X extends num> implements A {
  void foo([covariant X a]); // No value will work as default.
}

class C implements B<int> {
  void foo([int a = 42]) {} // Here we can specify the default value again.
}

class D implements B<double> {
  void foo([double a = 42.0]) {} // (Warning if default is specified in `A`).
}

Each override is checked that it does specify the identical default value.
Is this still an error in legacy and NNBD?

class A {
  void foo([int a = 42]) {}
}
class B extends A {
  void foo([int a]) {}
}

Right, that's an error in NNBD (missing default) and a warning in legacy code (different default value). In this case the omitted default value is on a concrete method, which means that there must be an implicit default value that we can use at run time, and an implicit default parameter value in Dart is always null.

[scheglov]

@eernstg Thank you, it is much more clear now.

Is this rule for allowing to omit default values for abstract methods should be applied only for NNBD opted-in libraries, or for legacy libraries as well? There are co19_2 tests that fail on my CL.

[eernstg]

Ah, I didn't get around to answer here all day yesterday, sorry about that. ;-)

Here's what comes to mind first: The rule that the default value can be omitted for an abstract method should not be relevant when the method declaration occurs in an opted-out library, because they are always allowed to omit the default value for any named parameter.

However, checking why there is a failure on those tests, the issue there is that the syntactically omitted default value gives rise to a warning in the same way as we would have warned on an explicit = null.

So in legacy code the named parameter with no syntactic default value is treated exactly as if it had had that explicit = null.

Given that we want to avoid breaking existing code, I think it would make sense to treat named parameters in legacy code like that (treating a missing default value as if there had been an explicit = null).

For nnbd code we have the justification that the missing default value on a parameter of an abstract method declaration can actually be considered to be "existential" (and I think that this can be useful), so I still think that it is worthwhile to do that and avoid the warnings about "this default value was null, now it's 0" and such.

[scheglov]

@eernstg Thank you for the information. So, analyzer does it as expected for legacy libraries (implicit = null).

Although I don't understand why not reporting a warning would be breaking existing code (unless we consider tests as such existing code).

[eernstg]

I don't understand why not reporting a warning would be breaking existing code

Right, it is not a very significant difference (and I suspect that warnings are currently ignored, so adding/removing them might not even break any test runs). But we need to do something, and I think that preserving the current behavior in legacy code is a safe choice.