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

RESOLVED FIXED

Status

()

--
trivial
RESOLVED FIXED
17 years ago
5 years ago

People

(Reporter: neil, Unassigned)

Tracking

({js1.5, regression})

Trunk
x86
Windows 95
js1.5, regression
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: JS Strict Mode)

Attachments

(3 attachments, 3 obsolete attachments)

(Reporter)

Description

17 years ago
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

17 years ago
So what should have happened?
(Reporter)

Comment 2

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

Comment 3

17 years ago
Well, the js compiler already gave a js strict warning. Was the warning not
clear enough?
(Reporter)

Comment 4

17 years ago
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

17 years ago
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?
(Reporter)

Comment 6

17 years ago
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

17 years ago
Created attachment 54895 [details]
HTML testcase, in JavaScript versions 1.1 through current

Comment 8

17 years ago
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 -
Status: NEW → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → INVALID

Comment 9

17 years ago
Marking Verified -

neil@parkwaycc.co.uk: 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.
Status: RESOLVED → VERIFIED
(Reporter)

Comment 10

17 years ago
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

17 years ago
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? 
(Reporter)

Comment 12

17 years ago
Mozilla Build ID: 2001101103

Comment 13

17 years ago
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...
(Reporter)

Comment 14

17 years ago
Reopening because shaver tells me this is because I have strict mode on.
Status: VERIFIED → REOPENED
Resolution: INVALID → ---
Whiteboard: JS Strict Mode

Comment 15

17 years ago
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.
Summary: if (a = b && c == d) compiles as if (a == (b && c == d)) → In JS strict mode, |if (a = b && c == d)| compiles as |if (a == (b && c == d))|

Comment 16

17 years ago
Reassigning to Kenton; cc'ing Brendan in case this is of interest -
Assignee: rogerl → khanson
Status: REOPENED → NEW
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

17 years ago
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=
Created attachment 55845 [details] [diff] [review]
proposed fix, please r= and sr= and I'll check in.
You know I want to rip this out -- I've been telling you so since 1997. =)
Comment on attachment 55845 [details] [diff] [review]
proposed fix, please r= and sr= and I'll check in.

sr=shaver.  BEGONE!
Attachment #55845 - Flags: superreview+

Comment 22

17 years ago
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.
Attachment #55845 - Flags: review+
Tested already, fix is in now.

/be
Status: NEW → RESOLVED
Last Resolved: 17 years ago17 years ago
Resolution: --- → FIXED

Comment 24

17 years ago
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).
Urk, you're right -- thanks, Phil.  Simpler patch coming up.

/be
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Created attachment 57046 [details] [diff] [review]
better fix, diff -wu coming up for reviewers
Attachment #55845 - Attachment is obsolete: true
Would like to repair this for 0.9.6 -- status quo ante and all that.

/be
Keywords: js1.5, mozilla0.9.6
Target Milestone: --- → mozilla0.9.6
Created attachment 57047 [details] [diff] [review]
diff -wu of better fix
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.
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

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

sr=jband
Attachment #57046 - Flags: superreview+
Created attachment 57196 [details] [diff] [review]
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
Attachment #57046 - Attachment is obsolete: true
Attachment #57047 - Attachment is obsolete: true
Comment on attachment 57196 [details] [diff] [review]
best fix, diff -wu format

I dig it.  Thanks for the clue.

r=shaver
Attachment #57196 - Flags: review+

Comment 34

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

sr=jband
Attachment #57196 - Flags: superreview+
Hoping asa will a= for 0.9.6.

/be
Keywords: regression
a=blizzard on behalf of drivers for 0.9.6
Keywords: mozilla0.9.6 → mozilla0.9.6+
Has this been checked into the 0.9.6 branch yet?
Sorry, I was out of town Monday and Tuesday.  I'll do it now.

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

/be
Status: REOPENED → ASSIGNED
Fix is in trunk now.

/be
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago17 years ago
Resolution: --- → FIXED

Comment 41

17 years ago
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

17 years ago
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

17 years ago
Created attachment 58640 [details]
HTML testcase for the simple assignment |if (a = b)|

Comment 44

17 years ago
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

17 years ago
Marking Verified. These test results are just what one would expect
from Brendan's patch in Comment #32.
Status: RESOLVED → VERIFIED

Updated

13 years ago
Flags: testcase?

Comment 46

13 years ago
/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.
Status: VERIFIED → REOPENED
Flags: testcase? → testcase+
Resolution: FIXED → ---

Updated

13 years ago
Assignee: khanson → general
Status: REOPENED → NEW
QA Contact: pschwartau → general
Target Milestone: mozilla0.9.6 → ---

Updated

12 years ago
Flags: blocking1.9?
Igor ripped out all the pre-ECMA code, so this is FIXED.

/be
Status: NEW → RESOLVED
Last Resolved: 17 years ago12 years ago
Resolution: --- → FIXED

Updated

12 years ago
Flags: blocking1.9?

Comment 48

12 years ago
there is still no warning.
(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

12 years ago
(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. ;-)
You need to log in before you can comment on or make changes to this bug.