Fix JS-backend when MirrorsUsed target a static field, but there is no mirrors usage.

Fix JS-backend when MirrorsUsed target a static field, but there is no mirrors usage.

r31964 (https://codereview.chromium.org/141753002) ensures that type-declarations are resolved, but that wasn't the case for static fields that weren't referenced except through "MirrorsUsed". Dart2Js didn't resolve or compile the static, but then tried to emit its getters/setters (which ended up calling the "_resolveTypeDeclaration" in members.dart).

With this patch we always enqueue static variables that are referenced by MirrorsUsed. If the user doesn't use mirrors it will increase the code-size (slightly) but that should be a rare case.

R=johnniwinther@google.com, kasperl@google.com

Committed: https://code.google.com/p/dart/source/detail?r=32159

[floitsch, 2014-01-29 23:32:13]

Forgot to take libraries and classes into account.
Uploaded new patch set.

https://codereview.chromium.org/142193005/diff/60001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/js_backend/backend.dart (right):

https://codereview.chromium.org/142193005/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:1759: List
staticFields = [];
I wouldn't be surprised if there were better ways to do this.
If there are, please enlighten me :)

[Johnni Winther, 2014-01-30 11:47:18]

You talk about static fields but only test for top-level fields.

https://codereview.chromium.org/142193005/diff/80001/tests/lib/mirrors/unused...
File tests/lib/mirrors/unused_mirrors5_test.dart (right):

https://codereview.chromium.org/142193005/diff/80001/tests/lib/mirrors/unused...
tests/lib/mirrors/unused_mirrors5_test.dart:1: // Copyright (c) 2013, the Dart
project authors.  Please see the AUTHORS file
This test looks identical to unused_mirrors2_test.

https://codereview.chromium.org/142193005/diff/80001/tests/lib/mirrors/unused...
File tests/lib/mirrors/unused_mirrors6_test.dart (right):

https://codereview.chromium.org/142193005/diff/80001/tests/lib/mirrors/unused...
tests/lib/mirrors/unused_mirrors6_test.dart:1: // Copyright (c) 2013, the Dart
project authors.  Please see the AUTHORS file
This test looks identical to unused_mirrors3_test.

https://codereview.chromium.org/142193005/diff/80001/tests/lib/mirrors/unused...
File tests/lib/mirrors/unused_mirrors8_test.dart (right):

https://codereview.chromium.org/142193005/diff/80001/tests/lib/mirrors/unused...
tests/lib/mirrors/unused_mirrors8_test.dart:1: // Copyright (c) 2013, the Dart
project authors.  Please see the AUTHORS file
This test looks identical to unused_mirrors2_test.

https://codereview.chromium.org/142193005/diff/80001/tests/lib/mirrors/unused...
File tests/lib/mirrors/unused_mirrors_test.dart (right):

https://codereview.chromium.org/142193005/diff/80001/tests/lib/mirrors/unused...
tests/lib/mirrors/unused_mirrors_test.dart:1: // Copyright (c) 2013, the Dart
project authors.  Please see the AUTHORS file
This test looks identical to unused_mirrors4_test.

[floitsch, 2014-01-30 12:27:22]

Static fields inside classes should work too.

https://codereview.chromium.org/142193005/diff/80001/tests/lib/mirrors/unused...
File tests/lib/mirrors/unused_mirrors5_test.dart (right):

https://codereview.chromium.org/142193005/diff/80001/tests/lib/mirrors/unused...
tests/lib/mirrors/unused_mirrors5_test.dart:1: // Copyright (c) 2013, the Dart
project authors.  Please see the AUTHORS file
On 2014/01/30 11:47:18, Johnni Winther wrote:
> This test looks identical to unused_mirrors2_test.

It has a getter instead of a top-level field.

https://codereview.chromium.org/142193005/diff/80001/tests/lib/mirrors/unused...
File tests/lib/mirrors/unused_mirrors6_test.dart (right):

https://codereview.chromium.org/142193005/diff/80001/tests/lib/mirrors/unused...
tests/lib/mirrors/unused_mirrors6_test.dart:1: // Copyright (c) 2013, the Dart
project authors.  Please see the AUTHORS file
On 2014/01/30 11:47:18, Johnni Winther wrote:
> This test looks identical to unused_mirrors3_test.

Getter instead of field.

https://codereview.chromium.org/142193005/diff/80001/tests/lib/mirrors/unused...
File tests/lib/mirrors/unused_mirrors8_test.dart (right):

https://codereview.chromium.org/142193005/diff/80001/tests/lib/mirrors/unused...
tests/lib/mirrors/unused_mirrors8_test.dart:1: // Copyright (c) 2013, the Dart
project authors.  Please see the AUTHORS file
On 2014/01/30 11:47:18, Johnni Winther wrote:
> This test looks identical to unused_mirrors2_test.

The static foo is inside a class, but the MirrorsUsed goes to the library.

https://codereview.chromium.org/142193005/diff/80001/tests/lib/mirrors/unused...
File tests/lib/mirrors/unused_mirrors_test.dart (right):

https://codereview.chromium.org/142193005/diff/80001/tests/lib/mirrors/unused...
tests/lib/mirrors/unused_mirrors_test.dart:1: // Copyright (c) 2013, the Dart
project authors.  Please see the AUTHORS file
On 2014/01/30 11:47:18, Johnni Winther wrote:
> This test looks identical to unused_mirrors4_test.
4-test uses a getter.

[kasperl, 2014-01-30 13:37:14]

https://codereview.chromium.org/142193005/diff/120001/sdk/lib/_internal/compi...
File sdk/lib/_internal/compiler/implementation/js_backend/backend.dart (right):

https://codereview.chromium.org/142193005/diff/120001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:1759: List
staticFields = [];
If you only need them to be resolved, maybe you should not do this if the
enqueuer is the compilation queue? Maybe move the collects the static fields
somewhere else (just add a helper method).

https://codereview.chromium.org/142193005/diff/120001/sdk/lib/_internal/compi...
File sdk/lib/_internal/compiler/implementation/js_emitter/class_emitter.dart
(right):

https://codereview.chromium.org/142193005/diff/120001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/js_emitter/class_emitter.dart:100: if
(element.isClass()) {
The code looks pretty weird now.

[floitsch, 2014-01-30 14:10:23]

PTAL.

https://codereview.chromium.org/142193005/diff/120001/sdk/lib/_internal/compi...
File sdk/lib/_internal/compiler/implementation/js_backend/backend.dart (right):

https://codereview.chromium.org/142193005/diff/120001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:1759: List
staticFields = [];
On 2014/01/30 13:37:15, kasperl wrote:
> If you only need them to be resolved, maybe you should not do this if the
> enqueuer is the compilation queue? Maybe move the collects the static fields
> somewhere else (just add a helper method).

Done.

https://codereview.chromium.org/142193005/diff/120001/sdk/lib/_internal/compi...
File sdk/lib/_internal/compiler/implementation/js_emitter/class_emitter.dart
(right):

https://codereview.chromium.org/142193005/diff/120001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/js_emitter/class_emitter.dart:100: if
(element.isClass()) {
On 2014/01/30 13:37:15, kasperl wrote:
> The code looks pretty weird now.

Done.

[kasperl, 2014-01-30 14:14:47]

LGTM.

https://codereview.chromium.org/142193005/diff/140001/sdk/lib/_internal/compi...
File sdk/lib/_internal/compiler/implementation/js_backend/backend.dart (right):

https://codereview.chromium.org/142193005/diff/140001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:1755: } else
if (!targetsUsed.isEmpty && enqueuer.isResolutionQueue) {
I'd still prefer a helper method that finds all the static elements.

   ...
   List staticFields = findStaticFieldTargets(targetsUsed);
   enqueuer.enqueueReflectiveStaticField(staticFields);
   ...

[Johnni Winther, 2014-01-30 14:34:18]

lgtm

[floitsch, 2014-01-30 15:03:45]

https://codereview.chromium.org/142193005/diff/140001/sdk/lib/_internal/compi...
File sdk/lib/_internal/compiler/implementation/js_backend/backend.dart (right):

https://codereview.chromium.org/142193005/diff/140001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:1755: } else
if (!targetsUsed.isEmpty && enqueuer.isResolutionQueue) {
On 2014/01/30 14:14:48, kasperl wrote:
> I'd still prefer a helper method that finds all the static elements.
> 
>    ...
>    List staticFields = findStaticFieldTargets(targetsUsed);
>    enqueuer.enqueueReflectiveStaticField(staticFields);
>    ...
> 

done.
Also refactored so it recursively traverses classes when the initial target was
a library. There was already a test for this (and apparently it wasn't
necessary), but it shouldn't cost much and seems cleaner.

[floitsch, 2014-01-30 15:04:40]

Committed patchset #9 manually as r32159 (presubmit successful).

[floitsch, 2014-03-25 10:12:34]

LGTM.

minimal example:
===
@MirrorsUsed(targets: const ["foo"])
import 'dart:mirrors';

final foo = reflect(reflect(9)).getField(#getField);

void main() {}
===

[floitsch, 2014-03-25 10:12:51]

On 2014/03/25 10:12:34, floitsch wrote:
> LGTM.
> 
> minimal example:
> ===
> @MirrorsUsed(targets: const ["foo"])
> import 'dart:mirrors';
> 
> final foo = reflect(reflect(9)).getField(#getField);
> 
> void main() {}
> ===

never mind. wrong CL.