Emit warning on import of dart:mirrors.

Emit warning on import of dart:mirrors.

BUG=
R=floitsch@google.com

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

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

[Johnni Winther, 2014-09-22 13:02:48]

johnniwinther@google.com changed reviewers:
+ floitsch@google.com

[floitsch, 2014-09-22 20:52:59]

LGTM!

I really like the output.
@Seth: we now need a URI to link to.

https://codereview.chromium.org/588183002/diff/20001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/compiler.dart (right):

https://codereview.chromium.org/588183002/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/compiler.dart:1200: return new
Future.value();
return null is the same, since we are already in a new Future.sync.

https://codereview.chromium.org/588183002/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/compiler.dart:1206:
loadedLibraries.forEachImportChain(
I would make this a function on LoadedLibraries:

loadedLibraries.importChainsForUri(DART_MIRRORS, verbose: verbose);

https://codereview.chromium.org/588183002/diff/20001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/library_loader.dart (right):

https://codereview.chromium.org/588183002/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/library_loader.dart:1083: void
processImports(LibraryElement library,
If the method is not in the public API I would make it private.

[Johnni Winther, 2014-11-12 13:06:26]

https://codereview.chromium.org/588183002/diff/20001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/compiler.dart (right):

https://codereview.chromium.org/588183002/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/compiler.dart:1200: return new
Future.value();
On 2014/09/22 20:52:59, floitsch wrote:
> return null is the same, since we are already in a new Future.sync.

Done.

https://codereview.chromium.org/588183002/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/compiler.dart:1206:
loadedLibraries.forEachImportChain(
On 2014/09/22 20:52:59, floitsch wrote:
> I would make this a function on LoadedLibraries:
> 
> loadedLibraries.importChainsForUri(DART_MIRRORS, verbose: verbose);

Added a TODO. We need to store the libraries for incremental compilation anyway.

https://codereview.chromium.org/588183002/diff/20001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/library_loader.dart (right):

https://codereview.chromium.org/588183002/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/library_loader.dart:1083: void
processImports(LibraryElement library,
On 2014/09/22 20:52:59, floitsch wrote:
> If the method is not in the public API I would make it private.

Done.

[Johnni Winther, 2014-11-12 13:21:15]

Committed patchset #6 (id:100001) manually as 41688 (presubmit successful).

[Johnni Winther, 2014-11-13 10:41:16]

Patchset #7 (id:120001) has been deleted

[Johnni Winther, 2014-11-18 14:14:36]

Patchset #8 (id:160001) has been deleted

[Johnni Winther, 2014-11-18 14:15:10]

PTAL @ the latest patch

[floitsch, 2014-11-18 15:50:01]

LGTM.

https://codereview.chromium.org/588183002/diff/180001/pkg/compiler/lib/src/co...
File pkg/compiler/lib/src/compiler.dart (right):

https://codereview.chromium.org/588183002/diff/180001/pkg/compiler/lib/src/co...
pkg/compiler/lib/src/compiler.dart:659: final bool useContentSecurityPolicy;
Move `final bool enableExperimentalMirrors;` to here.

https://codereview.chromium.org/588183002/diff/180001/pkg/compiler/lib/src/co...
pkg/compiler/lib/src/compiler.dart:1246: callback:(Link<Uri>
importChainReversed) {
space after ':'.

https://codereview.chromium.org/588183002/diff/180001/pkg/compiler/lib/src/co...
pkg/compiler/lib/src/compiler.dart:1264: return
codeLocation.relativize(rootUri);
This indentation just doesn't work for me... (even though it's style-guide).

String importChain =
    compactImportChain.map((CodeLocation codeLocation) {
      return codeLocation.relativize(rootUri);
    }).join(' => ');

https://codereview.chromium.org/588183002/diff/180001/pkg/compiler/lib/src/li...
File pkg/compiler/lib/src/library_loader.dart (right):

https://codereview.chromium.org/588183002/diff/180001/pkg/compiler/lib/src/li...
pkg/compiler/lib/src/library_loader.dart:1081: void forEachImportChain(Uri uri,
uri -> targetUri ?

https://codereview.chromium.org/588183002/diff/180001/pkg/compiler/lib/src/li...
pkg/compiler/lib/src/library_loader.dart:1089: /// Compute the set of
(unreversed) paths to [uri].
Computes

https://codereview.chromium.org/588183002/diff/180001/pkg/compiler/lib/src/li...
pkg/compiler/lib/src/library_loader.dart:1090: void
computeSuffixes(LibraryElement library,
More comments. Eg:

Finds all paths (suffixes) from the current library to [uri] and stores it in
[suffixChainMap].

For every found suffix it prepends the given [prefix] (+ the current library)
and invokes the [callback] with the concatenated chain.

https://codereview.chromium.org/588183002/diff/180001/pkg/compiler/lib/src/li...
pkg/compiler/lib/src/library_loader.dart:1099: if (uri == canonicalUri) {
for some reason I prefer to put the "changing" thing first.

canonicalUri == uri

https://codereview.chromium.org/588183002/diff/180001/pkg/compiler/lib/src/li...
pkg/compiler/lib/src/library_loader.dart:1108: if
(!suffixChainMap.containsKey(import.importedLibrary)) {
The 'if' checks if 'import.importedLibrary' is alread in the suffixChainMap and
then only computes the suffixes.

But then, just after the `if` we have exactly the same code...

https://codereview.chromium.org/588183002/diff/180001/pkg/compiler/lib/src/wa...
File pkg/compiler/lib/src/warnings.dart (right):

https://codereview.chromium.org/588183002/diff/180001/pkg/compiler/lib/src/wa...
pkg/compiler/lib/src/warnings.dart:2244: * Starting with Dart 1.8, you must use
the
1.9

[Johnni Winther, 2014-11-19 08:52:20]

https://codereview.chromium.org/588183002/diff/180001/pkg/compiler/lib/src/co...
File pkg/compiler/lib/src/compiler.dart (right):

https://codereview.chromium.org/588183002/diff/180001/pkg/compiler/lib/src/co...
pkg/compiler/lib/src/compiler.dart:659: final bool useContentSecurityPolicy;
On 2014/11/18 15:50:01, floitsch wrote:
> Move `final bool enableExperimentalMirrors;` to here.

Done.

https://codereview.chromium.org/588183002/diff/180001/pkg/compiler/lib/src/co...
pkg/compiler/lib/src/compiler.dart:1246: callback:(Link<Uri>
importChainReversed) {
On 2014/11/18 15:50:01, floitsch wrote:
> space after ':'.

Done.

https://codereview.chromium.org/588183002/diff/180001/pkg/compiler/lib/src/co...
pkg/compiler/lib/src/compiler.dart:1264: return
codeLocation.relativize(rootUri);
On 2014/11/18 15:50:01, floitsch wrote:
> This indentation just doesn't work for me... (even though it's style-guide).
> 
> String importChain =
>     compactImportChain.map((CodeLocation codeLocation) {
>       return codeLocation.relativize(rootUri);
>     }).join(' => ');

Done. Didn't like it either.

https://codereview.chromium.org/588183002/diff/180001/pkg/compiler/lib/src/li...
File pkg/compiler/lib/src/library_loader.dart (right):

https://codereview.chromium.org/588183002/diff/180001/pkg/compiler/lib/src/li...
pkg/compiler/lib/src/library_loader.dart:1081: void forEachImportChain(Uri uri,
On 2014/11/18 15:50:01, floitsch wrote:
> uri -> targetUri ?

Done.

https://codereview.chromium.org/588183002/diff/180001/pkg/compiler/lib/src/li...
pkg/compiler/lib/src/library_loader.dart:1089: /// Compute the set of
(unreversed) paths to [uri].
On 2014/11/18 15:50:01, floitsch wrote:
> Computes

Done.

https://codereview.chromium.org/588183002/diff/180001/pkg/compiler/lib/src/li...
pkg/compiler/lib/src/library_loader.dart:1090: void
computeSuffixes(LibraryElement library,
On 2014/11/18 15:50:01, floitsch wrote:
> More comments. Eg:
> 
> Finds all paths (suffixes) from the current library to [uri] and stores it in
> [suffixChainMap].
> 
> For every found suffix it prepends the given [prefix] (+ the current library)
> and invokes the [callback] with the concatenated chain.

Done.

https://codereview.chromium.org/588183002/diff/180001/pkg/compiler/lib/src/li...
pkg/compiler/lib/src/library_loader.dart:1099: if (uri == canonicalUri) {
On 2014/11/18 15:50:01, floitsch wrote:
> for some reason I prefer to put the "changing" thing first.
> 
> canonicalUri == uri

Done.

https://codereview.chromium.org/588183002/diff/180001/pkg/compiler/lib/src/li...
pkg/compiler/lib/src/library_loader.dart:1108: if
(!suffixChainMap.containsKey(import.importedLibrary)) {
On 2014/11/18 15:50:01, floitsch wrote:
> The 'if' checks if 'import.importedLibrary' is alread in the suffixChainMap
and
> then only computes the suffixes.
> 
> But then, just after the `if` we have exactly the same code...

[callback] should only have been call in this loop if the (sub)suffixes had been
precomputed.

Updated and comment added.

https://codereview.chromium.org/588183002/diff/180001/pkg/compiler/lib/src/wa...
File pkg/compiler/lib/src/warnings.dart (right):

https://codereview.chromium.org/588183002/diff/180001/pkg/compiler/lib/src/wa...
pkg/compiler/lib/src/warnings.dart:2244: * Starting with Dart 1.8, you must use
the
On 2014/11/18 15:50:01, floitsch wrote:
> 1.9

I will not cherry-pick this CL them.

[Johnni Winther, 2014-11-19 09:01:05]

Patchset #9 (id:200001) has been deleted

[Johnni Winther, 2014-11-19 09:21:07]

Committed patchset #9 (id:220001) manually as 41822 (presubmit successful).

[rmacnak, 2015-01-14 18:30:28]

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

[rmacnak, 2015-01-14 18:30:29]

https://codereview.chromium.org/588183002/diff/220001/pkg/compiler/lib/src/ap...
File pkg/compiler/lib/src/apiimpl.dart (right):

https://codereview.chromium.org/588183002/diff/220001/pkg/compiler/lib/src/ap...
pkg/compiler/lib/src/apiimpl.dart:83: enableExperimentalMirrors:
Uhh, what do you intend to put behind this flag?

[floitsch, 2015-01-15 10:43:52]

https://codereview.chromium.org/588183002/diff/220001/pkg/compiler/lib/src/ap...
File pkg/compiler/lib/src/apiimpl.dart (right):

https://codereview.chromium.org/588183002/diff/220001/pkg/compiler/lib/src/ap...
pkg/compiler/lib/src/apiimpl.dart:83: enableExperimentalMirrors:
On 2015/01/14 18:30:29, rmacnak wrote:
> Uhh, what do you intend to put behind this flag?

The implementation of dart:mirrors in dart2js is gated by this flag.

[rmacnak, 2015-01-15 21:48:53]

https://codereview.chromium.org/588183002/diff/220001/pkg/compiler/lib/src/ap...
File pkg/compiler/lib/src/apiimpl.dart (right):

https://codereview.chromium.org/588183002/diff/220001/pkg/compiler/lib/src/ap...
pkg/compiler/lib/src/apiimpl.dart:83: enableExperimentalMirrors:
On 2015/01/15 10:43:52, floitsch wrote:
> On 2015/01/14 18:30:29, rmacnak wrote:
> > Uhh, what do you intend to put behind this flag?
> 
> The implementation of dart:mirrors in dart2js is gated by this flag.

The existing implementation? That's quite a significant breaking change.

[floitsch, 2015-01-15 22:35:15]

https://codereview.chromium.org/588183002/diff/220001/pkg/compiler/lib/src/ap...
File pkg/compiler/lib/src/apiimpl.dart (right):

https://codereview.chromium.org/588183002/diff/220001/pkg/compiler/lib/src/ap...
pkg/compiler/lib/src/apiimpl.dart:83: enableExperimentalMirrors:
On 2015/01/15 21:48:52, rmacnak wrote:
> On 2015/01/15 10:43:52, floitsch wrote:
> > On 2015/01/14 18:30:29, rmacnak wrote:
> > > Uhh, what do you intend to put behind this flag?
> > 
> > The implementation of dart:mirrors in dart2js is gated by this flag.
> 
> The existing implementation? That's quite a significant breaking change.

If adding a flag to a compilation process is a significant breaking change, then
yes.
Fwiw it's not going to happen in 1.9.

[sethladd, 2015-01-15 23:24:16]

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

[sethladd, 2015-01-15 23:24:17]

After lots of discussion, we'd like to propose:

* Do not include an opt-in flag now. Revisit the flag once alternative solutions
are in place.

* Ensure the link displayed in the warning output is actionable.

* Investigate how the warning is displayed in our tools (IDEs, pub build, etc).
Does the warning zoom by and is easily missed? Is it printed at the end of the
process?