Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(718)

Issue 1522773002: Use string interpolation in JS intrinsics to simplify large templates. (Closed)

Created:
5 years ago by ochafik
Modified:
5 years ago
Reviewers:
vsm, Jennifer Messerly, sra1
CC:
dev-compiler+reviews_dartlang.org
Base URL:
git@github.com:dart-lang/dev_compiler.git@master
Target Ref:
refs/heads/master
Visibility:
Public.

Description

Use string interpolation in JS intrinsics to simplify large templates. This helps define large runtime methods that refer to lots of helpers. The following pattern: f(a, b, c) => JS('', '''((a, b, c, someHelper) => { return someHelper(a + b + c); })(#, #, #)''', a, b, c, someHelper); Becomes: f(a, b, c) => JS('', ''(() => { return $someHelper($a + $b + $c); })()'''); BUG= R=vsm@google.com Committed: https://github.com/dart-lang/dev_compiler/commit/13e0c5c1ca587800eb6016ab0fd1ab135423bc52

Patch Set 1 #

Patch Set 2 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+70 lines, -51 lines) Patch
M lib/src/codegen/js_codegen.dart View 1 1 chunk +23 lines, -3 lines 0 comments Download
M tool/input_sdk/private/utils.dart View 3 chunks +47 lines, -48 lines 0 comments Download

Messages

Total messages: 12 (4 generated)
ochafik
5 years ago (2015-12-13 19:08:36 UTC) #3
vsm
nice! lgtm.
5 years ago (2015-12-14 01:06:37 UTC) #4
ochafik
Committed patchset #2 (id:20001) manually as 13e0c5c1ca587800eb6016ab0fd1ab135423bc52 (presubmit successful).
5 years ago (2015-12-14 01:14:15 UTC) #6
Jennifer Messerly
Hi guys, I'm really confused by this change. It looks like string interpolation, but it's ...
5 years ago (2015-12-15 22:08:38 UTC) #8
sra1
On 2015/12/15 22:08:38, John Messerly wrote: > Hi guys, > > I'm really confused by ...
5 years ago (2015-12-15 22:34:47 UTC) #9
ochafik
Thanks for the comments! On 2015/12/15 22:34:47, sra1 wrote: > On 2015/12/15 22:08:38, John Messerly ...
5 years ago (2015-12-16 01:46:22 UTC) #10
sra1
On 2015/12/16 01:46:22, ochafik wrote: > Thanks for the comments! > > On 2015/12/15 22:34:47, ...
5 years ago (2015-12-16 02:34:45 UTC) #11
ochafik
5 years ago (2015-12-16 13:51:21 UTC) #12
Message was sent while issue was closed.
On 2015/12/16 02:34:45, sra1 wrote:
> Your CL does two independent things:
> 
> 1. Make JS(..., '... $a ...') be syntax for  JS(..., '... # ...', a)

Yes (and that's all it does, see below)

> 2. rewrite ((p)=>{... p ...})(e)  (()=>{... e ...})()

Nope, sorry if the bogus issue description does this shortcut: this was done by
another CL (only for functions which body is a JS intrinsic), see discussion
with John in
https://codereview.chromium.org/1486473002/diff/60001/lib/src/codegen/js_code...

We should cut back on it now that there's quasiquotes, see below.

> #2 has a multitude of hazards.
> 
> JS is declared as a function, so we must keep the order of evaluation of
> arguments.
> Substituting expressions in a JS function body could change the order.

Yup, but please note this is done only in the context of macro-expanding
"function body" JS calls, as an internal convention, which assumes the
simplified identifiers are stable / local (+ only simplifies the expression if
they are indeed identifiers). This allows defining top-level js functions that
look like js functions (and not like `function f() { return (() => { ... })();
}`).

We can & should definitely ditch that with the new quasiquotes / string
interpolation, and only optimize the no-arg case (which should always safe, but
I think we should only do that in JS intrinsics: `(() => { ... })()` -> `{ ...
}`).

> JavaScript variables may be captured and subsequently updated.
> dart2js will reject function bodies with placeholders, DDC just does not
enforce
> that yet.

I think these are two separate matters: the placeholder thing is a static matter
happening during compilation, while side-effects of JS captures (the good old
problem of macro expansion) will happen at runtime.

I'd be keen to see a concrete example to explain the rationale behind that
enforcement, as I can think of cases without function bodies that can still
cause the same kind of issues :-)

(again, Lisp and Scala have had quasiquotes for ages, they work great no matter
how big the code is, and it works massively well with DDC)

> Given that you should not do #2, i see no advantage.
> The extra 'tax' is restating the parameters in the JS function, a line of #'s
in
> the outer function application, and restating the parameters in the JS 'call.
> You have to do this anyway to preserve function call semantics.
> 
>     JS(..., '((p)=>{... p ...})($a)')
> 
> is little improvement over
> 
>     JS(..., '((p)=>{... p ...})(#)', a)

What I'm suggesting is directly this:

    JS(..., '(() => {... $a ...})()')

And to have it simplified, in the case of a top-level function, to:
 
    JS(...,  '... $a ...')

Note that there are 160 such JS calls, many of which pull many references. Take
isClassSubType in types.dart, for instance: with # templates, we would need a
definition like:

    isClassSubType(t1, t2) => JS('', '((t1, t2, assert_, canonicalType,
getGenericArgs, getGenericClass, getImplements, getMixins, isClassSubType,
isSubtype, Object) => {
      ...t1...assert_...
    })(#, #, #, #, #, #, #, #, #, #, #)''', t1, t2, assert_, canonicalType,
getGenericArgs, getGenericClass, getImplements, getMixins, isClassSubType,
isSubtype, Object);

Instead, with quasiquotes, it is:

    isClassSubType(t1, t2) => JS('', '''(() => {
      ...$t1...$assert_...
    })()''');

The # version is hard to write / maintain, you need to count how many # you
inserted, make sure there's no mismatch of args (neither Dart nor JavaScript
will help you much here), etc.
Both versions are *exactly* equivalent (they're strictly compiled to the same
code), and just work.

And this is not even an extreme case (see more in
https://codereview.chromium.org/1530563003/)...

Powered by Google App Engine
This is Rietveld 408576698