715359 - Harmony is/isnt operators

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine, defect)

Comment 1

[Tom S [:evilpie]]

Attachment: WIP

This mostly works! Yaay. I am not sure about the parsing side, but it seems to work. jsop.tbl wants a precedence, but I have no idea how to come up with that number, for the moment I just coped JSOP_EQ.

Comment 2

[Tom S [:evilpie]]

Okay, I just made some test, and for example this:

  (a == 
    0) is 1;

Doesn't work, but from my understanding it should. I hope, I don't need to do some deeper change to the parser, would appreciate some pointers :)

Comment 3

[Brendan Eich [:brendan]]

That while loop's body logic looks wrong. You want if ... else if ... else break.

When checking line numbers, beware bogus pn_pos.end values (latent bugs). Check parenthesized expression parsing. What values do you see in GDB for the two lines?

/be

Comment 4

[Tom S [:evilpie]]

Attachment: WIP v1

Yep Brendan you are right. Unless there is some other way to do this, we need to fix the position for every token. In some simple cases this already works.

Comment 5

[Brendan Eich [:brendan]]

Suggest using const ref to consolidate tokenStream.currentToken() repetition.

No paren-expression end fix?

/be

Comment 6

[Brendan Eich [:brendan]]

Most  nodes have correct end cords -- we should test and fix the ones that do not. No fear.

/be

Comment 7

[Tom S [:evilpie]]

>No paren-expression end fix?
What?

>Most  nodes have correct end cords -- we should test and fix the ones that do not. No fear.
Good idea, indeed.

Comment 8

[Brendan Eich [:brendan]]

(In reply to Tom Schuster (evilpie) from comment #7)
> >No paren-expression end fix?
> What?

The end coord for a parenthesized expression used to be correct. The needed assignment was inadvertently lost by the cset landed for bug 461269 (cc'ing jorendorff). Here's a patch:

diff --git a/js/src/frontend/Parser.cpp b/js/src/frontend/Parser.cpp
--- a/js/src/frontend/Parser.cpp
+++ b/js/src/frontend/Parser.cpp
@@ -7154,16 +7154,17 @@ Parser::primaryExpr(TokenKind tt, JSBool
         JSBool genexp;
 
         pn = parenExpr(&genexp);
         if (!pn)
             return NULL;
         pn->setInParens(true);
         if (!genexp)
             MUST_MATCH_TOKEN(TOK_RP, JSMSG_PAREN_IN_PAREN);
+        pn->pn_pos.end = tokenStream.currentToken().pos.end;
         break;
       }
 
       case TOK_STRING:
         pn = atomNode(PNK_STRING, JSOP_STRING);
         if (!pn)
             return NULL;
         break;

Ugh, JSBool! Feel free to make a less minimal fix :-).

/be

Comment 9

[Tom S [:evilpie]]

Attachment: v1

So, I introduced a slight change in behavior, but only the warning messages for generators with no closing ), in a switch, with etc. changes, so not really important.

Comment 10

[Tom S [:evilpie]]

Attachment: v1.1

Sorry made some small mistake, and confused begin and end!

Comment 11

[Brendan Eich [:brendan]]

Why are you pushing out the isGenerator parameter so a number (three?) of call sites of parenExpr need to declare and pass an utterly useless outparam?

Note also C++ mutable ref out params are bad style.

Really, this patch should be more minimal. Now there are two assignments to pn_pos.end depending on whether the paren-expr is a genexp, and widely separated at that. Yuck.

/be

Comment 12

[Tom S [:evilpie]]

Attachment: v2

Fixed that, have the review as you obviously care about this.

Comment 13

[Tom S [:evilpie]]

Try run doesn't look very promising
https://tbpl.mozilla.org/?tree=Try&rev=ac069f589e9d

Okay something that I expected, but didn't really think of any actual testcase.
I thought that the case where, I currently throw always throws.
Some of other test, do the following:

>a++
>is(b);

We try to parse this as [a++] is (b);

To fix this wee need to rewrite to:
>if (TokenIsHarmonyEquality(tokenStream.currentToken(), context) &&
>    left->pn_pos.end.lineno == tokenStream.currentToken().pos.begin.lineno) {

Comment 14

[Tom S [:evilpie]]

Attachment: v3

Comment 15

[Brendan Eich [:brendan]]

I can't parse comment 13. Tom, what needs to happen to move this forward?

/be

Comment 16

[Tom S [:evilpie]]

This should actually just be ready now.

Comment 17

[Jason Orendorff [:jorendorff]]

I apologize for not getting to this review this week. First thing Monday.

Comment 18

[Jason Orendorff [:jorendorff]]

Comment on attachment 588905 [details] [diff] [review]
v3

Lovely patch.

Let's hold off on landing this. I have concerns about this language feature. I'm discussing them with Dave Herman and Brendan.

>+bool
>+TokenIsHarmonyEquality(const Token &token, JSContext *cx)
>+{
>+    if (token.type != TOK_NAME)
>+        return false;
>+    if (token.name() == cx->runtime->atomState.isAtom)
>+        return true;
>+    if (token.name() == cx->runtime->atomState.isntAtom)
>+        return true;
>+    return false;
>+}

This looks like a one-liner to me:

    return token.type == TOK_NAME &&
           (token.name() == cx->runtime->atomState.isAtom ||
            token.name() == cx->runtime->atomState.isntAtom);

> END_CASE(JSOP_NE)
> 
>+
>+
> #undef EQUALITY_OP

The style rule in js/src is, at most one blank line at a time. Please remove the extras.

>+#define SAME_VALUE_OP(OP, COND)                                             \
>+    JS_BEGIN_MACRO                                                          \
>+        const Value &rref = regs.sp[-1];                                    \
>+        const Value &lref = regs.sp[-2];                                    \
>+        JSBool equal;                                                       \
>+        if (!SameValue(cx, lref, rref, &equal))                             \
>+            goto error;                                                     \
>+        COND = equal OP JS_TRUE;                                            \
>+        regs.sp--;                                                          \
>+    JS_END_MACRO
>+
>+BEGIN_CASE(JSOP_IS)
>+{
>+    bool cond;
>+    SAME_VALUE_OP(==, cond);
>+    regs.sp[-1].setBoolean(cond);
>+}
>+END_CASE(JSOP_IS)

Macros are kind of yucky, and comparing a JSBool to JS_TRUE with == is definitely yucky. How about:

BEGIN_CASE(JSOP_IS)
{
    JSBool equal;
    if (!SameValue(cx, regs.sp[-1], regs.sp[-2], &equal))
        goto error;
    regs.sp--;
    regs.sp[-1].setBoolean(equal);
}
END_CASE(JSOP_IS)

And the same for JSOP_ISNT; the code duplication is only two lines more without the ten-line macro than with it, so it seems clear the macro isn't carrying its weight.

Please don't make a test directory named "harmony"; "es6" or else "is-isnt" would be fine. (I set aside a whole directory for "for-of"

>+assertEq(0 is 0, true);
>+assertEq(0 is -0, false);
>+assertEq(-0 is 0, false);

Add the fourth corner of the square:
  assertEq(-0 is -0, true);


>+function throws(code, expected) {

You can
  load(extdir + "asserts.js");
  assertThrowsInstanceOf(function () { eval("0\n is 1"); }, SyntaxError);

Please add a test or two that checks the operator precedence.

This also needs a few Reflect.parse tests.

And decompiler tests.

We may also want a pref so that we can disable this feature; let me ask around and see what we're doing about this.

Comment 19

[Tom S [:evilpie]]

I doubt this will ever happen.

Comment 20

[Brendan Eich [:brendan]]

TC39 needs to decide, but sentiment was against adding these operators. Object.is (or Object.sameValue -- better name?) should happen, though. It's "just API" and can be polyfilled. Is that this bug or another one?

/be

Comment 21

[David Bruant]

Closing based on meeting notes [1]
bug 839979 is the new place to be

[1] https://github.com/rwldrn/tc39-notes/blob/master/es6/2013-01/jan-29.md#41-isisnt-operators