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

Cannot access Annotation from ElementAnnotation #17307

Closed
blois opened this issue Mar 6, 2014 · 15 comments
Closed

Cannot access Annotation from ElementAnnotation #17307

blois opened this issue Mar 6, 2014 · 15 comments
Assignees
Labels
area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. P2 A bug or feature request we're likely to work on type-enhancement A request for a change that isn't a bug

Comments

@blois
Copy link
Contributor

blois commented Mar 6, 2014

We have a number of code gen scenarios where we need to access the parameters passed to the annotation, it's currently a pain to do this when dealing with elements (have to get the index of the annotation, go up to the element, get the node, then get the annotation by that index).

Would be great if the Annotation AST node was exposed from ElementAnnotation.

@clayberg
Copy link

clayberg commented Mar 6, 2014

Set owner to @bwilkerson.
Added this to the 1.3 milestone.
Removed Type-Defect, Priority-Unassigned labels.
Added Type-Enhancement, Priority-Medium labels.

@blois
Copy link
Contributor Author

blois commented Mar 6, 2014

In addition, would be nice if Annotation.element went back to ElementAnnotation.

@bwilkerson
Copy link
Member

We have a number of code gen scenarios where we need to access the parameters passed to the
annotation, it's currently a pain to do this when dealing with elements (have to get the index of
the annotation, go up to the element, get the node, then get the annotation by that index).

Would be great if the Annotation AST node was exposed from ElementAnnotation.

I agree. Unfortunately, there are some technical challenges with providing that support. If you have ideas on how to address these challenges I'd love to hear them.

In order to be able to better manage the amount of memory being used by analyzer we decided to discard the AST structures when we no longer needed them. One requirement to allow this is that the element model (which is longer lived) cannot have any reference back to the AST nodes.

And while there is currently a method in Element (getNode()) that violates that principle, it has three problems. First, it can be expensive because if the AST has been discarded it will have to be re-created, which means re-reading and parsing the file. Second, it only works when you're accessing the element model through a context. And third, and perhaps most importantly, because of the first problem we can't support this operation in a world in which only async IO is allowed (which means it either needs to go away or be converted to return a future.

In addition, would be nice if Annotation.element went back to ElementAnnotation.

Makes sense, but ElementAnnotation isn't an Element. We could, however, provide a different accessor that would return the ElementAnnotation. I assume that would work as well.

@blois
Copy link
Contributor Author

blois commented Mar 7, 2014

We're creating a lot of transformers which are metadata driven, most of these are either operating against a resolved AST or are moving in that direction (otherwise it's a fancy string search).

It really seems like proper processing of metadata requires a mix of elements and AST- removing Element.getNode would break a lot of code.

@bwilkerson
Copy link
Member

It really seems like proper processing of metadata requires a mix of elements and AST ...

That's not surprising. It might be easier if we actually computed the values of the annotations so that you could interact with the objects (or mirrors of them), but we've been reluctant to implement even the subset of execution semantics needed for constants.

Is there additional information we could attach to the Element model to negate the need to get back to the AST?

... removing Element.getNode would break a lot of code.

We can leave the API if we modify it to return a Future. Not sure whether that will be good enough. I expect that many of the methods in AnalysisContext will also need to either return a Future or be removed.

Would it work for you to traverse the resolved AST's rather than the elements (after we've resolved Annotations to ElementAnnotations)?

@blois
Copy link
Contributor Author

blois commented Mar 8, 2014

This is effectively what we are doing already. If you look at Angular,
Polymer or DI- all of them make heavy use of annotation parameters. But
they limit the syntax supported.

The arguments to the annotation constructor.

We've had a lot of issues with async-only APIs, I'd strongly recommend
providing a synchronous version of the APIs as well. It's probably a larger
discussion to be had elsewhere, but lets discuss if you're considering
removing sync APIs.

This would probably work- I do this in a couple of places since it's so
difficult to go the other way.

@bwilkerson
Copy link
Member

Access from an Annotation to the ElementAnnotation is provided in https://codereview.chromium.org/193833003/ (bleeding edge revision 33532).

It's probably a larger discussion to be had elsewhere, but lets discuss if you're
considering removing sync APIs.

A discussion might be good. I'm guessing, though, that the best way to do that would be to have both sync and async versions of each method (named "foo" and "fooSync") where the synchronous version can throw an exception if it requires synchronous IO in an environment where synchronous IO isn't allowed.

@clayberg
Copy link

clayberg commented Apr 9, 2014

Removed this from the 1.3 milestone.
Added this to the 1.4 milestone.

@kasperl
Copy link

kasperl commented May 8, 2014

Removed this from the 1.4 milestone.
Added this to the 1.5 milestone.

@bwilkerson
Copy link
Member

Removed this from the 1.5 milestone.
Added this to the Later milestone.

@kasperl
Copy link

kasperl commented Jul 10, 2014

Removed this from the Later milestone.
Added Oldschool-Milestone-Later label.

@kasperl
Copy link

kasperl commented Aug 4, 2014

Removed Oldschool-Milestone-Later label.

@sigurdm
Copy link
Contributor

sigurdm commented May 8, 2015

That's not surprising. It might be easier if we actually computed the values of the annotations so that you could interact
with the objects (or mirrors of them), but we've been reluctant to implement even the subset of execution semantics needed for constants.

In dart2js-land we concluded that we need two views on constants: ConstantExpression and ConstantValue.
The ConstantExpression tells how the constant is constructed (variables, fields, constructors (with a mapping arg -> value), operations on constants etc.)

ConstantValues (the evaluation of an expression in a constant environment) are primitives lists, maps or
constructed constants (with a mapping field->value).

I think for the purpose of making language tools (transformers etc.) with the analyzer package a similar representation would be very beneficial.

For example for the reflectable package we want to be able to recognize any annotation of the "value" of a given class (similar to the problem in issue #15467).

Also we want to be able to insert the default value from some function-argument into some code - here we need the expression to be able to generate code that creates the same constant.

@blois blois added Type-Enhancement area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. labels May 8, 2015
@sigurdm
Copy link
Contributor

sigurdm commented Jun 15, 2015

Are there any updates on this?

@kevmoo kevmoo added P2 A bug or feature request we're likely to work on type-enhancement A request for a change that isn't a bug and removed triaged labels Mar 1, 2016
@bwilkerson
Copy link
Member

I'm closing this as stale. Please re-open if this is still an issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. P2 A bug or feature request we're likely to work on type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests

6 participants