Analyzer: incorrect default value override warning.

[leafpetersen]

The following code:

lib.dart

class O {}

class A<T extends Object> {  // <---- This class is being migrated
  void foo({Iterable<T> p1 = const <Never>[]}) {
    print(p1.runtimeType);
  }
}

opted_out.dart

// @dart=2.9
import "lib.dart";

class C implements A<O> {
  @override
  void foo({Iterable<O> p1 = const <Never>[]}) {
    // <---- Complains here.
    print(p1.runtimeType);
  }
}

void main() {
  C().foo();
  A().foo();
}

Produces the following error from the dart analyzer:

leafp-macbookpro2:sdk leafp$ ~/src/dart-repo/sdk/xcodebuild/ReleaseARM64/dart analyze ~/tmp/example.dart
Analyzing example.dart...              0.3s

warning • example.dart:6:13 • Parameters can't override default values, this method overrides 'A.foo' where 'p1' has a different value. Try using the same default
          value in both methods. • invalid_override_different_default_values_named

1 issue found.

I believe this is incorrect. The CFE emits no diagnostics here, and the runtime values printed are both List<Never>

I suspect there is an ambiguity in the spec: in one place we indicate that Never shall be treated as Null in legacy libraries, but for constant canonicalization, we specify that Never should be treated as Never*.

Unfortunately this leaves users with no good way to migrate the first library above without breaking the second library.

I wonder if perhaps we should simply drop this warning, either entirely (since that's where null safety is going), or else not ever issue it across opted in/opted out libraries? That is, even if we warn when an opted out method overrides another opted out method with a different default, we probably shouldn't be warning if an opted out method overrides an opted in method with a different default.

cc @lrhn @eernstg @munificent @scheglov @bwilkerson @srawlins @johnniwinther

[scheglov]

Yes, it looks that the analyzer uses runtime type equality for constants.
We get List<Null*>* vs List<Never>, normalized into List<Null>* vs List<Never>.
And then Null is not equal to Never.

I vague remember that we decided (considered?) dropping the warning for not equal default value?

[leafpetersen]

I vague remember that we decided (considered?) dropping the warning for not equal default value?

I also have a vague memory that we had decided to do this globally (including non-null safe code), but if so apparently I never got around to filing the correct issues.

[munificent]

I also have a vague memory that we had decided to do this globally (including non-null safe code), but if so apparently I never got around to filing the correct issues.

I remember discussing this and a vague sentiment towards doing that but I don't know if we ever fully decided. I would be fine with removing this warning.

[johnniwinther]

The reason the CFE doesn't complain about this is that it never checks default values on override - the check was simply never implemented in the CFE.

[eernstg]

Eliminating the warning entirely seems both future-safe and benign.

(We might want to have a lint for the same thing, such that developers and organizations who consider the default value of a parameter to be part of the public API can be notified about cases where an override changes the default.)

However, there is an informal property about constants and legacy code which is/was useful during migration, mentioned here:

Const evaluation is modified so that both type literals and legacy and
opted-in instances canonicalize more consistently

The fact that corner cases like const <Never>[] canonicalize to distinct objects when written in a legacy respectively a migrated library may be inconvenient when it comes to default value overriding, but it is probably much more dangerous when it occurs for constant expressions where the value is intended to be identical to the value of some other constant expression.

So maybe we should focus on flagging those expressions as dangerous when in a mixed program, rather than working harder to let them slip through silently?

This calls for another look at the potential fixes:

Unfortunately this leaves users with no good way to migrate the first
library above without breaking the second library.

I was wondering why the @dart = 2.9 library contained the word Never at all (and, presumably, the migrated library 'lib.dart' also contained the word Never before it was migrated)?

Anyway, I suppose the following will qualify as a 'not good way', but 'lib.dart' could help in the following way (especially if the maintainers of 'lib.dart' get a heads-up about the subtle problems with const <Never>[]):

// Library 'lib.dart'.

class O {}

class A<T extends Object> {  // <---- This class is being migrated
  void foo({Iterable<T> p1 = const <Never>[]}) {
    print(p1.runtimeType);
  }
  static const fooP1Default = const <Never>[]; // This declaration helps legacy importers.
}

// Library 'opted_out.dart'.

// @dart=2.9
import "n009lib.dart";

class C implements A<O> {
  @override
  void foo({Iterable<O> p1 = A.fooP1Default}) {
    print(p1.runtimeType);
  }
}

void main() {
  C().foo();
  A().foo();
}

[eernstg]

One more possible approach: Use the (potential future) ability to denote default values explicitly, as proposed in dart-lang/language#2269:

// --- Library 'lib.dart'
class O {}

class A<T extends Object> {  // <---- This class is being migrated
  void foo({Iterable<T> p1 = const <Never>[]}) {
    print(p1.runtimeType);
  }
}

// --- Library 'opted_out.dart'.
// @dart=2.9
import "lib.dart";

class C implements A<O> {
  @override
  void foo({Iterable<O> p1 = A.default}) { // This line has changed, the rest is unchanged.
    print(p1.runtimeType);
  }
}

void main() {
  C().foo();
  A().foo();
}

We could have tool support for changing default values to use default when the default value is identical to the one in the overridden method in a superclass, and S.default when it is identical to a unique superinterface that declares foo with an identical value.

[jacob314]

Fyi @srawlins it might make sense to prioritize this issue to help with null safety migrations.

[srawlins]

Is this affecting null safety migrations? How are people working around it?

[jacob314]

More context is at
b/234056539
The number of people actually having to ignore invalid_override_different_default_values_named
is low so the issue probably doesn't impact a significant fraction of people migrating or people figure out other workarounds on their own or few people are at a partially migrated state where this is a problem.

[srawlins]

We're now hitting this in mockito-generated code, so I will remove invalid_override_different_default_values_named and invalid_override_different_default_values_positional from analyzer entirely.

[bwilkerson]

The analyzer code base currently suggests that these are warnings from the spec (by defining them in StaticWarningCode). I haven't been able to find them in the spec, so it's possible that they've already been removed.

In either case, it would be good for us to do an audit to see which warnings analyzer has that it thinks are from the spec but that actually aren't, and, if we want to keep them, make it clear that they're not from the spec.