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

Provide better way to access constants through source mirrors. #18346

Closed
efortuna opened this issue Apr 21, 2014 · 11 comments
Closed

Provide better way to access constants through source mirrors. #18346

efortuna opened this issue Apr 21, 2014 · 11 comments
Assignees
Labels
closed-not-planned Closed as we don't intend to take action on the reported issue library-mirrors web-dart2js

Comments

@efortuna
Copy link
Contributor

Currently with a String constant, you can say Dart2JsStringConstantMirror.reflectee and get the original String object.

Why can't we do this with composite objects like Map and List?

As it is, to get the original value, I need to check if the particular mirror is of type Dart2JsMapConstantMirror or Dart2JsListConstantMirror and then call the corresponding method, which seems pretty suboptimal, especially since those aren't public interfaces.

@efortuna
Copy link
Contributor Author

Marked this as blocking #17982.

@johnniwinther
Copy link
Member

First, this will only be possible for constants that consists solely of maps, list, numbers, bools and strings, that is not constant classes.

Secondly, the interfaces MapInstanceMirror and ListInstanceMirror located in source_mirrors.dart should be sufficient to implement the current state-of-affairs.

Thirdly, why is this blocking 18346?

@peter-ahe-google
Copy link
Contributor

Unmarked this as blocking #17982.

@floitschG
Copy link
Contributor

It looks like we will have to create a ConstantMirror that provides enough information. As Johnni points out, we can provide the constants for non-dart:core constants (like const A()). In any case it wouldn't give enough information, because we lose the trace of how the constant was created.
For example:

class A implements B {
  A._internal(this.x): this.y = 499;
  final x, y;
}

class B {
  const factory B(x) = A;
}

foo([arg = const B(499)]) {}

When writing the documentation for foo we want to write that const B(499) is the default value. However the actual constant is an instance of A and the only way to construct it (using A's constructors) would be the A._internal constructor. Clearly, that's not good enough for documentation.

@peter-ahe-google
Copy link
Contributor

In an email to Emily, I suggested that she might need to use AST nodes. I've volunteered to provide a sample if needed.

@efortuna
Copy link
Contributor Author

Thanks for the example, Florian, that clarifies the issue. I didn't realize all the complications surrounding the feature request. I think I'll go the AST nodes route to get what was actually specified.

@efortuna
Copy link
Contributor Author

shall we close this feature request for now as won't fix?

@peter-ahe-google
Copy link
Contributor

This CL provides a prototype API and a sample for how to print constants with links to definitions:

https://codereview.chromium.org/257863002


Set owner to @peter-ahe-google.
Added Started label.

@floitschG
Copy link
Contributor

I changed the summary of this issue.

In the near-future we can go through the AST, but I would prefer if that was not necessary. We will discuss this.


Changed the title to: "Provide better way to access constants through source mirrors.".

@peter-ahe-google
Copy link
Contributor

Florian, I'd like to understand what your concerns are in more detail. Personally, I don't like exposing the AST of dart2js, because it hasn't been designed for general purpose and because doing so could make implementing new language features problematic.

However, I imagine that you could make some AST API that wouldn't have this problem.

Is your concern about specifically exposing the current AST nodes of dart2js, or is it more fundamentally that you think there is a different way to solve this problem that doesn't involve an AST API (in any form)?

@sigmundch
Copy link
Member

Now that dartdoc was moved away from the source-mirrors library and is using the analyzer parser directly, we will deprecate the source-mirrors library. So we are not planning to add this feature anymore.
@sigmundch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed-not-planned Closed as we don't intend to take action on the reported issue library-mirrors web-dart2js
Projects
None yet
Development

No branches or pull requests

5 participants