More work on strict mode.

* Support implemented for checking for inconsistent return usage. Contribution
from Roshan James <roshanj@google.com>.
* Fixed check for assignment in conditional.

js/rhino/src/org/mozilla/javascript/Node.java

@@ -23,6 +23,7 @@
  *
  * Contributor(s):
  *   Norris Boyd
+ *   Roshan James
  *   Roger Lawrence
  *   Mike McCabe
  *
@@ -79,7 +80,9 @@ public class Node
         LABEL_ID_PROP      = 15, // label id: code generation uses it
         MEMBER_TYPE_PROP   = 16, // type of element access operation
         NAME_PROP          = 17, // property name
-        LAST_PROP          = 17;
+        CONTROL_BLOCK_PROP = 18, // flags a control block that can drop off
+        PARENTHESIZED_PROP = 19, // expression is parenthesized
+        LAST_PROP          = 19;
 
     // values of ISNUMBER_PROP to specify
     // which of the children are Number types
@@ -455,6 +458,8 @@ public class Node
                 case LABEL_ID_PROP:      return "label_id_prop";
                 case MEMBER_TYPE_PROP:   return "member_type_prop";
                 case NAME_PROP:          return "name_prop";
+                case CONTROL_BLOCK_PROP: return "control_block_prop";
+                case PARENTHESIZED_PROP: return "parenthesized_prop";
 
                 default: Kit.codeBug();
             }
@@ -578,6 +583,335 @@ public class Node
         if (type != Token.TARGET) Kit.codeBug();
         putIntProp(LABEL_ID_PROP, labelId);
     }
+    
+
+    /**
+     * Does consistent-return analysis on the function body when strict mode is
+     * enabled.
+     *
+     *   function (x) { return (x+1) }
+     * is ok, but
+     *   function (x) { if (x < 0) return (x+1); }
+     * is not becuase the function can potentially return a value when the
+     * condition is satisfied and if not, the function does not explicitly
+     * return value.
+     *
+     * This extends to checking mismatches such as "return" and "return <value>"
+     * used in the same function. Warnings are not emitted if inconsistent
+     * returns exist in code that can be statically shown to be unreachable.
+     * Ex.
+     *   function (x) { while (true) { ... if (..) { return value } ... } }
+     * emits no warning. However if the loop had a break statement, then a
+     * warning would be emitted.
+     *
+     * The consistency analysis looks at control structures such as loops, ifs,
+     * switch, try-catch-finally blocks, examines the reachable code paths and
+     * warns the user about an inconsistent set of termination possibilities.
+     *
+     * Caveat: Since the parser flattens many control structures into almost
+     * straight-line code with gotos, it makes such analysis hard. Hence this
+     * analyser is written to taken advantage of patterns of code generated by
+     * the parser (for loops, try blocks and such) and does not do a full
+     * control flow analysis of the gotos and break/continue statements.
+     * Future changes to the parser will affect this analysis.
+     */
+
+    /**
+     * These flags enumerate the possible ways a statement/function can
+     * terminate. These flags are used by endCheck() and by the Parser to
+     * detect inconsistent return usage.
+     *
+     * END_UNREACHED is reserved for code paths that are assumed to always be
+     * able to execute (example: throw, continue)
+     *
+     * END_DROPS_OFF indicates if the statement can transfer control to the
+     * next one. Statement such as return dont. A compound statement may have
+     * some branch that drops off control to the next statement.
+     *
+     * END_RETURNS indicates that the statement can return (without arguments)
+     * END_RETURNS_VALUE indicates that the statement can return a value.
+     *
+     * A compound statement such as
+     * if (condition) {
+     *   return value;
+     * }
+     * Will be detected as (END_DROPS_OFF | END_RETURN_VALUE) by endCheck()
+     */
+    static final int END_UNREACHED = 0;
+    static final int END_DROPS_OFF = 1;
+    static final int END_RETURNS = 2;
+    static final int END_RETURNS_VALUE = 4;
+
+    /**
+     * Checks that every return usage in a function body is consistent with the
+     * requirements of strict-mode.
+     * @return true if the function satisfies strict mode requirement.
+     */
+    public boolean hasConsistentReturnUsage()
+    {
+        int n = endCheck();
+        return (n & END_RETURNS_VALUE) == 0 ||
+               (n & (END_DROPS_OFF|END_RETURNS)) == 0;
+    }
+
+    /**
+     * Returns in the then and else blocks must be consistent with each other.
+     * If there is no else block, then the return statement can fall through.
+     * @return logical OR of END_* flags
+     */
+    private int endCheckIf()
+    {
+        Node th, el;
+        int rv = END_UNREACHED;
+
+        th = next;
+        el = ((Jump)this).target;
+
+        rv = th.endCheck();
+
+        if (el != null)
+            rv |= el.endCheck();
+        else
+            rv |= END_DROPS_OFF;
+
+        return rv;
+    }
+
+    /**
+     * Consistency of return statements is checked between the case statements.
+     * If there is no default, then the switch can fall through. If there is a
+     * default,we check to see if all code paths in the default return or if
+     * there is a code path that can fall through.
+     * @return logical OR of END_* flags
+     */
+    private int endCheckSwitch()
+    {
+        Node n;
+        int rv = END_UNREACHED;
+
+        // examine the cases
+        for (n = first.next; n != null; n = n.next)
+        {
+            if (n.type == Token.CASE) {
+                rv |= ((Jump)n).target.endCheck();
+            } else
+                break;
+        }
+
+        // we don't care how the cases drop into each other
+        rv &= ~END_DROPS_OFF;
+
+        // examine the default
+        n = ((Jump)this).getDefault();
+        if (n != null)
+            rv |= n.endCheck();
+        else
+            rv |= END_DROPS_OFF;
+
+        // remove the switch block
+        rv |= getIntProp(CONTROL_BLOCK_PROP, END_UNREACHED);
+
+        return rv;
+    }
+
+    /**
+     * If the block has a finally, return consistency is checked in the
+     * finally block. If all code paths in the finally returns, then the
+     * returns in the try-catch blocks don't matter. If there is a code path
+     * that does not return or if there is no finally block, the returns
+     * of the try and catch blocks are checked for mismatch.
+     * @return logical OR of END_* flags
+     */
+    private int endCheckTry()
+    {
+        Node n;
+        int rv = END_UNREACHED;
+
+        // check the finally if it exists
+        n = ((Jump)this).getFinally();
+        if(n != null) {
+            rv = n.next.first.endCheck();
+        } else {
+            rv = END_DROPS_OFF;
+        }
+
+        // if the finally block always returns, then none of the returns
+        // in the try or catch blocks matter
+        if ((rv & END_DROPS_OFF) != 0) {
+            rv &= ~END_DROPS_OFF;
+
+            // examine the try block
+            rv |= first.endCheck();
+
+            // check each catch block
+            n = ((Jump)this).target;
+            if (n != null)
+            {
+                // point to the first catch_scope
+                for (n = n.next.first; n != null; n = n.next.next)
+                {
+                    // check the block of user code in the catch_scope
+                    rv |= n.next.first.next.first.endCheck();
+                }
+            }
+        }
+
+        return rv;
+    }
+
+    /**
+     * Return statement in the loop body must be consistent. The default
+     * assumption for any kind of a loop is that it will eventually terminate.
+     * The only exception is a loop with a constant true condition. Code that
+     * follows such a loop is examined only if one can statically determine
+     * that there is a break out of the loop.
+     *  for(<> ; <>; <>) {}
+     *  for(<> in <> ) {}
+     *  while(<>) { }
+     *  do { } while(<>)
+     * @return logical OR of END_* flags
+     */
+    private int endCheckLoop()
+    {
+        Node n;
+        int rv = END_UNREACHED;
+
+        // To find the loop body, we look at the second to last node of the
+        // loop node, which should be the predicate that the loop should
+        // satisfy.
+        // The target of the predicate is the loop-body for all 4 kinds of
+        // loops.
+        for (n = first; n.next != last; n = n.next)
+            /* skip */;
+        if (n.type != Token.IFEQ)
+            return END_DROPS_OFF;
+
+        // The target's next is the loop body block
+        rv = ((Jump)n).target.next.endCheck();
+
+        // check to see if the loop condition is true
+        if (n.first.type == Token.TRUE)
+            rv &= ~END_DROPS_OFF;
+
+        // look for effect of breaks
+        rv |= getIntProp(CONTROL_BLOCK_PROP, END_UNREACHED);
+
+        return rv;
+    }
+
+
+    /**
+     * A general block of code is examined statement by statement. If any
+     * statement (even compound ones) returns in all branches, then subsequent
+     * statements are not examined.
+     * @return logical OR of END_* flags
+     */
+    private int endCheckBlock()
+    {
+        Node n;
+        int rv = END_DROPS_OFF;
+
+        // check each statment and if the statement can continue onto the next
+        // one, then check the next statement
+        for (n=first; ((rv & END_DROPS_OFF) != 0) && n != null; n = n.next)
+        {
+            rv &= ~END_DROPS_OFF;
+            rv |= n.endCheck();
+        }
+        return rv;
+    }
+
+    /**
+     * A labelled statement implies that there maybe a break to the label. The
+     * function processes the labelled statement and then checks the
+     * CONTROL_BLOCK_PROP property to see if there is ever a break to the
+     * particular label.
+     * @return logical OR of END_* flags
+     */
+    private int endCheckLabel()
+    {
+        int rv = END_UNREACHED;
+
+        rv = next.endCheck();
+        rv |= getIntProp(CONTROL_BLOCK_PROP, END_UNREACHED);
+
+        return rv;
+    }
+
+    /**
+     * When a break is encountered annotate the statement being broken
+     * out of by setting its CONTROL_BLOCK_PROP property.
+     * @return logical OR of END_* flags
+     */
+    private int endCheckBreak()
+    {
+        Node n = ((Jump) this).jumpNode;
+        n.putIntProp(CONTROL_BLOCK_PROP, END_DROPS_OFF);
+        return END_UNREACHED;
+    }
+
+    /**
+     * endCheck() examines the body of a function, doing a basic reachability
+     * analysis and returns a combination of flags END_* flags that indicate
+     * how the function execution can terminate. These constitute only the
+     * pessimistic set of termination conditions. It is possible that at
+     * runtime certain code paths will never be actually taken. Hence this
+     * analysis will flag errors in cases where there may not be errors.
+     * @return logical OR of END_* flags
+     */
+    private int endCheck()
+    {
+        switch(type)
+        {
+            case Token.BREAK:
+                return endCheckBreak();
+
+            case Token.CONTINUE:
+            case Token.THROW:
+                return END_UNREACHED;
+
+            case Token.RETURN:
+                if (this.first != null)
+                    return END_RETURNS_VALUE;
+                else
+                    return END_RETURNS;
+
+            case Token.TARGET:
+                if (next != null)
+                    return next.endCheck();
+                else
+                    return END_DROPS_OFF;
+
+            case Token.LOOP:
+                return endCheckLoop();
+
+            case Token.LOCAL_BLOCK:
+            case Token.BLOCK:
+                // there are several special kinds of blocks
+                if (first == null)
+                    return END_DROPS_OFF;
+
+                switch(first.type) {
+                    case Token.LABEL:
+                        return first.endCheckLabel();
+
+                    case Token.IFNE:
+                        return first.endCheckIf();
+
+                    case Token.SWITCH:
+                        return first.endCheckSwitch();
+
+                    case Token.TRY:
+                        return first.endCheckTry();
+
+                    default:
+                        return endCheckBlock();
+                }
+
+            default:
+                return END_DROPS_OFF;
+        }
+    }
 
     public boolean hasSideEffects()
     {
@@ -881,4 +1215,3 @@ public class Node
      */
     private PropListItem propListHead;
 }
-

js/rhino/src/org/mozilla/javascript/Parser.java

@@ -95,6 +95,7 @@ public class Parser
     private ObjArray loopSet;
     private ObjArray loopAndSwitchSet;
     private boolean hasReturnValue;
+    private int functionEndFlags;
 // end of per function variables
 
     // Exception to unwind
@@ -516,6 +517,7 @@ public class Parser
         ObjArray savedLoopAndSwitchSet = loopAndSwitchSet;
         loopAndSwitchSet = null;
         boolean savedHasReturnValue = hasReturnValue;
+        int savedFunctionEndFlags = functionEndFlags;
 
         Node body;
         try {
@@ -544,6 +546,13 @@ public class Parser
             body = parseFunctionBody();
             mustMatchToken(Token.RC, "msg.no.brace.after.body");
 
+            if (compilerEnv.isStrictMode() && !body.hasConsistentReturnUsage())
+            {
+              String msg = name.length() > 0 ? "msg.no.return.value"
+                                             : "msg.anon.no.return.value";
+              addStrictWarning(msg, name);
+            }
+
             decompiler.addToken(Token.RC);
             functionSourceEnd = decompiler.markFunctionEnd(functionSourceStart);
             if (functionType != FunctionNode.FUNCTION_EXPRESSION) {
@@ -565,6 +574,7 @@ public class Parser
         }
         finally {
             hasReturnValue = savedHasReturnValue;
+            functionEndFlags = savedFunctionEndFlags;
             loopAndSwitchSet = savedLoopAndSwitchSet;
             loopSet = savedLoopSet;
             labelSet = savedLabelSet;
@@ -610,17 +620,20 @@ public class Parser
     private Node condition()
         throws IOException, ParserException
     {
-        Node pn;
         mustMatchToken(Token.LP, "msg.no.paren.cond");
         decompiler.addToken(Token.LP);
-        pn = expr(false);
+        Node pn = expr(false);
         mustMatchToken(Token.RP, "msg.no.paren.after.cond");
         decompiler.addToken(Token.RP);
 
-        if (pn.getType() == Token.ASSIGN)
+        // Report strict warning on code like "if (a = 7) ...". Suppress the
+        // warning if the condition is parenthesized, like "if ((a = 7)) ...".
+        if (pn.getProp(Node.PARENTHESIZED_PROP) == null &&
+            (pn.getType() == Token.SETNAME || pn.getType() == Token.SETPROP ||
+             pn.getType() == Token.SETELEM))
+        {
             addStrictWarning("msg.equal.as.assign", "");
-        // there's a check here in jsparse.c that corrects = to ==
-
+        }
         return pn;
     }
 
@@ -1080,6 +1093,20 @@ public class Parser
                 hasReturnValue = true;
             }
             pn = nf.createReturn(retExpr, lineno);
+
+            // see if we need a strict mode warning
+            if (retExpr == null) {
+                if (functionEndFlags == Node.END_RETURNS_VALUE)
+                    addStrictWarning("msg.return.inconsistent", "");
+
+                functionEndFlags |= Node.END_RETURNS;
+            } else {
+                if (functionEndFlags == Node.END_RETURNS)
+                    addStrictWarning("msg.return.inconsistent", "");
+
+                functionEndFlags |= Node.END_RETURNS_VALUE;
+            }
+
             break;
           }
 
@@ -1317,19 +1344,14 @@ public class Parser
     private Node condExpr(boolean inForInit)
         throws IOException, ParserException
     {
-        Node ifTrue;
-        Node ifFalse;
-
         Node pn = orExpr(inForInit);
 
         if (matchToken(Token.HOOK)) {
             decompiler.addToken(Token.HOOK);
-            ifTrue = assignExpr(false);
+            Node ifTrue = assignExpr(false);
             mustMatchToken(Token.COLON, "msg.no.colon.cond");
             decompiler.addToken(Token.COLON);
-            ifFalse = assignExpr(inForInit);
-            if (pn.getType() == Token.ASSIGN)
-                addStrictWarning("msg.equal.as.assign", "");
+            Node ifFalse = assignExpr(inForInit);
             return nf.createCondExpr(pn, ifTrue, ifFalse);
         }
 
@@ -2027,6 +2049,7 @@ public class Parser
              * think) in the C IR as 'function call.'  */
             decompiler.addToken(Token.LP);
             pn = expr(false);
+            pn.putProp(Node.PARENTHESIZED_PROP, Boolean.TRUE);
             decompiler.addToken(Token.RP);
             mustMatchToken(Token.RP, "msg.no.paren");
             return pn;
@@ -2140,4 +2163,3 @@ public class Parser
         return true;
     }
 }
-

js/rhino/src/org/mozilla/javascript/ScriptRuntime.java

@@ -1781,7 +1781,8 @@ public class ScriptRuntime {
             // been defined, creates a new property in the
             // top scope unless strict mode is specified.
             if (cx.hasFeature(Context.FEATURE_STRICT_VARS)) {
-                throw Context.reportRuntimeError1("msg.assn.create.strict", id);
+                Context.reportWarning(
+                    ScriptRuntime.getMessage1("msg.assn.create.strict", id));
             }
             // Find the top scope by walking up the scope chain.
             bound = ScriptableObject.getTopLevelScope(scope);

js/rhino/src/org/mozilla/javascript/resources/Messages.properties

@@ -137,7 +137,7 @@ msg.eval.nonstring =\
 
 msg.eval.nonstring.strict =\
     Calling eval() with anything other than a primitive string value is not \
-    allowed in the strict mode.
+    allowed in strict mode.
 
 # NativeCall
 msg.only.from.new =\
@@ -407,6 +407,15 @@ msg.no.brace.catchblock =\
 msg.try.no.catchfinally =\
     ''try'' without ''catch'' or ''finally''
 
+msg.no.return.value =\
+  function {0} does not always return a value
+
+msg.anon.no.return.value =\
+  anonymous function does not always return a value
+
+msg.return.inconsistent =\
+  return statement is inconsistent with previous usage
+
 msg.syntax =\
     syntax error
 
@@ -436,8 +445,7 @@ msg.var.hides.arg =\
 
 # ScriptRuntime
 msg.assn.create.strict =\
-    Attempt to assign non-existing name "{0}" in the strict mode. \
-    It could indicate a missing variable statement.
+    Assignment to undeclared variable {0}
 
 msg.ref.undefined.prop =\
 	Referenced to undefined property "{0}"

js/rhino/toolsrc/org/mozilla/javascript/tools/resources/Messages.properties

@@ -60,7 +60,9 @@ msg.shell.usage =\
     \    -opt [-1|0-9]\n\
     \    -f script-filename\n\
     \    -e script-source\n\
-    \    -debug
+    \    -debug\n\
+    \    -strict\n\
+    \    -fatal-warnings
 
 
 msg.help =\

js/rhino/toolsrc/org/mozilla/javascript/tools/shell/Main.java

@@ -247,6 +247,7 @@ public class Main
             }
             if (arg.equals("-strict")) {
                 shellContextFactory.setStrictMode(true);
+                errorReporter.setIsReportingWarnings(true);
                 continue;
             }
             if (arg.equals("-fatal-warnings")) {