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

Analyzer >= 0.26.1+15 breaks reflectable #24890

Closed
kevmoo opened this Issue Nov 12, 2015 · 24 comments

Comments

Projects
None yet
5 participants
@kevmoo
Copy link
Member

kevmoo commented Nov 12, 2015

@eernstg

This comment has been minimized.

Copy link
Member

eernstg commented Nov 12, 2015

Reflectable too, see dart-archive/code_transformers#29, and Polymer, see #24735.

@mit-mit mit-mit added this to the 1.14 milestone Nov 18, 2015

@kevmoo

This comment has been minimized.

Copy link
Member Author

kevmoo commented Dec 10, 2015

@bwilkerson Any updates on this?

I have a work-around – should that be what @eernstg is using, too?

@bwilkerson

This comment has been minimized.

Copy link
Member

bwilkerson commented Dec 10, 2015

I'm confused. Does this bug still manifest after moving to analyzer version 0.27.0? I thought that fixed the problem.

@kevmoo

This comment has been minimized.

Copy link
Member Author

kevmoo commented Dec 10, 2015

@bwilkerson See this branch of source_gen

https://github.com/dart-lang/source_gen/tree/brian_weird_fix

Removed the work around – many tests start to fail

@eernstg

This comment has been minimized.

Copy link
Member

eernstg commented Dec 10, 2015

Hi,

I've made several attempts to use a newer version of the analyzer (the
newest one that I could get pub to select without overrides was 0.26.4, but
I guess version 0.3.something-really-fresh will allow me to try out
0.27.*), and I do not see the infinite loop that made us insert the
'<=0.26.1+14' constraint a while ago. So there's a lot of hope! On the
other hand, I keep getting null in a number of situations where we didn't
get them before; it's possible that a hint from Brian that I just noticed
in my inbox will help with that. I'll experiment some more with this
tomorrow.

On Thu, Dec 10, 2015 at 10:23 PM, Kevin Moore notifications@github.com
wrote:

@bwilkerson https://github.com/bwilkerson See this branch of source_gen

https://github.com/dart-lang/source_gen/tree/brian_weird_fix

Removed the work around – many tests start to fail


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

Erik Ernst - Google Danmark ApS
Skt Petri Passage 5, 2 sal, 1165 København K, Denmark
CVR no. 28866984

@bwilkerson

This comment has been minimized.

@kevmoo

This comment has been minimized.

Copy link
Member Author

kevmoo commented Dec 16, 2015

I've validated the fix with 0.27.1 in source_gen – can close as fixed?

@eernstg

This comment has been minimized.

Copy link
Member

eernstg commented Dec 17, 2015

In support of this: 0.27.1 works with reflectable version 0.5.0 as well.

@donny-dont

This comment has been minimized.

Copy link

donny-dont commented Dec 22, 2015

I've been having problems in dogma_codegen with 0.27.1+1 where evaluationResult is null.

https://github.com/dogma-dart/dogma-codegen/blob/features/refactor-library-metadata/lib/src/analyzer/annotation.dart#L58

Putting the pubspec at 0.27.1 works but 0.27.1+1 fails.

@eernstg

This comment has been minimized.

Copy link
Member

eernstg commented Dec 22, 2015

Yes, 0.27.1+1 also breaks reflectable (the nulls from evaluationResult are indeed the main problem). We have notified the analyzer folks.

@kevmoo

This comment has been minimized.

Copy link
Member Author

kevmoo commented Dec 22, 2015

source_gen tests pass with the latest analyzer – perhaps my tests don't push the API in the same way.

@donny-dont

This comment has been minimized.

Copy link

donny-dont commented Dec 22, 2015

@eernstg is there a separate bug? Seemed similar to this issue.

@eernstg

This comment has been minimized.

Copy link
Member

eernstg commented Dec 22, 2015

The issue dart-lang/reflectable#54 tracks the evaluationResult == null analyzer issue as seen from reflectable. We just published 0.5.1 today which insists on analyzer 0.27.1 (because 0.27.0 and 0.27.1+1 both have this null issue).

@kevmoo

This comment has been minimized.

Copy link
Member Author

kevmoo commented Jan 13, 2016

@donny-dont @eernstg have you tried analyzer 0.27.1+2 +

@bwilkerson bwilkerson modified the milestones: 1.14, 1.15 Jan 13, 2016

@eernstg

This comment has been minimized.

Copy link
Member

eernstg commented Jan 14, 2016

Hi,

yes, we tried that, and it has the same issue (constants not resolved,
reported as null). 'Messages' in https://codereview.chromium.org/1531393002/
contains more details about how it fails.

On Wed, Jan 13, 2016 at 11:59 PM, Kevin Moore notifications@github.com
wrote:

@donny-dont https://github.com/donny-dont @eernstg
https://github.com/eernstg have you tried analyzer 0.27.1+2 +


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

Erik Ernst - Google Danmark ApS
Skt Petri Passage 5, 2 sal, 1165 København K, Denmark
CVR no. 28866984

@kevmoo kevmoo changed the title Analyzer >= 0.26.1+15 breaks source_gen Analyzer >= 0.26.1+15 breaks reflectable Jan 14, 2016

@kevmoo

This comment has been minimized.

Copy link
Member Author

kevmoo commented Jan 14, 2016

Updated the title to "reflect" reflectable

@mit-mit

This comment has been minimized.

Copy link
Member

mit-mit commented Mar 1, 2016

@bwilkerson this is tagged P1 but I don't see any updates for more than a month. What is the status here!?

@eernstg

This comment has been minimized.

Copy link
Member

eernstg commented Mar 1, 2016

To be precise the title should indicate that there is one recent version that does not break reflectable, namely 0.27.1 (which is the one that we have been using ). E.g., 'Analyzer >0.27.1 breaks reflectable' might do. But I'm not sure it makes sense to change the title of an existing issue several times (it's difficult to understand the history when it does not match the title any more). There is also the issue dart-lang/reflectable#54 which is directly concerned with the situation as seen from reflectable.

I just confirmed that we still get null when evaluating many constants. The symptom as of today is that we get a message in the following format for every entry point being transformed:

[Info from Reflectable on <my-entry-point>.dart]: Ignoring entry point <my-entry-point> that does not include the class Reflectable.

Given that resolution is no longer completed on the LibraryElements we obtain from the analyzer via code_transformers, we need APIs that will allow us to reliably detect when a null valued constant is unresolved, and to request the resolution of that unresolved constant.

@kevmoo kevmoo added type-bug and removed Type-Defect labels Mar 1, 2016

@eernstg

This comment has been minimized.

Copy link
Member

eernstg commented Mar 2, 2016

The changes could affect code_transformers and/or analyzer. Reflectable has no access to the relevant AnalysisContext so in reflectable we cannot directly make calls like context.computeLibraryElement(source), but code_transformer presumably has no access to Source values for libraries which are not given as assets (and neither does reflectable), so there is a need for getting those source values out of the analyzer, or being able to ask the analyzer to complete the resolution of any given LibraryElement.

@bwilkerson

This comment has been minimized.

Copy link
Member

bwilkerson commented Mar 2, 2016

Actually, every element knows the context in which it was created, so, given the classElement that has the annotation you're interested in, you could write:

classElement.context.computeLibraryElement(classElement.library.definingCompilationUnit.source);
@mit-mit

This comment has been minimized.

Copy link
Member

mit-mit commented Mar 2, 2016

Clearing out 1.15 milestone as the last full push to dev has happened. If any changes are required before 1.15 is shipped, please file a merge request

@eernstg

This comment has been minimized.

Copy link
Member

eernstg commented Mar 3, 2016

Wow, I've got something that works now! \o//
Testing, double-checking, etc.

@eernstg

This comment has been minimized.

Copy link
Member

eernstg commented Mar 4, 2016

https://codereview.chromium.org/1761423002/ uses lazy and fine-grained constant resolution based on Resolver.computeLibraryElement, and allows reflectable to depend on 'analyzer' ^0.27.2 rather than the single version 0.27.1. When landed, that should resolve this issue.

@eernstg

This comment has been minimized.

Copy link
Member

eernstg commented Mar 7, 2016

@eernstg eernstg closed this Mar 7, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.