823310 - don't emit strict warnings in strict mode code

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

This didn't reproduce for me until I closed and reopened Nightly. Then it reproduced fine.

1. Visit http://codemirror.net/demo/marker.html
2. Open Web console.
3. See SyntaxErrors (but with the "warning" icon, not the "error" icon).

http://cl.ly/image/030p1o0w451w

Expected: no strict warnings at all, since javascript.options.strict is set to false in a clean profile. (Note that options.strict is a completely separate thing from "use strict". They check different things. "use strict" is new and standard, and it never generates warnings.)

But even if javascript.options.strict is enabled, these shouldn't appear as "SyntaxError"s in the console. They're just warnings, not errors.

Comment 1

[Panos Astithas (he/him) [:past] (please ni?)]

I see it in Nightly, but not in 17.0.1. Could this be related to bug 821103?

Comment 2

[Mihai Sucan [:msucan]]

Panos: interesting find. I tried a backout of that bug. I still get those errors as SyntaxError warnings.

Jason: thank you for the bug report. This doesn't seem to be a web console bug. Those messages are displayed both as warnings and SyntaxErrors in the Error Console as well. This is what we get from the nsIScriptErrors coming from the JS engine. Error toString says it's a syntax error and the message flag says it's a warning. :/

I suggest we move this bug to Core :: JS Engine and CC more people.

Comment 3

[Jason Orendorff [:jorendorff]]

Hmm. I think there are two bugs here.

1. Warnings shouldn't have "SyntaxError" in the UI. I guess this bug is in XPConnect somewhere; nsIScriptError doesn't come from the JS engine, not directly anyway.

2. "use strict" should not cause us to emit strict warnings. This is definitely a js engine bug. You can see it in the shell:

  js> options()
  "methodjit,typeinfer"
  js> "use strict"; if (x = 3) print('ok!');
  typein:1:0 strict warning: test for equality (==) mistyped as assignment (=)?:
  typein:1:0 strict warning: "use strict"; if (x = 3) print('ok!');
  typein:1:0 strict warning: .......................^
  typein:1:12 ReferenceError: assignment to undeclared variable x

We're not supposed to emit strict warnings unless JSOPTION_STRICT is set.
Benjamin Peterson may have regressed this:

  <benjamin> oh, really?
  <benjamin> well then, it's my fault

Comment 4

[:Benjamin Peterson]

The reason they appear as SyntaxError is that's the error type they're given in js.msg.

Comment 5

[:Benjamin Peterson]

Attachment: don't give strict warnings in strict mode

I apparently have never known what a strict warning was. I was thought it was just a warning that appeared in strict mode, but it's actually just a old SM linting mode, which is not related to ES5 strict mode. So, we should only emit them when cx->hasStrictOption() is true and not in ES5 strict mode.

Comment 6

[Nicholas Nethercote [inactive]]

Comment on attachment 694647 [details] [diff] [review]
don't give strict warnings in strict mode

Review of attachment 694647 [details] [diff] [review]:
-----------------------------------------------------------------

> I apparently have never known what a strict warning was.

Goodness.  Now you understand why I wanted you to use |strictMode| as the name throughout, instead of just |strict|.

Just for clarity, here's my understanding of how strict mode and strict warnings interact:

- Strict mode results in *errors*, which halt execution.  It's a standard ES5 thing.

- JSOPTION_STRICT results in *warnings*, which don't halt execution (unless |werror| is specified).  It's a SpiderMonkey-only thing.

- The things that JSOPTION_STRICT complains about include everything that strict mode complains about, plus a couple more.

I think that covers it.

::: js/src/jit-test/tests/basic/bug823310.js
@@ +1,4 @@
> +"use strict";
> +options("werror");
> +var x;
> +eval("if (x = 3) {}");

I'm confused enough now that I don't know (a) what behaviour we're currently getting on this, and (b) what behaviour we're supposed to get.

Is the assignment an example of a complaint we get with JSOPTION_STRICT but not in strict mode, and therefore the correct behaviour is to not get a warning/error?

Comment 7

[:Benjamin Peterson]

(In reply to Nicholas Nethercote [:njn] from comment #6)
> Comment on attachment 694647 [details] [diff] [review]
> don't give strict warnings in strict mode
> 
> Review of attachment 694647 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> > I apparently have never known what a strict warning was.
> 
> Goodness.  Now you understand why I wanted you to use |strictMode| as the
> name throughout, instead of just |strict|.

Indeed. :( After this, I'm going to file a bug to rename JSOPTION_STRICT to something without "strict" in it (like JSOPTION_LINT).

> 
> Just for clarity, here's my understanding of how strict mode and strict
> warnings interact:
> 
> - Strict mode results in *errors*, which halt execution.  It's a standard
> ES5 thing.
> 
> - JSOPTION_STRICT results in *warnings*, which don't halt execution (unless
> |werror| is specified).  It's a SpiderMonkey-only thing.
> 
> - The things that JSOPTION_STRICT complains about include everything that
> strict mode complains about, plus a couple more.
> 
> I think that covers it.

Hopefully, Jason can confirm.

> 
> ::: js/src/jit-test/tests/basic/bug823310.js
> @@ +1,4 @@
> > +"use strict";
> > +options("werror");
> > +var x;
> > +eval("if (x = 3) {}");
> 
> I'm confused enough now that I don't know (a) what behaviour we're currently
> getting on this, and (b) what behaviour we're supposed to get.
> 
> Is the assignment an example of a complaint we get with JSOPTION_STRICT but
> not in strict mode, and therefore the correct behaviour is to not get a
> warning/error?

That's exactly it. The code the test evals causes the "test for equality (==) mistyped as assignment (=)?" strict warning. Currently, being in strict mode causes the warning to happen, which with |options("werror")| will cause the test to fail. With the patch, the correctly correctly gives no warning (or error).

Comment 8

[Nicholas Nethercote [inactive]]

> > Is the assignment an example of a complaint we get with JSOPTION_STRICT but
> > not in strict mode, and therefore the correct behaviour is to not get a
> > warning/error?
> 
> That's exactly it.

Ok.  Can you add a comment to that effect?  Thanks.

Comment 9

[Jason Orendorff [:jorendorff]]

(In reply to Nicholas Nethercote [:njn] from comment #6)
> - Strict mode results in *errors*, which halt execution.  It's a standard
> ES5 thing.

Strict mode also affects semantics, particularly around 'this' and direct eval.
  https://people.mozilla.com/~jorendorff/es5.1-final.html#sec-C

> - JSOPTION_STRICT results in *warnings*, which don't halt execution (unless
> |werror| is specified).  It's a SpiderMonkey-only thing.

Right.

> - The things that JSOPTION_STRICT complains about include everything that
> strict mode complains about, plus a couple more.

There's overlap but ES5 strict mode prevents some things (with statements, .callee, possibly more) that JSOPTION_STRICT doesn't bother with.

Comment 10

[Jason Orendorff [:jorendorff]]

Comment on attachment 694647 [details] [diff] [review]
don't give strict warnings in strict mode

Review of attachment 694647 [details] [diff] [review]:
-----------------------------------------------------------------

extraz reviewz

Comment 11

[Jason Orendorff [:jorendorff]]

("JSOPTION_LINT" sounds delightful. Unfortunately there is also a thing called jslint, but at least that is outside our codebase! (also, it should be dying off, since the jshint fork.))

Comment 12

[:Benjamin Peterson]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5fddad57aa05

Comment 13

[Nicholas Nethercote [inactive]]

I'd dearly love to entirely remove JSOPTION_STRICT.  I asked about it a while back on the jsapi mailing list but Wes Garland said "I like it" and nobody else responded...

Comment 14

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/5fddad57aa05

Comment 15

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Nicholas Nethercote [:njn] from comment #13)
> I'd dearly love to entirely remove JSOPTION_STRICT.  I asked about it a
> while back on the jsapi mailing list but Wes Garland said "I like it" and
> nobody else responded...

Please don't. Now that I'm doing more work with the frontend, it actually is really useful. It generates warnings on accesses to properties that don't exist. Normal strict mode doesn't do this.