New treatment of native methods.

New treatment of native methods.

R=ahe@google.com

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

[Anton Muhin, 2012-06-18 10:04:52]

It's not complete: we get warning like void function returns string.  I'll fix
those if the overall approach is correct.

https://chromiumcodereview.appspot.com/10558023/diff/1/lib/compiler/implement...
File lib/compiler/implementation/native_handler.dart (left):

https://chromiumcodereview.appspot.com/10558023/diff/1/lib/compiler/implement...
lib/compiler/implementation/native_handler.dart:164:
listener.endExpressionStatement(token);
now we treat all natives as returns, even though there are cases like native
'this.x = 42' which formally has no returns.  That leads to returning a bool if
we need to wrap HForeign into HReturn.

https://chromiumcodereview.appspot.com/10558023/diff/1/lib/compiler/implement...
File lib/compiler/implementation/native_handler.dart (right):

https://chromiumcodereview.appspot.com/10558023/diff/1/lib/compiler/implement...
lib/compiler/implementation/native_handler.dart:155: /* Fails thanks to
lib/isolate/frog/isolateimpl.dart:143 --- native method w/ Dart body.
is it valid Dart?  I mean code in isolateimpl.dart.

https://chromiumcodereview.appspot.com/10558023/diff/1/lib/compiler/implement...
File lib/compiler/implementation/ssa/builder.dart (right):

https://chromiumcodereview.appspot.com/10558023/diff/1/lib/compiler/implement...
lib/compiler/implementation/ssa/builder.dart:2635: if
(!native.handleSsaNative(this, node.expression)) return;
ugliness raises its head here.

[ahe, 2012-06-18 10:37:39]

Let's chat about this. As far as I'm can tell, you're not sure this approach is
working.

https://chromiumcodereview.appspot.com/10558023/diff/1/lib/compiler/implement...
File lib/compiler/implementation/native_handler.dart (right):

https://chromiumcodereview.appspot.com/10558023/diff/1/lib/compiler/implement...
lib/compiler/implementation/native_handler.dart:155: /* Fails thanks to
lib/isolate/frog/isolateimpl.dart:143 --- native method w/ Dart body.
On 2012/06/18 10:04:52, antonmuhin wrote:
> is it valid Dart?

No. Nothing about "native" is valid Dart.

>  I mean code in isolateimpl.dart.

Whatever is used to implement that stuff is needed. If you think it is easier,
convert the code to use the JS function instead. Let me know if you need to some
pointers.

[Anton Muhin, 2012-06-20 15:12:18]

Peter, I think it's now ready for a review, may you have a look, please?

[sra1, 2012-06-20 18:28:50]

On 2012/06/20 15:12:18, antonmuhin wrote:
> Peter, I think it's now ready for a review, may you have a look, please?

What are you trying to achieve with this CL?

I am planning (not immediately) to add a redirecting native clause for fields. 
Does that fit with these changes?

Example, we might expose the id JavaScript property of an Element using a
private name

class  ... native '*Element' {

   String _id native 'id';
}

The Dart expression elem._id gets compiled to JavaScript elem.get$_id()  where
get$_id = function(){ return this.id }
which hopefully gets inlined to elem.id when the compiler can determine the type
of 'elem'.

[sra1, 2012-06-20 18:29:05]

https://chromiumcodereview.appspot.com/10558023/diff/1/lib/compiler/implement...
File lib/compiler/implementation/native_handler.dart (left):

https://chromiumcodereview.appspot.com/10558023/diff/1/lib/compiler/implement...
lib/compiler/implementation/native_handler.dart:164:
listener.endExpressionStatement(token);
On 2012/06/18 10:04:52, antonmuhin wrote:
> now we treat all natives as returns, even though there are cases like native
> 'this.x = 42' which formally has no returns.  That leads to returning a bool
if
> we need to wrap HForeign into HReturn.

I'm not sure that makes sense; some of the native strings are statements, not
expressions.

https://chromiumcodereview.appspot.com/10558023/diff/1/lib/compiler/implement...
File lib/compiler/implementation/native_handler.dart (right):

https://chromiumcodereview.appspot.com/10558023/diff/1/lib/compiler/implement...
lib/compiler/implementation/native_handler.dart:155: /* Fails thanks to
lib/isolate/frog/isolateimpl.dart:143 --- native method w/ Dart body.
On 2012/06/18 10:04:52, antonmuhin wrote:
> is it valid Dart?  I mean code in isolateimpl.dart.

Native methods with both a string JavaScript body and a Dart body was a trick
used by Frog to mark the dependencies of the native code.  For example, if the
native code throws a FooException, the Dart code would contain a call to the
constructor of FooException to ensure that the compiler knew it was used,
otherwise no code for FooException might be emitted.

We should rewrite all code using this trick as Dart code with embedded JS()
elements.

[Anton Muhin, 2012-06-20 18:51:49]

https://chromiumcodereview.appspot.com/10558023/diff/1/lib/compiler/implement...
File lib/compiler/implementation/native_handler.dart (left):

https://chromiumcodereview.appspot.com/10558023/diff/1/lib/compiler/implement...
lib/compiler/implementation/native_handler.dart:164:
listener.endExpressionStatement(token);
Stephen, both Peter and me are well aware of this problem---see the discussion. 
Still we think it's the best approach.  But if you have any over proposals,
that'd be really great.

Plus there are plans to rework native system completely which will probably
render this CL fully obsolete, so I'd rather not spend too much time theorizing
about ideal approach but rather keep things as simple as possible.

That's my stance, Peter may very well have completely different view of the
story.

On 2012/06/20 18:29:05, sra1 wrote:
> On 2012/06/18 10:04:52, antonmuhin wrote:
> > now we treat all natives as returns, even though there are cases like native
> > 'this.x = 42' which formally has no returns.  That leads to returning a bool
> if
> > we need to wrap HForeign into HReturn.
> 
> I'm not sure that makes sense; some of the native strings are statements, not
> expressions.

https://chromiumcodereview.appspot.com/10558023/diff/1/lib/compiler/implement...
File lib/compiler/implementation/native_handler.dart (right):

https://chromiumcodereview.appspot.com/10558023/diff/1/lib/compiler/implement...
lib/compiler/implementation/native_handler.dart:155: /* Fails thanks to
lib/isolate/frog/isolateimpl.dart:143 --- native method w/ Dart body.
Thanks a lot for clarifications.  I wonder if it works now at all, but it might.

On 2012/06/20 18:29:05, sra1 wrote:
> On 2012/06/18 10:04:52, antonmuhin wrote:
> > is it valid Dart?  I mean code in isolateimpl.dart.
> 
> Native methods with both a string JavaScript body and a Dart body was a trick
> used by Frog to mark the dependencies of the native code.  For example, if the
> native code throws a FooException, the Dart code would contain a call to the
> constructor of FooException to ensure that the compiler knew it was used,
> otherwise no code for FooException might be emitted.
> 
> We should rewrite all code using this trick as Dart code with embedded JS()
> elements.

[ahe, 2012-06-21 14:19:13]

LGTM if you add a test.

Cheers,
Peter

[Anton Muhin, 2012-06-21 14:37:24]

tests are added, submitting.  Thanks a lot for review.

On 2012/06/21 14:19:13, ahe wrote:
> LGTM if you add a test.
> 
> Cheers,
> Peter