From dc21b185818a51c39e50049711558817613a7d11 Mon Sep 17 00:00:00 2001 From: "nboyd%atg.com" Date: Mon, 4 Jun 2007 13:18:39 +0000 Subject: [PATCH] More work on strict mode. * Support implemented for checking for inconsistent return usage. Contribution from Roshan James . * Fixed check for assignment in conditional. --- js/rhino/src/org/mozilla/javascript/Node.java | 337 +++++++++++++++++- .../src/org/mozilla/javascript/Parser.java | 48 ++- .../org/mozilla/javascript/ScriptRuntime.java | 3 +- .../javascript/resources/Messages.properties | 14 +- .../tools/resources/Messages.properties | 4 +- .../mozilla/javascript/tools/shell/Main.java | 1 + 6 files changed, 387 insertions(+), 20 deletions(-) diff --git a/js/rhino/src/org/mozilla/javascript/Node.java b/js/rhino/src/org/mozilla/javascript/Node.java index f3e137bc68f..6dedac3d1a9 100644 --- a/js/rhino/src/org/mozilla/javascript/Node.java +++ b/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 " + * 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; } - diff --git a/js/rhino/src/org/mozilla/javascript/Parser.java b/js/rhino/src/org/mozilla/javascript/Parser.java index 4f96db5c88e..68029d42c1d 100644 --- a/js/rhino/src/org/mozilla/javascript/Parser.java +++ b/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; } } - diff --git a/js/rhino/src/org/mozilla/javascript/ScriptRuntime.java b/js/rhino/src/org/mozilla/javascript/ScriptRuntime.java index 76d9d61a4ff..7cdd21dd08e 100644 --- a/js/rhino/src/org/mozilla/javascript/ScriptRuntime.java +++ b/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); diff --git a/js/rhino/src/org/mozilla/javascript/resources/Messages.properties b/js/rhino/src/org/mozilla/javascript/resources/Messages.properties index af1ff6dbe21..b4d0c38a7ee 100644 --- a/js/rhino/src/org/mozilla/javascript/resources/Messages.properties +++ b/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}" diff --git a/js/rhino/toolsrc/org/mozilla/javascript/tools/resources/Messages.properties b/js/rhino/toolsrc/org/mozilla/javascript/tools/resources/Messages.properties index 6e605e7273a..0310b77a8ec 100644 --- a/js/rhino/toolsrc/org/mozilla/javascript/tools/resources/Messages.properties +++ b/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 =\ diff --git a/js/rhino/toolsrc/org/mozilla/javascript/tools/shell/Main.java b/js/rhino/toolsrc/org/mozilla/javascript/tools/shell/Main.java index 05f600d8921..51db9925e9e 100644 --- a/js/rhino/toolsrc/org/mozilla/javascript/tools/shell/Main.java +++ b/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")) {