New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

breaking change: dart:mirrors check types of arguments passed to reflectively invoked methods #35611

Closed
mraleph opened this Issue Jan 9, 2019 · 9 comments

Comments

Projects
None yet
7 participants
@mraleph
Copy link
Contributor

mraleph commented Jan 9, 2019

Background

In Dart 2 and Dart 2.1 releases due to a bug in dart:mirrors implementation it was possible to violate strong mode type safety via dart:mirrors.

For example the following code:

import 'dart:mirrors';

class A {
  void method(int v) {
    if (v != null && v is! int) {
      print("This should be impossible: expected null or int got ${v}");
    }
  }
}

void main() {
  final obj = A();
  reflect(obj).invoke(#method, ['not-an-number']);
}

In Dart 2.1 release would print

This should be impossible: expected null or int got not-an-number

In Dart 2.1.1 we are addressing this issue meaning that this code would start throwing TypeError

Unhandled exception:
type 'String' is not a subtype of type 'int' of 'v'
#0      _TypeError._throwNew (dart:core/runtime/liberrors_patch.dart:89:51)
#1      _LocalInstanceMirror._invoke (dart:mirrors/runtime/libmirrors_impl.dart:327:37)
#2      _LocalInstanceMirror.invoke (dart:mirrors/runtime/libmirrors_impl.dart:323:25)
#3      main (file:///tmp/test.dart:13:16)
#4      _startIsolate.<anonymous closure> (dart:isolate/runtime/libisolate_patch.dart:289:19)
#5      _RawReceivePortImpl._handleMessage (dart:isolate/runtime/libisolate_patch.dart:171:12)

Expected impact

We expect that only programs that are incorrect with respect to strong mode would break.

However this might also reveal issues where programs relying on mirrors were not taking care of creating instances of generic classes with correct type arguments - and instead relied on Dart 1 "dynamic-is-bottom" semantics.

For example the following code would stop working:

import 'dart:mirrors';

class B<T> {
}

class A {
  void method(B<int> v) {
  }
}

void main() {
  final obj = A();
  reflect(obj).invoke(#method, [B()]);
}

This would fail with

type 'B<dynamic>' is not a subtype of type 'B<int>' of 'v'

Addressing the breakage

You would need to change your reflective code to pass arguments of correct types, which also means creating instances with correct type arguments.

Note that if you need to construct object reflectively with the given type arguments you can use reflectType to create type mirror with the given type arguments and then cast this mirror to ClassMirror and use ClassMirror.newInstance:

print((reflectType(B, [int]) as ClassMirror).newInstance(const Symbol(""), []).reflectee);  // Instance of B<int>

Known Affected Packages

package:rpc see #35009

/cc @mit-mit

@mit-mit

This comment has been minimized.

Copy link
Member

mit-mit commented Jan 9, 2019

cc @dgrove @Hixie @matanlurey @devoncarew any concerns with this breaking change?

@devoncarew

This comment has been minimized.

Copy link
Member

devoncarew commented Jan 9, 2019

Mirrors and strong mode have mostly impacted us in the backend of dartpad, which uses package:rpc (which relies in mirrors). @jcollins-g would know more about our exposure here.

@jcollins-g

This comment has been minimized.

Copy link
Contributor

jcollins-g commented Jan 9, 2019

Isn't this the same bug that I requested a revert for the fix for in 2.1? It looks like it from the description.

If so, I have a workaround in my back pocket now (dart-lang/dart-services#354) that will bypass this temporarily for dart-services, and the longer-term plan (hopefully this quarter pending grpc-web being done) is to port to grpc.

@mit-mit

This comment has been minimized.

Copy link
Member

mit-mit commented Jan 9, 2019

Yes, it is the same bug which we reverted for 2.1.0. With that, I assume that you have no objections over this breaking change landing in 2.1.1?

@jcollins-g

This comment has been minimized.

Copy link
Contributor

jcollins-g commented Jan 9, 2019

Yep, no objections.

@matanlurey

This comment has been minimized.

Copy link
Contributor

matanlurey commented Jan 9, 2019

LGTM (nothing related to web infra uses mirrors in any form anymore).

@Hixie

This comment has been minimized.

Copy link
Contributor

Hixie commented Jan 10, 2019

lgtm, i thought the plan was to drop mirrors entirely anyway

@dgrove

This comment has been minimized.

Copy link
Member

dgrove commented Jan 11, 2019

LGTM.

@mit-mit

This comment has been minimized.

Copy link
Member

mit-mit commented Jan 13, 2019

There have been no objections neither here, nor on the Dart Misc/Announce groups, hence this change is approved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment