dart-lang / sdk

The Dart SDK, including the VM, JS and Wasm compilers, analysis, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
10.05k stars 1.55k forks source link

Dart2+VM+Mixins = Hard to debug type failures #33122

Open matanlurey opened 6 years ago

matanlurey commented 6 years ago

This is from trying to fix a set of internal test cases using mixins + protocol buffers.

Here is a reproduction case on the latest VM + --preview-dart-2:

import 'dart:collection';

import 'package:test/test.dart';

void main() {
  test('should work', () {
    new DynamicFieldValue()..values.addAll(['Hello', 'World']);
  });
}

// Relevant parts of class PbList in dart-lang/protobuf.
class PbList<E> extends ListBase<E> {
  final List<E> _wrappedList = [];

  @override
  int get length => _wrappedList.length;

  @override
  set length(int length) => _wrappedList.length = length;

  @override
  E operator [](int index) => _wrappedList[index];

  @override
  operator []=(int index, E value) => _wrappedList[index] = value;
}

// User-defined mixin for protocol buffer messages.
class DynamicFieldMixin extends MapBase<String, dynamic> {
  static dynamic _readField() => new PbList<String>();

  // Not really how it is implemented. Just an example.
  //
  // In reality it calls "readField(1)", reading a PbList<String> from a protocol buffer.
  @override
  List get values => _readField();

  @override
  operator [](Object key) => throw new UnimplementedError();

  @override
  operator []=(String key, value) => throw new UnimplementedError();

  @override
  void clear() => throw new UnimplementedError();

  @override
  Iterable<String> get keys => throw new UnimplementedError();

  @override
  remove(Object key) => throw new UnimplementedError();
}

// Protocal buffer object with the mixin applied.
class DynamicFieldValue extends DynamicFieldMixin {}

... this fails, at runtime, with the following exception:

  type 'List<dynamic>' is not a subtype of type 'Iterable<String>' of 'iterable'
  dart:collection/list.dart                                     _ListBase&Object&ListMixin.addAll
  test/dart2_vm_mixins_test.dart 7:37                           main.<fn>

Unfortunately, missing from that stack trace is:

The "fix" here is changing the tests to <String>['Hello', 'World'], unfortunately.

I don't know yet if this will effect real code (i.e. DDC once all type checks are enabled).

lrhn commented 6 years ago

The actual function called is indeed addAll of ListBase's superclass. That superclass is Object with ListMixin<E>, which is represented by Object&ListMixin, which is admittedly confusing (and I don't know where _ListBase comes from). I thin it would be more useful to just say that it's the method ListMixin.addAll, and give its position, then the user knows where to look. Mentioning the mixin application class is technically correct, but not actually useful.

matanlurey commented 6 years ago

I agree @lrhn thanks!