Ensure that a complete library element has constants evaluated (issue 24890)

Ensure that a complete library element has constants evaluated (issue 24890)

R=danrubel@google.com, eernst@google.com, paulberry@google.com, scheglov@google.com

Committed: https://github.com/dart-lang/sdk/commit/1f3dabd9c7ef391c468d2697294695194a67c569

[Brian Wilkerson, 2015-11-30 19:42:48]

brianwilkerson@google.com changed reviewers:
+ paulberry@google.com, scheglov@google.com

[Brian Wilkerson, 2015-11-30 19:42:48]

This is the second attempt at this fix. (Last time I forgot to run tests locally
the way the bots run them and ended up reverting because of time constraints.)

I'm particularly concerned by the added references to LIBRARY_ELEMENT8 in the
server code. If you have suggestions for avoiding this leakage of what ought to
be internal details, I'd love to hear them.

[scheglov, 2015-11-30 19:57:09]

On 2015/11/30 19:42:48, Brian Wilkerson wrote:
> This is the second attempt at this fix. (Last time I forgot to run tests
locally
> the way the bots run them and ended up reverting because of time constraints.)
> 
> I'm particularly concerned by the added references to LIBRARY_ELEMENT8 in the
> server code. If you have suggestions for avoiding this leakage of what ought
to
> be internal details, I'd love to hear them.

LGTM

[Paul Berry, 2015-11-30 20:44:35]

lgtm

https://codereview.chromium.org/1486663003/diff/1/pkg/analysis_server/lib/src...
File pkg/analysis_server/lib/src/analysis_server.dart (right):

https://codereview.chromium.org/1486663003/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/analysis_server.dart:1325: if
(context.getResult(librarySource, LIBRARY_ELEMENT8) == null) {
I'm also concerned about the leakage of LIBRARY_ELEMENT8 into analysis server. 
Why is this necessary?  At first blush it seems like we ought to be able to keep
doing getLibraryElement, but I'm assuming you have a good reason why that won't
work.

https://codereview.chromium.org/1486663003/diff/1/pkg/analyzer/lib/src/task/d...
File pkg/analyzer/lib/src/task/dart.dart (right):

https://codereview.chromium.org/1486663003/diff/1/pkg/analyzer/lib/src/task/d...
pkg/analyzer/lib/src/task/dart.dart:313: * The same as a [LIBRARY_ELEMENT7].
Next time I'm in the office I'd like to better understand why it's necessary to
distinguish between LIBRARY_ELEMENT7 and LIBRARY_ELEMENT8, given that the two
are "the same".  But I don't want to block this fix from landing.

[danrubel, 2015-11-30 20:56:10]

danrubel@google.com changed reviewers:
+ danrubel@google.com

[danrubel, 2015-11-30 20:56:10]

LGTM

[Brian Wilkerson, 2015-11-30 21:35:39]

https://codereview.chromium.org/1486663003/diff/1/pkg/analysis_server/lib/src...
File pkg/analysis_server/lib/src/analysis_server.dart (right):

https://codereview.chromium.org/1486663003/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/analysis_server.dart:1325: if
(context.getResult(librarySource, LIBRARY_ELEMENT8) == null) {
> I'm also concerned about the leakage of LIBRARY_ELEMENT8 into analysis server.

> Why is this necessary?  At first blush it seems like we ought to be able to
keep
> doing getLibraryElement, but I'm assuming you have a good reason why that
won't
> work.

As I'm sure you know, the method getLibraryElement does not do any work. If the
LIBRARY_ELEMENT has been computed, then it is returned. If not, then null is
returned.

The state that used to be known as LIBRARY_ELEMENT was reliably computed for
libraries in the SDK. That state has now been renamed to LIBRARY_ELEMENT_8 and
is still reliably computed for libraries in the SDK. The state that is now known
as LIBRARY_ELEMENT is *not* always computed. As a result, methods like this one
started getting null when they used to get a non-null value.

https://codereview.chromium.org/1486663003/diff/1/pkg/analyzer/lib/src/task/d...
File pkg/analyzer/lib/src/task/dart.dart (right):

https://codereview.chromium.org/1486663003/diff/1/pkg/analyzer/lib/src/task/d...
pkg/analyzer/lib/src/task/dart.dart:313: * The same as a [LIBRARY_ELEMENT7].
> Next time I'm in the office I'd like to better understand why it's necessary
to
> distinguish between LIBRARY_ELEMENT7 and LIBRARY_ELEMENT8, given that the two
> are "the same".  But I don't want to block this fix from landing.

Good question. I was assuming that there was a distinction between a
LIBRARY_ELEMENT7 and a LIBRARY_ELEMENT prior to my making this change, but the
difference wasn't documented and I forgot to go back to look to see what the
difference is in order to finish this comment.

[Brian Wilkerson, 2015-12-11 19:50:10]

PTAL. Not much has changed, but it has been updated some as a result of the
breaking changes branch.

[scheglov, 2015-12-11 19:52:39]

LGTM

[danrubel, 2015-12-11 20:05:56]

LGTM

[eernst, 2015-12-14 10:01:38]

eernst@google.com changed reviewers:
+ eernst@google.com

[eernst, 2015-12-14 10:01:39]

LGTM - just adding a drive by comment.

https://codereview.chromium.org/1486663003/diff/1/pkg/analysis_server/lib/src...
File pkg/analysis_server/lib/src/analysis_server.dart (right):

https://codereview.chromium.org/1486663003/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/analysis_server.dart:1325: if
(context.getResult(librarySource, LIBRARY_ELEMENT8) == null) {
On 2015/11/30 21:35:39, Brian Wilkerson wrote:
> > I'm also concerned about the leakage of LIBRARY_ELEMENT8 into analysis
server.
> 
> > Why is this necessary?  At first blush it seems like we ought to be able to
> keep
> > doing getLibraryElement, but I'm assuming you have a good reason why that
> won't
> > work.
> 
> As I'm sure you know, the method getLibraryElement does not do any work. If
the
> LIBRARY_ELEMENT has been computed, then it is returned. If not, then null is
> returned.
> 
> The state that used to be known as LIBRARY_ELEMENT was reliably computed for
> libraries in the SDK. That state has now been renamed to LIBRARY_ELEMENT_8 and
> is still reliably computed for libraries in the SDK. The state that is now
known
> as LIBRARY_ELEMENT is *not* always computed. As a result, methods like this
one
> started getting null when they used to get a non-null value.

DBC: If LIBRARY_ELEMENT8 and maybe other, similarly named entities are too
private for widespread usage in the analysis_server, and if usages have common
traits (`context.getResult(librarySource, LIBRARY_ELEMENT8)` is used more than
once, for instance) then it might be helpful to encapsulate those usages into a
couple of functions (which would then be called from the locations where
LIBRARY_ELEMENT8 is now used explicitly). This would at least centralize the
locations where changes will be needed if it is renamed again, or if it changes
semantics. But I do recognize that the usages are so diverse that these
functions would easily deteriorate into a single getter that returns
LIBRARY_ELEMENT8, which would only provide the meaningful name and no
abstraction.

[Brian Wilkerson, 2015-12-14 15:38:27]

https://codereview.chromium.org/1486663003/diff/1/pkg/analysis_server/lib/src...
File pkg/analysis_server/lib/src/analysis_server.dart (right):

https://codereview.chromium.org/1486663003/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/analysis_server.dart:1325: if
(context.getResult(librarySource, LIBRARY_ELEMENT8) == null) {
> ... it might be helpful to encapsulate those usages into a couple of functions
...

Absolutely. We had a discussion in the office about this and agreed that there
are probably a small number of the LIBRARY_ELEMENTn states that would be useful
to clients, and that we needed to (a) identify which states those are and (b)
design an API to make them available. But we also decided not to hold up fixing
this bug until we could finish that design work. I plan to address the need for
this API in a future CL.

[eernst, 2015-12-14 15:43:53]

On 2015/12/14 15:38:27, Brian Wilkerson wrote:
>
https://codereview.chromium.org/1486663003/diff/1/pkg/analysis_server/lib/src...
> File pkg/analysis_server/lib/src/analysis_server.dart (right):
> 
>
https://codereview.chromium.org/1486663003/diff/1/pkg/analysis_server/lib/src...
> pkg/analysis_server/lib/src/analysis_server.dart:1325: if
> (context.getResult(librarySource, LIBRARY_ELEMENT8) == null) {
> > ... it might be helpful to encapsulate those usages into a couple of
functions
> ...
> 
> Absolutely. We had a discussion in the office about this and agreed that there
> are probably a small number of the LIBRARY_ELEMENTn states that would be
useful
> to clients, and that we needed to (a) identify which states those are and (b)
> design an API to make them available. But we also decided not to hold up
fixing
> this bug until we could finish that design work. I plan to address the need
for
> this API in a future CL.

Very good, thanks! (adding another LGTM, just in case that is needed in order to
avoid disrupting the workflow).

[Brian Wilkerson, 2015-12-14 15:51:10]

Description was changed from

==========
Ensure that a complete library element has constants evaluated (issue 24890)
==========

to

==========
Ensure that a complete library element has constants evaluated (issue 24890)

R=danrubel@google.com, eernst@google.com, paulberry@google.com,
scheglov@google.com

Committed:
https://github.com/dart-lang/sdk/commit/1f3dabd9c7ef391c468d2697294695194a67c569
==========

[Brian Wilkerson, 2015-12-14 15:51:11]

Committed patchset #2 (id:20001) manually as
1f3dabd9c7ef391c468d2697294695194a67c569 (presubmit successful).

[eernst, 2015-12-16 17:33:44]

On 2015/12/14 15:51:11, Brian Wilkerson wrote:
> Committed patchset #2 (id:20001) manually as
> 1f3dabd9c7ef391c468d2697294695194a67c569 (presubmit successful).

I tried this:

  $ git clone 'https://github.com/dart-lang/sdk'
  $ cd sdk
  $ git co 1f3dabd9c7ef391c468d2697294695194a67c569
  $ srcdir=`pwd`

Then I used the output from `echo "path: $srcdir"` in a 
'dependency_overrides:' section in 'pubspec.yaml'.

This still produces a failure in almost all transformed-code tests, and they all
fail with a 'The null object does not have a method ..' (typically the method is
`classMirrorForInstance` or `classMirrorForType`).

Focusing on some individual test cases, we still have the `final _data = {};`,
which means that there is no reflectable data available (so all operations will
fail), and this is still caused by evaluation of constants in the transformer
which yields null (where analyzer 0.26.1+14 provided the expected, non-null
values).