Convert dart_utils.js to input_sdk/lib/_internal/utils.dart (#310)

Convert dart_utils.js to input_sdk/lib/_internal/utils.dart (#310)

BUG=
R=jmesserly@google.com, vsm@google.com

Committed: https://github.com/dart-lang/dev_compiler/commit/d91add0d8490bbb59f72fe676fe70cfeedcd1e91

[ochafik, 2015-11-29 14:38:10]

ochafik@google.com changed reviewers:
+ jmesserly@google.com, vsm@google.com

[ochafik, 2015-11-29 14:38:10]

Notes:
- dart_utils is now just another module (its imports are special-cased to not
import dart/_runtime)
- Renamed dart.export_ and dart_utils.assert_: ideally we'd need @js.JS
annotations in utils.dart to keep their current name (assert being a Dart
keyword and export a JS one), but importing 'package:js/js.dart' from utils.dart
just hangs the analyzer big time (any idea as to why?).

[Jennifer Messerly, 2015-11-30 16:49:47]

On 2015/11/29 14:38:10, ochafik wrote:
> Notes:
> - dart_utils is now just another module (its imports are special-cased to not
> import dart/_runtime)
> - Renamed dart.export_ and dart_utils.assert_: ideally we'd need @js.JS
> annotations in utils.dart to keep their current name (assert being a Dart
> keyword and export a JS one), but importing 'package:js/js.dart' from
utils.dart
> just hangs the analyzer big time (any idea as to why?).

Well I'm not sure about "hangs", but it's not possible to import a "package:"
URI from a "dart:" URI, so that could be the reason.

What we likely need is to define our own "@JS" annotation somewhere in corelibs
(tool/input_sdk/private/annotations.dart maybe) that we can reference. It will
be in a private library so the SDK will be able to use it, but nobody else will.
That breaks what would otherwise be an import cycle (dart:core -> runtime ->
package:js -> dart:core)

[vsm, 2015-11-30 16:50:06]

lgtm

https://codereview.chromium.org/1486473002/diff/60001/lib/runtime/dart/_inter...
File lib/runtime/dart/_internal.js (right):

https://codereview.chromium.org/1486473002/diff/60001/lib/runtime/dart/_inter...
lib/runtime/dart/_internal.js:623: return new (ExpandIterator$(S,
T))(this[_iterable][dartx.iterator], dart.as(this[_f], __CastType0$(S, T)));
Do you know what change triggered this?  The new version looks better, but we
probably ought to fix the Dart code to eliminate this cast.

[Jennifer Messerly, 2015-11-30 17:26:20]

On 2015/11/30 16:50:06, vsm wrote:
> lgtm
> 
>
https://codereview.chromium.org/1486473002/diff/60001/lib/runtime/dart/_inter...
> File lib/runtime/dart/_internal.js (right):
> 
>
https://codereview.chromium.org/1486473002/diff/60001/lib/runtime/dart/_inter...
> lib/runtime/dart/_internal.js:623: return new (ExpandIterator$(S,
> T))(this[_iterable][dartx.iterator], dart.as(this[_f], __CastType0$(S, T)));
> Do you know what change triggered this?  The new version looks better, but we
> probably ought to fix the Dart code to eliminate this cast.

FYI I have a few comments on this one. They're minor though.

[Jennifer Messerly, 2015-11-30 18:54:11]

A few comments but LGTM overall.

https://codereview.chromium.org/1486473002/diff/60001/lib/runtime/dart/_inter...
File lib/runtime/dart/_internal.js (right):

https://codereview.chromium.org/1486473002/diff/60001/lib/runtime/dart/_inter...
lib/runtime/dart/_internal.js:623: return new (ExpandIterator$(S,
T))(this[_iterable][dartx.iterator], dart.as(this[_f], __CastType0$(S, T)));
On 2015/11/30 16:50:06, vsm wrote:
> Do you know what change triggered this?  The new version looks better, but we
> probably ought to fix the Dart code to eliminate this cast.

Just a guess: I changed Analyzer to use FunctionElement instead of fake typedefs
for function typed parameters, and fixed its enclosingElement to correctly track
type parameters. It looks like the result is they're tracking generic
substitutions better. It wasn't my intent but it looks like maybe it fixed a
soundness issue.

https://codereview.chromium.org/1486473002/diff/60001/lib/src/codegen/js_code...
File lib/src/codegen/js_codegen.dart (right):

https://codereview.chromium.org/1486473002/diff/60001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:122: _isDartUtils = currentLibrary.isInSdk &&
you shouldn't need the isInSdk check, it's just checking if the library name
starting with "dart." ... IMO, checking the URI should be sufficient.

https://codereview.chromium.org/1486473002/diff/60001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:211: js.statement("let # = #.dartx;",
[_dartxVar, _runtimeLibVar]);
const?

https://codereview.chromium.org/1486473002/diff/60001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:1302: if (currentLibrary.isInSdk &&
I try and avoid LibraryElement.isInSdk, because any library whose name starts
with "dart." can pretend to be part of the SDK.

(Aside, we should try and get this fixed in Analyzer... it's a bit of a trap
right now)

So far we've been checking if the source.uri.scheme == 'dart', but going
forward, source.isInSystemLibrary might be better. It's equivalent but reads
nicer IMO.

https://codereview.chromium.org/1486473002/diff/60001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:1324: JS.Fun _simplifyFun(JS.Fun fn) {
love this!

Regarding:
    JS('', '(function() { let x = #; ... })()', x);

it might be nicer to write this as:

    JS('', '((x) => { ... })(#)', x);

In other words, use arrow functions & parameters.

Not a big deal though, as they should be equivalent. Don't worry about it unless
you're feeling inspired :)

https://codereview.chromium.org/1486473002/diff/60001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:1340: bool _isInlineJSBody(FunctionBody body) {
It might be nice to combine this with the simplification step, so the two
operations are done "atomically":

JS.Fun _emitSdkJsFunction(body);

And then it does checks & simplification internally. That might make the
pre/post conditions more clear and guarantee we only simplify if _isInlineJSBody
is true.

https://codereview.chromium.org/1486473002/diff/60001/tool/build_sdk.sh
File tool/build_sdk.sh (right):

https://codereview.chromium.org/1486473002/diff/60001/tool/build_sdk.sh#newco...
tool/build_sdk.sh:14: dart:_utils \
hmmm, one idea here is: if this is imported by other libraries, in which case it
should be picked up automatically. Is that working for you?

[Jennifer Messerly, 2015-11-30 19:00:03]

oops forgot to check one file. Few more comments :)

https://codereview.chromium.org/1486473002/diff/60001/tool/input_sdk/private/...
File tool/input_sdk/private/utils.dart (right):

https://codereview.chromium.org/1486473002/diff/60001/tool/input_sdk/private/...
tool/input_sdk/private/utils.dart:19: // TODO(ochafik): Add ES6 class syntax
support to JS intrinsics to avoid this.
ah, yeah, good idea :)

https://codereview.chromium.org/1486473002/diff/60001/tool/input_sdk/private/...
tool/input_sdk/private/utils.dart:30: void throwStrongModeError(message) =>
JS('', '''(function() {
should we explicitly pass in "message"? The current usage will work in our
compiler, but the reason dart2js uses "#" is so they can rename things when they
minify. Maybe it will never be an issue for us... I'm not sure.

https://codereview.chromium.org/1486473002/diff/60001/tool/input_sdk/private/...
tool/input_sdk/private/utils.dart:48: let desc = getOwnPropertyDescriptor(obj,
name);
I wonder if some of these could be written in Dart? Or is the idea to just start
off with a mostly automatic transform?

[ochafik, 2015-12-01 14:40:01]

On 2015/11/30 16:49:47, John Messerly wrote:
> On 2015/11/29 14:38:10, ochafik wrote:
> > Notes:
> > - dart_utils is now just another module (its imports are special-cased to
not
> > import dart/_runtime)
> > - Renamed dart.export_ and dart_utils.assert_: ideally we'd need @js.JS
> > annotations in utils.dart to keep their current name (assert being a Dart
> > keyword and export a JS one), but importing 'package:js/js.dart' from
> utils.dart
> > just hangs the analyzer big time (any idea as to why?).
> 
> Well I'm not sure about "hangs", but it's not possible to import a "package:"
> URI from a "dart:" URI, so that could be the reason.
> 
> What we likely need is to define our own "@JS" annotation somewhere in
corelibs
> (tool/input_sdk/private/annotations.dart maybe) that we can reference. It will
> be in a private library so the SDK will be able to use it, but nobody else
will.
> That breaks what would otherwise be an import cycle (dart:core -> runtime ->
> package:js -> dart:core)

Mmh, yeah we need... something like @JsName :-D
https://github.com/dart-lang/dev_compiler/commit/72890d156518eaf603b7a94abdf1...

If that's okay with you, leaving TODOs for this as a follow-up, as using the
heavily-overloaded name JS seems tricky (can't be in either of _js_helper or
_foreign_helper without much unpleasant aliasing), probably deserves a good chat
/ review of its own.

[ochafik, 2015-12-01 14:41:03]

Thanks guys!

https://codereview.chromium.org/1486473002/diff/60001/lib/src/codegen/js_code...
File lib/src/codegen/js_codegen.dart (right):

https://codereview.chromium.org/1486473002/diff/60001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:1302: if (currentLibrary.isInSdk &&
On 2015/11/30 18:54:11, John Messerly wrote:
> I try and avoid LibraryElement.isInSdk, because any library whose name starts
> with "dart." can pretend to be part of the SDK.
> 
> (Aside, we should try and get this fixed in Analyzer... it's a bit of a trap
> right now)
> 
> So far we've been checking if the source.uri.scheme == 'dart', but going
> forward, source.isInSystemLibrary might be better. It's equivalent but reads
> nicer IMO.

Done.

https://codereview.chromium.org/1486473002/diff/60001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:1324: JS.Fun _simplifyFun(JS.Fun fn) {
On 2015/11/30 18:54:11, John Messerly wrote:
> love this!
> 
> Regarding:
>     JS('', '(function() { let x = #; ... })()', x);
> 
> it might be nicer to write this as:
> 
>     JS('', '((x) => { ... })(#)', x);
> 
> In other words, use arrow functions & parameters.
> 
> Not a big deal though, as they should be equivalent. Don't worry about it
unless
> you're feeling inspired :)

I was equally bothered by `let x = x` and the silent pass-through, but your
suggestion is much cleaner, thanks, Done!

https://codereview.chromium.org/1486473002/diff/60001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:1340: bool _isInlineJSBody(FunctionBody body) {
On 2015/11/30 18:54:11, John Messerly wrote:
> It might be nice to combine this with the simplification step, so the two
> operations are done "atomically":
> 
> JS.Fun _emitSdkJsFunction(body);
> 
> And then it does checks & simplification internally. That might make the
> pre/post conditions more clear and guarantee we only simplify if
_isInlineJSBody
> is true.

I can't find a very clean way to combine them (have to do the checks outside the
call, or do it inside and return null, etc).

I've switched the pattern to only using arrow functions, which means the rewrite
should always be correct (I think? even for `this` bindings): I don't think that
method (renamed to _simplifyPassThroughArrowFunCallBody) needs a precondition
anymore.

https://codereview.chromium.org/1486473002/diff/60001/tool/build_sdk.sh
File tool/build_sdk.sh (right):

https://codereview.chromium.org/1486473002/diff/60001/tool/build_sdk.sh#newco...
tool/build_sdk.sh:14: dart:_utils \
On 2015/11/30 18:54:11, John Messerly wrote:
> hmmm, one idea here is: if this is imported by other libraries, in which case
it
> should be picked up automatically. Is that working for you?

Ha! Added a phony import in js_primitives.dart + todo.

https://codereview.chromium.org/1486473002/diff/60001/tool/input_sdk/private/...
File tool/input_sdk/private/utils.dart (right):

https://codereview.chromium.org/1486473002/diff/60001/tool/input_sdk/private/...
tool/input_sdk/private/utils.dart:30: void throwStrongModeError(message) =>
JS('', '''(function() {
On 2015/11/30 19:00:03, John Messerly wrote:
> should we explicitly pass in "message"? The current usage will work in our
> compiler, but the reason dart2js uses "#" is so they can rename things when
they
> minify. Maybe it will never be an issue for us... I'm not sure.

Now passing args with the `foo(x) => JS('', '((x) => ...)(#)', x)` pattern you
suggested :-)

https://codereview.chromium.org/1486473002/diff/60001/tool/input_sdk/private/...
tool/input_sdk/private/utils.dart:48: let desc = getOwnPropertyDescriptor(obj,
name);
On 2015/11/30 19:00:03, John Messerly wrote:
> I wonder if some of these could be written in Dart? Or is the idea to just
start
> off with a mostly automatic transform?
> 

Yes definitely, I would expect more "idiomatic" refactorings once we have
something that works. Left a TODO at the top of the file.

[Jennifer Messerly, 2015-12-01 15:56:37]

LGTM++ !

one further suggestion. Could definitely be addressed in a follow up though.

https://codereview.chromium.org/1486473002/diff/80001/tool/sdk_expected_error...
File tool/sdk_expected_errors.txt (right):

https://codereview.chromium.org/1486473002/diff/80001/tool/sdk_expected_error...
tool/sdk_expected_errors.txt:4: severe: [AnalyzerMessage] 'const' variables must
be constant value (dart:_utils, line 13, col 24)
Declare these variables as "final"?

If that generates weird code, maybe have js_codegen special case the following
pattern:

final foo = JS(...);

[ochafik, 2015-12-01 18:48:25]

Thanks John!

https://codereview.chromium.org/1486473002/diff/80001/tool/sdk_expected_error...
File tool/sdk_expected_errors.txt (right):

https://codereview.chromium.org/1486473002/diff/80001/tool/sdk_expected_error...
tool/sdk_expected_errors.txt:4: severe: [AnalyzerMessage] 'const' variables must
be constant value (dart:_utils, line 13, col 24)
On 2015/12/01 15:56:37, John Messerly wrote:
> Declare these variables as "final"?
> 
> If that generates weird code, maybe have js_codegen special case the following
> pattern:
> 
> final foo = JS(...);

Good ideal, preparing a follow-up now as the special-casing impacts some other
files (js_mirrors, isolate_helpers, js)

[ochafik, 2015-12-01 19:02:46]

Description was changed from

==========
Convert dart_utils.js to input_sdk/lib/_internal/utils.dart (#310)

BUG=
==========

to

==========
Convert dart_utils.js to input_sdk/lib/_internal/utils.dart (#310)

BUG=
R=jmesserly@google.com, vsm@google.com

Committed:
https://github.com/dart-lang/dev_compiler/commit/d91add0d8490bbb59f72fe676fe7...
==========

[ochafik, 2015-12-01 19:02:47]

Committed patchset #7 (id:120001) manually as
d91add0d8490bbb59f72fe676fe70cfeedcd1e91 (presubmit successful).