Skip to content
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

Allow using dart:mirrors #1150

Closed
stevenroose opened this issue Jan 8, 2016 · 61 comments
Closed

Allow using dart:mirrors #1150

stevenroose opened this issue Jan 8, 2016 · 61 comments

Comments

@stevenroose
Copy link

@stevenroose stevenroose commented Jan 8, 2016

I know that mirrors is blocked now because it causes the generated packages to be very large.

However, on the long term, it should remain a goal to find a solution for this. Many libraries use mirrors so not being able to use mirrors for Flutter apps will make the framework a lot less powerful.

The trade-off between large package sizes and using mirrors should be left to the user. A possible solution might be to use an explicit --allow-mirrors flag. But of course the ultimate solution should be to find a way to use mirrors without blowing up package sizes.

Java has reflection and Objective-C has Runtime Reference, so why would Flutter not have a reflection system?

@Hixie
Copy link
Member

@Hixie Hixie commented Jan 8, 2016

Allowing mirrors would mean we couldn't remove unused code (since all the code is implicitly used). That's what causes the code size increase.

Java apps are huge, that's one of the things we want to avoid. Objective C, as I understand it, doesn't trim unused members from classes either. It also doesn't strip any classes that are exported, because it uses dynamic linking.

Dart uses static linking (effectively), and using static analysis we can strip out anything that isn't used ("tree shaking"). If you import a huge Dart library but only use a self-contained two-line method, then you only pay the cost of the two-line method, even if that Dart library itself imports dozens and dozens of other libraries.

(At least, in theory. I'm not sure how much of this we implement yet.)

We could enable mirrors only for classes that you opt into (similar to Delphi's $M+). That would require support from the language.

@stevenroose
Copy link
Author

@stevenroose stevenroose commented Jan 8, 2016

@Hixie Isn't that what the @MirrorsUsed annotation is for? I thing dart2js also does tree shaking using the @MirrorsUsed annotation.

@Hixie
Copy link
Member

@Hixie Hixie commented Jan 8, 2016

I think that's more about avoiding name mangling, which is a whole other kettle of fish (we don't do any name mangling). But it would be a similar kind of thing, yes. To my knowledge the snapshotter doesn't support any annotation of that kind currently.

@stevenroose
Copy link
Author

@stevenroose stevenroose commented Jan 8, 2016

I quote from this article from @sethladd

Mirrors are really powerful, but they introduce challenges to dart2js's tree shaking abilities. Normally, dart2js can look at the entire app and deduce what code is required to run the app. However, as of the time of this writing, mirrors (due to their API design and highly dynamic nature) more-or-less disable dart2js's tree shaking.

Enter @MirrorsUsed, a metadata annotation, which explicitly states what parts of the program are reflected. Tools like dart2js can use @MirrorsUsed to re-enable tree shaking.

I think that the guys that wrote tree shaking for dart2js can really help you out here since it seems they had the same problem and managed to fix it. Of course, package size will still be larger when using mirrors, but the difference won't be as significant when tree shaking is done fairly efficiently.

@Hixie
Copy link
Member

@Hixie Hixie commented Jan 8, 2016

Ah, yeah, I didn't see 'targets' and 'metaTargets' before. Those could be used to do this, if the snapshotter supported them.

@sethladd
Copy link
Contributor

@sethladd sethladd commented Jan 8, 2016

MirrorsUsed is not supported and has edge cases it doesn't cover. I wouldn't build a solution around it.

@stevenroose
Copy link
Author

@stevenroose stevenroose commented Jan 8, 2016

@sethladd I noticed it said "experimental", yes.
But is it still the only thing that helps dart2js tree shaking with mirrors? Or are there other strategies.

Anyways, dart2js and Flutter seem to have a similar issue when it comes to mirrors usage, so I guess they can use the same "solution", however complete that solution might be.

I believe dart2js still warns users of large output size when it sees mirrors usage, that is a good practice. Better than preventing users from using it...

@floitschG
Copy link
Contributor

@floitschG floitschG commented Jan 9, 2016

There is also a reflectable package. It can either run untransformed, using
dart:mirrors, or transformed, where it extracts the information beforehand.
It is not complete, though. For example there is no way to access private
identifiers.
It also adds an additional build step...
On Jan 8, 2016 23:49, "Steven Roose" notifications@github.com wrote:

@sethladd https://github.com/sethladd I noticed it said "experimental",
yes.
But is it still the only thing that helps dart2js tree shaking with
mirrors? Or are there other strategies.

Anyways, dart2js and Flutter seem to have a similar issue when it comes to
mirrors usage, so I guess they can use the same "solution", however
complete that solution might be.

I believe dart2js still warns users of large output size when it sees
mirrors usage, that is a good practice. Better than preventing users from
using it...


Reply to this email directly or view it on GitHub
#1150 (comment).

@eernstg
Copy link
Contributor

@eernstg eernstg commented Jan 12, 2016

For completeness of reflectable, please check out the 'Known limitations' section at the end of https://github.com/dart-lang/reflectable/tree/master/reflectable.

@abarth
Copy link
Contributor

@abarth abarth commented Oct 16, 2016

We seem to have gotten pretty far without dart:mirrors. I think we can safely close this issue.

@abarth abarth closed this Oct 16, 2016
@stevenroose
Copy link
Author

@stevenroose stevenroose commented Oct 16, 2016

I don't agree.

We seem to have gotten pretty far without dart:mirrors.

You know that it makes no sense to say that, right? People know that mirrors is not supported so they either leave the platform (I don't think you can deny that traction is not really great), removed features from their application or used alternative libraries.

Both Android and iOS support reflection, which means that as a competitor against their default development platforms, this will always be a setback for some developers.

While reflectable is a decent enough way to bridge the gap for now, it is far from ideal and as I already pointed out, dart:mirrors + MirrorsUsed should be able to get exactly the same performance than reflectable.

@abarth
Copy link
Contributor

@abarth abarth commented Oct 16, 2016

The approach we use when considering which features to implement is to compare the use cases for those features against the cost of implementing and maintaining the feature. In the case of dart:mirrors, we've largely found less costly ways of addressing these use cases. It's possible that we'll eventually encounter a compelling use case that requires dart:mirrors, in which case we can reconsider implementing that feature, but having this bug open doesn't help us make progress.

Rather than bugs requesting particular solutions, we prefer to work from bugs that state problems, which gives us the flexibility to address those problems in a different ways and potentially find better solutions.

@stevenroose
Copy link
Author

@stevenroose stevenroose commented Oct 16, 2016

Well, I agree that this is not of a particular priority, that's for sure.
But is there a problem withs issues that live a long time? You can tag them
as improvement or postponed or low priority. I just think closing an issue
that still is one just because you want to reduce the backlog, is a
strategy I don't really agree with.

On Sun, Oct 16, 2016 at 6:03 PM, Adam Barth notifications@github.com
wrote:

The approach we use when considering which features to implement is to
compare the use cases for those features against the cost of implementing
and maintaining the feature. In the case of dart:mirrors, we've largely
found less costly ways of addressing these use cases. It's possible that
we'll eventually encounter a compelling use case that requires
dart:mirrors, in which case we can reconsider implementing that feature,
but having this bug open doesn't help us make progress.

Rather than bugs requesting particular solutions, we prefer to work from
bugs that state problems, which gives us the flexibility to address those
problems in a different ways and potentially find better solutions.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#1150 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AA0F3MfDSMonLKa1XGIvYprTX2049D_0ks5q0krTgaJpZM4HBcjM
.

@Hixie
Copy link
Member

@Hixie Hixie commented Oct 16, 2016

The problem is this isn't really an issue. It's not describing something you can't do, it's just a request for a particular solution. We prefer to track the actual issues. You should definitely feel free to file bugs describing the things you want to do that you can't do without mirrors.

@stevenroose
Copy link
Author

@stevenroose stevenroose commented Nov 4, 2016

@Hixie To quickly come back to this: the concrete issue it not about a specific functionality that is missing, but more the inability to use any library that relies on mirrors.

I think a lot of libraries will have to start doing things like this: PointyCastle/pointycastle#107

@Hixie
Copy link
Member

@Hixie Hixie commented Nov 4, 2016

Those libraries are going to stop working in general once Dart drops mirrors entirely, so I'm not too worried about that.

There are certain things that aren't available to a Dart library in Flutter. Some of those things are obvious and nobody thinks about them, like "C++ syntax" or "Perl libraries". Others are less obvious initially, like "no dart:js" or "no dart:html", but they make sense with a quick examination. And then there's some that aren't obvious at all unless you really know how it works, like "no dart:mirrors". There's libraries affected by all of these. I don't think it really matters on the long run. People will just avoid the libraries that don't work with Flutter because of using dart:mirrors just like they avoid using the libraries that don't work with Flutter because they're written in Lua, or whatever.

@stevenroose
Copy link
Author

@stevenroose stevenroose commented Nov 4, 2016

Are there plans to discontinue mirrors entirely?

I completely disagree with your reasoning btw. For the things you
mention there are valid reasons that prevent these features being
available in Flutter: Dart does not support Perl syntax or Flutter apps
have no notion of a DOM. There is nothing that prevents mirrors from
being available in Flutter. The reason it is not supported is binary
size, which is exactly the same concern for dart2js. In dart2js however
this trade-off is left to the user.

Don't get me wrong, I totally understand that support for mirrors is a
lot of work and there might be valid arguments that it's not worth it.
However, it is a feature that people may want and it is possible to
support, so the place for it should be on the long-term wish list.

On 4 Nov 2016 20:29, "Ian Hickson" <notifications@github.com
mailto:notifications@github.com> wrote:

Those libraries are going to stop working in general once Dart drops
mirrors entirely, so I'm not too worried about that.

There are certain things that aren't available to a Dart library in
Flutter. Some of those things are obvious and nobody thinks about
them, like "C++ syntax" or "Perl libraries". Others are less obvious
initially, like "no dart:js" or "no dart:html", but they make sense
with a quick examination. And then there's some that aren't obvious
at all unless you really know how it works, like "no dart:mirrors".
There's libraries affected by all of these. I don't think it really
matters on the long run. People will just avoid the libraries that
don't work with Flutter because of using dart:mirrors just like they
avoid using the libraries that don't work with Flutter because
they're written in Lua, or whatever.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<https://github.com/flutter/flutter/issues/1150#issuecomment-258526243>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AA0F3NmHdkceWwtsxMvu8hbjhf8Xz2LKks5q64eLgaJpZM4HBcjM>.
@Hixie
Copy link
Member

@Hixie Hixie commented Nov 5, 2016

Nothing prevents us from having a Perl interpreter either. :-)

The question is, what can't you do with Flutter today that you could do if you had mirrors? "Use libraries that use mirrors" is not a good answer to that, because it has the simple retort "just rewrite the libraries not to use mirrors", just like "You should support Perl libraries so that I can use Perl libraries" isn't a good description of a problem (just write those libraries in Dart instead).

What actual thing can't you do today that you could do with mirrors? You should file a bug saying that you can't do that specific thing. If mirrors are the best way to address that problem, and if the problem is worth addressing, then we'd figure out how to introduce mirrors.

Are there plans to discontinue mirrors entirely?

I don't know about specific plans but with the push towards strong mode it would make sense to me to drop things like mirrors. (Personally I'd like to replace mirrors with metaclasses, so we get all the benefits of mirrors with none of the tree-shaking costs, but that's also probably not going to happen.)

@eernstg
Copy link
Contributor

@eernstg eernstg commented Nov 8, 2016

Are there plans to discontinue mirrors entirely?

Drive-by comment: This is the Flutter repo, so this discussion is distinct from discussions about how/whether Dart will support reflection on other platforms. Besides, as long as it's possible to edit Dart code manually it will also be possible to use techniques like the ones in reflectable to obtain some level of support for reflection. In that sense it isn't even possible to discontinue mirrors entirely.

@branflake2267
Copy link

@branflake2267 branflake2267 commented Apr 23, 2017

Adding a note for future searches. I can't use the json_object library, it depends on dart_mirrors, which is blocked. Would be nifty if the IDE would detect blacklisted libraries or transitives that are blocked.

pubsec.yaml "json_object: any"
import 'package:json_object/json_object.dart';

@matejthetree
Copy link

@matejthetree matejthetree commented Dec 16, 2018

@zoechi
Copy link
Contributor

@zoechi zoechi commented Dec 16, 2018

@matejthetree as already mentioned above #1150 (comment) (and the following discussion)

@rrousselGit
Copy link
Contributor

@rrousselGit rrousselGit commented Feb 13, 2019

I don't think #26125 is a duplicate of this one, but I'll put my points here.

I personally agree that we do not need dart:mirror in a shipped application, as code-generation solves most of it.

But there's a catch: code-generators themselves may want to depend on flutter

A typical example is a package of mine: https://github.com/rrousselGit/functional_widget

It is a code generator that generates Widget subclass and debugFillProperties overrides. As such, functional_widget heavily depends on Flutter features.

But due to dart:mirror being disabled, it is impossible for such code generator to depend on Flutter as some generations dependencies use mirroring (such as build_runner and source_gen).

It currently works thanks to multiple workarounds. But there are many limitations


Overall, I think all dev dependencies should be allowed to use mirroring. The use-case is not limited to code generation, and it has no impact on what is shipped in the final app.

Here's a list of cool dev-only features I have in mind:

  • snapshot testing using dart2json (as we don't want to alter the model just for tests)
  • mockito spies
  • running only tests impacted by local git changes
@rmawatson
Copy link

@rmawatson rmawatson commented Mar 29, 2019

This should definitely be made available to depend on for dev_dependencies. It already works if imported, and is infact used by source_gen such as in TypeChecker.fromRuntime, which obviosly works in a flutter project, but an import of dart:mirrors in a flutter project generator will be shown as invalid in IntelliJ IDEA and code completion will not work.

I think if anything a prominent warning should be shown if a flutter project depends on mirrors, but it should be up to the user to be competent enough to manage their dependencies so as not to use mirrors in the actual app, if a bloated binary is the problem, rather then this being disabled altogether.

@anaisbetts
Copy link
Contributor

@anaisbetts anaisbetts commented Apr 1, 2019

I really wish this decision would be reconsidered, it's having some pretty significant effects across the ecosystem - having something as extremely common as JSON deserialization requiring people to manually set up build-time codegen is a huge barrier to adoption; no other mobile platform requires this.

Xamarin has a similar problem with AOT and Reflection, and solves it via allowing the developer to explicitly opt-in classes via linker directives - while it can be a pain in the ass in large apps to whack-a-mole types over and over until your app works, it is at least a Straightforward, Easy To Use process.

While alternatives such as reflectable exist, the fact that it can't be used in a library (i.e. you shouldn't publish *.reflectable.dart files) makes it pretty untenable as a General Solution to this problem

@rrousselGit
Copy link
Contributor

@rrousselGit rrousselGit commented Apr 1, 2019

requiring people to manually set up build-time codegen is a huge barrier to adoption; no other mobile platform requires this.

Hopefully, Flutter did some progress on that topic. On master branch, code-gen integration is working out of the box (at least that's what is promised).

So this should make libraries that requires code-generators a lot more viable.

@eernstg
Copy link
Contributor

@eernstg eernstg commented Apr 1, 2019

@paulcbetts wrote:

you shouldn't publish *.reflectable.dart files

This is a trade-off. Given that reflectable allows you to specify things like @GlobalQuantifyCapability(r"^dart.core.List", reflector), which means that every subtype of List will support the reflective capabilities specified for reflector, there is no way you could generate code that would be able to fulfill that promise for subtypes of List declared in an arbitrary library of the "current program" unless you allow the code generator to run in the context of the client library (that defines what "the program" is). So you wouldn't publish *.reflectable.dart files, because they won't contain the code for the whole program, only for code reachable from your package.

But if you're happy saying that the classes for which there is reflection support is a closed set (that is, everything that you can see from your published package can be covered if there's a reflector that matches it, but you don't care that nothing is covered if it is declared elsewhere, e.g., in the client package that defines the "program", even if it matches a reflector), then you could publish your *.reflectable.dart files.

@ishowfun
Copy link

@ishowfun ishowfun commented Apr 23, 2019

To flutter:越俎代庖

@Nastya-Scherbakova
Copy link

@Nastya-Scherbakova Nastya-Scherbakova commented May 26, 2019

Well, it's really sad. Many things like json serialization/deserealization and SQLite creation of tables, requests are not comfortable to work with. I thought I can improve that using dart:mirrors, but has found this issue. It causes bad experience on projects with many classes.

truongsinh added a commit to truongsinh/graphql-flutter that referenced this issue May 31, 2019
dart test, either in flutter or web browser, does not support mirror,
see dart-lang/sdk#30538 and
flutter/flutter#1150.
Using `Directory.current.path` might still be problematic for web browser later on,
but now let's focus on flutter only.
truongsinh added a commit to truongsinh/graphql-flutter that referenced this issue May 31, 2019
dart test, either in flutter or web browser, does not support mirror,
see dart-lang/sdk#30538 and
flutter/flutter#1150.
Using `Directory.current.path` might still be problematic for web browser later on,
but now let's focus on flutter only.
@kasp1
Copy link

@kasp1 kasp1 commented Oct 22, 2019

My reason to use mirrors was to set a user property in a user class from my library that doesn't know the property/object name in advance.

Please suggest a better solution without mirrors if you can think of any: https://stackoverflow.com/questions/58493417/assign-property-from-external-object/58501443#58501443

@matejthetree
Copy link

@matejthetree matejthetree commented Oct 22, 2019

@kasp1
Copy link

@kasp1 kasp1 commented Oct 22, 2019

@matejthetree Quick check, does code generation require me or the user to use commands in their IDE that would generate additional files in the project/lib?

@matejthetree
Copy link

@matejthetree matejthetree commented Oct 22, 2019

@kasp1
Copy link

@kasp1 kasp1 commented Oct 22, 2019

@matejthetree I personally believe simplicity of usage (and ease of learning) are the keys to success of any lib/framework/app. For this reason, I'd prefer to avoid code generation (as the user always has to learn something additional) and keep my "creative" syntax instead.

I'm not blaming Flutter, it's a great framework and if disabling mirrors is a worthy trade off for something else, fine then. Maybe as it's been already mentioned here, optional mirrors support might be handy but I can understand it may be alot of work. Just adding here a reason to do so.

@mildsunrise
Copy link

@mildsunrise mildsunrise commented Nov 7, 2019

So far I haven't seen a technical explanation of what it takes to allow the user to use dart:mirrors... Isn't tree shaking performed by the snapshotter, i.e. Dart? Why/how is Flutter involved in this decision?

@icatalud
Copy link

@icatalud icatalud commented Nov 21, 2019

Quantifying the cost of adding mirrors should be clarified, so people can get convinced it's not good idea to add mirrors. Say something like "This is what would happen to the program if mirrors is enabled: uses as 200mb" (for example).

Could someone from the Dart team give an idea of the current cost?

@mildsunrise
Copy link

@mildsunrise mildsunrise commented Nov 22, 2019

I'm not a Dart expert but I can now confirm this has nothing to do with Flutter, it's entirely a Dart issue, and I'll try to give some technical insights.

There are actually two places in the AOT compilation pipeline where tree shaking is performed:

  1. In the frontend server, when compiling Dart code to the kernel binary (.dill): Type Flow Analysis is performed (source code), and a two-pass tree shaking is done as part of this process.

  2. In gen_snapshot, which compiles the kernel binary to an AOT snapshot (libapp.so): in the precompilation step (Dart_Precompile which calls Precompiler::DoCompileAll) a second tree shaking is performed.

In my experiments the second tree-shaking barely removed anything that the TFA hadn't done already.

I've patched out the tree-shaking entirely: in a test Flutter project, it increased the snapshot file size from 9MB to 30MB and snapshot loading times by up to 5x. So yeah, it's quite an important optimization. I haven't made any RAM / performance tests.

However I think it'd be easy to process @MirrorsUsed annotations in the tree shaker and skip those, just as it's already done with the vm:entry-point pragma. I still have a lot to learn about the VM, so I could be missing some important details...

@k-paxian
Copy link

@k-paxian k-paxian commented Dec 10, 2019

If someone is looking for a decent solution based on reflectable I've got it
https://github.com/k-paxian/dart-json-mapper
@eernstg Thank you for all your hard work on that approach.

@pedromassango
Copy link
Member

@pedromassango pedromassango commented Apr 6, 2020

Overall, I think all dev dependencies should be allowed to use mirroring. The use-case is not limited to code generation, and it has no impact on what is shipped in the final app.

I agree with @rrousselGit. Most of the libraries that uses Code generator (and want to use dart:mirrors) are dev dependencies so maybe they don't impact the binary size.

@aspectgfg
Copy link

@aspectgfg aspectgfg commented Apr 11, 2020

Pre-mirrors supporter here:
While I think eliminating mirror support is awful because dev's carry the cost of mirroring and are fully capable of handling mirroring themselves, the generator libs are actually pretty awesome and satisfy needs of developers very well. So the "added cost" of mirrors, is kind of moot because generators satisfy.

@lock
Copy link

@lock lock bot commented Apr 25, 2020

This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of flutter doctor -v and a minimal reproduction of the issue.

@lock lock bot locked and limited conversation to collaborators Apr 25, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet