106244 - In JS strict mode, |if (a = b && c == d)| compiles as |if (a == (b && c == d))|

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[[e-mail]]

The JavaScript engine likes to think that
  if (a = b)
was mistyped as
  if (a == b)
which it usually is. However I had mistyped
  if (a == b && c == d)
as
  if (a = b && c == d)
which (because of precedence of =) compiled (after JS warning) as
  if (a == (b && c == d))
If I hadn't had JS warnings on I would have been really confused.

Comment 1

[jag (Peter Annema)]

So what should have happened?

Comment 2

[[e-mail]]

I'm not sure. I just wanted to point out that "correcting" = to == isn't simple.

Comment 3

[jag (Peter Annema)]

Well, the js compiler already gave a js strict warning. Was the warning not
clear enough?

Comment 4

[[e-mail]]

No, because its assumed == still has the precedence of = which causes problems
in complex conditional statements. If you are going to assume == I would
appreciate it to have the precedence of ==.

Comment 5

[jag (Peter Annema)]

Oh! I see what you mean now.

I kept misreading this:

  if (a == (b && c == d))

as this:

  if (a = (b && c == d))

and was wondering what was wrong.

Now, I've run a little test in xpcshell, and this is what I get:

js> var a = 0, b = 1, c = 2, d = 2;
js> if (a = b && c == d) print("yes"); else print("no");
typein:2: strict warning: test for equality (==) mistyped as assignment (=)?:
typein:2: strict warning: if (a = b && c == d) print("yes"); else print("no");

typein:2: strict warning: ...................^
yes
js> a
true

As you can see ("yes" was printed, and |a| now is |true|, not |0|), this code
was executed like this:

if (a = (b && c == d)) print("yes"); else print("no");

I would be surprised if the js compiler tried "correcting" your code to use |==|
instead of |=|, but it's not impossible. Can you attach a simple test case that
shows this bug, so we can fix it, or explain why it's doing what it's doing?

Comment 6

[[e-mail]]

The JS console appears to try to correct = to ==. Try this:

var a = false, b = true, c = false, d = true;
if (a = b && c == d) alert('true'); else alert('false');

JS console alerts true, showing it has compared a to the result of b && c == d.
Both
if (a == b && c == d) alert('true'); else alert('false');
and
if ((a = b) && c == d) alert('true'); else alert('false');
alert false.

Comment 7

[Phil Schwartau]

Attachment: HTML testcase, in JavaScript versions 1.1 through current

Comment 8

[Phil Schwartau]

SUMMARY: 

The bug claims that expressions like (a = b && c == d)
are being coerced to                 (a == (b && c == d))


This seems to have been true in JS versions 1.1, 1.2.
The testcase shows, however, that this does NOT happen in
JS versions 1.3 and greater. When you run the testcase, 
notice the warnings in the JS Console for versions 1.1 and 1.2:

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


The warnings from JS1.3 and greater, however, say only this:

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


The tetcase results also show that the expression was not coerced.
Therefore marking this bug Invalid. As far as I can see, this is
another one of those quirks of JS1.2 and earlier -

Comment 9

[Phil Schwartau]

Marking Verified -

[e-mail]: thank you for this report; it's a tricky one.
If for some reason your results on this testcase differ from mine,
please reopen this bug. I was using a Mozilla trunk binary 2001101603.

Comment 10

[[e-mail]]

Strange. Using my current profile all the expressions evaluate to true.
But when I created a new profile the problem went away. Any ideas?

Comment 11

[Phil Schwartau]

Yes, I cannot understand that. When I try the testcase with a 
Mozilla trunk build 2001101603, the output is:

[JS1.1, JS1.2]
true
true

[JS1.3, JS1.4, JS1.5, JS]
false
true


What build of Mozilla or N6 are you using? 

Comment 12

[[e-mail]]

Mozilla Build ID: 2001101103

Comment 13

[Phil Schwartau]

Perhaps a new bug should be filed against Profile Manager BackEnd - ?
Or should we say, "All's well that ends well" ? I've never heard of a
different Profile affecting the outcome of a pure JS function...

Comment 14

[[e-mail]]

Reopening because shaver tells me this is because I have strict mode on.

Comment 15

[Phil Schwartau]

You are right! If (mozilla binary path)/bin/defaults/pref/all.js has

              pref("javascript.options.strict", false);


then the above testcase gives the results I have reported:

[JS1.1, JS1.2]
true
true

[JS1.3, JS1.4, JS1.5, JS]
false
true


But if I change to pref("javascript.options.strict", true); 
the results are:

[JS1.1, JS1.2JS1.3, JS1.4, JS1.5, JS]
true
true


Thus in JS strict mode, the reporter is correct: all versions of JS
compile

                     if (a = b && c == d)
as                   if (a == (b && c == d))


The warnings in the JS Console for ALL versions now say:

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


In non-strict mode, as I have reported above, this only happens 
in JS1.2 or less.

Comment 16

[Phil Schwartau]

Reassigning to Kenton; cc'ing Brendan in case this is of interest -

Comment 17

[Brendan Eich [:brendan]]

Yes, strict trumps version here.  Maybe we should make this a quirk-only
(version check for 1.2 or lower) "feature" and wash our hands of it.  I kept it
as a strict warning too, and didn't think about the problem further.  But Neil
rightly points out that we want to reparse the expression as if the = were ==
and then check that the root of the parse tree is still that same == (original
=) op.  Or some such tedious waste of code footprint.

I'm for removing this as a strict warning.  Who's with me?

/be

Comment 18

[Kenton Hanson (gone)]

I agree.  Rewriting code is not a good idea.  If the warning goes unnoticed and
the logic gets changed we are not helping the user.

=Kenton=

Comment 19

[Brendan Eich [:brendan]]

Attachment: proposed fix, please r= and sr= and I'll check in.

Comment 20

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

You know I want to rip this out -- I've been telling you so since 1997. =)

Comment 21

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 55845 [details] [diff] [review]
proposed fix, please r= and sr= and I'll check in.

sr=shaver.  BEGONE!

Comment 22

[John Bandhauer]

Comment on attachment 55845 [details] [diff] [review]
proposed fix, please r= and sr= and I'll check in.

r=jband
Looks right to me.
I trust you like tested it and stuff.

Comment 23

[Brendan Eich [:brendan]]

Tested already, fix is in now.

/be

Comment 24

[Phil Schwartau]

Using Mozilla trunk binary 20011105xx on WinNT. The fix has done this:
when I load the HTML testcase, the results are the same whether I am 
in strict mode or non-strict mode:

------------------  OUTPUT FROM TESTCASE  -------------------
[JS1.1, JS1.2]
true
true

[JS1.3, JS1.4, JS1.5, JS]
false
true


------------------  JS CONSOLE MESSAGES   -------------------
[JS1.1, JS1.2]
Warning: test for equality (==) mistyped as assignment (=)?
Assuming equality test

[JS 1.3 and greater]
[NO WARNING; NOTHING]


Are we sure this is a good idea? Don't we still want to provide the
WARNING, without affecting the output? This is what non-strict mode did
in JS1.3+ before this fix went in (see Additional Comment #8 above).

Comment 25

[Brendan Eich [:brendan]]

Urk, you're right -- thanks, Phil.  Simpler patch coming up.

/be

Comment 26

[Brendan Eich [:brendan]]

Attachment: better fix, diff -wu coming up for reviewers

Comment 27

[Brendan Eich [:brendan]]

Would like to repair this for 0.9.6 -- status quo ante and all that.

/be

Comment 28

[Brendan Eich [:brendan]]

Attachment: diff -wu of better fix

Comment 29

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

I think the warning sucks, since you can't avoid it with the usual extra-parens
trick.  But if you insist, I'll grant an r.

Comment 30

[Brendan Eich [:brendan]]

shaver: sure you can -- extra parens ensure that pn->pn_type != TOK_ASSIGN
(it'll equal TOK_RP, for a parenthesized expression, aka JSOP_GROUP).  That
suppresses the warning.

/be

Comment 31

[John Bandhauer]

Comment on attachment 57046 [details] [diff] [review]
better fix, diff -wu coming up for reviewers

sr=jband

Comment 32

[Brendan Eich [:brendan]]

Attachment: best fix, diff -wu format

Give the warning only if the condition's root expression is a plain assignment
(=), and its right operand has greater precedence than ==.  Note that operator
token types are now ordered from least precedence to greatest.

/be

Comment 33

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 57196 [details] [diff] [review]
best fix, diff -wu format

I dig it.  Thanks for the clue.

r=shaver

Comment 34

[John Bandhauer]

Comment on attachment 57196 [details] [diff] [review]
best fix, diff -wu format

sr=jband

Comment 35

[Brendan Eich [:brendan]]

Hoping asa will a= for 0.9.6.

/be

Comment 36

[Christopher Blizzard (:blizzard)]

a=blizzard on behalf of drivers for 0.9.6

Comment 37

[Christopher Blizzard (:blizzard)]

Has this been checked into the 0.9.6 branch yet?

Comment 38

[Brendan Eich [:brendan]]

Sorry, I was out of town Monday and Tuesday.  I'll do it now.

/be

Comment 39

[Brendan Eich [:brendan]]

checked into the branch -- waiting for trunk to open.

/be

Comment 40

[Brendan Eich [:brendan]]

Fix is in trunk now.

/be

Comment 41

[Phil Schwartau]

Using Mozilla trunk binaries 20011120xx on WinNT, Linux, Mac 9.1
Using 0.9.6 branch binary 20011120xx on WinNT.

When I load the HTML testcase, the results are the same whether
I am in strict mode or non-strict mode, for ALL versions of JS: 


------------------  OUTPUT FROM TESTCASE  -------------------
[JS1.1, JS1.2, JS1.3, JS1.4, JS1.5]
false
true


------------------  JS CONSOLE MESSAGES   -------------------
[JS1.1, JS1.2, JS1.3, JS1.4, JS1.5]
[NO WARNING; NOTHING]

Comment 42

[Phil Schwartau]

This shows no coercion took place on |if (a = b && c == d)| 
in any version of JS, and no warning was given.

To test what happens on the simpler assignment |if (a = b)|, 
I will attach an HTML testcase below -

Comment 43

[Phil Schwartau]

Attachment: HTML testcase for the simple assignment |if (a = b)|

Comment 44

[Phil Schwartau]

The results of the simple assignment test are as follows:


------------------  OUTPUT FROM TESTCASE  -------------------
[JS1.1, JS1.2]
false

[JS 1.3 and greater]
true


------------------  JS CONSOLE MESSAGES   -------------------
[JS1.1, JS1.2]
Warning: test for equality (==) mistyped as assignment (=)?
Assuming equality test

[JS 1.3 and greater]
Warning: test for equality (==) mistyped as assignment (=)?

Comment 45

[Phil Schwartau]

Marking Verified. These test results are just what one would expect
from Brendan's patch in Comment #32.

Comment 46

[Bob Clary [:bc] (inactive)]

/cvsroot/mozilla/js/tests/js1_5/Regress/regress-106244.js,v  <--  regress-106244.js
initial revision: 1.1

this regressed at least in Mozilla 1.4/winxp.

Comment 47

[Brendan Eich [:brendan]]

Igor ripped out all the pre-ECMA code, so this is FIXED.

/be

Comment 48

[Bob Clary [:bc] (inactive)]

there is still no warning.

Comment 49

[Brendan Eich [:brendan]]

(In reply to comment #48)
> there is still no warning.

The summary and comment 0 do not complain about lack of warning, so that's not this bug.  Since Igor in fact ripped out the warning along with the pre-ECMA "error correction" code, you shouldn't see a warning.  Can we declare victory, or do you want a GCC-ish strict warning for an assignment in a condition?  If so, new bug depending on the pre-ECMA version cleanup bug.

/be

Comment 50

[Bob Clary [:bc] (inactive)]

(In reply to comment #49)
that's fine. i'll add the test to spidermonkey-n.tests. i just want it out of my logs. ;-)