349362 - toString on generator shows the function instead of "[object Generator]"

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Jesse Ruderman]

Steps to reproduce:

1. Enter the following into the address bar:
     javascript:y = function(){ yield 3}; g = y();

Result: "function () { yield 3; }", which is confusing, because g is not the function.

Expected: "[object Generator]", which is what previous nightlies showed.

This regressed between 2006-08-17 and 2006-08-19 (Mac trunk nightlies).

Comment 1

[Brendan Eich [:brendan]]

Attachment: fix

Yah, code elimination.  Leaving toSource for diagnostic goodness, although I am interested in opinions on the utility of naming generators by their one-line decompiled source in diagnostics.  Is there a better way that does not reduce all culprits to "[object Generator]"?

/be

Comment 2

[Brendan Eich [:brendan]]

s/Yah/Yay/

/be

Comment 3

[Jesse Ruderman]

Since toString doesn't have to compile, I guess it could include the function name or source...

"[object Generator -- function foo]" or "[object Generator -- function foo() { yield 3; }]"

Comment 4

[Igor Bukanov]

Comment on attachment 235448 [details] [diff] [review]
fix

Patches with just minuses are cool :)

Comment 5

[Brendan Eich [:brendan]]

(In reply to comment #3)
> Since toString doesn't have to compile, I guess it could include the function
> name or source...
> 
> "[object Generator -- function foo]" or "[object Generator -- function foo() {
> yield 3; }]"

The "[object ClassName]" pattern is from Object.prototype.toString, and I'm loath to mess with it.

Adding the whole generator source, even on one line, is what worries me more, though, for the current toSource usage in diagnostics, and for any proposed Generator.prototype.toString that includes that decompilation.  It's possible to write really long generator functions.  The value of the string diminishes with length in all human-reading-toString-output contexts, and for diagnostics, there's no pointer to the offending statement.

Just the generator name would be enough for common cases, in which case "[generator GeneratorName]" might be better.  Anonymous generators might use something else (Pythonic address sprintf'd via %p?).

/be

Comment 6

[Mike Schroepfer]

plusing since we are generally trying to take all the JS fixes for 1.8...

Comment 7

[Brendan Eich [:brendan]]

Generator's toSource arguably should show a call to the generator function, not the generator function itself -- this argument is based on round-trips through uneval and eval yielding an isomorphic or identical value.  But for diagnostic purposes, it would be best if the generator iterator expression were decompiled.  The two goals are in conflict.

/be

Comment 8

[Brendan Eich [:brendan]]

(In reply to comment #7)
> Generator's toSource arguably should show a call to the generator function, not
> the generator function itself -- this argument is based on round-trips through
> uneval and eval yielding an isomorphic or identical value.  But for diagnostic
> purposes, it would be best if the generator iterator expression were
> decompiled.  The two goals are in conflict.

Example:

js> function gen(){try{yield 1}finally{yield 2}}
js> it = gen()
function gen() {
    try {
        yield 1;
    } finally {
        yield 2;
    }
}
js> it.next()
1
js> it.close()
typein:1: TypeError: yield from closing generator function gen() {try {yield 1;} finally {yield 2;}}

If I were debugging, I might rather the error looked either like this:

typein:1: TypeError: yield from closing generator gen

or like this:

typein:1: TypeError: yield from closing generator
function gen() {try {yield 1;} finally {yield 2;}}
........................................^

I don't think blaming the generator-iterator by its reference |it| in the diagnostic helps.

For the first case, round-trip serialization and deserialization goodness, an example would look like this:

it = gen()
it.next()
uneval(it) => something that evals to a generator-iterator advanced by 1 .next()

The current toSource result is completely useless for uneval:

js> it = gen()
function gen() {
    try {
        yield 1;
    } finally {
        yield 2;
    }
}
js> it.toSource()
function gen() {try {yield 1;} finally {yield 2;}}
js> it2 = eval(uneval(it))
js> typeof it2
undefined

My thought for js1.7 is to rip out toSource and change toString to work better for diagnostics.  More in a bit, comments welcome.

/be

Comment 9

[Brendan Eich [:brendan]]

Attachment: rip out to{Source,String}, use better fallback for one js_DVG call

Comment 10

[Igor Bukanov]

(In reply to comment #9)
> Created an attachment (id=236514) [edit]
> rip out to{Source,String}, use better fallback for one js_DVG call
> 

Nice, now for the test case:

function gen()
{
        let [target, f] = yield;
        f.call(target);
}


let iter = gen();
var genFunctions = [iter.close, iter.throw, iter.send, iter.next];
iter.close();

for each (let f in genFunctions) {
        let iter = gen();
        iter.next();
        try {
                iter.send([iter, f]);
        } catch (e if e instanceof TypeError) {
                print(f.name+":\t"+e);
        }
}

I got very easy to grasp printout:
close:  TypeError: already executing generator target
throw:  TypeError: already executing generator target
send:   TypeError: already executing generator target
next:   TypeError: already executing generator target

Comment 11

[Brendan Eich [:brendan]]

Comment on attachment 236514 [details] [diff] [review]
rip out to{Source,String}, use better fallback for one js_DVG call

Fixed on trunk:

Checking in jsiter.c;
/cvsroot/mozilla/js/src/jsiter.c,v  <--  jsiter.c
new revision: 3.38; previous revision: 3.37
done

This is a very safe user-experience change, which via exception handling is a necessary API fix for js1.7.

/be

Comment 12

[Bob Clary [:bc] (inactive)]

Checking in regress-349362.js;
/cvsroot/mozilla/js/tests/js1_7/geniter/regress-349362.js,v  <--  regress-349362.js
initial revision: 1.1

Comment 13

[Bob Clary [:bc] (inactive)]

verified fixed 1.9 20060903 windows/mac*/linux

Comment 14

[Mike Schroepfer]

Comment on attachment 236514 [details] [diff] [review]
rip out to{Source,String}, use better fallback for one js_DVG call

a=schrep for drivers.

Comment 15

[Brendan Eich [:brendan]]

Fixed on the 1.8 branch.

/be

Comment 16

[Bob Clary [:bc] (inactive)]

verified fixed 1.8 20060906 windows/mac*/linux