1130785 - JS_HAS_EXPR_CLOSURES macro is used in wrong places.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Tooru Fujisawa [:arai]]

in frontend/Parser.cpp, code in `#if JS_HAS_EXPR_CLOSURES` is also used by arrow function, and turning JS_HAS_EXPR_CLOSURES off breaks it.

https://hg.mozilla.org/mozilla-central/file/193c4c5c7ec2/js/src/frontend/Parser.cpp#l1043
>     if (type == StatementListBody) {
>         pn = statements();
>         if (!pn)
>             return null();
>     } else {
>         MOZ_ASSERT(type == ExpressionBody);
>         MOZ_ASSERT(JS_HAS_EXPR_CLOSURES);

and

https://hg.mozilla.org/mozilla-central/file/193c4c5c7ec2/js/src/frontend/Parser.cpp#l2571
> #if JS_HAS_EXPR_CLOSURES
>     if (bodyType == StatementListBody) {
> #endif
>         bool matched;
>         if (!tokenStream.matchToken(&matched, TOK_RC))
>             return false;
>         if (!matched) {
>             report(ParseError, false, null(), JSMSG_CURLY_AFTER_BODY);
>             return false;
>         }
>         funbox->bufEnd = pos().begin + 1;
> #if JS_HAS_EXPR_CLOSURES
>     } else {
>         if (tokenStream.hadError())
>             return false;
>         funbox->bufEnd = pos().end;
>         if (kind == Statement && !MatchOrInsertSemicolon(tokenStream))
>             return false;
>     }
> #endif

Comment 1

[Tooru Fujisawa [:arai]]

Attachment: Fix JS_HAS_EXPR_CLOSURES macro conditions.

With this patch, we can run tests with expression closure disabled build :)
No change for expression closure enabled build.

Comment 2

[Shu-yu Guo [:shu]]

Comment on attachment 8561023 [details] [diff] [review]
Fix JS_HAS_EXPR_CLOSURES macro conditions.

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

Seems okay to me. I'd like clarification on fun->setIsExprClosure(). Will that be removed and will JSFunction::setArrow be used at some point?

::: js/src/frontend/Parser.cpp
@@ +2563,4 @@
>  
>          tokenStream.ungetToken();
>          bodyType = ExpressionBody;
>          fun->setIsExprClosure();

So right now, arrow functions are marked as expression closures, regardless of JS_HAS_EXPR_CLOSURES?

@@ +2585,1 @@
>      } else {

This else clause can only be hit in the case of kind == Arrow, right? Could we assert this in the case of #ifndef JS_HAS_EXPR_CLOSURES?

Comment 3

[Tooru Fujisawa [:arai]]

Thank you for reviewing :D

(In reply to Shu-yu Guo [:shu] from comment #2)
> Comment on attachment 8561023 [details] [diff] [review]
> Fix JS_HAS_EXPR_CLOSURES macro conditions.
> 
> Review of attachment 8561023 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Seems okay to me. I'd like clarification on fun->setIsExprClosure(). Will
> that be removed and will JSFunction::setArrow be used at some point?
> 
> ::: js/src/frontend/Parser.cpp
> @@ +2563,4 @@
> >  
> >          tokenStream.ungetToken();
> >          bodyType = ExpressionBody;
> >          fun->setIsExprClosure();
> 
> So right now, arrow functions are marked as expression closures, regardless
> of JS_HAS_EXPR_CLOSURES?

Oops, it should also be inside `#if JS_HAS_EXPR_CLOSURES`, since there is only one consumer for arrow function's case in jsreflect.cpp, and it's also inside `#if`.

https://hg.mozilla.org/mozilla-central/file/3436787a82d0/js/src/jsreflect.cpp#l3284
>     bool isExpression =
> #if JS_HAS_EXPR_CLOSURES
>         func->isExprClosure();
> #else
>         false;
> #endif

Also, I guess `setArrow` shouldn't be called there, it's already specified here:
https://hg.mozilla.org/mozilla-central/file/2cb22c058add/js/src/frontend/Parser.cpp#l1249
>     JSFunction::Flags flags = (kind == Expression)
>                               ? JSFunction::INTERPRETED_LAMBDA
>                               : (kind == Arrow)
>                                 ? JSFunction::INTERPRETED_LAMBDA_ARROW
>                                 : JSFunction::INTERPRETED;


> @@ +2585,1 @@
> >      } else {
> 
> This else clause can only be hit in the case of kind == Arrow, right? Could
> we assert this in the case of #ifndef JS_HAS_EXPR_CLOSURES?

Yeah, should have it :)
I guess it would be better to use `#if not JS_HAS_EXPR_CLOSURES`, since JS_HAS_EXPR_CLOSURES is defined as a number, and 0 means false for `#if JS_HAS_EXPR_CLOSURES`.

Comment 4

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f13b5763c5ce

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f13b5763c5ce