Is anyone still using JS strict warnings?

[Jason Orendorff]

So if you go to about:config and set the javascript.options.strict pref,
you'll get warnings about accessing undefined properties.

js> Math.TAU
undefined
/!\ ReferenceError: reference to undefined property Math.TAU

(It says "ReferenceError", but your code still runs normally; it really is
just a warning.)

Is anyone using this? Bug 1113380 points out that the rules about what kind
of code can cause a warning are a little weird (on purpose, I think). Maybe
it's time to retire this feature.

https://bugzilla.mozilla.org/show_bug.cgi?id=1113380

Please speak up now, if you're still using it!

-j

[J. Ryan Stinnett]

Some prior discussion of this feature happened in the platform thread
"Disabling
strict warnings as errors in xpcshell"[1].

A few people argued for the extra warnings to be removed, while one person
said they were useful.

No clear conclusion was reached.

[1]:
https://groups.google.com/d/topic/mozilla.dev.platform/znIkVsh5YYA/discussion

On Fri, Dec 19, 2014 at 2:19 PM, Jason Orendorff <joren...@mozilla.com>
wrote:

> _______________________________________________
> dev-platform mailing list
> dev-pl...@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform
>

[William McCloskey]

I was the person in the previous thread who found them useful, and I still
do. Some of the extraWarnings stuff is of questionable value, but the
undefined property stuff is really useful.

I don't really know if other people use this stuff. extraWarnings are
enabled by default in debug builds, and I think a lot of people don't
realize the difference between these "extra warnings" and normal JS errors.

Writing front-end JS code for Firefox is generally a painful experience:
terrible error messages, often no filename or line number information, or
even no output at all. But I think we should be making things better, not
worse. Remove features like this takes us in the wrong direction. If the
undefined property warning is broken, we should fix it.

-Bill

[Chris Peterson]

On 12/19/14 12:45 PM, William McCloskey wrote:
> I don't really know if other people use this stuff. extraWarnings are
> enabled by default in debug builds, and I think a lot of people don't
> realize the difference between these "extra warnings" and normal JS errors.
>
> Writing front-end JS code for Firefox is generally a painful experience:
> terrible error messages, often no filename or line number information, or
> even no output at all. But I think we should be making things better, not
> worse. Remove features like this takes us in the wrong direction. If the
> undefined property warning is broken, we should fix it.

I don't know if Firefox front-end developers typically use debug builds,
but I see many warnings in front-end JS when I run debug builds. If
extraWarnings are useful, they could be enabled for Nightly release
builds (specifically to aid Firefox developers) instead of just debug
builds.

[Nick Fitzgerald]

I generally don't find them useful, but instead annoying, and that they
provide a lot of noise to filter out to find actual relevant errors. This
is including the undefined property errors. It is a common JS style to pass
around configuration/option objects that will be missing many properties
that get accessed by functions they are passed to. My understanding is that
part of this is special cased not to generate these messages, but in my
experience it isn't close to enough.

At minimum, we should mark them all JSEXN_NONE in js.msg so that they don't
appear as misleading ReferrenceError or whatever other kind of error.

​We also shouldn't call them "strict" errors, since they have nothing to do
with strict mode.​


Additionally, whether these messages show up depends on how the JS is
loaded. I haven't looked at the specifics in a while, but IIRC it depended
upon whether you were using JSMs or Cu.evalInSandbox, etc. We have tests in
devtools that fail if there are any errors in the console, and so changing
the way we load JS sources with no other changes to the code, results in
tests breaking for perfectly good code. It can be *very* frustrating.

I'm like the idea of providing optional JS Hint style linting with these
messages, but it isn't really optional right now, and even if you don't
want these messages, they are still forced upon you.

Personally, I'm A-OK with retiring these messages, but it seems at least
some people do get value from them. Maybe this should be a flag?
--enable-extra-js-warnings? A pref?

Just not enabled by default, please...

[Gijs Kruitbosch]

On 19/12/2014 20:45, William McCloskey wrote:
> I was the person in the previous thread who found them useful, and I still
> do. Some of the extraWarnings stuff is of questionable value, but the
> undefined property stuff is really useful.

Can you give an example of a useful undefined property warning? Because
my experience is the same as fitzgen's in that they are basically never
useful to me.

> Writing front-end JS code for Firefox is generally a painful experience:
> terrible error messages, often no filename or line number information, or
> even no output at all. But I think we should be making things better, not
> worse.

Agreed that we should be improving this, but I think we differ in that I
don't think this change makes things worse - in fact, removing these is
better, IMO, in that real errors now don't get hidden inbetween warnings
pretending to be errors.

We should be fixing the filename/line number info (fwiw, that part is
unrecognizable to me in the general sense - have you filed bugs?), and
parse errors generating no errors at all (I think there are bugs on file
for this), but keeping these warnings won't help us do that.

~ Gijs

[Jim Porter]

On 12/19/2014 02:19 PM, Jason Orendorff wrote:
> So if you go to about:config and set the javascript.options.strict pref,
> you'll get warnings about accessing undefined properties.
>

> Please speak up now, if you're still using it!

I find these warnings quite useful (granted, I'm the sort of person who
compiles C++ with `-Wall -Wextra -Werror -pedantic`), and I've often
wished that I could make these errors show up at "compile"/lint time in
Firefox OS.

In particular, I really like seeing warnings about accessing undefined
properties when I'm refactoring code or changing an API. It's easy to
forget to change a single instance of a property when refactoring, and
having a warning at least gets me partway to being able to verify that I
did things correctly. (Tests help too, of course, but even a good test
suite probably won't catch *every* possible bug.)

- Jim

[David Rajchenbach-Teller]

I am going to suggest, once again, that warnings generally noise and
should be replaced by actionable errors, at least when the code is
executed in a test suite.

See
https://groups.google.com/forum/#!topic/mozilla.dev.platform/gqSIOc5b-BI

Cheers,
David

On 19/12/14 21:19, Jason Orendorff wrote:
> So if you go to about:config and set the javascript.options.strict pref,
> you'll get warnings about accessing undefined properties.
>

> js> Math.TAU
> undefined
> /!\ ReferenceError: reference to undefined property Math.TAU
>
> (It says "ReferenceError", but your code still runs normally; it really is
> just a warning.)
>
> Is anyone using this? Bug 1113380 points out that the rules about what kind
> of code can cause a warning are a little weird (on purpose, I think). Maybe
> it's time to retire this feature.
>
> https://bugzilla.mozilla.org/show_bug.cgi?id=1113380
>

> Please speak up now, if you're still using it!
>

> -j
> _______________________________________________
> dev-platform mailing list
> dev-pl...@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform
>

--
David Rajchenbach-Teller, PhD
Performance Team, Mozilla

[William McCloskey]

On Fri, Dec 19, 2014 at 2:34 PM, Gijs Kruitbosch <gijskru...@gmail.com>
wrote:

>
> Can you give an example of a useful undefined property warning? Because my
> experience is the same as fitzgen's in that they are basically never useful
> to me.

I can't cite any bugzilla bugs, no. They're more useful while still
developing patches. Say you have a counter that's a property of your object
and you accidentally write "this.conter++". You'll end up with the property
set to NaN, which probably won't show up until some later time. Without
warnings, you need to stick print statements all over the place to figure
out where the problem is. The warning tells you exactly where to look.

Writing front-end JS code for Firefox is generally a painful experience:
>> terrible error messages, often no filename or line number information, or
>> even no output at all. But I think we should be making things better, not
>> worse.
>>
>
> Agreed that we should be improving this, but I think we differ in that I
> don't think this change makes things worse - in fact, removing these is
> better, IMO, in that real errors now don't get hidden inbetween warnings
> pretending to be errors.
>

I'm definitely sympathetic to that point of view. I run with MOZ_QUIET and
MOZ_IGNORE_WARNINGS for this reason. It seems fine to me to have these
warnings be optional. I suspect that people who value static types are the
ones who like these warnings, and everybody else doesn't.

> We should be fixing the filename/line number info (fwiw, that part is
> unrecognizable to me in the general sense - have you filed bugs?)

Bug 1067942 is the worst example of that that I'm aware of right now. I try
to file these problems as I see them.

-Bill

[Jim Blandy]

The bug is surprising, in that it claims that the bytecode that consumes
the value determines whether a warning is issued (SETLOCAL;CALL), rather
than the bytecode doing the fetch.

Is that the intended behavior? I can't see how that makes much sense.
On Dec 19, 2014 2:55 PM, "David Rajchenbach-Teller" <dte...@mozilla.com>
wrote:

[Jason Orendorff]

On Fri, Dec 19, 2014 at 5:13 PM, Jim Blandy <ji...@red-bean.com> wrote:

> The bug is surprising, in that it claims that the bytecode that consumes
> the value determines whether a warning is issued (SETLOCAL;CALL), rather
> than the bytecode doing the fetch.
>
> Is that the intended behavior? I can't see how that makes much sense.
>

That's intentional, yes. The idea is that phrases like these:

if (obj.prop === undefined) // ok

if (obj.prop && obj.prop.prop2) // ok

shouldn't cause warnings. These are "detecting" uses: obj.prop is fetched
for the purpose of determining whether or not it exists.

But to distinguish "detecting" uses from others, we have to look at the
bytecode that consumes the value. All this is one reason I'd like to remove
the feature --- the bytecode inspection here is pretty sloppy. See comment
4 in the bug.

-j

[ISHIKAWA,Chiaki]

(2014/12/20 5:19), Jason Orendorff wrote:
> So if you go to about:config and set the javascript.options.strict pref,
> you'll get warnings about accessing undefined properties.
>
> js> Math.TAU
> undefined
> /!\ ReferenceError: reference to undefined property Math.TAU
>
> (It says "ReferenceError", but your code still runs normally; it really is
> just a warning.)
>
> Is anyone using this? Bug 1113380 points out that the rules about what kind
> of code can cause a warning are a little weird (on purpose, I think). Maybe
> it's time to retire this feature.
>
> https://bugzilla.mozilla.org/show_bug.cgi?id=1113380
>
> Please speak up now, if you're still using it!

thunderbird relies on many JS source files, and
sometimes a bug creeps in, for example,

- attributes which were set before, do not get set any more
after a modification, and

- some other files are still referencing the now non-existing
attributes.

JS strict warnings are the only way for me to realize such bugs exists.
Most of the codes are written many years ago, and so frankly nobody owns
them, so to speak these days.

(I also notice that there are typos that can only be uncovered by
JS strict warnings, etc.)

From the software engneerng point of view, it is essential to keep the
large amount of JS source files of TB in shape IMHO.

I use the JS strict warnings that are printed from the DEBUG build of
C-C TB to find and fix remaining and newly introduced bugs in TB all the
time. (The number of such warnings and errors printed during |make
mozmill| tests are staggering, and I created a script to sort them and
prioritize them. I see probably a few dozen different bugs and attack
the most frequent or most disturbing-looking bugs/warnings first.)

TIA

[Jim Blandy]

On Fri, Dec 19, 2014 at 2:22 PM, Nick Fitzgerald <nfitz...@mozilla.com>
wrote:

> I generally don't find them useful, but instead annoying, and that they
> provide a lot of noise to filter out to find actual relevant errors. This
> is including the undefined property errors. It is a common JS style to pass
> around configuration/option objects that will be missing many properties
> that get accessed by functions they are passed to. My understanding is that
> part of this is special cased not to generate these messages, but in my
> experience it isn't close to enough.
>

In a recent message, Jason explained that code that tests for the presence
of a property:

if (obj.prop)
if (obj.prop === undefined)
if (obj.prop && ...)

are not supposed to generate warnings.

Do you find that you're getting these warnings from code that has one of
those forms?

[Martin Thomson]

How does this relate to "use strict"?

On Dec 19, 2014 12:19 PM, "Jason Orendorff" <joren...@mozilla.com> wrote:

> So if you go to about:config and set the javascript.options.strict pref,
> you'll get warnings about accessing undefined properties.
>
> js> Math.TAU
> undefined
> /!\ ReferenceError: reference to undefined property Math.TAU
>
> (It says "ReferenceError", but your code still runs normally; it really is
> just a warning.)
>
> Is anyone using this? Bug 1113380 points out that the rules about what kind
> of code can cause a warning are a little weird (on purpose, I think). Maybe
> it's time to retire this feature.
>
> https://bugzilla.mozilla.org/show_bug.cgi?id=1113380
>
> Please speak up now, if you're still using it!
>

[Steve Fink]

On 12/20/2014 10:11 AM, Martin Thomson wrote:
> How does this relate to "use strict"?
>

It doesn't. It's an unfortunate name collision. Strict warnings came
first, and in retrospect should have been called extra warnings or lint
warnings or something. But when there was no such thing as "use strict",
it was a sensible name.

[Philip Chee]

On 20/12/2014 06:22, Nick Fitzgerald wrote:

> At minimum, we should mark them all JSEXN_NONE in js.msg so that they don't
> appear as misleading ReferrenceError or whatever other kind of error.
>
> ​We also shouldn't call them "strict" errors, since they have nothing to do
> with strict mode.​

I think it was Gerv who suggested "pedantic" rather than strict.

Phil

--
Philip Chee <phi...@aleytys.pc.my>, <phili...@gmail.com>
http://flashblock.mozdev.org/ http://xsidebar.mozdev.org
Guard us from the she-wolf and the wolf, and guard us from the thief,
oh Night, and so be good for us to pass.

[ishikawa]

Reading these discussions and how fragile the detection of "undefined"-ness
can be
in some situations,
now I am beginning to see the cause of the bug reported in
https://groups.google.com/forum/#!topic/mozilla.dev.tech.js-engine/EkJwTOZUmv0

and specifically
Bug 1003240 - JS Engine reports FALSE-POSITIVE(?) "strict warning" for
"undefined property" under a certain condition

Now that the particular strict warning does not get produced any more [I
checked yesterday], maybe
I should close the bug 1003240 (assuming that the JS engine's handling of
this issue more correct or
more in line with user expectation.) ???

What do people involved in JS Engine development think?

TIA

[ishikawa]

[Paolo Amadini]

On 12/19/2014 8:19 PM, Jason Orendorff wrote:
> Maybe it's time to retire this feature.

I'd be fine with this.

One of the confusing aspects of these warnings is that they are enabled
or disabled by default based on whether the build is a debug build
("javascript.options.strict" vs "javascript.options.strict.debug").

Front-end developers doing front-end debugging are encouraged to work on
release builds, since debug builds are just too slow for the purpose
(talking with gps and other build folks, this seems the way to go at
present, rather than improving the performance of debug builds or
introducing a third build type).

The result is these warnings are just unseen.

I don't have a good solution for detecting the type of errors this
warning is trying to detect (typos), but I'd say that static rather
than dynamic analysis is more indicated. Maybe we could make available
an opt-in code linting solution - and these typos would become errors,
not warnings, that are actionable, as David suggested.

Cheers,
Paolo

[Nick Fitzgerald]

Yes there are cases that are specialized, but there are cases that aren't
found. I'm having trouble reproducing because, as I mentioned before, the
way the code is loaded/eval'd determines if the messages are logged or not
and it doesn't seem they are in the console or scratchpad. This is hugely
annoying for tests that just start failing all of a sudden because of these
"errors" after you go from common js style loading to JSM loading or
something like that, for whatever reason.

Does the engine special case these?

let foo = config.foo;
if (foo) ...

of

function foobar(option) {
if (option) ...
}
foobar(config.option);

But really, we should really be making these warnings *optional*. I don't
really care too much to debate the specific warnings. Right now, they are
currently forced upon us without a choice. Tests shouldn't use the flag
(because that imposes on everyone), and if you want them for your
development, you should still be able to use them, but in a way that
doesn't force those who don't want them to do so.

[Wesley Hardman]

On a side note here, is there any usefulness of fixing these warnings (more specifically in web content than chrome content)? If you use any JS libraries like jQuery, they tend to spew a lot of these warnings. But take for example this code:

s = "Some text";
console.log(s);

With javascript.options.strict set to true, it outputs "ReferenceError: assignment to undeclared variable s". Are there any advantages to actually fixing it? The code obviously works just fine either way.

Wesley Hardman

[Jeff Walden]

On 12/26/2014 07:24 AM, Wesley Hardman wrote:
> s = "Some text";
> console.log(s);
>
> With javascript.options.strict set to true, it outputs "ReferenceError: assignment to undeclared variable s". Are there any advantages to actually fixing it? The code obviously works just fine either way.

It's trivial to put a var in front of this. Declaring global variables may make code run faster. In this super-limited example, maybe not. But teaching when it's faster and when it's not is far less easy than a simple blanket rule to "create global variables using declarations". In general that's how most of the extra-warnings warnings work -- don't do it this way, because this way can be bad, although we're not going to conclusively say it's not bad in this exact instance because that requires expert human confirmation.

Jeff

[Jeff Walden]

On 12/22/2014 08:08 AM, Paolo Amadini wrote:
> Maybe we could make available
> an opt-in code linting solution - and these typos would become errors,
> not warnings,

One substantial problem from a JS engine point of view is that we're implementing a specification. That specification says these behaviors are not errors (let alone early errors that prevent script execution overall). Introducing our own proprietary version of JS with these as errors creates a purely self-inflicted language, that we are not going to sufficiently document to actively support it -- if we even wanted to support it, which I don't think we do because this isn't The Open Web.

Jeff

[Paolo Amadini]

On 12/26/2014 5:59 PM, Jeff Walden wrote:
> On 12/22/2014 08:08 AM, Paolo Amadini wrote:
>> Maybe we could make available
>> an opt-in code linting solution - and these typos would become errors,
>> not warnings,
>

> Introducing our own proprietary version of JS with these as errors

To clarify, these would be linting errors, just like it would be an
error to inconsistently indent a line by one space instead of two. An
automated tool in Continuous Integration could then tell you in advance
that your patch will be rejected if this is not fixed (so the reviewer
doesn't have to do the check or detect typos manually).

> creates a purely self-inflicted language

But I agree we may not want to enforce this particular check during code
linting, and allow a more liberal style, even for chrome code (that by
our choice has higher requirements than web page code).

If you're thinking about the usefulness of these warnings for web page
authors, my guess is that it would be quite limited, for the reasons
you mentioned.

Paolo

[Gijs Kruitbosch]

On 29/12/2014 13:48, Paolo Amadini wrote:
> On 12/26/2014 5:59 PM, Jeff Walden wrote:
>> On 12/22/2014 08:08 AM, Paolo Amadini wrote:
>>> Maybe we could make available
>>> an opt-in code linting solution - and these typos would become errors,
>>> not warnings,
>>
>> Introducing our own proprietary version of JS with these as errors
>
> To clarify, these would be linting errors, just like it would be an
> error to inconsistently indent a line by one space instead of two. An
> automated tool in Continuous Integration could then tell you in advance
> that your patch will be rejected if this is not fixed (so the reviewer
> doesn't have to do the check or detect typos manually).

Unfortunately quite some of the 'strict warnings' are run-time ones and
cannot easily be detected at compile-time / using CI (I'm thinking in
particular of "reference to undefined property foo.bar").

~ Gijs

[bgrin...@mozilla.com]

Just a heads up that the default value for javascript.options.strict.debug is now false, as of Bug 1138781.

Brian