Bug 624199: Correctly skip emitting bytecode for useless string literals and complain, while not flagging directives. r=brendan

Never emit bytecode for expression statements consisting of a single string
literal. Complain about them as useless code only if they are not part of a
Directive Prologue. The comments in recognizeDirectivePrologue explain the
details.

Fix bad names of directive-prologue-related parse node member functions.

js/src/jsemit.cpp

@@ -5921,8 +5921,9 @@ js_EmitTree(JSContext *cx, JSCodeGenerator *cg, JSParseNode *pn)
              * API users may also set the JSOPTION_NO_SCRIPT_RVAL option when
              * calling JS_Compile* to suppress JSOP_POPV.
              */
-            wantval = !(cg->flags & (TCF_IN_FUNCTION | TCF_NO_SCRIPT_RVAL));
-            useful = wantval || pn->isDirectivePrologueMember();
+            useful = wantval = !(cg->flags & (TCF_IN_FUNCTION | TCF_NO_SCRIPT_RVAL));
+
+            /* Don't eliminate expressions with side effects. */
             if (!useful) {
                 if (!CheckSideEffects(cx, cg, pn2, &useful))
                     return JS_FALSE;
@@ -5935,14 +5936,21 @@ js_EmitTree(JSContext *cx, JSCodeGenerator *cg, JSParseNode *pn)
              * labeled compound statement.
              */
             if (!useful &&
-                (!cg->topStmt ||
-                 cg->topStmt->type != STMT_LABEL ||
-                 cg->topStmt->update < CG_OFFSET(cg))) {
-                CG_CURRENT_LINE(cg) = pn2->pn_pos.begin.lineno;
-                if (!ReportCompileErrorNumber(cx, CG_TS(cg), pn2,
-                                              JSREPORT_WARNING | JSREPORT_STRICT,
-                                              JSMSG_USELESS_EXPR)) {
-                    return JS_FALSE;
+                cg->topStmt &&
+                cg->topStmt->type == STMT_LABEL &&
+                cg->topStmt->update >= CG_OFFSET(cg)) {
+                useful = true;
+            }
+
+            if (!useful) {
+                /* Don't complain about directive prologue members; just don't emit their code. */
+                if (!pn->isDirectivePrologueMember()) {
+                    CG_CURRENT_LINE(cg) = pn2->pn_pos.begin.lineno;
+                    if (!ReportCompileErrorNumber(cx, CG_TS(cg), pn2,
+                                                  JSREPORT_WARNING | JSREPORT_STRICT,
+                                                  JSMSG_USELESS_EXPR)) {
+                        return JS_FALSE;
+                    }
                 }
             } else {
                 op = wantval ? JSOP_POPV : JSOP_POP;

js/src/jsparse.cpp

@@ -3351,10 +3351,10 @@ Parser::functionExpr()
 }
 
 /*
- * Recognize Directive Prologue members and directives.  Assuming pn
- * is a candidate for membership in a directive prologue, return
- * true if it is in fact a member.  Recognize directives and set
- * tc's flags accordingly.
+ * Recognize Directive Prologue members and directives. Assuming |pn| is a
+ * candidate for membership in a directive prologue, recognize directives and
+ * set |tc|'s flags accordingly. If |pn| is indeed part of a prologue, set its
+ * |pn_prologue| flag.
  *
  * Note that the following is a strict mode function:
  *
@@ -3365,17 +3365,34 @@ Parser::functionExpr()
  *   "use strict"
  * }
  *
- * That is, a statement can be a Directive Prologue member, even
- * if it can't possibly be a directive, now or in the future.
+ * That is, even though "use\x20loose" can never be a directive, now or in the
+ * future (because of the hex escape), the Directive Prologue extends through it
+ * to the "use strict" statement, which is indeed a directive.
  */
 bool
 Parser::recognizeDirectivePrologue(JSParseNode *pn, bool *isDirectivePrologueMember)
 {
-    *isDirectivePrologueMember = pn->isDirectivePrologueMember();
+    *isDirectivePrologueMember = pn->isStringExprStatement();
     if (!*isDirectivePrologueMember)
         return true;
-    if (pn->isDirective()) {
-        JSAtom *directive = pn->pn_kid->pn_atom;
+
+    JSParseNode *kid = pn->pn_kid;
+    if (kid->isEscapeFreeStringLiteral()) {
+        /*
+         * Mark this statement as being a possibly legitimate part of a
+         * directive prologue, so the byte code emitter won't warn about it
+         * being useless code. (We mustn't just omit the statement entirely yet,
+         * as it could be producing the value of an eval or JSScript execution.)
+         *
+         * Note that even if the string isn't one we recognize as a directive,
+         * the emitter still shouldn't flag it as useless, as it could become a
+         * directive in the future. We don't want to interfere with people
+         * taking advantage of directive-prologue-enabled features that appear
+         * in other browsers first.
+         */
+        pn->pn_prologue = true;
+
+        JSAtom *directive = kid->pn_atom;
         if (directive == context->runtime->atomState.useStrictAtom) {
             /*
              * Unfortunately, Directive Prologue members in general may contain

js/src/jsparse.h

@@ -138,6 +138,9 @@ JS_BEGIN_EXTERN_C
  *                                     pn_right: initializer
  * TOK_RETURN   unary       pn_kid: return expr or null
  * TOK_SEMI     unary       pn_kid: expr or null statement
+ *                          pn_prologue: true if Directive Prologue member
+ *                              in original source, not introduced via
+ *                              constant folding or other tree rewriting
  * TOK_COLON    name        pn_atom: label, pn_expr: labeled statement
  *
  * <Expressions>
@@ -376,7 +379,9 @@ struct JSParseNode {
         struct {                        /* one kid if unary */
             JSParseNode *kid;
             jsint       num;            /* -1 or sharp variable number */
-            JSBool      hidden;         /* hidden genexp-induced JSOP_YIELD */
+            JSBool      hidden;         /* hidden genexp-induced JSOP_YIELD
+                                           or directive prologue member (as
+                                           pn_prologue) */
         } unary;
         struct {                        /* name, labeled statement, etc. */
             union {
@@ -427,6 +432,7 @@ struct JSParseNode {
 #define pn_kid          pn_u.unary.kid
 #define pn_num          pn_u.unary.num
 #define pn_hidden       pn_u.unary.hidden
+#define pn_prologue     pn_u.unary.hidden
 #define pn_atom         pn_u.name.atom
 #define pn_objbox       pn_u.name.objbox
 #define pn_expr         pn_u.name.expr
@@ -565,13 +571,21 @@ public:
     }
 
     /*
-     * True if this statement node could be a member of a Directive
-     * Prologue.  Note that the prologue may contain strings that
-     * cannot themselves be directives; that's a stricter test.
-     * If Statement begins to simplify trees into this form, then
-     * we'll need additional flags that we can test here.
+     * True if this statement node could be a member of a Directive Prologue: an
+     * expression statement consisting of a single string literal.
+     *
+     * This considers only the node and its children, not its context. After
+     * parsing, check the node's pn_prologue flag to see if it is indeed part of
+     * a directive prologue.
+     *
+     * Note that a Directive Prologue can contain statements that cannot
+     * themselves be directives (string literals that include escape sequences
+     * or escaped newlines, say). This member function returns true for such
+     * nodes; we use it to determine the extent of the prologue.
+     * isEscapeFreeStringLiteral, below, checks whether the node itself could be
+     * a directive.
      */
-    bool isDirectivePrologueMember() const {
+    bool isStringExprStatement() const {
         if (PN_TYPE(this) == js::TOK_SEMI) {
             JS_ASSERT(pn_arity == PN_UNARY);
             JSParseNode *kid = pn_kid;
@@ -581,23 +595,26 @@ public:
     }
 
     /*
-     * True if this node, known to be a Directive Prologue member,
-     * could be a directive itself.
+     * Return true if this node, known to be a string literal, could be the
+     * string of a directive in a Directive Prologue. Directive strings never
+     * contain escape sequences or line continuations.
      */
-    bool isDirective() const {
-        JS_ASSERT(isDirectivePrologueMember());
-        JSParseNode *kid = pn_kid;
-        JSString *str = ATOM_TO_STRING(kid->pn_atom);
+    bool isEscapeFreeStringLiteral() const {
+        JS_ASSERT(pn_type == js::TOK_STRING && !pn_parens);
+        JSString *str = ATOM_TO_STRING(pn_atom);
 
         /*
-         * Directives must contain no EscapeSequences or LineContinuations.
          * If the string's length in the source code is its length as a value,
-         * accounting for the quotes, then it qualifies.
+         * accounting for the quotes, then it must not contain any escape
+         * sequences or line continuations.
          */
         return (pn_pos.begin.lineno == pn_pos.end.lineno &&
                 pn_pos.begin.index + str->length() + 2 == pn_pos.end.index);
     }
 
+    /* Return true if this node appears in a Directive Prologue. */
+    bool isDirectivePrologueMember() const { return pn_prologue; }
+
 #ifdef JS_HAS_GENERATOR_EXPRS
     /*
      * True if this node is a desugared generator expression.

js/src/tests/ecma_5/extensions/jstests.list

@@ -7,6 +7,7 @@ script String-toSource.js
 script proxy-strict.js
 script regress-bug567606.js
 script string-literal-getter-setter-decompilation.js
+script uneval-strict-functions.js
 script watch-inherited-property.js
 script watch-replaced-setter.js
 script watch-setter-become-setter.js

js/src/tests/ecma_5/extensions/uneval-strict-functions.js

@@ -0,0 +1,61 @@
+/*
+ * Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/licenses/publicdomain/
+ */
+
+/* Check that strict mode functions get decompiled properly. */
+function lenient() { return typeof this == "object"; }
+assertEq(eval(uneval(lenient) + "lenient;")(), true);
+
+function strict()  { 'use strict'; return typeof this == "undefined"; }
+print(uneval(strict));
+assertEq(eval(uneval(strict) + "strict;")(), true);
+
+function lenient_outer() {
+    function lenient_inner() {
+        return typeof this == "object";
+    }
+    return lenient_inner;
+}
+assertEq(eval(uneval(lenient_outer()) + "lenient_inner;")(), true);
+
+function strict_outer() {
+    "use strict";
+    function strict_inner() {
+        return typeof this == "undefined";
+    }
+    return strict_inner;
+}
+assertEq(eval(uneval(strict_outer()) + "strict_inner;")(), true);
+
+function lenient_outer_closure() {
+    return function lenient_inner_closure() { 
+               return typeof this == "object"; 
+           };
+}
+assertEq(eval(uneval(lenient_outer_closure()))(), true);
+
+function strict_outer_closure() {
+    "use strict";
+    return function strict_inner_closure() { 
+               return typeof this == "undefined"; 
+           };
+}
+assertEq(eval(uneval(strict_outer_closure()))(), true);
+
+function lenient_outer_expr() {
+    return function lenient_inner_expr() (typeof this == "object");
+}
+assertEq(eval(uneval(lenient_outer_expr()))(), true);
+
+/*
+ * This doesn't work, because we have no way to include strict mode
+ * directives in expression closures.
+ *
+ *   function strict_outer_expr() {
+ *       return function strict_inner_expr() (typeof this == "undefined");
+ *   }
+ *   assertEq(eval(uneval(strict_outer_expr()))(), true);
+ */
+
+reportCompare(true, true);

js/src/tests/js1_8_5/regress/jstests.list

@@ -78,4 +78,5 @@ script regress-620376-1.js
 script regress-620376-2.js
 script regress-621814.js
 script regress-620750.js
+script regress-624199.js
 script regress-624547.js

js/src/tests/js1_8_5/regress/regress-624199.js

@@ -0,0 +1,17 @@
+/*
+ * Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/licenses/publicdomain/
+ */
+function roundTrip(f) {
+    try {
+        eval(uneval(f));
+        return true;
+    } catch (e) {
+        return '' + e;
+    }
+}
+
+function f() { if (true) { 'use strict'; } var eval; }
+assertEq(roundTrip(f), true);
+
+reportCompare(true,true);