1113380 - JSOPTION_EXTRA_WARNINGS reports warning on JSOP_CALL but not JSOP_SETLOCAL

Status: RESOLVED DUPLICATE of bug 1619177

(Core :: JavaScript Engine, defect)

Description

[Andrew Paprocki]

The code:

  var foo = someObj.propDoesntExist;
  func(foo);

... does not report a warning because it is a JSOP_SETLOCAL, which is marked JOF_DETECTING. This line of code is definitely not "detecting", so this appears to be a bug with JSOPTION_EXTRA_WARNINGS.

  func(someObj.propDoesntExist);

... does report a warning because it is a JSOP_CALL, which is *not* marked JOF_DETECTING.

When running with this option and JSOPTION_WERROR, this confuses users because the latter throws a ReferenceError exception while the first does not. That seems wrong.

Question: Should JOF_DETECTING be removed from JSOP_SETLOCAL and any related opcodes? Why is it still there?

Meta Question: Should JSOPTION_EXTRA_WARNINGS be removed entirely or made to strictly follow ES spec? If I am running in ES5 `use strict` mode, am I actually breaking spec compliance by enabling JSOPTION_EXTRA_WARNINGS?

Comment 1

[Brendan Eich [:brendan]]

Time to retire some options.

/be

Comment 2

[Jason Orendorff [:jorendorff]]

I imagine JSOP_SETLOCAL is JOF_DETECTING on purpose; without a more involved analysis, each bytecode is a choice between some volume of false positives and false negatives.

> If I am running in ES5 `use strict` mode, am I actually breaking spec compliance by enabling JSOPTION_EXTRA_WARNINGS?

I don't think so. Warnings are a side channel; they aren't meant to affect program behavior. It's JSOPTION_WERROR that breaks spec compliance.

> Time to retire some options.

I decided to ask dev.platform about this one:
    https://groups.google.com/forum/#!topic/mozilla.dev.platform/l7LwHMaCLMk

IIRC, Gecko hackers once used this pref. It's been years since I heard anything about it, though.

Comment 3

[Jason Orendorff [:jorendorff]]

ap_bb: Come to think of it, you tell me. Are these warnings useful to your developers?

Comment 4

[Jason Orendorff [:jorendorff]]

Cool bug:

    > var p = new Proxy({x: 1}, {});
    > p.x
    1
    ReferenceError: reference to undefined property p.x

The warning message is wrong about what caused it: it really appears because the proxy handler doesn't have a get() method. Proxy's use of handler.get is morally a "detecting" use of the property, but it's not implemented in bytecode, so the warning code gets confused.

Comment 5

[Andrew Paprocki]

The warnings-as-errors were useful to some extent because typically accessing a property that does not exist would be an error of some sort. The intent was to learn about these early instead of wasting a lot of time debugging where an 'undefined' value appeared. If it is ES-breaking, I'm not opposed to removing it, but I just wanted to know for sure what the intended behavior should be and whether the engine will keep the warnings at all going forward.

Comment 6

[Tom S [:evilpie]]

We removed this mode.