rm DeclEnv(Object|Class)

RESOLVED WONTFIX

Status

()

RESOLVED WONTFIX
7 years ago
7 years ago

People

(Reporter: luke, Assigned: luke)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

7 years ago
Created attachment 615976 [details] [diff] [review]
rm

It would be simpler not to have to deal with DeclEnvObjects on the scope chain.  You always have to remember to skip over them when dealing with the scope chain (which complicates bug 690135) and 'callee' requires custom handling in the frontend.  The fix is to do something similar to what bug 740446 did for 'arguments': after parsing the function body, create a binding for a named lambda's name if there isn't already a shadowing decl.

A named lambda's name is read-only which almost perfectly fits the CONSTANT BindingKind.  The only (infuriatingly small) issue is that strict-mode assignment to real 'const' locals is an early error whereas assignment to the named lambda name is a runtime error.  Ok, that that's not too hard: just emit JSOP_SETNAME and let JSPROP_READONLY handle it.  However, the frontend goes out of its way to try to erase assignment to 'const' (adding a bunch of complexity).  Thus, this patch also removes the const hackery and, in the process, fixes like 4 existing decompiler bugs in js/src/tests.

diffstat: 203 insertions(+), 355 deletions(-)
Attachment #615976 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 1

7 years ago
Created attachment 615977 [details] [diff] [review]
combined patch for fuzzing (applies to cset df9ea73ec1f4) v.1

The patch is relatively simple but, still touching dark ancient magiks; requesting fuzzing (which was a real help in bug 740446!).
Attachment #615977 - Flags: feedback?(gary)
Attachment #615977 - Flags: feedback?(choller)
Comment on attachment 615977 [details] [diff] [review]
combined patch for fuzzing (applies to cset df9ea73ec1f4) v.1

Quickly marking this as feedback+ based on the fact that it doesn't blow up after 10 mins' fuzzing.

Having little resources now as I'm on a work week and away from the office for a couple days.
Attachment #615977 - Flags: feedback?(gary) → feedback+
Comment on attachment 615977 [details] [diff] [review]
combined patch for fuzzing (applies to cset df9ea73ec1f4) v.1

Haven't found any bugs specific to this patch during fuzzing.
Attachment #615977 - Flags: feedback?(choller) → feedback+
(Assignee)

Comment 4

7 years ago
Thanks guys!  Also green on try.

Comment 5

7 years ago
May want to try getting a different reviewer.  Jeff is apparently pretty busy these days.

Comment 6

7 years ago
Comment on attachment 615976 [details] [diff] [review]
rm

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

I find "lambda" to be an insufficiently clear name for "function expression".  Any chance I could convince you to switch the name to that?  Someone trying to understand what our code does will be baffled by the "lambda" name, but "function expression" is what the spec calls it, it's what JS developers usually call it, and it's English even.  ;-)

This is pretty much nits...except for the "How does this" paragraph.  If that works correctly, r=me if you add a test for it.  I couldn't find such a test in a skim of all instances of "eval" located within two lines of "function" in our tests.

::: js/src/frontend/Parser.cpp
@@ +732,5 @@
>          }
>      }
>  
> +    /*
> +     * Add a constant local binding for the name of a named lambda unless the

Could you use "function expression" rather than "lambda" here?  I know the "lambda" name is long-standing, but "function expression" is the standard name for it, and for that reason I find it simpler and clearer.

@@ +735,5 @@
> +    /*
> +     * Add a constant local binding for the name of a named lambda unless the
> +     * name has been shadowed by a local definition.
> +     */
> +    if (tc->fun()->isLambda() && tc->fun()->atom) {

I might have isNamedFunctionExpression() for this.

@@ +748,5 @@
> +        } else if (!tc->bindings.hasBinding(context, lambdaName)) {
> +            if (!tc->bindings.addConstant(context, lambdaName))
> +                return NULL;
> +            tc->noteNeedsCallee(false);
> +        }

How does this handle this case?

  var a = function f() { eval("var f = 17;"); return f; };
  assertEq(a(), 17);

There won't be a definition.  There won't be a binding.  So a constant will get added.  But |var f = 17;| has to shadow that, and absent strict mode it's going to redeclare a constant (i.e. have no effect), and assigning 17 to it will do nothing.  Assuming this does work -- which comments here have me somewhat skeptical of, and I'm not seeing code that would appear to make this work -- how this works at least needs a comment here.

@@ +1262,5 @@
>              /* Mark the outer dn as escaping. */
>              outer_dn->pn_dflags |= PND_CLOSED;
>          }
>  
> +        if (funtc->lexdeps->count()) {

I'd prefer to read |count() > 0| here.

::: js/src/jsobj.cpp
@@ +4803,1 @@
>                      JS_ASSERT(!obj->getProto());

Assert that this is a Call object?

::: js/src/jsopcode.tbl
@@ +344,5 @@
>  
>  /* Push a closure for a named or anonymous function expression. */
>  OPDEF(JSOP_LAMBDA,    130, "lambda",    NULL,         5,  0,  1, 19,  JOF_OBJECT)
>  
>  /* Used for named function expression self-naming, if lightweight. */

This comment should be updated to something like "Prolog op initializing the local slot for the function's name", or somesuch.
Attachment #615976 - Flags: review?(jwalden+bmo) → review+

Comment 7

7 years ago
Comment on attachment 615976 [details] [diff] [review]
rm

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

::: js/src/frontend/Parser.cpp
@@ +748,5 @@
> +        } else if (!tc->bindings.hasBinding(context, lambdaName)) {
> +            if (!tc->bindings.addConstant(context, lambdaName))
> +                return NULL;
> +            tc->noteNeedsCallee(false);
> +        }

Oh, and now I see the rollup patch, and it fails this.  I win one internets.
Attachment #615976 - Flags: review+ → review-
(Assignee)

Comment 8

7 years ago
Gross, you're right.  This is a non-trivial problem.

Incidentally, for

  (function f() { eval("var f = 3"); print(f); delete f; print(f) })()

we print 3 twice whereas v8 (correctly, I believe) prints 3 then the function.
(Assignee)

Comment 9

7 years ago
Le sigh.  Given that this is a patch about simplifying the scope world and I don't see a simple solution to comment 6 (particularly getting the example in comment 8 right), WONTFIX.
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.