Implement Reflect built-in (ES6+) #1025

fatcerberus · 2016-10-18T06:23:27Z

This pull implements the built-in Reflect object, first introduced in Ecmascript 2015 (ES6). Concrete implementation based on the latest Ecmascript standard at the time of writing (2016/ES7).

Ecmascript 2016 reference:
http://www.ecma-international.org/ecma-262/7.0/#sec-reflect-object

New Ecmascript calls introduced:

Reflect.apply()
Reflect.construct()
Reflect.defineProperty()
Reflect.deleteProperty()
Reflect.get()
Reflect.getOwnPropertyDescriptor()
Reflect.getPrototypeOf()
Reflect.has()
Reflect.isExtensible()
Reflect.ownKeys()
Reflect.preventExtensions()
Reflect.set()
Reflect.setPrototypeOf()

Reflect.enumerate() is NOT implemented. It was dropped in ES7 for placing too many constraints on Ecmascript implementors, see this GitHub issue:
tc39/ecma262#161

Checklist:

Implement Reflect, all functions (use ES7 as reference)
Testcases for all Reflect functions
Disable Reflect built-in for default lowmem build (baseline is ES5)
Website notes for Reflect
RELEASES entry

fatcerberus · 2016-10-18T07:31:18Z

The specification seems to require that Reflect.apply() tailcalls the target function so that apply() won't appear in a stack trace. I don't believe this is currently possible; I guess it's not a big deal if we don't implement that here.

Incidentally, the same requirement applies (no pun intended) to Function.prototype.apply in ES6.

svaarala · 2016-10-18T11:11:28Z

Nice, Reflect should be pretty easy indeed and provides a few very useful things (like Reflect.construct()).

For the most part things should be implementable by extending existing functions with a flag or two. For example, Object.defineProperty() and Reflect.defineProperty() differ in their throwing behavior. This can be implemented using a flag or maybe a safe call wrapper.

It might be good to implement the bindings in several pulls: some things are obvious while others may need some internal shuffling to minimize overlap between helpers etc.

svaarala · 2016-10-18T11:14:09Z

Does the Reflect object need its own class number? Quickly looking at ES6 I didn't see a @@toStringTag for it, or special handling in Object.prototype.toString but I might have missed it.

Node.js does this which matches my assumption:

> Object.prototype.toString.call(Reflect)
'[object Object]'

svaarala · 2016-10-18T11:14:51Z

In any case once Symbol support has been merged to master, I'd like to move all the non-required class numbers into @@toStringTags because the class number space is almost exhausted.

It's also not very modular, and cannot be used by user code (unlike @@toStringTag).

svaarala · 2016-10-18T11:17:37Z

So as far as flags are enough to distinguish a Reflect vs. Object function, they should use the same Duktape/C function internally, with a magic value providing the necessary flag(s).

fatcerberus · 2016-10-18T13:12:53Z

I'll remove the class number, that's fine. I mostly did it to match Math, which also doesn't really need a class number I think (although pre-ES3 had very weird semantics for things so I very well could be wrong there). Also I didn't realize you were implementing well-known symbols in the Symbol pull, that'll be nice.

As for using the same Duktape/C function, yes, that's the intent. In fact I already did that here - just that those functions don't look at the magic value yet so the semantics are wrong.

svaarala · 2016-10-18T13:15:55Z

Math at least used to need a class number because:

duk> Object.prototype.toString.call(Math)
= "[object Math]"

Object.prototype.toString() is the one and only place where classes matter (as far as outward appearances).

fatcerberus · 2016-10-18T13:16:05Z

One interesting thing I notice is that ES6 includes Reflect.enumerate() which was removed in ES7. It seems that no engines ever actually implemented it so I think we can safely leave it out too. :)

svaarala · 2016-10-18T13:20:51Z

ES6 updates behavior for Object.prototype.toString() a bit: http://www.ecma-international.org/ecma-262/6.0/#sec-object.prototype.tostring.

Special handling is required for: undefined, null, Array, String, Arguments, Function, Error, Boolean, Number, Date, and RegExp. Other classes should use @@toStringTag (the mentioned classes must not) and fall back to [object Object].

So, Math and JSON, for example, should be changed to use a @@toStringTag, and ES6 specifies these explicitly:

This should be fixed for Duktape, but doing so depends on the Symbol feature to be completed :)

What I could do to make the process a bit easier is to merge the genbuiltins.py Symbol syntax so that @@toStringTags can be declared in the metadata - even if the properties won't be effective yet in master.

svaarala · 2016-10-18T13:25:07Z

That ES6 change to Object.prototype.toString() has one important consequence: the call may have arbitrary side effects because @@toStringTag might be a getter.

So for internals, there's probably a need for two variants: one with side effects, and one guaranteed to have no side effects (and providing "incorrect" result for a getter - which is not likely to be encountered in practice). The former is needed for e.g. Object.prototype.toString() while the latter is (or may be) useful for e.g. summary strings which need to be side effect free.

fatcerberus · 2016-10-18T14:07:33Z

Trivia: Should the config option be DUK_USE_REFLECT_BUILTIN or DUK_USE_ES6_REFLECT? As Duktape gets more and more ES6 features it feels somewhat awkward to keep calling them out explicitly as such, especially when it's now possible to pick and choose even ES5 builtins.

svaarala · 2016-10-18T14:12:35Z

I'm fine with DUK_USE_REFLECT_BUILTIN without the ES6 part. It'd be nice to do a once-over for the config options and make it consistent in this respect - but that's mostly cosmetic.

In current convention DUK_USE_REFLECT_BUILTIN would specifically refer to whether the bindings are present or not. Sometimes a feature has built-in bindings but also some impact which may need a separate define.

For example, DUK_USE_JSON_SUPPORT controls whether JSON is supported at all, including the C API, duk_push_context_dump(), etc, while DUK_USE_JSON_BUILTIN indicates whether the JSON object is present. Sometimes the two are the same thing, and the DUK_USE_xxx_BUILTIN is then fine.

fatcerberus · 2016-10-18T15:23:54Z

ES6 retroactively changes the semantics of several equivalent Object methods to coerce the target to an object if it is not one. In ES5, e.g. Object.getOwnPropertyDescriptor() was specified to throw if the target was not an object (like the Reflect method now does). Should the Object methods be changed to match the ES6 coercion semantics?

fatcerberus · 2016-10-18T15:51:27Z

@svaarala As a reminder, if this is merged before Symbol support makes it to master, then Reflect.ownKeys() will need to be updated to also return Symbol keys.

fatcerberus · 2016-10-18T17:01:05Z

It's quite convenient that most of the Reflect methods are very simple and specified in terms of algorithms already defined since ES5. That should help keep footprint impact down.

svaarala · 2016-10-18T18:50:01Z

Should the Object methods be changed to match the ES6 coercion semantics?

Interesting question which hasn't really come up before. One solution would be to provide both semantics depending on which version is being configured for. That would mean necessary config options to configure for ES5.1 vs. "latest", or something.

In practice that is probably not worth the effort. Ecmascript is a living standard and tracking the latest behavior would make most sense to me. The ES5 profile build could then just use the ES5 built-ins (which is really for footprint control in most cases), with post-ES5 semantics for the built-ins.

fatcerberus · 2016-10-19T02:45:49Z

Ecmascript is a living standard

Thank goodness for that. 😄 The wait for ES6 was excruciating, so it's good to see that it's being updated more often now. Although it does give us as implementors more work to keep up with the latest changes... 😉

fatcerberus · 2016-10-19T03:38:57Z

Reflect.apply() was able to share its entire implementation with Function.prototype.apply(). Conveniently the internal valstack policy for the latter is an exact match with the argument order of the former. ~~As a result the diff is very minimal: b93fdd7~~ Now supports Reflect.construct() as well, so the changes were a bit more invasive (still pretty straightforward though). ;)

fatcerberus · 2016-10-19T04:54:45Z

So Reflect.construct() was also able to share a Duktape/C function with Function.prototype.apply(). So now all the Reflect methods are implemented, I just have to clean up the FIXMEs, write some testcases, etc. and this should be good to go before you know it. :)

fatcerberus · 2016-10-19T14:59:39Z

I want to try to write testcases for all Reflect methods, so that there will be decent test coverage for this feature in master right from the start. I kind of slacked off in #975 in that the tests weren't really comprehensive. I'll try to do a better job this time. Writing tests isn't nearly as fun as programming new features. ;)

@svaarala Quick question: Reflect.construct() takes an optional third argument which affects the value of new.target. Duktape doesn't currently support new.target, but is there any other place that value is exposed? If not, I can skip adding support for the third argument for now.

svaarala · 2016-10-19T18:42:34Z

Hmm, can't think off hand where it would matter now. Maybe leave unimplemented and now and document it in a Reflect built-in laundry list issue?

fatcerberus · 2016-10-20T11:56:56Z

Regarding Reflect.get/set() and #1027: I'd be fine with merging this without support for a receiver, but in that case it has to be very well documented that the optional last argument is not supported, because it completely changes the meaning of any call to get() or set() that includes it. Accessors will get the wrong this value, and redirection of property writes will not work at all which will definitely surprise any calling code that relies on it.

svaarala · 2016-10-20T12:00:00Z

Hmm, how about checking the third argument and if it's present and different from the object, TypeError with "unsupported"?

svaarala · 2016-10-20T12:00:46Z

I do agree that going forward with a very different operation doesn't pass the "principle of least surprise" test. And further, if you get your code working with that limitation and the limitation later gets fixed, the code will start behaving another way.

fatcerberus · 2016-10-20T12:07:08Z

That TypeError solution would be okay with me. We definitely can't just ignore the argument entirely unless we want to see some very confused JS programmers. ;)

svaarala · 2016-10-20T12:07:50Z

Principle of least surprise, yes :)

fatcerberus · 2016-10-20T13:18:44Z

I think I'll open a separate pull for the ES6 Object function coercion changes.

svaarala · 2016-10-24T23:16:11Z

ES6 specifies call/apply as tailcalls. Implementing that would give you yielding automatically I think. :)

That's true, but there's no simple way to "implement as tailcalls" though, that's just specification language. The .call() and .apply() functions will need to integrate directly with call setup to ensure the end result behaves like a tailcall but there's no internal support for Duktape/C tailcalls at present (and even if there was, it wouldn't necessarily handle .call() and .apply() correctly).

fatcerberus · 2016-10-24T23:22:16Z

I didn't mean to imply it would be a simple change - just pointing out that if a possible a more general solution like #1026 would be preferable to special casing for call/apply. I like generic designs :)

I realize sometimes a generic solution isn't always feasible though.

svaarala · 2016-10-24T23:53:25Z

Ah, you meant that. I agree it'd be great if the internals could use a duk_tailcall() C API without special handling. Let's see if that would be possible - hopefully so :-)

Yield/resume are a similar story but they do something completely different so they probably won't benefit from any tailcall handling rework.

fatcerberus · 2016-10-25T01:19:58Z

Hm, it seems Reflect.construct() third argument has some relevance to ES5. For example it affects where the prototype for the initial this is taken from:
http://www.ecma-international.org/ecma-262/7.0/#sec-ecmascript-function-objects-construct-argumentslist-newtarget

For now it'd be best I think to update the implementation to throw an error if newTarget !== target, like how the receiver argument is handled in get/set.

svaarala · 2016-10-25T01:29:36Z

I agree - you wouldn't be able to make that kind of constructor call in ES5 besides Reflect so it's not a big limitation.

fatcerberus · 2016-10-25T05:47:52Z

Almost done writing tests. Just need some testcases for Reflect.apply() and Reflect.construct() and this should be almost good to go.

@svaarala What are "fltoetc" compile checks? A few of them keep failing but I can't see out what I did wrong.

svaarala · 2016-10-25T09:49:28Z

They're just different build tests, fltoetc is shorthand for using -flto and other footprint optimizing link time options.

One example error printout is:

/tmp/ccYgsJPk.ltrans9.ltrans.o:(.rodata+0x48): undefined reference to `duk_bi_function_prototype_apply.5947.2861'
/tmp/ccYgsJPk.ltrans9.ltrans.o:(.rodata+0x4c): undefined reference to `duk_bi_object_constructor_define_property.5948.2862'
/tmp/ccYgsJPk.ltrans9.ltrans.o:(.rodata+0x50): undefined reference to `duk_bi_object_constructor_get_own_property_descriptor.5949.2863'
/tmp/ccYgsJPk.ltrans9.ltrans.o:(.rodata+0x54): undefined reference to `duk_bi_object_constructor_is_extensible.5950.2864'
/tmp/ccYgsJPk.ltrans9.ltrans.o:(.rodata+0x58): undefined reference to `duk_bi_object_constructor_keys_shared.5951.2865'
/tmp/ccYgsJPk.ltrans9.ltrans.o:(.rodata+0x5c): undefined reference to `duk_bi_object_constructor_prevent_extensions.5952.2866'
/tmp/ccYgsJPk.ltrans9.ltrans.o:(.rodata+0x60): undefined reference to `duk_bi_object_getprototype_shared.5953.2867'

What happens is that the stripped config options remove the Object built-in but keeps Reflect built-in (it's not disabled in config/examples/low_memory_strip.yaml) which causes Reflect to reference built-in functions no longer present.

The shared built-in functions would need to be:

#if defined(DUK_USE_OBJECT_BUILTIN) || defined(DUK_USE_REFLECT_BUILTIN)

instead of just:

#if defined(DUK_USE_OBJECT_BUILTIN)

fatcerberus · 2016-10-25T14:28:55Z

A useful followup work item might be to move the shared Duktape/C helpers for better organization, e.g. into duk_bi_shared.c.

fatcerberus · 2016-10-25T15:17:17Z

For whatever reason Git doesn't seem to have normalized the line endings for the .html files when I committed them, and the ones in my local repo are CRLF so the diff showed the entire file being changed. I fixed it. One of the dangers of doing primary development on Windows I suppose ;)

@svaarala This change is nearly done now; just waiting on the webhook tests to see if the stripped builds have been fixed. I'll give this one final going-over tonight, but feel free to add review notes now if you want.

svaarala · 2016-10-25T15:33:06Z

A duk_bi_shared.c would be rather wide in scope but maybe duk_bi_object_reflect.c could provide the two built-ins because they're so directly related.

svaarala · 2016-10-25T16:05:30Z

Ok, will do 👍

svaarala

A few memory unsafe assumptions (value stack duk_tval pointer stability). Otherwise looking good 👍.

svaarala · 2016-10-25T22:55:28Z

src-input/duk_bi_reflect.c

+
+	(void) duk_require_hobject(ctx, 0);
+	tv_obj = duk_require_tval(ctx, 0);
+	(void) duk_to_string(ctx, 1);


This duk_to_string() call may have side effects that invalidate the tv_obj above.

duk_strict_equals() is side effect free, but it'd be best to read the tval pointers just before they're used.

Agree, the code also reads much cleaner that way (better separation of concerns).

svaarala · 2016-10-25T22:57:16Z

src-input/duk_bi_reflect.c

+		DUK_ERROR_UNSUPPORTED((duk_hthread *) ctx);
+	}
+
+	(void) duk_hobject_getprop(thr, tv_obj, tv_key);


How about avoiding the tv_obj and tv_key pointers entirely by just doing:

duk_set_top(ctx, 2); /* -> [ obj key ] (target, if present, popped off) */ duk_get_prop(ctx); return 1;

svaarala · 2016-10-25T22:58:15Z

src-input/duk_bi_reflect.c

+		DUK_ERROR_UNSUPPORTED((duk_hthread *) ctx);
+	}
+
+	ret = duk_hobject_putprop(thr, tv_obj, tv_key, tv_val, 0 /*throw_flag*/);


Same duk_tval stability issue here as above. Also same question here, how about duk_set_top(ctx, 3) + duk_put_prop() + return 1;?

Note the 0 /*throw_flag*/. The public API calls use strict mode semantics; here I instead need it to return false when it would otherwise throw (for example due to a read-only property). Initial drafts of this pull used duk_{get|put}_prop() but the semantics were wrong - if I were to make that change here, the tests would break.

There is of course another option: A safe call. But that seems like it'd be even messier than the duk_tval pointers.

The concrete problem with duk_put_prop() is this bit:

If the property write fails, throws an error (strict mode semantics).

Reflect.set() returns false in that case, but doesn't throw. However errors thrown by setters, etc. will still propagate as usual, so wrapping the access in a safe call won't work either.

svaarala · 2016-10-25T23:01:29Z

tests/ecmascript/test-bi-reflect-apply-construct.js

+}
+
+try {
+    applyTest();


For coverage it'd be good to have an apply test for near-maximum number of supported arguments (say 250). This ensures that the implementation's duk_require_stack() calls are right (they are, but to ensure they keep on being right).

Good point, I'll add something to exercise that. I was honestly not sure what to do with the apply and construct tests; once I tested the basic functionality there didn't seem to be too many corner cases to worry about.

svaarala · 2016-10-25T23:02:14Z

tests/ecmascript/test-bi-reflect-apply-construct.js

+    maggie.talk();
+    var darkling = Reflect.construct(Person, [ "Darkling" ]);
+    print(darkling instanceof Person);
+    darkling.talk();


Same argument count comment for .construct().

svaarala · 2016-10-25T23:08:06Z

I have some very small bits here and there which I can fix in a separate pull (takes longer to write them out than to try how they work / affect footprint :-). The memory unsafe bits need fixing, and there's a few testcase coverage issues which would be nice to add. Other than that I can merge whenever you think the pull is done. :)

All Reflect functions specified in ECMAScript 2016 are implemented, and most share Duktape/C helpers with their ES5 twins from Object. * Reflect.apply() * Reflect.construct() * Reflect.defineProperty() * Reflect.deleteProperty() * Reflect.get() * Reflect.getOwnPropertyDescriptor() * Reflect.getPrototypeOf() * Reflect.has() * Reflect.isExtensible() * Reflect.ownKeys() * Reflect.preventExtensions() * Reflect.set() * Reflect.setPrototypeOf() Presence of the Reflect object is controlled by DUK_USE_REFLECT_BUILTIN and enabled by default in the standard configuration. note: Reflect.enumerate() was retroactively removed in ES7, so it will not be implemented in Duktape.

Low-memory baseline is E5.1.

fatcerberus · 2016-10-26T04:41:31Z

@svaarala Assuming I haven't broken the tests, this is ready to merge.

I reorganized the duk_tval retrieval, makes for cleaner-looking code :) As mentioned in the review comments (which are now hidden), we need to call the internal duk_hobject_*() helpers directly for at least put and delete because Reflect needs "lax mode" semantics + success flag whereas the public API mimics strict mode.

Reflect.get() and Reflect.has() could probably use the public API without issue, but for consistency it seemed best to use the hobject helpers throughout.

Testcases cover all functions. There is additionally an argument policy test which checks that all functions will throw for a non-object argument.

fatcerberus · 2016-10-26T06:47:23Z

The updated testcase for Reflect.apply() successfully passes 9001 (!) arguments to a function. This seems like a useful way to sidestep the executor limit of 255 arguments, maybe something to note in supplemental documentation (wiki)?.

svaarala · 2016-10-26T10:08:47Z

The commits are now outdated - re: the putprop throw flag, you're right, I didn't think about that.

For internal code what would be useful is an extended duk_put_prop() which takes flags, and a throw flag could be one of them. #979 adds a flag to property operations in general so that would then be relatively straightforward.

So, OK as it was, with the exception of safe usage of duk_tval * pointers.

svaarala · 2016-10-26T10:13:26Z

The updated testcase for Reflect.apply() successfully passes 9001 (!) arguments to a function.

Oh right, the compiler has a limit for formal arguments but otherwise there can be more arguments.

That bytecode limitation of 255 arguments is probably going away next time I touch call opcodes. There will be some higher limit most likely though.

svaarala · 2016-10-26T10:35:37Z

Looks good now :)

fatcerberus · 2016-10-26T13:27:12Z

I may have had a bit too much fun writing the .apply()/.construct() test... :)

svaarala · 2016-10-26T13:31:47Z

Hehe, well I don't mind as long as the test are useful.

svaarala added the es2015 label Oct 18, 2016

fatcerberus changed the title ~~Implement ES6 'Reflect' built-in~~ Implement built-in Reflect object (ES6+) Oct 19, 2016

fatcerberus mentioned this pull request Oct 20, 2016

Add internal support for [[Get]]/[[Set]] receiver #1027

Closed

3 tasks

svaarala requested changes Oct 25, 2016

View reviewed changes

fatcerberus added 2 commits October 26, 2016 00:18

Config: Disable ES6 Reflect in lowmem build

b7c8436

Low-memory baseline is E5.1.

fatcerberus added 3 commits October 26, 2016 02:15

Testcases for ES6 Reflect built-in

bd83e70

Testcases cover all functions. There is additionally an argument policy test which checks that all functions will throw for a non-object argument.

Website notes: ES6 Reflect

d7e5a8e

Releases: ES6 Reflect

a9d6366

fatcerberus changed the title ~~Implement built-in Reflect object (ES6+)~~ Implement Reflect built-in (ES6+) Oct 26, 2016

svaarala approved these changes Oct 26, 2016

View reviewed changes

svaarala merged commit 4cfd390 into svaarala:master Oct 26, 2016

svaarala added this to the v2.0.0 milestone Oct 26, 2016

fatcerberus deleted the es6-reflect-builtin branch October 27, 2016 03:28

Implement Reflect built-in (ES6+) #1025

Implement Reflect built-in (ES6+) #1025

Conversation

fatcerberus commented Oct 18, 2016 • edited Loading

fatcerberus commented Oct 18, 2016

svaarala commented Oct 18, 2016

svaarala commented Oct 18, 2016

svaarala commented Oct 18, 2016 • edited Loading

svaarala commented Oct 18, 2016

fatcerberus commented Oct 18, 2016

svaarala commented Oct 18, 2016

fatcerberus commented Oct 18, 2016

svaarala commented Oct 18, 2016

svaarala commented Oct 18, 2016 • edited Loading

fatcerberus commented Oct 18, 2016

svaarala commented Oct 18, 2016 • edited Loading

fatcerberus commented Oct 18, 2016 • edited Loading

fatcerberus commented Oct 18, 2016

fatcerberus commented Oct 18, 2016

svaarala commented Oct 18, 2016

fatcerberus commented Oct 19, 2016

fatcerberus commented Oct 19, 2016 • edited Loading

fatcerberus commented Oct 19, 2016

fatcerberus commented Oct 19, 2016

svaarala commented Oct 19, 2016

fatcerberus commented Oct 20, 2016

svaarala commented Oct 20, 2016

svaarala commented Oct 20, 2016

fatcerberus commented Oct 20, 2016

svaarala commented Oct 20, 2016

fatcerberus commented Oct 20, 2016

svaarala commented Oct 24, 2016

fatcerberus commented Oct 24, 2016

svaarala commented Oct 24, 2016

fatcerberus commented Oct 25, 2016

svaarala commented Oct 25, 2016

fatcerberus commented Oct 25, 2016

svaarala commented Oct 25, 2016

fatcerberus commented Oct 25, 2016

fatcerberus commented Oct 25, 2016

svaarala commented Oct 25, 2016

svaarala commented Oct 25, 2016

svaarala left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fatcerberus Oct 26, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

svaarala commented Oct 25, 2016

fatcerberus commented Oct 26, 2016

fatcerberus commented Oct 26, 2016

svaarala commented Oct 26, 2016

svaarala commented Oct 26, 2016 • edited Loading

svaarala commented Oct 26, 2016

fatcerberus commented Oct 26, 2016

svaarala commented Oct 26, 2016

fatcerberus commented Oct 18, 2016 •

edited

Loading

svaarala commented Oct 18, 2016 •

edited

Loading

svaarala commented Oct 18, 2016 •

edited

Loading

svaarala commented Oct 18, 2016 •

edited

Loading

fatcerberus commented Oct 18, 2016 •

edited

Loading

fatcerberus commented Oct 19, 2016 •

edited

Loading

fatcerberus Oct 26, 2016 •

edited

Loading

svaarala commented Oct 26, 2016 •

edited

Loading