1220702 - Another round of Reflect.parse tune-ups

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

In reviving <https://github.com/jorendorff/reflect-stringify>, I have found a bunch of minor bugs in Reflect.parse, mostly places where we return identical JSON for two programs that do not behave the same.

Comment 1

[Jason Orendorff [:jorendorff]]

Attachment: Part 1: Replace callback() and newNode() with variadic templates. What could go wrong?

Comment 2

[Jason Orendorff [:jorendorff]]

Attachment: Part 2: Fix the .method property of certain FunctionDeclaration nodes

Comment 3

[Jason Orendorff [:jorendorff]]

Attachment: Part 3: Distinguish ES6 generators from legacy generators in Reflect.parse() output

Comment 4

[Jason Orendorff [:jorendorff]]

That's it for now. Not so shabby!

Comment 5

[Jeff Walden [:Waldo]]

Comment on attachment 8682730 [details] [diff] [review]
Part 1: Replace callback() and newNode() with variadic templates. What could go wrong?

Review of attachment 8682730 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/builtin/ReflectParse.cpp
@@ +301,5 @@
> +    bool callbackHelper(HandleValue fun, Value* begin, Value* p, TokenPos* pos,
> +                        MutableHandleValue dst)
> +    {
> +        // The end of the implementation of callback(). All arguments except
> +        // loc have already been stored in the half-open range `begin..p`.

"in the range [begin, p)."

I will be shocked (shocked, I say) if hazard builds can divine their way through this enough to recognize there are no hazards here.

@@ +320,5 @@
> +                        HandleValue head, Arguments&&... tail)
> +    {
> +        // Recursive loop to store the arguments in the array. This eventually
> +        // bottoms out in a call to the non-template callbackHelper() above.
> +        *p++ = head;

It really feels to me like there's a way to do this without recursion, that would be clearer.  I don't really like recursion for what are basically purely-iterative things, as this is.  I think this alternative works, but its clarity is...unclear:

  auto ignored { (*p++ = Forward<Arguments>(tail))... };
  (void) ignored;

(Expanding a pack into a braced initializer list *does* evaluate in left-to-right order, interestingly enough -- C++11 [dcl.init.list]p4.)

@@ +384,5 @@
> +        // call below passes two fewer arguments than we received, as we omit
> +        // `name` and `value`. This eventually bottoms out in a call to the
> +        // non-template newNodeHelper() above.
> +        return defineProperty(obj, name, value)
> +               && newNodeHelper(obj, Forward<Arguments>(rest)...);

&& at end of previous line.  Tho, I'd also sort of go for |if (!defineProperty(...)) / return false;| too.

@@ +392,5 @@
> +    // or more properties passed in as arguments. The signature is really more
> +    // like:
> +    //
> +    //     bool newNode(ASTType type, TokenPos* pos,
> +    //                  {const char *name0, HandleValue value0,}...

s/const char */const char* / here, and above.

Feh, so many of these C strings are already in CommonPropertyNames, or could be added, to avoid all this atomization work.  Another bug.

@@ +628,2 @@
>  {
>      MOZ_ASSERT(type > AST_ERROR && type < AST_LIMIT);

MOZ_ASSERT(AST_ERROR < type && type < AST_LIMIT) for readability, IMO.

Comment 6

[Jason Orendorff [:jorendorff]]

Attachment: Part 1: Replace callback() and newNode() with variadic templates. What could go wrong?

Comment 7

[Jason Orendorff [:jorendorff]]

The attached patch should be more obviously GC-correct. I'll also attempt using `...` instead of recursion; don't have time at the moment.

Comment 8

[Jason Orendorff [:jorendorff]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5580050d54bdb1d447929f27e2c4b817f7b3fe69
Bug 1220702 - Part 1: Replace callback() and newNode() with variadic templates. What could go wrong? r=Waldo.

https://hg.mozilla.org/integration/mozilla-inbound/rev/a0d07644f5696d31ca8137f8060451197621013e
Bug 1220702 - Part 2: Fix the .method property of certain FunctionDeclaration nodes. r=Waldo.

https://hg.mozilla.org/integration/mozilla-inbound/rev/603f88cf0a06d0fe8fbabab5ea8c99f922b9d214
Bug 1220702 - Part 3: Distinguish ES6 generators from legacy generators in Reflect.parse() output. r=Waldo.

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5580050d54bd
https://hg.mozilla.org/mozilla-central/rev/a0d07644f569
https://hg.mozilla.org/mozilla-central/rev/603f88cf0a06