Sdk: [dart:mirrors] isSubclassOf not correctly migrated to NNBD

[knaeckeKami]

Code:

import 'dart:mirrors';

void main(){

  final stringMirror = reflect("").type;
  final objectMirror = reflect(Object()).type;

  objectMirror.isSubclassOf(stringMirror);

}

Expectations:
This runs without exceptions on projects that are migrated to NNBD.

Actual Behaviour:
throws

Unhandled exception:
type 'Null' is not a subtype of type 'ClassMirror' in type cast
#0      _ClassMirror.isSubclassOf (dart:mirrors-patch/mirrors_impl.dart:710:24)
#1      main (file:///Users/kami/development/mockoccino/test/subtype_test.dart:10:16)
#2      _startIsolate.<anonymous closure> (dart:isolate-patch/isolate_patch.dart:311:19)
#3      _RawReceivePortImpl._handleMessage (dart:isolate-patch/isolate_patch.dart:184:12)

Version:

dart --version
Dart SDK version: 2.12.0-29.10.beta (beta) (Tue Nov 17 10:50:22 2020 +0100) on "macos_x64"

It seems that this is also not fixed on master, though:
See https://github.com/dart-lang/sdk/blob/ea50b03cbee9971a3e3f1cc5c832fc2dfb941654/sdk/lib/_internal/vm/lib/mirrors_impl.dart#L710

The issue is the cast c.superclass as ClassMirror which throws if c.superclass is null (for Object).

bool isSubclassOf(ClassMirror other) {
    // TODO: Remove these null checks once all code is opted into strong nonnullable mode.
    if (other == null) {
      throw new ArgumentError.notNull('other');
    }
    ClassMirror otherDeclaration = other.originalDeclaration as ClassMirror;
    ClassMirror /*THIS SHOULD BE 'ClassMirror?' */ c = this; 
    while (c != null) {
      c = c.originalDeclaration as ClassMirror;
      if (c == otherDeclaration) return true;
      c = c.superclass as ClassMirror /*THIS SHOULD BE 'as ClassMirror?' */;
    }
    return false;
  }

There's still a TODO indicating that the code hasn't been migrated to NNBD yet, but the other code in that file already uses nullable and non-nullable types. Since NNBD is now in beta, in my understanding all core dart:xxx libraries should work with NNBD as of now, is that correct?

[simolus3]

I opened https://dart-review.googlesource.com/c/sdk/+/173900 to add the nullability suffix you suggested. That does fix the problem, but I don't know how this ever worked considering that there are migrated tests for this?

[mraleph]

That does fix the problem, but I don't know how this ever worked considering that there are migrated tests for this?

Well, it did not. These tests are failing, e.g. https://dart-ci.appspot.com/log/any/dartk-strong-linux-release-x64/3084/lib/mirrors/relation_subclass_test, on all strong mode (sound nullability) configurations.

dart:mirrors library unfortunately is not maintained and it might end up being marked as deprecated and slated for removal any time now. Though decision is not made yet - leaving library hanging in the limbo state for now.

[knaeckeKami]

dart:mirrors library unfortunately is not maintained and it might end up being marked as deprecated and slated for removal any time now. Though decision is not made yet - leaving library hanging in the limbo state for now.

Good to know, I just started looking into a mirrors-based approach for making Mockito work as it did before sound nullability. But I think I'll quit doing that now - I noticed a few other quirks/missing features with null safety in mirros.

[simolus3]

It looks like this can be closed now (thanks for taking care of my CL @mraleph), but it would be nice to have some kind of warning that a core dart: library is essentially unmaintained.

[mraleph]

but it would be nice to have some kind of warning that a core dart: library is essentially unmaintained.

Yeah, we are going to do that. I have pinged relevant folks - we need to assess impact and figure out clear migration path for users that depend on dart:mirrors currently.

Fixed by @simolus3 in 9818a0cc9340102afc4e46c8bcddffe7010a32e0

[pedromassango]

but it would be nice to have some kind of warning that a core dart: library is essentially unmaintained.

Yeah, we are going to do that. I have pinged relevant folks - we need to assess impact and figure out clear migration path for users that depend on dart:mirrors currently.

Fixed by @simolus3 in 9818a0c

I really want to know (from anyone with more experience than me), is that possible to "fix"Mockito with dart Mirrors? Considering that it will not be deprecated. If not, what other options we have then (if any)?

@mraleph what about the options you shared on Twitter?
https://twitter.com/mraleph/status/1331235202207715328?s=20

[knaeckeKami]

@pedromassango I spent a few hours yesterday on that and got promising results, even without runtime code generation.

I tried writing a createInstance(Type t); method that tries to create an instance from the given type.
That function would be used in places where Mockito just uses null as placeholder now.
It worked by looking up constructors of the class and trying to create an instance by looking up the constructors of the class and its subtypes and recursively calling createInstance() for the parameter types of the constructors until one of the constructors succeed.

By using a few heuristics (prefer empty constructors, and constructors with only primitive types) and fast paths for primitive types and known container types like Iterable (just return an empty iterable), Future (return a Future.error), Stream (return an empty stream...), I got it working pretty well.

Although there are probably a lot of edge cases (recursive classes like Trees, constructors that do a lot of custom validation on their parameters) where Mockito would need some additional hints from the user of the library to work.

And unfortunately, mirrors is missing some generics-related features that make it more difficult to support custom container-types (like Option for example) and some other quirks where dart:mirrors does not behave as expected with null safety.

So, is it possible to "fix" Mockito with mirrors with my approach? Not in the general case. But it would probably be possible to make it work for like 95% of the normal use cases and provide the user with some way to workaround this for the rest.

I don't know if it's worth it though. The user experience on the cases where that approach fails would probably be pretty bad (and intransparent, many users probably wouldn't know why it failed) and it would not be fun to support this library on GitHub ;).

And the performance wouldn't be great, since it's iterating over ALL classes when a given type is abstract and does not have a delegating constructor to find a suitable subtype that can create an instance of that given type.

[srawlins]

@knaeckeKami when you say "fix" Mockito, are you saying that Mockito's null safety approach of using codegen is the thing that is broken?

[knaeckeKami]

I would not call it broken, we were just looking into alternatives to codegen to keep the current API even with Nullsafety and to keep Mockito as frictionless to use as it is now.

But given the current state of dart:mirrors I gave up on this for now. Codegen is not that bad anyway ;)

[pedromassango]

@knaeckeKami when you say "fix" Mockito, are you saying that Mockito's null safety approach of using codegen is the thing that is broken?

I can say, Yes.

It is hard to imagine running tests plus code-gen. We all know how code gen is not good in the sense that it takes lots of time, plus it is hanover thing in our routine to do.

Of course we have the option to use it without code gen, but it is not good as well because of all the boilerplate.

I will probably not migrate my tests file (by adding the SDK version in the head of the file), if that works I will definitely not migrate to NNBD too soon.

[srawlins]

I would not call it broken, we were just looking into alternatives to codegen to keep the current API even with Nullsafety and to keep Mockito as frictionless to use as it is now.

I get it. You put "fix" in quotes so I thought I understood where you were coming from.

There was a lot of internal discussion about this aspect of null safety and mockito. Codegen (great tooling inside tooling, but definitely more friction for pub packages) and manual overriding (yuck for most cases) were the most viable options we landed on. We didn't explore mirrors too much because of the problems you've encountered. I feel like there is a good solution out there that would rely on some kind of macro support, but I haven't explored that, and I don't think the language team has explored much in terms of language-specified macro support. Such a macro system would be sort of a "runtime-supported codegen" rather than a "ahead-of-time, build-system codegen".

[srawlins]

@pedromassango

It is hard to imagine running tests plus code-gen. We all know how code gen is not good in the sense that it takes lots of time, plus it is hanover thing in our routine to do.

I'm not really familiar with these downsides. But I haven't gotten my hands dirty either. Tests should be run with pub run build_runner test, right? Which would take extra time over pub run test the first go, but follow-up runs would not take much longer? Or do they? I should benchmark mockito's codegen in tests.

https://pub.dev/packages/build_runner#built-in-commands

Of course we have the option to use it without code gen, but it is not good as well because of all the boilerplate.

Yeah I can think of almost no situations where I would recommend the manual overrides over codegen. Sort of just an escape hatch for the hypothetical situation where codegen is a no go.

If you stub/verify ~1 method which has a non-nullable parameter or return type, it might make sense to manual override. But hopefully _most_ of your APIs become non-nullable, rather than most of your APIs remaining nullable, so this seems very hypothetical.

Flutter also took a look at their mockito use cases and removed their mockito dependencies in many (all?) cases, because they were making very light use, and could replace with manual mocks or Fakes.

[knaeckeKami]

My biggest annoyance with codegen is that it interrupts the workflow.

Say I write a new tests with a new Mock, I need to write some code, run build_runner, wait (for big projects often > 1 minute), and then continue.

For this reason, I'll probably mostly use manual fakes and just use Mockito for some advanced use cases.

If build_runner was integrated transparently in the build process when running 'dart/flutter test', I think just that would also help a lot.

[pedromassango]

For this reason, I'll probably mostly use manual fakes and just use Mockito for some advanced use cases.

This means you will need to write more code (for manual fakes) as well. This means more time spent on writing tests and more time to deliver features.

[knaeckeKami]

Yes. But for me, that's not that big of an issue. I don't use Mockito that much.

Mostly for mocking API responses. For me it's not that big of a deal to write fakes for that. But I understand that code that relies more heavily on Mockito and uses more advanced features is more painful to migrate.