683738 - Function can contain both a return statement with an argument and a yield expression

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

js> (function() { if (x) {return this;} else {(yield 3);}})
(function () {if (x) {return this;} else {yield 3;}})

js> (function () {if (x) {return this;} else {yield 3;}})
typein:3: TypeError: anonymous generator function returns a value:
typein:3: (function () {if (x) {return this;} else {yield 3;}})
typein:3: ..........................................^

I think the bug is that the first function is *not* rejected. Before rev 81c343a150a4, it was rejected.

jsfunfuzz found this because yield *statements* are rejected, and the expression happened to turn into a statement in this case.

The first bad revision is:
changeset:   81c343a150a4
user:        Dave Herman
date:        Thu Jun 16 08:20:18 2011 -0700
summary:     disallow yield and arguments in generator expressions (bug 634472, r=cdleary)

Comment 1

[Dave Herman [:dherman]]

This is definitely bad, and definitely my fault, and I'm going to try to fix it tonight.

But it might not be *too* bad. I believe generator functions are always run in the interpreter, and I suspect the interpreter just pushes the return value onto the stack harmlessly before throwing StopIteration. Nevertheless, this is definitely supposed to be statically rejected.

I'll report back tonight after I dig into this.

Dave

Comment 2

[Dave Herman [:dherman]]

Attachment: recheck TCF_RETURN_EXPR after lazily setting TCF_FUN_IS_GENERATOR

Pretty simple mistake: since we lazily set TCF_FUN_IS_GENERATOR, the eager check whenever it encounters a `yield' token is insufficient; we need a check at the point where we actually set TCF_FUN_IS_GENERATOR to see if TCF_RETURN_EXPR is set.

I also went through to see where else jsparse.cpp ever checks TCF_FUN_IS_GENERATOR, in case there might be other subtle bugs caused by the lazy checking, but all of them happen in the context of Parser::analyzeFunctions(), which gets called after the containing function body has been completely parsed (so all lazy `yield'-checking is completely finished).

The patch also includes a new regression test file.

Dave

Comment 3

[Dave Herman [:dherman]]

PS All this stuff should be simpler with the new function* syntax in ES6: the * token makes it clear up front that you're in a generator, so you don't have to infer your context lazily.

Unfortunately, we probably won't be able to remove this at least in the short term, since existing Firefox-specific code uses generators with the current syntax.

Dave

Comment 4

[Dave Herman [:dherman]]

Update: I'm out for the next two weeks. Chris has graciously agreed to shepherd this bug while I'm out. If it is deemed unimportant I can take it over again when I get back, but otherwise Chris can take over the patch and land it as he sees fit.

Thanks,
Dave

Comment 5

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/01751bc07cca