Invoking operator== on a mirror and passing null as an argument, how should it be handled?

[sstrickl]

In CL 153202, I'm fixing Function::DoArgumentTypesMatch to be NNBD-compliant, among other things. On the main branch, that method skips argument checks if the argument is null, but that no longer happens after my CL (instead it appropriately checks the argument type using Instance::NullIsAssignableTo). This is causing a test failure in tests/lib/mirrors/null_test.dart, where we have:

  InstanceMirror im1 = reflect(null);
  Expect.equals(cm, im1.type);
  Expect.isTrue(im1.invoke(const Symbol("=="), [null]).reflectee);

and in sdk/lib/core/object.dart, we have:

  external bool operator ==(Object other);

Thus, when Instance::NullIsAssignableTo is checked with the type Object in strong mode, it fails and causes the type error:

Unhandled exception:
type 'Null' is not a subtype of type 'Object' of 'other'
#0      _TypeError._throwNew (dart:core-patch/errors_patch.dart:98:34)
#1      _InstanceMirror._invoke (dart:mirrors-patch/mirrors_impl.dart:337:37)
#2      _InstanceMirror.invoke (dart:mirrors-patch/mirrors_impl.dart:333:25)
#3      test (file:///usr/local/google/home/sstrickl/dart/sdk/tests/lib/mirrors/null_test.dart:21:21)
#4      main (file:///usr/local/google/home/sstrickl/dart/sdk/tests/lib/mirrors/null_test.dart:12:5)
#5      _startIsolate.<anonymous closure> (dart:isolate-patch/isolate_patch.dart:301:19)
#6      _RawReceivePortImpl._handleMessage (dart:isolate-patch/isolate_patch.dart:168:12)

For now, I'm going to approve the failure (and add comments in the test pointing at this issue). I'm happy to fix things, but I need to know which of the approaches to take here:

1. We can just declare this a type error and change the test to use expect.throwsTypeError() in this and similar cases throughout the file.
2. We can change InstanceMirror.invoke in sdk/lib/_internal/vm/lib/mirrors_impl.dart to specifically check for Symbol('==') and a null argument, and return the correct value based on the reflectee value. This means any overrides of Object.== will not be taken into consideration (which may return true for == despite having a non-null receiver).
3. We can change Instance::Invoke in the VM to check to see if the function that will be invoked is Object.== and then do the check there. However, this means that trying to invoke == on an instance where a subclass overrides Object.== will continue to fail, unless the subclass allows a nullable argument.

Of course, if another alternative is preferred, I can look at implementing that as well.

\cc @lrhn

[eernstg]

It is a property of the dynamic semantics of Dart that e1 == e2 will never invoke operator == with an actual argument which is null, and almost all implementations of operator == are expected to have a non-nullable parameter type, so when null is passed via a mirror invocation then the natural behavior is to throw.

If a developer wishes to emulate the evaluation of e1 == e2 then it is not unfair to ask that developer to perform the null checks in explicit code. I'm not sure where that information could be given such that it is accessible to the relevant developers, but there might be some Dartdoc on a mirror where it could be squeezed in. ;)

[sstrickl]

Perfect, thanks for the quick response! I'll just mark the appropriate invocations as throwing type errors instead of returning answers (in this CL, since I haven't submitted it yet).

[lrhn]

Agree with Erik. :)