1019181 - Minor change to semantics of const

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

Attachment: simple-const-change

I've been having trouble with const declarations in bug 990729. When we have the code "const x = 1;", we generate two bytecode ops: JSOP_DEFCONST and JSOP_SETCONST. Both of these ops call JSObject::defineProperty with JSPROP_READONLY. In bug 990729, I have a patch that tries to "mirror" changes to one object onto another object. The patch uses JS_CopyPropertyFrom to do the mirroring, which defines the property using js::DefineOwnProperty. So when you try to mirror "const x = 1;", we make two calls to js::DefineOwnProperty. The second call fails, because we're changing the value of a read-only property. The only reason this doesn't normally happen is that JSObject::defineProperty ignores JSPROP_READONLY.

The simplest way I can think to fix this is to have JSOP_DEFCONST define the property without JSPROP_READONLY, and JSOP_SETCONST define it as JSPROP_READONLY. This seems to be closer to what ES6 is going to do to const anyway: you're supposed to be allowed to define it and then set it exactly once.

Our behavior doesn't actually change much with this patch. Most of our const checking is done by the parser, so it's unchanged. I'll try to enumerate the changes here.

---
const x = 1; const x = 2;
Same: Gives TypeError before and after. The parser generates this error.
---
eval("const x = 1;"); eval("const x = 2;");
Different: Gives error without patch (and leaves x=1), succeeds and sets x=2 with patch.
---
const x = 1; x = 2;
Same: No error, but x=1 afterwards.
---
const x; x = 1;
Different: Gives TypeError without patch, succeeds and sets x=1 with patch.
---
"use strict"; const x = 2; x = 1;
Same: Gives TypeError before and after.
---
const x = 1; this.x = 2;
Same: No error, x=1 afterwards.
---
"use strict"; const x = 1; this.x = 2;
Same: TypeError before and after.
---

I had to change one test, since it was testing the thing in the second example. That's the only behavioral change here that I would consider undesirable. However, it's hard to see how it would really affect anyone.

Comment 1

[Jeff Walden [:Waldo]]

Comment on attachment 8432728 [details] [diff] [review]
simple-const-change

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

So...I don't have any good ideas on what are more desirable semantics here.  Other than the actual ES6 semantics that would make this slightly nicer, in that you actually couldn't have this weirdness visible.  I'm not much sure I like these semantics, or this change, to be sure.  Punting to jorendorff who is thinking more deeply about this than I am right now, and who knows ES6 semantics somewhat better, too...

Comment 2

[Jason Orendorff [:jorendorff]]

Comment on attachment 8432728 [details] [diff] [review]
simple-const-change

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

Clearing review - as discussed on IRC, we have no great options but it seems to me like

    JSOP_DEFCONST should define the property with JSPROP_READONLY but not JSPROP_PERMANENT
    JSOP_SETCONST should redefine it with both READONLY and PERMANENT

Hard to be confident this won't bust something important. But it is at least compliant with what ES5 says should be possible when defining properties.

Comment 3

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

Attachment: permanent change

Here you go!

Comment 4

[Jason Orendorff [:jorendorff]]

Comment on attachment 8437335 [details] [diff] [review]
permanent change

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

Clearing review for now, but ping me.

::: js/src/jit/IonBuilder.cpp
@@ +9757,4 @@
>      if (JSOp(*pc) == JSOP_DEFCONST)
>          attrs |= JSPROP_READONLY;
> +    else
> +        attrs |= JSPROP_PERMANENT;

Does it make sense to assert here that script->isForEval() is false?

::: js/src/tests/js1_5/Regress/regress-103602.js
@@ -54,5 @@
>  try
>  {
>    /*
> -   * Redeclaration of const should be a compile-time error.
> -   * Hide so it will be detected at run-time.

Argh. This should still throw, because the first const actually executed. The global property it defined should now be permanent and readonly, and trying to redefine it should fail.

Can you explain why it succeeds?

Comment 5

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

Comment on attachment 8437335 [details] [diff] [review]
permanent change

Sorry, I had meant to remove the test hunk from the patch. The test does succeed now with permanent.

Comment 6

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

Attachment: permanent change v2

I guess I might as well make the changes.

Comment 7

[Jason Orendorff [:jorendorff]]

Comment on attachment 8437924 [details] [diff] [review]
permanent change v2

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

Oh, look, three copies of the same code. Sigh.

::: js/src/vm/Interpreter.cpp
@@ +2913,4 @@
>      if (*REGS.pc == JSOP_DEFCONST)
>          attrs |= JSPROP_READONLY;
> +    else if (!REGS.fp()->isEvalFrame())
> +        attrs |= JSPROP_PERMANENT;

What we're doing here is weird enough to want a comment, maybe pointing at this bug. Alternatively, put the comment in vm/Opcodes.cpp.

Comment 8

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/1b5f05eeebd5

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1b5f05eeebd5