61129 - JS strict option warns about AND rewrites if(a=b) to if(a==b)

Status: VERIFIED WONTFIX

(Core :: JavaScript Engine, defect, P3)

Description

[Juergen Saar]

The combination of setting a value of a variable an "if" does not work:

if (var1 = testFunction(arg1) ) {
	// only if testFunction does not return false
}

Comment 1

[Matthew Wilson]

Attachment: Test case

Comment 2

[Matthew Wilson]

When running an executable compiled from source with DEBUG=1, I had the 
following on the Javascript console:

Warning: test for equality (==) mistyped as assignment (=?) Assuming equality 
test

I note that jsparse.c also says

    /*
     * Check for (a = b) and "correct" it to (a == b).
     * XXX not ECMA, but documented in several books -- need a compile option
     */

Comment 3

[Phil Schwartau]

The "=" sign is an assignment operator. The return value of an assignment
is the value of the assignment. 


Example: the expression (x = 3) assigns the value 3 to x, and returns 3. 
Therefore if you do this:

if (x = 3)
{
  print('Hello');
}
    

you will get an output ('Hello'). The expression (x=3) evaluates to 3,
and 3 evaluates to true in the if-clause. 


But if you do this:

if (x = 0)
{
  print('Hello');
}


you will get no output: the return value of (x = 0) is 0, which evaluates
to false in the if-clause.



The comparison operator is "==", not "=".  So if you do this: 


if (x == 3)
{
  print('Hello');
}


you will really be testing the value of x. You will only get output if 
x is equal to 3 (or can be converted to 3). 


Also note there is an even stricter comparison operator, "===".



Have to mark this bug as invalid, since JavaScript is behaving as expected.
Thank you for using Bugzilla!

Comment 4

[Phil Schwartau]

Marking Verified -

Comment 5

[Matthew Wilson]

I would like to re-open this for consideration (except that I don't have
sufficient privileges).

The problem is *not* that "=" is the assignment operator - that would be correct
behaviour as you say - it is that sometimes it is *converted* to an equality
test by the JS engine.

This does not seem to happen on downloaded nightly builds, only on executables
compiled from source with DEBUG=1. This seems to put the JS engine into strict mode.

Please see again the warning I reported from the JS console, and the comment
from the code, both saying that they have seen an assignment operator "=" which
will be treated as an equality test "==".

I do not believe the engine should be attempting to alter valid JS code to
something with different semantics.

Comment 6

[Phil Schwartau]

Reopening for the consideration of the programmers - the reporter is asking, 
"Should the JS Engine proactively convert code written as :

                          if (x = 3)
                          {
                            print('Hello');
                          }


to:

                          if (x == 3)
                          {
                            print('Hello');
                          }


?"



Here is the function from jsparse.c referred to above:



static JSParseNode *
Condition(JSContext *cx, JSTokenStream *ts, JSTreeContext *tc)
{
    JSParseNode *pn, *pn2;

    MUST_MATCH_TOKEN(TOK_LP, JSMSG_PAREN_BEFORE_COND);
    pn = Expr(cx, ts, tc);
    if (!pn)
        return NULL;
    MUST_MATCH_TOKEN(TOK_RP, JSMSG_PAREN_AFTER_COND);

    /*
     * Check for (a = b) and "correct" it to (a == b).
     * XXX not ECMA, but documented in several books -- need a compile option
     */
    if (pn->pn_type == TOK_ASSIGN && pn->pn_op == JSOP_NOP) {
        JSBool rewrite = JS_HAS_STRICT_OPTION(cx) ||
                         !JSVERSION_IS_ECMA(cx->version);
        if (!js_ReportCompileErrorNumber(cx, ts,
                                         JSREPORT_WARNING | JSREPORT_STRICT,
                                         JSMSG_EQUAL_AS_ASSIGN,
                                         rewrite
                                         ? "\nAssuming equality test"
                                         : "")) {
            return NULL;
        }
        if (rewrite) {
            pn->pn_type = TOK_EQOP;
            pn->pn_op = (JSOp)cx->jsop_eq;
            pn2 = pn->pn_left;
            switch (pn2->pn_op) {
              case JSOP_SETNAME:
                pn2->pn_op = JSOP_NAME;
                break;
              case JSOP_SETPROP:
                pn2->pn_op = JSOP_GETPROP;
                break;
              case JSOP_SETELEM:
                pn2->pn_op = JSOP_GETELEM;
                break;
              default:
                JS_ASSERT(0);
            }
        }
    }
    return pn;
}



cc'ing Brendan -

Comment 7

[Brendan Eich [:brendan]]

We do this only if the strict option is enabled (that's done by default for
DEBUG Mozilla builds), controlled by the javascript.options.strict boolean pref,
or if a non-ECMA version of JS has been selected explicitly (because we have
always rewritten such expressions in pre-ECMA JS).

The strict warning is analogous to one given by gcc for the same, easily misread
condition.  It's trying to nag you into parenthesizing, which suppresses this
strict warning.  Revising subject, marking WONTFIX.

/be

Comment 8

[Phil Schwartau]

Marking verified. Juergen and Matthew, thank you for the report -

Comment 9

[Brendan Eich [:brendan]]

Yet more accurate subject.  The analogy with gcc breaks down in that JS's strict
option both enables a warning and the rewrite from = to ==, while gcc merely
gives a warning.  But both JS and gcc leave you alone if you parenthesize the
assignment expression, which is the better style here anyway.

/be