[dart2wasm] Incorrect ParamInfo for non-nullable non-optional named args with no default values

[osa1]

class Cat {
  bool eatFood(String food) => true;
}

class MockCat implements Cat {
  dynamic noSuchMethod(Invocation invocation) {
    var arg = invocation.positionalArguments[0];
    return arg is String && arg.isNotEmpty;
  }
}

class MockCat2 extends MockCat {
  noSuchMethod(_);
}

class MockCat3 extends MockCat2 implements Cat {
  bool eatFood(String food, {double amount});

  dynamic noSuchMethod(Invocation invocation) {
    if (invocation.memberName == #scratch) {
      return invocation.positionalArguments.join(',');
    }

    return (invocation.positionalArguments[0] as String).isNotEmpty &&
        invocation.namedArguments[#amount] > 0.5;
  }
}

void main() {
  MockCat mock = new MockCat();
  print((mock as dynamic).eatFood("cat food"));
  print(mock.eatFood(""));

  var mock2 = new MockCat2();
  print(mock2.eatFood("cat food"));

  var mock3 = new MockCat3();
  print(mock3.eatFood("cat food", amount: 0.9));
  print(mock3.eatFood("cat food", amount: 0.3));
}

In this program, the ParamInfo for eatFood selector has a named parameter with NullConstant(null) as the default value of the parameter.

However type of the parameter is f64 and inputs of the Wasm function for this member is [ref #Top, ref Object, f64]. So when we try to initialize the missing non-required named argument with a null value, convertType assumes this is dead code (as null is not a valid f64 value) and generates unreachable.

The problem seems to be the type, which should be nullable. If I print invocation object in noSuchMethod and don't pass amount in the last line, AOT prints {Symbol("amount"): null} as the named arguments. So null should be a valid value for that named argument.

[osa1]

I wonder if there's a Kernel bug involved here. In the FunctionNode for MockCat3.eatFood, type of amount is double and initializer is ConstantExpression(null) and isRequired is false. null is not a valid double value.

[johnniwinther]

It could be a bug in the CFE computation of the noSuchMethod forwarder created for MockCat3.eatFood:

  no-such-method-forwarder method eatFood(core::String food, {core::double amount = #C4}) → core::bool
    return this.{self::MockCat3::noSuchMethod}(new core::_InvocationMirror::_withType(#C1, 0, #C2, core::List::unmodifiable<dynamic>(core::_GrowableList::_literal1<dynamic>(food)), core::Map::unmodifiable<core::Symbol*, dynamic>(<core::Symbol*, dynamic>{#C5: amount}))){(core::Invocation) → dynamic} as{TypeError,ForDynamic,ForNonNullableByDefault} core::bool;

where #C4 = null.

Or it could be a lack specification as of how to handle default values for noSuchMethod forwarders in face of null safety. Currently we assume that optional parameters have nullable types (which they are required to if they are non-abstract) and therefore converts the missing default value to null. We can't invent a valid default value on the fly and leaving it empty would invalidate the invariant (probably assumed by backends) that a default value is provided for non-nullable optional parameters.

@eernstg Do you have an idea of what the correct handling of noSuchMethod forwarders should be in this case?

[mkustermann]

Why is it valid to write

class Cat {
  bool eatFood(String food) => true;
}
class MockCat3 extends MockCat2 implements Cat {
  bool eatFood(String food, {double amount});
}

Doesn't the language require any optional parameters added in overriden methods to be either nullable (so they can have null as default value) or require a default value? Is this a special case because eatFood is a declaration but not a definition?

@eernstg Do you have an idea of what the correct handling of noSuchMethod forwarders should be in this case?

Friendly ping @dart-lang/language-team

This doesn't seem to be a dart2wasm issue (and currently the code compiles & runs the same way in wasm as in VM).

[eernstg]

Oh, I didn't see the reference to me in this issue at first, and it's several hundred days ago, sorry about that!

We do have an issue on precisely this topic: dart-lang/language#3331.

Or it could be a lack specification as of how to handle default values for noSuchMethod forwarders in face of null safety.

Yes, a noSuchMethod forwarder (or thrower) should have a signature which is obtained from the interface of the run-time type of the object. So eatFood in an instance of MockCat3 should have the signature bool eatFood(String, {double amount}).

This cannot be implemented directly because double isn't nullable, and there is no specified default value.

My proposal in dart-lang/language#3331 is that this should be a run-time error: No default value is available, but it was needed, so we throw.

Another possible response could be to generate a noSuchMethod forwarder whose parameter type is nullable, using the default value null. That would satisfy all the normal rules of the language because the class actually has an implementation of the given method which is a correct override of the statically known member signature.

According to dart-lang/language#3331, dart compile js throws when such a member is called, but the VM does not, and this issue shows that dart2wasm work has brought up the question.

This means that no particular approach is being used consistently today, and we might be able to do whatever we want without considering this to be a significantly breaking change.

The 2nd approach above (just adding null to the relevant parameter type) might be the least breaking approach, because it doesn't throw when the given member is invoked without an actual argument for that parameter.

On the other hand, changing the member signature in order to obtain an implementable signature which can be used to generate a noSuchMethod forwarder or thrower, that seems to be a novel idea, and not a pretty one.

If we go down that path then we could also say that "every parameter type of a noSuchMethod forwarder has type dynamic", and we could also invent new method signatures in various other ways (just because it's easier, or faster, or whatever). The problem is that this would make the noSuchMethod forwarder a less faithful stand-in for a real implementation of the method, and it might mask bugs in a test if this causes a mock method to accept invocations that would have thrown in a real implementation (say, passing null explicitly to a parameter whose declared type is non-nullable, but which was made non-nullable because of the topic of this issue).

[mkustermann]

@eernstg Should we make this a duplicate of dart-lang/language#3331?

[eernstg]

I think there's a language part (dart-lang/language#3331) that needs to be resolved, and then there's an issue which is concerned with the implementation of whatever behavior turns out to be the specified one (that is this issue, plus perhaps some other issues dealing with the same question in the context of other tools).

Maybe the latter are 'blocked-on' dart-lang/language#3331?

[mkustermann]

I think there's a language part (dart-lang/language#3331) that needs to be resolved, and then there's an issue which is concerned with the implementation of whatever behavior turns out to be the specified one (that is this issue, plus perhaps some other issues dealing with the same question in the context of other tools).

Once the language has specified the right behavior, language team can add tests for the behavior. If those tests fail for any backend, please file backend-specific issues. Right now there's nothing we need to do for dart2wasm IIUC.

So I'm going to close this one (in favor of dart-lang/language#3331)

[eernstg]

OK, thanks!