616294 - delete x, in function t() { return eval("var x; (function() { return delete x; })")(); }, doesn't work correctly

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

Various mutations to evaluate the delete x elsewhere don't fail -- returning delete x directly works, moving the function into a second eval works, moving the delete to be a sibling of var x (and returning the result of that eval) works, etc.

I ran into this writing tests for bug 514568 -- will likely look into fixing it in order to not have to write crippled tests there (because I absolutely don't trust any possible implementation I will write without many, many such tests).

Comment 1

[Jeff Walden [:Waldo]]

js> var x; function test(){ return eval("var x; (function() { return delete x; })"); } dis(test())
flags: LAMBDA NULL_CLOSURE
main:
00000:  false
00001:  return
00002:  stop

Source notes:

So we think we can optimize when we really can't.  I bet this is easy to fix, then...

Comment 2

[Jeff Walden [:Waldo]]

Attachment: Patch and test

It was a thinko in how we were testing whether a delete could be effective, easily addressed.

Comment 3

[Jeff Walden [:Waldo]]

Comment on attachment 494874 [details] [diff] [review]
Patch and test

Actually, I think this has a bug in it, or at least it's imprecise.  More in a sec.

Comment 4

[Jeff Walden [:Waldo]]

Attachment: Tests only

The previous patch fails a fair number of the tests in this patch -- isTopLevel() didn't mean what I thought it meant.  I'm not actually sure how to go about testing this -- do we even have this information around anywhere to check?  I tried various things like walking up the tree context change and checking decls, but it didn't work in some of the cases in this test.  I had difficulty reading the variable-binding code in the parser to figure out exactly how I might possibly determine this.  :-(  So for now I'm tabling this, maybe to get back to it in short order -- we'll see, or maybe someone else can pick it up so I don't have to kneecap the eval tests I'm writing for strict mode eval.

Comment 5

[Jeff Walden [:Waldo]]

CCing various people who've worked on the parser/emitter who might have a clue how to actually do this...

Comment 6

[Brendan Eich [:brendan]]

So with http://infomonkey.cdleary.com/questions/91/given-jsdefinition-dn-how-would-i-determine-whether-that-definition-is-at-the-top-level-of-a-program q'ed and a'ed, you are back in business here, right?

/be

Comment 7

[Jeff Walden [:Waldo]]

I think so, yes -- patch coming up shortly, I hope.

Comment 8

[Jeff Walden [:Waldo]]

Attachment: 1 - Rename JSParseNode::isTopLevel and PND_TOPLEVEL to isSubFunction and PND_SUBFUNCTION

JSParseNode::isTopLevel() is a seductive red herring, as the InfoMonkey question above demonstrates.  It's really only used to detect function sub-statements -- the extension to ECMA to allow function statements other than as direct children of a Program or FunctionBody production in the grammar (rather as children of Blocks or in other contexts where a generic Statement is permitted).  To avoid its name leading future readers astray, I inverted its meaning and renamed it to PND_SUBFUNCTION (and the test inline to isSubFunction).  I also added some explanatory comments describing its exact behavior -- which is now narrower since it's *only* on function sub-statements and nothing else.

The NameNode::initCommon change is the only really interesting change here.  As far as I can tell, we set PND_TOPLEVEL on all name nodes at top level in a program or function.  I can't figure out why we did this; we only queried it on function nodes, excepting the def/use chain verification in JSParseNode::test (which motivates the initCommon change being more than just a simple inversion of logic).  Also, if I make the modification I note here, nothing in the test suite fails, so I'm not sure what it was doing before -- maybe quelling some wide-ranging assert like that in JSParseNode::test?

Comment 9

[Jeff Walden [:Waldo]]

Attachment: 2 - Introduce JSParseNode::isTopLevel that does what JSOP_DELNAME in eval actually needs, and give a comment explaining exactly what it does

In examining the meaning of the old PND_TOPLEVEL, I realized it's not really what I want here (hence the rename/meaning-change).  I really need a new bit entirely that indicates the meaning here.  Unfortunately this requires cutting down on the block id range from 2**20 to 2**19; if it's important this not change, I guess we can add another field (not a bitfield) for it.

The new bit has isTopLevel() again, but it includes a much larger, more verbose comment than occurred before, explaining its exact meaning.  That meaning is *different* from what isTopLevel() was.  I am open to a different name if you can think of one, since it does have the "top level" confusion to it.  But at least for now you can review the logic of it, and we can figure out a reasonable name at the same time that I'm working on other things I need to do.

The let-with-eval test is currently marked as failing because of bug 604301.  I think the test is okay if let-with-eval worked, but I could very easily be wrong about that.  I'll flag a dependency or something when this lands so that the test gets enabled, and fixed as necessary, when that bug's fixed.

Comment 10

[Brendan Eich [:brendan]]

We don't want to reduce block limit to 2^19 without good reason. What is the reason to split hairs about top-level vs. sub-function? I already explained that the latter is a subset of the former.

Comment 8 and comment 9 show overthinking. There is no *operational* benefit to this flag splitting, even if it helped you distinguish uses of "declared form was at top of body". We need only one bit for "declared form at top level of body", and that's all we should have.

/be

Comment 11

[Jeff Walden [:Waldo]]

(In reply to comment #10)
> What is the reason to split hairs about top-level vs. sub-function? I
> already explained that the latter is a subset of the former.

The reason is that the top-level that matters for function statements is not the same as the top-level that matters for var declarations, for the purpose of this bug.  The "top-level" that matters for function statements is whether the function is at top level in a Program or FunctionBody -- that is, whether it is an ECMA-sanctioned function statement.  If it's not that, then it is an ECMA-extension function sub-statement.  The "top-level" that matters here is whether the definition is for a name at top level of a Program.  A function statement nested directly in a function body is "top-level" for the first definition but not for the second.

> We need only one bit for "declared form at top level of body", and
> that's all we should have.

Top level of function body or program, or merely top level of program?  Function statements need the former; I need the latter.  How can these two bits of data fit in a single bit?

I must still be missing something here, because it seems to me the two concepts of "top level" are different, and the existing one is not repurposable into the one I need.

Comment 12

[Jeff Walden [:Waldo]]

Or never mind!  If I look closer I see PND_TOPLEVEL is only ever queried in debug-only code, so it's not even necessary to have it in existence.  So I can just "remove" it, and "readd" it with the changed meaning.  Will get on that now...

Comment 13

[Jeff Walden [:Waldo]]

Attachment: Better patch

Comment 14

[Brendan Eich [:brendan]]

Comment on attachment 496416 [details] [diff] [review]
Better patch

Jeff, sorry for misreading you -- I see what you want now. It doesn't matter where the var goes, all three strings:

if (1) var x = "I'm top level!";
function foo() { var y = "I'm not top level"; assertEq(x, "I'm top level"); }

show the sense of "top level" that you want.

The patch looks great, alas it leaves cg->atTopLevel() and the topLevel local in Parser::functionDef using the old meaning: syntactically at top of program or function body. Suggest renaming to atBodyLevel or something better. Ideas on not-too-long name? Maybe we should chat over IRC for greater efficiency.

r=me with the TBD renaming.

/be

Comment 15

[Jeff Walden [:Waldo]]

First the bugfix:

http://hg.mozilla.org/tracemonkey/rev/e1ef77fd8605

Then the atTopLevel rename:

http://hg.mozilla.org/tracemonkey/rev/64e001d84e7a

I buy atBodyLevel.  It's not perfect, but it's more than reasonable, and the explanatory comment should make up the difference.

Comment 16

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/e1ef77fd8605