Skip to content

Restrict dart:mirrors on DDC #32294

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
vsmenon opened this issue Feb 23, 2018 · 7 comments
Closed

Restrict dart:mirrors on DDC #32294

vsmenon opened this issue Feb 23, 2018 · 7 comments
Assignees
Labels
P1 A high priority bug; for example, a single project is unusable or has many test failures web-dev-compiler
Milestone

Comments

@vsmenon
Copy link
Member

vsmenon commented Feb 23, 2018

We're no longer supporting dart:mirrors on the web. We have limited support for it right now and the emit-metadata flag is required to preserve some annotation info.

We should:

  • Disallow importing except via special flag (maybe emit-metadata) - minimum bar for milestone
  • Remove more static metadata that can't be read without mirrors (e.g., constructor / static method type info)
  • Eventually (once pageloader is removed) delete mirrors code.

@jmesserly

@vsmenon vsmenon added this to the 2.0-beta1 milestone Feb 23, 2018
@jmesserly jmesserly self-assigned this Feb 23, 2018
@dgrove dgrove modified the milestones: (Please move issues from the milestone) 2.0-beta1, Dart2 Beta 3 Mar 22, 2018
@jmesserly
Copy link

I'm working on a CL for this. There are a few tests in tests/*_2 that import dart:mirrors, so that needs triage. I'm also running internal tests to look for any breakage caused by restricting the import.

@jmesserly
Copy link

I've got a ways to go before the CL is ready (because status files), but this CL fixes the tests: https://dart-review.googlesource.com/c/sdk/+/49624

@dgrove
Copy link
Contributor

dgrove commented Apr 10, 2018

any updates on this?

@jmesserly
Copy link

Slowly making progress.

This CL reduces signature data DDC stores: https://dart-review.googlesource.com/c/sdk/+/48455. More things will not be reflectable unless emit-metadata is enabled. I'm waiting on a code review.

I fixed the language tests to reflect correct behavior in https://dart-review.googlesource.com/c/sdk/+/49624 (moving dart:mirrors tests into lib_2/mirrors, fixing tests that don't need to use mirrors, splitting a few other tests that legitimately tested both mirrors and non-mirrors functionality). However I can't send that CL out because it needs more status file fixes. Those are very difficult/time consuming so I'm not sure how long it will take.

Once that lands, and we've got internal code fixed too, we can land a CL that restricts dart:mirrors import. I'll send you a message offline regarding internal repository details.

@dgrove dgrove modified the milestones: Dart2 Beta 3, Dart2 Beta 4 Apr 11, 2018
@jmesserly
Copy link

this is the CL that disables mirrors (in DDC, need DDK as well) https://dart-review.googlesource.com/c/sdk/+/53163 ... I'm going to see if this can be rolled into internal code or if we need fixes there.

@jmesserly jmesserly added the P1 A high priority bug; for example, a single project is unusable or has many test failures label Apr 30, 2018
@jmesserly
Copy link

@jmesserly
Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P1 A high priority bug; for example, a single project is unusable or has many test failures web-dev-compiler
Projects
None yet
Development

No branches or pull requests

3 participants