899296 - Frontend: Parser no longer desugars foo["ab"] to foo.ab

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Eric Faust [:efaust]]

Attachment: Testcase

Test case above yields

00000:  this
00001:  string "ab"
00006:  getelem
00007:  pop
00008:  this
00009:  string "a"
00014:  string "b"
00019:  add
00020:  getelem
00021:  pop
00022:  stop

on inbound tip.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Seems to be OK on Aurora 24, so nightly-only regression so far.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Bisect says:

The first bad revision is:
changeset:   137884:32c80f595de5
user:        Jason Orendorff <[e-mail]>
date:        Wed Jul 10 08:14:02 2013 -0500
summary:     Bug 844805, part 3 - Remove a call to FoldConstants from Parser::memberExpr. r=Waldo.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Note that that's the regressing changeset for both the "ab" and "a" + "b" versions of the test...

Comment 4

[Boris Zbarsky [:bzbarsky]]

This is interesting.  Consider this testcase:

  var x = 'obj["a" + "b"]; obj["cd"];';
  var f = new Function(x);
  dis(f);

this outputs:

  main:
  00000:  getgname "obj"
  00005:  getprop "ab"
  00010:  pop
  00011:  getgname "obj"
  00016:  getprop "cd"
  00021:  pop
  00022:  stop

but an actual function expression being parsed normally ends up with getelem calls!

Comment 5

[Boris Zbarsky [:bzbarsky]]

The PNK_ELEM case in Fold() that bug 844805 added is not reached at all for the normal function case, as far as I can tell.

If I turn off lazy parsing, then I get the bytecode I expect getting emitted.

I don't see frontend::CompileLazyFunction doing any FoldConstants.  Should it be?

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Is this at all a sane thing to do?

Comment 7

[Boris Zbarsky [:bzbarsky]]

Er, I suppose with null() instead of NULL.

Comment 8

[Jason Orendorff [:jorendorff]]

The first bad revision is:
changeset:   137884:32c80f595de5
user:        Jason Orendorff <[e-mail]>
date:        Wed Jul 10 08:14:02 2013 -0500
summary:     Bug 844805, part 3 - Remove a call to FoldConstants from Parser::memberExpr. r=Waldo.

Darn it.

Comment 9

[Jason Orendorff [:jorendorff]]

Comment on attachment 782913 [details] [diff] [review]
Is this at all a sane thing to do?

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

Clearing feedback flag.

This is the right idea, but the actual FoldConstants call should go in BytecodeCompiler.cpp, in the interest of consistency.

These three entry points to the compiler should each contain one FoldConstants call:
- frontend::CompileScript
- frontend::CompileLazyFunction
- frontend::CompileFunctionBody

...and ideally, nothing else should, parsing and constant folding being two separate passes. Though we're stuck with one or two calls from the parser for the time being.

I need to look at it some more.

Comment 10

[Boris Zbarsky [:bzbarsky]]

For what it's worth, I tried putting the FoldConstants in CompileLazyFunction initially.  It crashes with a null-deref because the ParseContext that the parser used in standaloneLazyFunction() has gone out of scope so parser.context is null.

I agree that it would be nice to not have that dependency.

Comment 11

[Lukas Blakk [:lsblakk] use ?needinfo]

Tracking as a regression in 25.

Comment 12

[Jason Orendorff [:jorendorff]]

Comment on attachment 782913 [details] [diff] [review]
Is this at all a sane thing to do?

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

Try Servering this patch.

Comment 13

[Jason Orendorff [:jorendorff]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4473d84c6ab6

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4473d84c6ab6

Comment 15

[Jason Orendorff [:jorendorff]]

Comment on attachment 782913 [details] [diff] [review]
Is this at all a sane thing to do?

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 844805
User impact if declined:
  JavaScript performance regression going from FF24 to FF25.
Testing completed (on m-c, etc.): Landed in m-c 4 days ago.
Risk to taking this patch (and alternatives if risky): Low.
String or IDL/UUID changes made by this patch: None.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/2def7ab553b5

Comment 17

[Jason Orendorff [:jorendorff]]

I'm still getting daily tracking emails about this, but I thought it was done. Please let me know what remains here, or clear the bit.