Skip to content

dart:mirrors does not yet work with --preview-dart-2 #32345

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

Closed
itsjoeconway opened this issue Feb 28, 2018 · 20 comments
Closed

dart:mirrors does not yet work with --preview-dart-2 #32345

itsjoeconway opened this issue Feb 28, 2018 · 20 comments
Assignees
Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. front-end-kernel legacy-area-front-end Legacy: Use area-dart-model instead. P2 A bug or feature request we're likely to work on

Comments

@itsjoeconway
Copy link

This code prints false when running with --preview-dart-2.

import 'dart:mirrors';

enum Enum {
  option1, option2
}

void main() {
  final isEnum = reflectClass(Enum).isEnum;
  print("${isEnum}");
}
@kmillikin
Copy link

Support for dart:mirrors is currently unimplemented with --preview-dart-2. Some things will happen to work, but nothing is guaranteed to work. The things that happen to work today might randomly break tomorrow.

As far as I know, we have not made any official decision about supporting dart:mirrors on the Dart VM. It is not supported one the Web in Dart 2.0 (#30538) and it's also not supported in the Flutter mobile development framework (flutter/flutter#1150).

@kmillikin
Copy link

And now I do know: we have made an official announcement that dart:mirrors will continue to be fully supported on the Dart VM in Dart 2.0. It is here: https://groups.google.com/a/dartlang.org/d/msg/misc/djfFMNCWmkE/F7WE8a0JAwAJ

@kmillikin kmillikin self-assigned this Feb 28, 2018
@kmillikin kmillikin added area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. area-kernel legacy-area-front-end Legacy: Use area-dart-model instead. P2 A bug or feature request we're likely to work on labels Feb 28, 2018
@kmillikin kmillikin changed the title 'isEnum' is false on mirror on enum with --preview-dart-2 dart:mirrors does not yet work with --preview-dart-2 Feb 28, 2018
@itsjoeconway
Copy link
Author

Thanks very much for the info, will continue to track issue

@a-siva
Copy link
Contributor

a-siva commented Mar 1, 2018

The root cause of the enum not being report as 'isEnum' might be the same one as in #32365

@bergwerf
Copy link

bergwerf commented Mar 8, 2018

Phew, I'm glad dart:mirrors will continue to be supported, without it my code generation would become a bit cumbersome...
Also walked into the isEnum bug, but I see it is already fixed in the other thread (so will have to wait for a new dev release now)

@manuelF
Copy link

manuelF commented May 2, 2018

Also running into issues with preview Dart 2. Package unscripted [0] requires dart mirrors fetching method parameters and it fails with a runtime type issue:

type 'List' is not a subtype of type 'List' of 'function result' where
List is from dart:core
List is from dart:core
ParameterMirror is from dart:mirrors

dart:mirrors _LocalMethodMirror.parameters

[0] https://github.com/seaneagan/unscripted/blob/master/lib/src/util.dart#L38

@keertip keertip added this to the Dart2Beta4 milestone May 3, 2018
@dgrove
Copy link
Contributor

dgrove commented May 4, 2018

@vsmenon do you have any needs here?

@vsmenon
Copy link
Member

vsmenon commented May 4, 2018

@jmesserly

Not that we're aware of at this point. The CFE recently added support for parameter annotations (which we needed for temporary pageloader support).

@jmesserly
Copy link

As far as I know, mirrors has enough support for DDC+Kernel. We have limited test coverage though, outside of the internal use of pageloader. (When I've regressed something in DDC+Analyzer mirrors, I don't find out until we roll the SDK.)

@keertip keertip added P1 A high priority bug; for example, a single project is unusable or has many test failures and removed P2 A bug or feature request we're likely to work on labels May 8, 2018
@dgrove dgrove modified the milestones: Dart2Beta4, Dart2Stable May 9, 2018
@a-siva
Copy link
Contributor

a-siva commented May 16, 2018

It appears that there are a number of VM mirrors tests that are marked as failing in the status files which indicates we still have a number of issues with dart:mirrors functionality in --preview-dart-2 mode.

@kmillikin
Copy link

@manuelF Package unscripted itself is not a Dart 2 package. I run into a number of Dart 2 static errors when I try to compile it. (That's not to say that all the dart:mirrors stuff works there, but just that I haven't tested it).

@kmillikin
Copy link

I started to try to fix the failing mirrors tests. At least some of them are incorrect tests. We could argue about them on a case-by-case basis, because the current VM behavior is "incorrect" by that standard and changing the VM's behavior is a breaking change.

For example, in Dart 2 the classes defined by anonymous mixin applications should always be abstract, otherwise you will get Dart 2 static errors for any abstract members of the mixed-in class. One of the lib_2/mirrors tests tests that they are not abstract.

@kmillikin
Copy link

Another example: if an object that is not a Function is passed as the first argument to Function.apply, we will tear off the object's call method and pass that. If it is invoked with an incorrect number of arguments, that will not invoke the original object's noSuchMethod. lib_2/mirrors/apply3_test is broken for this reason.

So far, every test I've looked at is a test that was incorrectly ported from Dart 1. We'll need to fix them eventually but it isn't a high priority right now.

@kmillikin kmillikin added P2 A bug or feature request we're likely to work on and removed P1 A high priority bug; for example, a single project is unusable or has many test failures labels May 22, 2018
@kmillikin kmillikin removed this from the Dart2Stable milestone May 22, 2018
@jmesserly
Copy link

So far, every test I've looked at is a test that was incorrectly ported from Dart 1. We'll need to fix them eventually but it isn't a high priority right now.

yeah, I saw that when I moved these tests to lib_2/mirrors :\ ... a lot of tests in lib_2/corelib_2/language_2 still don't expect correct runtime behavior for Dart 2. I think the function call tearoff thing happened after the Dart 2 test migration, though. My guess is: we didn't update the tests after the call tearoff change, because many Dart 2 impls lack mirrors support, so it was hard to notice the test problem.

@itsjoeconway
Copy link
Author

If there is any way to contribute here as an outsider, please let me know. The issues with mirrors and --preview-dart-2 flag are blocking many of Aqueduct tests. I am anxious that this issue may get deferred until after Dart 2 release or late enough in the process that will delay us releasing with Dart 2. I recognize that there are many other priorities and y'all are killing it, but I did want to express that concern.

@dgrove
Copy link
Contributor

dgrove commented May 23, 2018 via email

@kmillikin
Copy link

To echo what @dgrove said, we could definitely use help identifying the high priority issues that affect you and your tests. You can file separate issues for them and just tag me for triage.

Eventually we will fix all those bugs (and probably modernize the API for Dart 2 features), but for now we want to prioritize fixing bugs "on demand" when we know that they are affecting customers.

@itsjoeconway
Copy link
Author

Very much appreciated. I've added #33207 and will tag you shortly.

dart-bot pushed a commit that referenced this issue Jun 5, 2018
Bug: #32345
Bug: #33326
Bug: #33345
Change-Id: I89fb8f0332ef6f9f40de59c219af2a7c23e5943f
Reviewed-on: https://dart-review.googlesource.com/58400
Commit-Queue: Ryan Macnak <[email protected]>
Reviewed-by: Vyacheslav Egorov <[email protected]>
Reviewed-by: Alexander Markov <[email protected]>
@kmillikin kmillikin added legacy-area-front-end Legacy: Use area-dart-model instead. front-end-kernel and removed legacy-area-front-end Legacy: Use area-dart-model instead. area-kernel labels Sep 19, 2018
@natebosch
Copy link
Member

@kmillikin - can we close this? I think mirrors are working now in the VM?

@a-siva
Copy link
Contributor

a-siva commented Jun 27, 2019

all reported issues against dart:mirrors has been fixed after the switch to Dart 2.

@a-siva a-siva closed this as completed Jun 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. front-end-kernel legacy-area-front-end Legacy: Use area-dart-model instead. P2 A bug or feature request we're likely to work on
Projects
None yet
Development

No branches or pull requests

10 participants