Closed
Bug 106244
Opened 23 years ago
Closed 18 years ago
In JS strict mode, |if (a = b && c == d)| compiles as |if (a == (b && c == d))|
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: neil, Unassigned)
References
Details
(Keywords: js1.5, regression, Whiteboard: JS Strict Mode)
Attachments
(3 files, 3 obsolete files)
5.08 KB,
text/html
|
Details | |
4.92 KB,
patch
|
shaver
:
review+
jband_mozilla
:
superreview+
|
Details | Diff | Splinter Review |
1.64 KB,
text/html
|
Details |
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•23 years ago
|
||
So what should have happened?
Reporter | ||
Comment 2•23 years ago
|
||
I'm not sure. I just wanted to point out that "correcting" = to == isn't simple.
Comment 3•23 years ago
|
||
Well, the js compiler already gave a js strict warning. Was the warning not
clear enough?
Reporter | ||
Comment 4•23 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•23 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•23 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•23 years ago
|
||
Comment 8•23 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
Closed: 23 years ago
Resolution: --- → INVALID
Comment 9•23 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•23 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•23 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•23 years ago
|
||
Mozilla Build ID: 2001101103
Comment 13•23 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•23 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•23 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•23 years ago
|
||
Reassigning to Kenton; cc'ing Brendan in case this is of interest -
Assignee: rogerl → khanson
Status: REOPENED → NEW
Comment 17•23 years ago
|
||
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•23 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=
Comment 19•23 years ago
|
||
Comment 20•23 years ago
|
||
You know I want to rip this out -- I've been telling you so since 1997. =)
Comment 21•23 years ago
|
||
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•23 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+
Comment 23•23 years ago
|
||
Tested already, fix is in now.
/be
Status: NEW → RESOLVED
Closed: 23 years ago → 23 years ago
Resolution: --- → FIXED
Comment 24•23 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).
Comment 25•23 years ago
|
||
Urk, you're right -- thanks, Phil. Simpler patch coming up.
/be
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 26•23 years ago
|
||
Attachment #55845 -
Attachment is obsolete: true
Comment 27•23 years ago
|
||
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
Comment 28•23 years ago
|
||
Comment 29•23 years ago
|
||
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•23 years ago
|
||
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•23 years ago
|
||
Comment on attachment 57046 [details] [diff] [review]
better fix, diff -wu coming up for reviewers
sr=jband
Attachment #57046 -
Flags: superreview+
Comment 32•23 years ago
|
||
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 33•23 years ago
|
||
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•23 years ago
|
||
Comment on attachment 57196 [details] [diff] [review]
best fix, diff -wu format
sr=jband
Attachment #57196 -
Flags: superreview+
Comment 36•23 years ago
|
||
a=blizzard on behalf of drivers for 0.9.6
Keywords: mozilla0.9.6 → mozilla0.9.6+
Comment 37•23 years ago
|
||
Has this been checked into the 0.9.6 branch yet?
Comment 38•23 years ago
|
||
Sorry, I was out of town Monday and Tuesday. I'll do it now.
/be
Comment 39•23 years ago
|
||
checked into the branch -- waiting for trunk to open.
/be
Status: REOPENED → ASSIGNED
Comment 40•23 years ago
|
||
Fix is in trunk now.
/be
Status: ASSIGNED → RESOLVED
Closed: 23 years ago → 23 years ago
Resolution: --- → FIXED
Comment 41•23 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•23 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•23 years ago
|
||
Comment 44•23 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•23 years ago
|
||
Marking Verified. These test results are just what one would expect
from Brendan's patch in Comment #32.
Status: RESOLVED → VERIFIED
Updated•20 years ago
|
Flags: testcase?
Comment 46•19 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•19 years ago
|
Assignee: khanson → general
Status: REOPENED → NEW
QA Contact: pschwartau → general
Target Milestone: mozilla0.9.6 → ---
Updated•18 years ago
|
Flags: blocking1.9?
Comment 47•18 years ago
|
||
Igor ripped out all the pre-ECMA code, so this is FIXED.
/be
Status: NEW → RESOLVED
Closed: 23 years ago → 18 years ago
Resolution: --- → FIXED
Updated•18 years ago
|
Flags: blocking1.9?
Comment 48•18 years ago
|
||
there is still no warning.
Comment 49•18 years ago
|
||
(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•18 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.
Description
•