Fix behavior when typed JS interop getters are called as functions.

Fix behavior when typed JS interop getters are called as functions.

BUG=
R=sigmund@google.com, sra@google.com

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

[Jacob, 2015-10-30 04:51:03]

jacobr@google.com changed reviewers:
+ sra@google.com

[Jacob, 2015-10-30 04:51:04]

Interesting example of trusting types to make dart2js codesize for JS interop
not as horrible as it would otherwise be.

This fixes
https://github.com/dart-lang/sdk/issues/24747

[Siggi Cherem (dart-lang), 2015-10-30 16:59:57]

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

[Siggi Cherem (dart-lang), 2015-10-30 16:59:57]

some small changes, but otherwise is looking good

https://codereview.chromium.org/1431523002/diff/1/pkg/compiler/lib/src/js_emi...
File pkg/compiler/lib/src/js_emitter/program_builder/program_builder.dart
(right):

https://codereview.chromium.org/1431523002/diff/1/pkg/compiler/lib/src/js_emi...
pkg/compiler/lib/src/js_emitter/program_builder/program_builder.dart:424: } else
if (_compiler.types.isSubtype(returnType, backend.coreTypes.functionType)) {
Like we chatted in person, let's do 2 small changes:
 - accept any dynamic getter
 - guard the use of types under the --trust-type-annotations flag


Q for Stephen: should we just turn this on by default? One idea we discussed was
to have it unguarded or define an internal boolean that is set to true to guard
the logic so we can easily find it later (sort of like a separate flag that is
not exposed on the commandline).

https://codereview.chromium.org/1431523002/diff/1/pkg/compiler/lib/src/js_emi...
pkg/compiler/lib/src/js_emitter/program_builder/program_builder.dart:444:
maxArgs = 1000;
what if I want 1001!? ;-)

[Siggi Cherem (dart-lang), 2015-10-30 17:00:49]

https://codereview.chromium.org/1431523002/diff/1/pkg/compiler/lib/src/js_emi...
File pkg/compiler/lib/src/js_emitter/program_builder/program_builder.dart
(right):

https://codereview.chromium.org/1431523002/diff/1/pkg/compiler/lib/src/js_emi...
pkg/compiler/lib/src/js_emitter/program_builder/program_builder.dart:424: } else
if (_compiler.types.isSubtype(returnType, backend.coreTypes.functionType)) {
On 2015/10/30 16:59:57, Siggi Cherem (dart-lang) wrote:
> Like we chatted in person, let's do 2 small changes:
>  - accept any dynamic getter
>  - guard the use of types under the --trust-type-annotations flag
> 
> 
> Q for Stephen: should we just turn this on by default? One idea we discussed
was
> to have it unguarded or define an internal boolean that is set to true to
guard
> the logic so we can easily find it later (sort of like a separate flag that is
> not exposed on the commandline).

Another idea here would be to ask users to annotate the code with metadata
rather than using the type, to indicate their intent. For example:

  @JS('bar')
  class Bar {
    @JSMethod
    get foo;
  }


or let users opt-in/opt-out from this feature at the class level:

  @JS('bar', functionGettersAreMethods: true)
  class Bar {
    Function get foo;
  }

[Jacob, 2015-10-30 19:47:17]

Added check for dynamic and some more tests.

https://codereview.chromium.org/1431523002/diff/1/pkg/compiler/lib/src/js_emi...
File pkg/compiler/lib/src/js_emitter/program_builder/program_builder.dart
(right):

https://codereview.chromium.org/1431523002/diff/1/pkg/compiler/lib/src/js_emi...
pkg/compiler/lib/src/js_emitter/program_builder/program_builder.dart:424: } else
if (_compiler.types.isSubtype(returnType, backend.coreTypes.functionType)) {
On 2015/10/30 17:00:49, Siggi Cherem (dart-lang) wrote:
> On 2015/10/30 16:59:57, Siggi Cherem (dart-lang) wrote:
> > Like we chatted in person, let's do 2 small changes:
> >  - accept any dynamic getter
> >  - guard the use of types under the --trust-type-annotations flag
> > 
> > 
> > Q for Stephen: should we just turn this on by default? One idea we discussed
> was
> > to have it unguarded or define an internal boolean that is set to true to
> guard
> > the logic so we can easily find it later (sort of like a separate flag that
is
> > not exposed on the commandline).
> 
> Another idea here would be to ask users to annotate the code with metadata
> rather than using the type, to indicate their intent. For example:
> 
>   @JS('bar')
>   class Bar {
>     @JSMethod
>     get foo;
>   }
> 
> 
> or let users opt-in/opt-out from this feature at the class level:
> 
>   @JS('bar', functionGettersAreMethods: true)
>   class Bar {
>     Function get foo;
>   }

NOOOO.  1. this means the analyzer can't do error checking as you would get if
you wrote

typedef SomeFunctionSignature(..,..)

SomeFunctionSignature get foo;

2. Annotation handling in dart2js is a pain...

3. adding functionGettersAreMethods isn't very discoverable. users will omit the
annotation and wonder why they can't call their functions.

https://codereview.chromium.org/1431523002/diff/1/pkg/compiler/lib/src/js_emi...
pkg/compiler/lib/src/js_emitter/program_builder/program_builder.dart:444:
maxArgs = 1000;
On 2015/10/30 16:59:57, Siggi Cherem (dart-lang) wrote:
> what if I want 1001!? ;-)

discussed offline. switched to 32767
also cleaned up code bellow to not throw after 52 args.

[Siggi Cherem (dart-lang), 2015-10-30 19:54:17]

let's wait on Stephens opinion about guarding this under a flag, other than that
lgtm

https://codereview.chromium.org/1431523002/diff/20001/pkg/compiler/lib/src/js...
File pkg/compiler/lib/src/js_emitter/program_builder/program_builder.dart
(right):

https://codereview.chromium.org/1431523002/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/program_builder/program_builder.dart:465: (i) =>
'p$i');
you could also keep the old naming scheme to optimize for functions with few
arguments. For example:

var candidateNames = 'abc....';
(i) => i < 52 ? candidateNames[i] : '${candidateNames[i % 52]}${i ~/ 52}'

[Siggi Cherem (dart-lang), 2015-10-30 20:14:57]

lgtm

https://codereview.chromium.org/1431523002/diff/40001/pkg/compiler/lib/src/js...
File pkg/compiler/lib/src/js_emitter/program_builder/program_builder.dart
(right):

https://codereview.chromium.org/1431523002/diff/40001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/program_builder/program_builder.dart:419: } else
if (member.isGetter) {
minor nit - up to you: if you only care about GetterElement, you could do
`member is GetterElement` here and get rid of the extra `getter` variable below

[[an aside rant: I'm not a big fan of the .isFoo checks, but they are everywhere
in the code. I much rather have a unique interface with what we care about and
then benefit from type propagation on branches]]

https://codereview.chromium.org/1431523002/diff/40001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/program_builder/program_builder.dart:430:
functionType = typedef.element.functionSignature.type;
actually, I think here you can do `typedef.unaliased` to get the canonical type

https://codereview.chromium.org/1431523002/diff/40001/tests/html/js_function_...
File tests/html/js_function_getter_test.dart (right):

https://codereview.chromium.org/1431523002/diff/40001/tests/html/js_function_...
tests/html/js_function_getter_test.dart:114: // These tests only work with trust
types enabled
2 options: 
- always run this test with TT
- split the test in two, and make the expectation different when you have or
don't have the flag.

For either case, you'll need to make a test run with the flag on. You can do
that with the `//SharedOptions=` comment on your test file like here:
https://github.com/dart-lang/sdk/blob/e666436d05f64c12105b9bf0d19961f663824db...

(although you may need to disable the test for dartium in that case, not sure)

[sra1, 2015-10-30 20:54:59]

lgtm

https://codereview.chromium.org/1431523002/diff/20001/pkg/compiler/lib/src/js...
File pkg/compiler/lib/src/js_emitter/program_builder/program_builder.dart
(right):

https://codereview.chromium.org/1431523002/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/program_builder/program_builder.dart:413: bool
isFunctionLike = false;
Explain (comment) that this is generating stubs for direct calls and stubs for
call-through.

https://codereview.chromium.org/1431523002/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/program_builder/program_builder.dart:414:
FunctionType functionType;
I like to add = null if it is not set one some path.

https://codereview.chromium.org/1431523002/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/program_builder/program_builder.dart:426:
returnType.treatAsDynamic) {
Horrible indentation.  If it is the formatter, can it be fixed by switching ||
operands?

https://codereview.chromium.org/1431523002/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/program_builder/program_builder.dart:450:
FunctionElement fn = member;
'fn' unused below.  Will fail if you TODO handle field elements

https://codereview.chromium.org/1431523002/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/program_builder/program_builder.dart:468:
stubName,
Do you get duplicate stubs?

https://codereview.chromium.org/1431523002/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/program_builder/program_builder.dart:470:
[parameters, member.name, parameters]),
This is correct for function stubs, but is it what you want for call-through
stubs? I guess they have to be the same.

https://codereview.chromium.org/1431523002/diff/20001/tests/html/js_function_...
File tests/html/js_function_getter_test.dart (right):

https://codereview.chromium.org/1431523002/diff/20001/tests/html/js_function_...
tests/html/js_function_getter_test.dart:121: // expect(() =>
foo.bar.add(4,5,10), throws);
same as above, delete?

[Jacob, 2015-10-30 22:31:53]

https://codereview.chromium.org/1431523002/diff/20001/pkg/compiler/lib/src/js...
File pkg/compiler/lib/src/js_emitter/program_builder/program_builder.dart
(right):

https://codereview.chromium.org/1431523002/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/program_builder/program_builder.dart:413: bool
isFunctionLike = false;
On 2015/10/30 20:54:59, sra1 wrote:
> Explain (comment) that this is generating stubs for direct calls and stubs for
> call-through. 

Done.

https://codereview.chromium.org/1431523002/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/program_builder/program_builder.dart:414:
FunctionType functionType;
On 2015/10/30 20:54:59, sra1 wrote:
> I like to add = null if it is not set one some path.

Done.

https://codereview.chromium.org/1431523002/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/program_builder/program_builder.dart:426:
returnType.treatAsDynamic) {
On 2015/10/30 20:54:59, sra1 wrote:
> Horrible indentation.  If it is the formatter, can it be fixed by switching ||
> operands?

done. shame line length isn't 100.

https://codereview.chromium.org/1431523002/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/program_builder/program_builder.dart:450:
FunctionElement fn = member;
On 2015/10/30 20:54:59, sra1 wrote:
> 'fn' unused below.  Will fail if you TODO handle field elements

done. forgot to remove this.

https://codereview.chromium.org/1431523002/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/program_builder/program_builder.dart:465: (i) =>
'p$i');
On 2015/10/30 19:54:17, Siggi Cherem (dart-lang) wrote:
> you could also keep the old naming scheme to optimize for functions with few
> arguments. For example:
> 
> var candidateNames = 'abc....';
> (i) => i < 52 ? candidateNames[i] : '${candidateNames[i % 52]}${i ~/ 52}'
not needed. these names will be minified anyway so the previous code was a
premature optimization.

https://codereview.chromium.org/1431523002/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/program_builder/program_builder.dart:468:
stubName,
On 2015/10/30 20:54:59, sra1 wrote:
> Do you get duplicate stubs?

no because of 
if (!stubNames.add(stubName.key)) break

siggi complained about my deep nesting so it is written this way which makes it
a little harder to spot this line.

https://codereview.chromium.org/1431523002/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/program_builder/program_builder.dart:470:
[parameters, member.name, parameters]),
On 2015/10/30 20:54:59, sra1 wrote:
> This is correct for function stubs, but is it what you want for call-through
> stubs? I guess they have to be the same.

this is awesome for call through stubs. It means it doesn't have to be a compile
time error if a JS interop subclass defines a method as a getter returning a
Function while the base class has a method... which frequently happens in d.ts
files as JavaScript devs don't really care about distinguishing the two.
Added a comment describing that this is the intended effect.

https://codereview.chromium.org/1431523002/diff/40001/pkg/compiler/lib/src/js...
File pkg/compiler/lib/src/js_emitter/program_builder/program_builder.dart
(right):

https://codereview.chromium.org/1431523002/diff/40001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/program_builder/program_builder.dart:419: } else
if (member.isGetter) {
On 2015/10/30 20:14:57, Siggi Cherem (dart-lang) wrote:
> minor nit - up to you: if you only care about GetterElement, you could do
> `member is GetterElement` here and get rid of the extra `getter` variable
below
> 
> [[an aside rant: I'm not a big fan of the .isFoo checks, but they are
everywhere
> in the code. I much rather have a unique interface with what we care about and
> then benefit from type propagation on branches]]

Acknowledged.

[Jacob, 2015-10-30 23:07:08]

Committed patchset #4 (id:60001) manually as
d0f456a910675bab3b47b80fddf1fda5ee9b7fa8 (presubmit successful).