dart-lang / sdk

The Dart SDK, including the VM, JS and Wasm compilers, analysis, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
10.22k stars 1.57k forks source link

Eliminating native syntax #28791

Open peter-ahe-google opened 7 years ago

peter-ahe-google commented 7 years ago

I wonder if we can eliminate the native syntax used by at least the Dart VM. At some point, dart2js used it as well in dart:html, but I'm not sure that's the case any more.

Here's an example of a getter using the native syntax:

int get length native "List_getLength";

And here's how I'd propose to change it (what Kernel uses today):

external @ExternalName("List_getLength")
int get length;

Or, if #28283 is fixed:

@ExternalName("List_getLength")
external int get length;

Please let me know what you think about this, and if you foresee any problems with this approach.

@rakudrama @a-siva @mhausner @sigmundch @kmillikin @asgerf @johnniwinther @stereotype441 @bwilkerson

mhausner commented 7 years ago

A few observations:

The spec-conformant Dart syntax would have to be:

external @ExternalName('List_getLength') int get length;

The metadata annotation is part of the function signature, which follows the optional 'external' keyword.

Using metadata to link a dart function to a native function body would burden the VM to evaluate the annotation. It currently does not evaluate annotations. The evaluation is necessary to ensure that the annotation resolves to a predefined class. Alternatively, ExternalName would have to be a built-in name.

(Side note: the VM takes a shortcut for @patch, which only has a predefined meaning in patch files. The shortcut is to recognize 'patch' as a predefined, reserved identifier, without evaluating the expression.)

Also note that today, a function with a native body must not be declared external. In fact, external means the function has no body and cannot be called, unless it is later patched. The native declaration is a way to specify a function body.

I prefer to keep the native declaration as it is today. We could add it to the language spec.

If we use annotations, I'd prefer to use the annotation @external('xxxx') and make that annotation a built-in identifier. The declaration would then be

@external('List_getLength') int get length;

And in a patch file, you might see something like this:

@patch @external('File_Exists') static _exists(String path);

On Thu, Feb 16, 2017 at 12:57 PM, Peter von der Ahé < notifications@github.com> wrote:

I wonder if we can eliminate the native syntax used by at least the Dart VM. At some point, dart2js used it as well in dart:html, but I'm not sure that's the case any more.

Here's an example of a getter using the native syntax:

int get length native "List_getLength";

And here's how I'd propose to change it (what Kernel uses today):

@ExternalName("List_getLength") external int get length;

Please let me know what you think about this, and if you foresee any problems with this approach.

@rakudrama https://github.com/rakudrama @a-siva https://github.com/a-siva @mhausner https://github.com/mhausner @sigmundch https://github.com/sigmundch @kmillikin https://github.com/kmillikin @asgerf https://github.com/asgerf @johnniwinther https://github.com/johnniwinther

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/dart-lang/sdk/issues/28791, or mute the thread https://github.com/notifications/unsubscribe-auth/AH-xMlbsOPdaSCH5e4n32rxg7H8lBCfMks5rdDm2gaJpZM4MC7E5 .

peter-ahe-google commented 7 years ago

I'm not suggesting making this change as long as the VM has to parse source code. I wouldn't make this change until we had a kernel-based front end.

Given that this syntax is only used in our implementation libraries, I would prefer to keep it out of the specification.

peter-ahe-google commented 7 years ago

Updated my original example based on @mhausner's point about specification conformance.

eernstg commented 7 years ago

On Thu, Feb 16, 2017 at 1:50 PM, Matthias Hausner notifications@github.com wrote:

A few observations:

The spec-conformant Dart syntax would have to be:

external @ExternalName('List_getLength') int get length;

The metadata annotation is part of the function signature, which follows the optional 'external' keyword.

Hi Matthias,

I don't want to interfere with the rest of this thread, but for the grammar we do have this issue: https://github.com/dart-lang/sdk/issues/28283. I have some ongoing work which will address this.

Using metadata to link a dart function to a native function body would

burden the VM to evaluate the annotation. It currently does not evaluate annotations. The evaluation is necessary to ensure that the annotation resolves to a predefined class. Alternatively, ExternalName would have to be a built-in name.

(Side note: the VM takes a shortcut for @patch, which only has a predefined meaning in patch files. The shortcut is to recognize 'patch' as a predefined, reserved identifier, without evaluating the expression.)

Also note that today, a function with a native body must not be declared external. In fact, external means the function has no body and cannot be called, unless it is later patched. The native declaration is a way to specify a function body.

I prefer to keep the native declaration as it is today. We could add it to the language spec.

If we use annotations, I'd prefer to use the annotation @external('xxxx') and make that annotation a built-in identifier. The declaration would then be

@external('List_getLength') int get length;

And in a patch file, you might see something like this:

@patch @external('File_Exists') static _exists(String path);

On Thu, Feb 16, 2017 at 12:57 PM, Peter von der Ahé < notifications@github.com> wrote:

I wonder if we can eliminate the native syntax used by at least the Dart VM. At some point, dart2js used it as well in dart:html, but I'm not sure that's the case any more.

Here's an example of a getter using the native syntax:

int get length native "List_getLength";

And here's how I'd propose to change it (what Kernel uses today):

@ExternalName("List_getLength") external int get length;

Please let me know what you think about this, and if you foresee any problems with this approach.

@rakudrama https://github.com/rakudrama @a-siva https://github.com/a-siva @mhausner https://github.com/mhausner @sigmundch https://github.com/sigmundch @kmillikin https://github.com/kmillikin @asgerf https://github.com/asgerf @johnniwinther https://github.com/johnniwinther

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/dart-lang/sdk/issues/28791, or mute the thread https://github.com/notifications/unsubscribe-auth/AH- xMlbsOPdaSCH5e4n32rxg7H8lBCfMks5rdDm2gaJpZM4MC7E5 .

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/dart-lang/sdk/issues/28791#issuecomment-280321976, or mute the thread https://github.com/notifications/unsubscribe-auth/AJKXUip_HuVO7EVElJS6X5CCnajn-_Usks5rdEYOgaJpZM4MC7E5 .

-- Erik Ernst - Google Danmark ApS Skt Petri Passage 5, 2 sal, 1165 København K, Denmark CVR no. 28866984

peter-ahe-google commented 7 years ago

Thank you, @eernstg. I updated my first comment with a note about that bug.

lrhn commented 7 years ago

On Thu, Feb 16, 2017 at 1:50 PM, Matthias Hausner notifications@github.com wrote:

A few observations:

The spec-conformant Dart syntax would have to be:

external @ExternalName('List_getLength') int get length;

The metadata annotation is part of the function signature, which follows the optional 'external' keyword.

As Erik says, that's a known bug in the spec.

Using metadata to link a dart function to a native function body would burden the VM to evaluate the annotation. It currently does not evaluate annotations. The evaluation is necessary to ensure that the annotation resolves to a predefined class. Alternatively, ExternalName would have to be a built-in name.

The VM only needs to recognize this in its own patch files or internal libraries. It can special-case however it wants to. I guess the problem here is that the VM wants the common front-end to read its internal libraries and patch files too, which means that an internal-only syntax extension leaks into that parser as well.

(Side note: the VM takes a shortcut for @patch, which only has a predefined meaning in patch files. The shortcut is to recognize 'patch' as a predefined, reserved identifier, without evaluating the expression.)

I'm sure you can do the same here. Nobody outside the internal/patch library needs to know about it. Anything that works will do :)

Also note that today, a function with a native body must not be declared external. In fact, external means the function has no body and cannot be called, unless it is later patched. The native declaration is a way to specify a function body.

At the language level, you are not specifying a Dart body. You are saying that the body/implementation is provided by the system in some way. That's exactly what external methods are for - providing a Dart signature for something that is implemented by the underlying system.

Whether you specify the implementation using a string co-located with the declaration, in a patch file, or in C++ code doesn't really matter. You can consider @ExternalName("Foo") external foo() as equivalent to, or a shorthand for, external foo(); + @patch foo() native "Foo";.

The advantage would be that the common front-end parser doesn't need to handle non-Dart syntax at all (which "native" is).

I prefer to keep the native declaration as it is today. We could add it to the language spec.

I don't think it's a feature that warrants being specified. It's a way for a platform implementation to provide an implementation of a function signature, just as any other external method. No user will ever need to see it, and agreeing on a notation is only relevant internally on the Dart team.

If we need the same parser to handle both user code abd our internal code, making the internal code look like normal Dart is an advantage, but keeping native is also an option - it just seems unnecessary to me. I can understand introducing the original notation, way back before either external methods or annotations were in the language, but we do have more features now, and I don't think we need a syntax extension any more to get the same effect. If there is a reason to remove it (and the common front-end is a reason), I think it would be nice to do so.

If we use annotations, I'd prefer to use the annotation @external('xxxx') and make that annotation a built-in identifier. The declaration would then be

@external('List_getLength') int get length;

And in a patch file, you might see something like this:

@patch @external('File_Exists') static _exists(String path);

The only problem here is that external is a built-in identifier, so it can't be the name of a class, and that makes the syntax again be invalid Dart and require special-casing in the parser. If you use @native("Lost_getLength") instead (or any other undeclared identifier), you don't have that problem. It might be malformed if there isn't a declaration of native in scope, but that should be easy to handle (just declare it in internal_patch.dart) and it's not a parsing problem.

The VM can still special case the meaning of it in any way it wants to.

Another option is to do what Dart2JS is doing:

int get length => JS_BUILTIN("some magic text");

The compiler recognizes the JS_BUILTIN function as special, and optimizes/compiles it into a function that does the right thing. No special syntax needed, only a special case in the compiler. The VM could do something similar:

int get length => VM_NATIVE("List_getLength");

with an unpatched external VM_NATIVE(String name); declared somewhere. The entire function is compiled into whatever it needs to be by the compiler recognizing a body on the right form.

Cheers /L

On Thu, Feb 16, 2017 at 12:57 PM, Peter von der Ahé < notifications@github.com> wrote:

I wonder if we can eliminate the native syntax used by at least the Dart VM. At some point, dart2js used it as well in dart:html, but I'm not sure that's the case any more.

Here's an example of a getter using the native syntax:

int get length native "List_getLength";

And here's how I'd propose to change it (what Kernel uses today):

@ExternalName("List_getLength") external int get length;

Please let me know what you think about this, and if you foresee any problems with this approach.

@rakudrama https://github.com/rakudrama @a-siva https://github.com/a-siva @mhausner https://github.com/mhausner @sigmundch https://github.com/sigmundch @kmillikin https://github.com/kmillikin @asgerf https://github.com/asgerf @johnniwinther https://github.com/johnniwinther

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/dart-lang/sdk/issues/28791, or mute the thread https://github.com/notifications/unsubscribe-auth/AH- xMlbsOPdaSCH5e4n32rxg7H8lBCfMks5rdDm2gaJpZM4MC7E5 .

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/dart-lang/sdk/issues/28791#issuecomment-280321976, or mute the thread https://github.com/notifications/unsubscribe-auth/AEo9B6rBIRxOVA3uvrme7Aqz_EsZFwPyks5rdEYMgaJpZM4MC7E5 .

-- Lasse R.H. Nielsen - lrn@google.com 'Faith without judgement merely degrades the spirit divine' Google Denmark ApS - Frederiksborggade 20B, 1 sal - 1360 København K

stereotype441 commented 7 years ago

I really like @lrhn's idea of int get length => VM_NATIVE("List_getLength").

mhausner commented 7 years ago

I like Lasse's suggestion, too.

In strong mode and the new, very restrictive patching rules, can there be a function like VM_NATIVE(String s) that is only present in the VM platform? Or do all platforms have to have this function in the core library, essentially as a no-op?

Matthias

On Thu, Feb 16, 2017 at 5:32 PM, Paul Berry notifications@github.com wrote:

I really like @lrhn https://github.com/lrhn's idea of int get length => VM_NATIVE("List_getLength").

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/dart-lang/sdk/issues/28791#issuecomment-280382493, or mute the thread https://github.com/notifications/unsubscribe-auth/AH-xMqS31_7N_zsPF-ngwze-Km4z9oyBks5rdHoGgaJpZM4MC7E5 .

rakudrama commented 7 years ago

I have been wanting to change dart2js dart:html to use external. I would be supportive of work to do this.

dart2js classes with an @Native() annotation are magical. The fields of the classes are not like normal fields, since they can have effects (like layout). The methods are magical too since the calling convention must match JavaScript. This magic results in quite a few 'isNative' tests sprinkled around the dart2js code.

I have held off in the past because using external + an annotation to say how the external member is found is much more verbose than the current magic.

Historically we had small apps, for which parsing and resolving dart:html is a significant fraction of the whole app cost. With bigger apps, or pre-processing to Kernel, I don't think this matters as much.

By verbose I mean you go from

int x;

to

@JSName('x') external int get x; @JSName('x') external void set x(int value);

and as 80% of the API surface is property getters and setters, this would roughly double the size of dart:html.

On Thu, Feb 16, 2017 at 8:32 AM, Paul Berry notifications@github.com wrote:

I really like @lrhn https://github.com/lrhn's idea of ​​ int get length => VM_NATIVE("List_getLength").

​Yes, we do have a magic method for inlined JavaScript expressions, but it would not work exactly as written above. Perhaps the the VM version would be:

int get length => VM_NATIVE("List_getLength", this);

The dart2js version for JSArray looks like this:

​ int get length => JS('JSUInt32', r'#.length', this);

I don't the idea of relying entirely on JS(...). It does not play well with the difference​ between Dart and JavaScript handling of optional arguments. For the VM it would be OK to force the optional argument handling in the Dart declaration, since there is some freedom to constrain how VM_NATIVE actually works.

Consider a native API with an optional argument:

external int foo(int x, [int y]);

It is a requirement from our customers that for these native APIs we reliably compile Dart x.foo(a) to JavaScript x.foo(a) and x.foo(a,b) to x.foo(a,b).

We can't cleanly express this as a combination of Dart and the magical JS function:

int foo(int x, [int y]) => *???*  ?  JS('', '#.foo(#)', this, x) :

JS('', '#.foo(#, #)', this, x, y);

​There is nothing we can write at ??? ​that allows us to always optimize out the test. We used to have a dispatch on y==null, but is was not satisfactory. First, it is incorrect, since in JavaScript the API can tell the number of arguments it was called with. The requirement is that x.foo(a,null) compiles to x.foo(a,null) and not x.foo(a). We can't use a private sentinel object since the API also must be strongly typed, and it is also very hard to tell that the sentinel constant does not flow to the argument.​ Simply relying on ever smarter compilation did not work well, so we have the concept that native methods have their own calling convention.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/dart-lang/sdk/issues/28791#issuecomment-280382493, or mute the thread https://github.com/notifications/unsubscribe-auth/ACAtlQskfrpfrmd3cAx4Q7YKm8t7dz_Rks5rdHoFgaJpZM4MC7E5 .

rakudrama commented 7 years ago

On Fri, Feb 17, 2017 at 6:36 PM, Matthias Hausner notifications@github.com wrote:

I like Lasse's suggestion, too.

In strong mode and the new, very restrictive patching rules, can there be a function like VM_NATIVE(String s) that is only present in the VM platform? Or do all platforms have to have this function in the core library, essentially as a no-op?

​It can't be a no-op. In dart2js the method must do something. VM_NATIVE, however it is defined, would exist only on the VM platform(s). ​

Matthias

On Thu, Feb 16, 2017 at 5:32 PM, Paul Berry notifications@github.com wrote:

I really like @lrhn https://github.com/lrhn's idea of int get length => VM_NATIVE("List_getLength").

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/dart-lang/sdk/issues/28791#issuecomment-280382493, or mute the thread https://github.com/notifications/unsubscribe- auth/AH-xMqS31_7N_zsPF-ngwze-Km4z9oyBks5rdHoGgaJpZM4MC7E5 .

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/dart-lang/sdk/issues/28791#issuecomment-280815384, or mute the thread https://github.com/notifications/unsubscribe-auth/ACAtlYew-3fOvLETgG6lQXXZbNgN2a8eks5rdlkqgaJpZM4MC7E5 .

peter-ahe-google commented 7 years ago

@mhausner dart2js has a library called dart:_foreign_helper. It defines a number of functions like this:

external JS(String typeDescription, String codeTemplate,
    [arg0, arg1, arg2, arg3, arg4, arg5, arg6, arg7, arg8, arg9, arg10, arg11]);

I don't think the VM should attempt to reuse dart:_foreign_helper, but it could have a similar library. Each place you use this library, you'd have to import it, just like dart:_internal.

An additional benefit to having these libraries is that you can document how to use it, for example, how we've documented how to correctly specify concrete types for JS.

peter-ahe-google commented 7 years ago

Concretely, to implement @lrhn's suggestion, we could add a library named "dart:_vm_native" to the VM's bootstrap set of libraries. It would look like this:

library dart._vm_native;

/// [externalName] is the name of an external function.
/// The compiler handles methods on the form `=> VM_NATIVE(f)`
/// by setting them up to call the external function `f`.
/// An external function is one defined in C++ using the
/// macro `DEFINE_NATIVE_ENTRY`.
external dynamic VM_NATIVE(String externalName);

This would be an external method that isn't patched, and recognized and handled specially by the compiler (probably at the kernel level). If this is what we decide to do, I'd also include this fact in the documentation.

mraleph commented 3 years ago

I am getting this rolling as part of deprecations of native extensions per @lrhn suggestion.

As far as I understand dart2js no longer relies on such syntax anywhere (though I found some instances in comments, maybe somebody on dart2js team could fix those /cc @rakudrama).

We are going to do this in three steps:

I will use this bug for tracking the effort. /cc @mkustermann @johnniwinther

rakudrama commented 3 years ago

dart2js still uses native without a name, see e.g. https://github.com/dart-lang/sdk/blob/master/sdk/lib/web_gl/dart2js/web_gl_dart2js.dart#L35

sigmundch commented 3 years ago

Note that the CFE lowering already converts the native syntax into an external declaration with a @ExternalName annotation from dart:_internal. Would that be sufficient for the VM, or is the pragma still needed (e.g. are there any uses of native outside dart:* libraries?)

Our plan in dart2js was to switch to external declarations as well. We could use the ExternalName annotation at first, that would get rid of the syntax. We may go further and try to integrate further with the JS-interop syntax in the future.

mraleph commented 3 years ago

dart2js still uses native without a name, see e.g. https://github.com/dart-lang/sdk/blob/master/sdk/lib/web_gl/dart2js/web_gl_dart2js.dart#L35

That explains why I missed it. I was looking for native ["'] only. What does native; mean? Is it just equivalent to external?

Note that the CFE lowering already converts the native syntax into an external declaration with a @ExternalName annotation from dart:_internal. Would that be sufficient for the VM, or is the pragma still needed (e.g. are there any uses of native outside dart:* libraries?)

I am aware of the lowering, but I wanted to ExternalName class as well - because I assumed that nobody but VM uses it. The lowering itself predates @pragma introduction and it's safe to say that if we had pragma we would not have ExternalName to begin with. We could migrate to @pragma('external-name', ...) instead if we want to use the same thing for both VM and other tools.

Embedders are free to install a native resolver on any library in the application so a dart:_internal scoped annotation does not work. (This is in fact used by some embedders, including internal G3 embedders).

rakudrama commented 3 years ago

On Mon, Sep 6, 2021 at 1:43 AM Vyacheslav Egorov @.***> wrote:

dart2js still uses native without a name, see e.g. https://github.com/dart-lang/sdk/blob/master/sdk/lib/web_gl/dart2js/web_gl_dart2js.dart#L35

That explains why I missed it. I was looking for native ["'] only. What does native; mean? Is it just equivalent to external?

Yes, the meaning has been reduced over the years to something that could be external, but that does not work for @Native classes yet. We don't need to use ExternalName, but we do have an annotation that serves the same purpose, JSName. That could be migrated to a @pragma.

I think we should consider making this happen in OKR planning for Q4.

mraleph commented 3 years ago

@rakudrama I think you might want to edit your comment, GitHub ate some of the annotations for privacy/spam reasons :)

dcharkes commented 2 years ago

Apparently we have more tests with native:

tests/language_2/const/native_factory_test.dart

class Cake {
  final name;
  const Cake(this.name);
  const factory Cake.BakeMeACake()
      native "Cake_BakeMeACake"; /*@compile-error=unspecified*/
}

main() {
  var c = const Cake("Sacher");
}

The test results are exotic, expecting compiler errors but only getting it on the dart2js backend: https://dart-ci.firebaseapp.com/current_results/#/filter=language_2/const/native_factory_test

I presume language tests are also run for dart2js. Is there a way to migrate such a test that works for both the VM and Dart2JS?

lrhn commented 2 years ago

Do the VM and Dart2JS have a shared way of specifying the binding of native external functions?

The migration should probably be to:

external const factory Cake.bakeMeACake();

with whichever annotations you think apply to your backend. Say:

@pragma("vm:native", "Cake_BakeMeACake")
@JS("bakeMeACake")

It should definitely use external, and drop the native.

sigmundch commented 2 years ago

My 2c - probably not worth trying to unify here at this time. Anything native is already very backend specific, so I'd instead consider this to be a test that should move to a VM specific test location. The dart2js's configuration is strict, and doesn't allow the native keyword outside the SDK and a vetted list of test folders meant only to test the functionality (e.g. test/web/native/). That's probably why this test fails automatically in dart2js.

For what it's worth: the CFE already lowers things to the @ExternalName external foo() representation, so if the goal is just to remove the native keyword, I think we are now in a position where making that change is possible (the backends already understand the new representation).

In the future, our goal is to remove all support for native on the web in the long run. We are using external for JSInterop, and once we migrate away from using dart:html in the future, there will be very few uses remaining, which we'll be able to convert to external as well. It's just not a top priority at the moment for us.

mkustermann commented 2 years ago

Do the VM and Dart2JS have a shared way of specifying the binding of native external functions?

The VM has settled on

@pragma('vm:external-name', '<name>')
external void foo(...);

Removing the old support has been started but never finished. I had an old CL pending to remove some legacy usages in corelibs - so I decided to land that and finish the migration. We'll remove support for native "<name>" (and the @ExternalName("<name>") lowering) in VM now.

mkustermann commented 1 year ago

The VM backend has now removed support for native "<name>" syntax (and it's lowering to @ExternalName("<name>")) in flutter, g3 and dart sdk - in favor of @pragma('vm:external-name', '<name>')

The last remaining uses seem to be for web / dart2js due to using native;. Once that is gone the CFE could drop support.

mraleph commented 1 year ago

@johnniwinther is it possible now to disable native syntax on VM and Flutter targets?

mraleph commented 1 year ago

We should disable native in VM and Flutter targets. The current experience is confusing: you get a runtime error (because native "Name" has no effect). See https://github.com/fuzzybinary/dart_shared_libray/issues/1

We should issue an error which asks to rewrite code to external & @pragma('vm:external-name') or to external + @FfiNative.

dcharkes commented 1 year ago

in VM and Flutter targets

We are still using it dart2js?

We could do it as (1) a visitor that generates errors in pkg/vm. Alternatively, we could (2) add supportsNativeSyntax in the target, and implement the error checks in the CFE. We should then probably also add a noNativeSyntaxError in the target so that the error message can be something different for dart2js if that ever deprecates native syntax. cc @johnniwinther @jensjoha @chloestefantsova