Skip to content
Browse files

Better support for catch blocks.

In JavaScript, 'catch' blocks should have their own scope--just like
functions--but we used to treat them as an ordinary blocks meaning
that their scope was combined with the outer function (or global).

This patch changes that by treating catch blocks as anon functions
with a special property '(catch)' that is 'true' only for catch
blocks. This allows us to detect undefined variables better than
before (see GH-247).

In IE, however, variables from the catch scope can leak into the
outer scope. We catch one such example (GH-618):

  var e = 2;

  try {
    throw 'boom';
  } catch (e) {
    // JSHint now warns that 'e' might get overwritten in IE.
  }

  console.log(e + 2); // 'boom2' instead of 4

Naturally, in 'node' environment this warning disappears.

Closes GH-618.
  • Loading branch information...
1 parent 5b9165e commit aa955585467b6c3043e89eacdd3e499b52f1bffd @valueof valueof committed
Showing with 140 additions and 31 deletions.
  1. +84 −30 jshint.js
  2. +9 −1 tests/regression/thirdparty.js
  3. +17 −0 tests/unit/core.js
  4. +23 −0 tests/unit/fixtures/gh247.js
  5. +7 −0 tests/unit/fixtures/gh618.js
View
114 jshint.js
@@ -151,7 +151,7 @@
/*members "\b", "\t", "\n", "\f", "\r", "!=", "!==", "\"", "%", "(begin)",
"(breakage)", "(character)", "(context)", "(error)", "(explicitNewcap)", "(global)",
"(identifier)", "(last)", "(lastcharacter)", "(line)", "(loopage)", "(metrics)",
- "(name)", "(onevar)", "(params)", "(scope)", "(statement)", "(verb)", "(tokens)",
+ "(name)", "(onevar)", "(params)", "(scope)", "(statement)", "(verb)", "(tokens)", "(catch)",
"*", "+", "++", "-", "--", "\/", "<", "<=", "==",
"===", ">", ">=", $, $$, $A, $F, $H, $R, $break, $continue, $w, Abstract, Ajax,
__filename, __dirname, ActiveXObject, Array, ArrayBuffer, ArrayBufferView, Audio,
@@ -215,13 +215,13 @@
removeEventListener, replace, report, require, reserved, resizeBy, resizeTo, resolvePath,
resumeUpdates, respond, rhino, right, runCommand, scroll, scope, screen, scripturl, scrollBy,
scrollTo, scrollbar, search, seal, self, send, serialize, sessionStorage, setInterval, setTimeout,
- setter, setterToken, shift, slice, smarttabs, sort, spawn, split, statementCount, stack, status,
- start, strict, sub, substr, supernew, shadow, supplant, sum, sync, test, toLowerCase, toString,
- toUpperCase, toint32, token, tokens, top, trailing, type, typeOf, Uint16Array, Uint32Array,
- Uint8Array, undef, undefs, unused, urls, validthis, value, valueOf, var, vars, version,
- verifyMaxParametersPerFunction, verifyMaxStatementsPerFunction, verifyMaxComplexityPerFunction,
- verifyMaxNestedBlockDepthPerFunction, WebSocket, withstmt, white, window, windows, Worker, worker,
- wsh*/
+ setter, setterToken, shift, slice, smarttabs, sort, spawn, split, statement, statementCount, stack,
+ status, start, strict, sub, substr, supernew, shadow, supplant, sum, sync, test, toLowerCase,
+ toString, toUpperCase, toint32, token, tokens, top, trailing, type, typeOf, Uint16Array,
+ Uint32Array, Uint8Array, undef, undefs, unused, urls, validthis, value, valueOf, var, vars,
+ version, verifyMaxParametersPerFunction, verifyMaxStatementsPerFunction,
+ verifyMaxComplexityPerFunction, verifyMaxNestedBlockDepthPerFunction, WebSocket, withstmt, white,
+ window, windows, Worker, worker, wsh*/
/*global exports: false */
@@ -1835,13 +1835,22 @@ klass: do {
}
// Define t in the current function in the current scope.
+ if (type === "exception") {
+ if (is_own(funct["(context)"], t)) {
+ if (funct[t] !== true && !option.node) {
+ warning("Value of '{a}' may be overwritten in IE.", nexttoken, t);
+ }
+ }
+ }
+
if (is_own(funct, t) && !funct["(global)"]) {
if (funct[t] === true) {
if (option.latedef)
warning("'{a}' was used before it was defined.", nexttoken, t);
} else {
- if (!option.shadow && type !== "exception")
+ if (!option.shadow && type !== "exception") {
warning("'{a}' is already defined.", nexttoken, t);
+ }
}
}
@@ -2779,7 +2788,10 @@ loop: for (;;) {
d;
inblock = ordinary;
- if (!ordinary || !option.funcscope) scope = Object.create(scope);
+
+ if (!ordinary || !option.funcscope)
+ scope = Object.create(scope);
+
nonadjacent(token, nexttoken);
t = nexttoken;
@@ -3491,16 +3503,16 @@ loop: for (;;) {
}
- function doFunction(i, statement) {
- var f,
- oldOption = option,
- oldScope = scope;
+ function doFunction(name, statement) {
+ var f;
+ var oldOption = option;
+ var oldScope = scope;
option = Object.create(option);
- scope = Object.create(scope);
+ scope = Object.create(scope);
funct = {
- "(name)" : i || "\"" + anonname + "\"",
+ "(name)" : name || "\"" + anonname + "\"",
"(line)" : nexttoken.line,
"(character)": nexttoken.character,
"(context)" : funct,
@@ -3511,12 +3523,16 @@ loop: for (;;) {
"(statement)": statement,
"(tokens)" : {}
};
+
f = funct;
token.funct = funct;
+
functions.push(funct);
- if (i) {
- addlabel(i, "function");
+
+ if (name) {
+ addlabel(name, "function");
}
+
funct["(params)"] = functionparams();
funct["(metrics)"].verifyMaxParametersPerFunction(funct["(params)"]);
@@ -3530,6 +3546,7 @@ loop: for (;;) {
funct["(last)"] = token.line;
funct["(lastcharacter)"] = token.character;
funct = funct["(context)"];
+
return f;
}
@@ -3837,7 +3854,7 @@ loop: for (;;) {
adjacent(token, nexttoken);
addlabel(i, "unction", token);
- doFunction(i, true);
+ doFunction(i, { statement: true });
if (nexttoken.id === "(" && nexttoken.line === token.line) {
error(
"Function declarations are not invocable. Wrap the whole function invocation in parens.");
@@ -3888,29 +3905,65 @@ loop: for (;;) {
});
blockstmt("try", function () {
- var b, e, s;
+ var b;
+
+ function doCatch() {
+ var oldScope = scope;
+ var e;
- block(false);
- if (nexttoken.id === "catch") {
- increaseComplexityCount();
advance("catch");
nonadjacent(token, nexttoken);
advance("(");
- s = scope;
- scope = Object.create(s);
+
+ scope = Object.create(oldScope);
+
e = nexttoken.value;
if (nexttoken.type !== "(identifier)") {
- warning("Expected an identifier and instead saw '{a}'.",
- nexttoken, e);
- } else {
- addlabel(e, "exception");
+ e = null;
+ warning("Expected an identifier and instead saw '{a}'.", nexttoken, e);
}
+
advance();
advance(")");
+
+ funct = {
+ "(name)" : "(catch)",
+ "(line)" : nexttoken.line,
+ "(character)": nexttoken.character,
+ "(context)" : funct,
+ "(breakage)" : funct["(breakage)"],
+ "(loopage)" : funct["(loopage)"],
+ "(scope)" : scope,
+ "(statement)": false,
+ "(metrics)" : createMetrics(nexttoken),
+ "(catch)" : true,
+ "(tokens)" : {}
+ };
+
+ if (e) {
+ addlabel(e, "exception");
+ }
+
+ token.funct = funct;
+ functions.push(funct);
+
block(false);
+
+ scope = oldScope;
+
+ funct["(last)"] = token.line;
+ funct["(lastcharacter)"] = token.character;
+ funct = funct["(context)"];
+ }
+
+ block(false);
+
+ if (nexttoken.id === "catch") {
+ increaseComplexityCount();
+ doCatch();
b = true;
- scope = s;
}
+
if (nexttoken.id === "finally") {
advance("finally");
block(false);
@@ -3919,6 +3972,7 @@ loop: for (;;) {
error("Expected '{a}' and instead saw '{b}'.",
nexttoken, "catch", nexttoken.value);
}
+
return this;
});
View
10 tests/regression/thirdparty.js
@@ -119,6 +119,7 @@ exports.lodash_0_6_1 = function () {
var src = fs.readFileSync(__dirname + '/libs/lodash.js', 'utf8');
var globals = { _: false, define: false };
var options = {
+ unused : true,
expr : true,
eqnull : true,
boss : true,
@@ -182,5 +183,12 @@ exports.lodash_0_6_1 = function () {
exports.json2 = function () {
var src = fs.readFileSync(__dirname + '/libs/json2.js', 'utf8');
- TestRun().test(src, { laxbreak: true }, { JSON: true });
+ TestRun()
+ .addError(177, "'key' is defined but never used.")
+ .addError(191, "'key' is defined but never used.")
+ .test(src, {
+ undef : true,
+ unused : true,
+ laxbreak: true
+ }, { JSON: true });
};
View
17 tests/unit/core.js
@@ -500,3 +500,20 @@ exports.testSparseArrays = function () {
TestRun()
.test(src, { es5: true });
};
+
+exports.testCatchBlocks = function () {
+ var src = fs.readFileSync(__dirname + '/fixtures/gh247.js', 'utf8');
+
+ TestRun()
+ .addError(11, "'w' is not defined.")
+ .test(src, { undef: true, devel: true });
+
+ src = fs.readFileSync(__dirname + '/fixtures/gh618.js', 'utf8');
+
+ TestRun()
+ .addError(5, "Value of 'x' may be overwritten in IE.")
+ .test(src, { undef: true, devel: true });
+
+ TestRun()
+ .test(src, { undef: true, devel: true, node: true });
+};
View
23 tests/unit/fixtures/gh247.js
@@ -0,0 +1,23 @@
+var i = 5;
+
+try {
+ var u = "I'm trying here!";
+} catch (e) {
+ var w = "Let's play catch.";
+}
+
+alert("i:" + i);
+alert("u:" + u);
+alert("w:" + w);
+
+function test() {
+ var w;
+
+ try {
+ alert("Hello.");
+ } catch (e) {
+ var w = "What's up?";
+ }
+
+ alert("w:" + w);
+}
View
7 tests/unit/fixtures/gh618.js
@@ -0,0 +1,7 @@
+var x = 3;
+
+try {
+ throw "boom";
+} catch (x) {}
+
+console.log(x);

0 comments on commit aa95558

Please sign in to comment.
Something went wrong with that request. Please try again.