From 1d2f787d6a80bf850c0dbb3160ede45e9bdee5cb Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Wed, 25 Mar 2020 09:42:50 +0100 Subject: [PATCH] Enable the ESLint `no-shadow` rule This rule is *not* currently enabled in mozilla-central, but it appears commented out[1] in the ESLint definition file; see https://searchfox.org/mozilla-central/rev/c80fa7258c935223fe319c5345b58eae85d4c6ae/tools/lint/eslint/eslint-plugin-mozilla/lib/configs/recommended.js#238-239 Unfortunately this rule is, for fairly obvious reasons, impossible to `--fix` automatically (even partially) and each case thus required careful manual analysis. Hence this ESLint rule is, by some margin, probably the most difficult one that we've enabled thus far. However, using this rule does seem like a good idea in general since allowing variable shadowing could lead to subtle (and difficult to find) bugs or at the very least confusing code. Please find additional details about the ESLint rule at https://eslint.org/docs/rules/no-shadow --- [1] Most likely, a very large number of lint errors have prevented this rule from being enabled thus far. --- .eslintrc | 2 +- external/builder/preprocessor2.js | 4 +-- gulpfile.js | 54 +++++++++++++++---------------- src/core/metrics.js | 10 ++++++ src/shared/message_handler.js | 12 +++---- test/add_test.js | 4 +-- test/driver.js | 6 ++-- test/font/ttxdriver.js | 14 ++++---- test/stats/statcmp.js | 18 +++++------ test/test.js | 12 +++---- test/webbrowser.js | 34 +++++++++---------- test/webserver.js | 12 +++---- 12 files changed, 95 insertions(+), 87 deletions(-) diff --git a/.eslintrc b/.eslintrc index 15a93b5d8..f4419923c 100644 --- a/.eslintrc +++ b/.eslintrc @@ -119,8 +119,8 @@ "no-catch-shadow": "error", "no-delete-var": "error", "no-label-var": "error", + "no-shadow": "error", "no-shadow-restricted-names": "error", - "no-shadow": "off", "no-undef-init": "error", "no-undef": ["error", { "typeof": true, }], "no-unused-vars": ["error", { diff --git a/external/builder/preprocessor2.js b/external/builder/preprocessor2.js index 10faa2702..699f48df1 100644 --- a/external/builder/preprocessor2.js +++ b/external/builder/preprocessor2.js @@ -289,7 +289,7 @@ function traverseTree(ctx, node) { for (var i in node) { var child = node[i]; if (typeof child === "object" && child !== null && child.type) { - var result = traverseTree(ctx, child); + const result = traverseTree(ctx, child); if (result !== child) { node[i] = result; } @@ -300,7 +300,7 @@ function traverseTree(ctx, node) { childItem !== null && childItem.type ) { - var result = traverseTree(ctx, childItem); + const result = traverseTree(ctx, childItem); if (result !== childItem) { child[index] = result; } diff --git a/gulpfile.js b/gulpfile.js index cf697257f..c8f3fc986 100644 --- a/gulpfile.js +++ b/gulpfile.js @@ -167,8 +167,6 @@ function createStringSource(filename, content) { } function createWebpackConfig(defines, output) { - var path = require("path"); - var versionInfo = getVersionJSON(); var bundleDefines = builder.merge(defines, { BUNDLE_VERSION: versionInfo.version, @@ -243,9 +241,9 @@ function createWebpackConfig(defines, output) { }; } -function webpack2Stream(config) { +function webpack2Stream(webpackConfig) { // Replacing webpack1 to webpack2 in the webpack-stream. - return webpackStream(config, webpack2); + return webpackStream(webpackConfig, webpack2); } function getVersionJSON() { @@ -378,28 +376,28 @@ function createImageDecodersBundle(defines) { .pipe(replaceJSRootName(imageDecodersAMDName, "pdfjsImageDecoders")); } -function checkFile(path) { +function checkFile(filePath) { try { - var stat = fs.lstatSync(path); + var stat = fs.lstatSync(filePath); return stat.isFile(); } catch (e) { return false; } } -function checkDir(path) { +function checkDir(dirPath) { try { - var stat = fs.lstatSync(path); + var stat = fs.lstatSync(dirPath); return stat.isDirectory(); } catch (e) { return false; } } -function replaceInFile(path, find, replacement) { - var content = fs.readFileSync(path).toString(); +function replaceInFile(filePath, find, replacement) { + var content = fs.readFileSync(filePath).toString(); content = content.replace(find, replacement); - fs.writeFileSync(path, content); + fs.writeFileSync(filePath, content); } function getTempFile(prefix, suffix) { @@ -407,9 +405,9 @@ function getTempFile(prefix, suffix) { var bytes = require("crypto") .randomBytes(6) .toString("hex"); - var path = BUILD_DIR + "tmp/" + prefix + bytes + suffix; - fs.writeFileSync(path, ""); - return path; + var filePath = BUILD_DIR + "tmp/" + prefix + bytes + suffix; + fs.writeFileSync(filePath, ""); + return filePath; } function createTestSource(testsName, bot) { @@ -527,10 +525,10 @@ gulp.task("buildnumber", function(done) { var version = config.versionPrefix + buildNumber; - exec('git log --format="%h" -n 1', function(err, stdout, stderr) { + exec('git log --format="%h" -n 1', function(err2, stdout2, stderr2) { var buildCommit = ""; - if (!err) { - buildCommit = stdout.replace("\n", ""); + if (!err2) { + buildCommit = stdout2.replace("\n", ""); } createStringSource( @@ -559,9 +557,9 @@ gulp.task("default_preferences-pre", function() { function babelPluginReplaceNonWebPackRequire(babel) { return { visitor: { - Identifier(path, state) { - if (path.node.name === "__non_webpack_require__") { - path.replaceWith(babel.types.identifier("require")); + Identifier(curPath, state) { + if (curPath.node.name === "__non_webpack_require__") { + curPath.replaceWith(babel.types.identifier("require")); } }, }, @@ -643,8 +641,8 @@ gulp.task("locale", function() { var locales = []; for (var i = 0; i < subfolders.length; i++) { var locale = subfolders[i]; - var path = L10N_DIR + locale; - if (!checkDir(path)) { + var dirPath = L10N_DIR + locale; + if (!checkDir(dirPath)) { continue; } if (!/^[a-z][a-z]([a-z])?(-[A-Z][A-Z])?$/.test(locale)) { @@ -656,7 +654,7 @@ gulp.task("locale", function() { locales.push(locale); - if (checkFile(path + "/viewer.properties")) { + if (checkFile(dirPath + "/viewer.properties")) { viewerOutput += "[" + locale + @@ -1163,9 +1161,9 @@ gulp.task( function babelPluginReplaceNonWebPackRequire(babel) { return { visitor: { - Identifier(path, state) { - if (path.node.name === "__non_webpack_require__") { - path.replaceWith(babel.types.identifier("require")); + Identifier(curPath, state) { + if (curPath.node.name === "__non_webpack_require__") { + curPath.replaceWith(babel.types.identifier("require")); } }, }, @@ -1358,9 +1356,9 @@ gulp.task("baseline", function(done) { } exec("git checkout " + baselineCommit, { cwd: workingDirectory }, function( - error + error2 ) { - if (error) { + if (error2) { done(new Error("Baseline commit checkout failed.")); return; } diff --git a/src/core/metrics.js b/src/core/metrics.js index 63373bab2..51b3da815 100644 --- a/src/core/metrics.js +++ b/src/core/metrics.js @@ -23,6 +23,7 @@ var getMetrics = getLookupTableFactory(function(t) { t["Courier-Bold"] = 600; t["Courier-BoldOblique"] = 600; t["Courier-Oblique"] = 600; + // eslint-disable-next-line no-shadow t["Helvetica"] = getLookupTableFactory(function(t) { t["space"] = 278; t["exclam"] = 278; @@ -340,6 +341,7 @@ var getMetrics = getLookupTableFactory(function(t) { t["imacron"] = 278; t["Euro"] = 556; }); + // eslint-disable-next-line no-shadow t["Helvetica-Bold"] = getLookupTableFactory(function(t) { t["space"] = 278; t["exclam"] = 333; @@ -657,6 +659,7 @@ var getMetrics = getLookupTableFactory(function(t) { t["imacron"] = 278; t["Euro"] = 556; }); + // eslint-disable-next-line no-shadow t["Helvetica-BoldOblique"] = getLookupTableFactory(function(t) { t["space"] = 278; t["exclam"] = 333; @@ -974,6 +977,7 @@ var getMetrics = getLookupTableFactory(function(t) { t["imacron"] = 278; t["Euro"] = 556; }); + // eslint-disable-next-line no-shadow t["Helvetica-Oblique"] = getLookupTableFactory(function(t) { t["space"] = 278; t["exclam"] = 278; @@ -1291,6 +1295,7 @@ var getMetrics = getLookupTableFactory(function(t) { t["imacron"] = 278; t["Euro"] = 556; }); + // eslint-disable-next-line no-shadow t["Symbol"] = getLookupTableFactory(function(t) { t["space"] = 250; t["exclam"] = 333; @@ -1483,6 +1488,7 @@ var getMetrics = getLookupTableFactory(function(t) { t["bracerightbt"] = 494; t["apple"] = 790; }); + // eslint-disable-next-line no-shadow t["Times-Roman"] = getLookupTableFactory(function(t) { t["space"] = 250; t["exclam"] = 333; @@ -1800,6 +1806,7 @@ var getMetrics = getLookupTableFactory(function(t) { t["imacron"] = 278; t["Euro"] = 500; }); + // eslint-disable-next-line no-shadow t["Times-Bold"] = getLookupTableFactory(function(t) { t["space"] = 250; t["exclam"] = 333; @@ -2117,6 +2124,7 @@ var getMetrics = getLookupTableFactory(function(t) { t["imacron"] = 278; t["Euro"] = 500; }); + // eslint-disable-next-line no-shadow t["Times-BoldItalic"] = getLookupTableFactory(function(t) { t["space"] = 250; t["exclam"] = 389; @@ -2434,6 +2442,7 @@ var getMetrics = getLookupTableFactory(function(t) { t["imacron"] = 278; t["Euro"] = 500; }); + // eslint-disable-next-line no-shadow t["Times-Italic"] = getLookupTableFactory(function(t) { t["space"] = 250; t["exclam"] = 333; @@ -2751,6 +2760,7 @@ var getMetrics = getLookupTableFactory(function(t) { t["imacron"] = 278; t["Euro"] = 500; }); + // eslint-disable-next-line no-shadow t["ZapfDingbats"] = getLookupTableFactory(function(t) { t["space"] = 278; t["a1"] = 974; diff --git a/src/shared/message_handler.js b/src/shared/message_handler.js index 3ff409d0a..5bcd1f65d 100644 --- a/src/shared/message_handler.js +++ b/src/shared/message_handler.js @@ -114,15 +114,15 @@ class MessageHandler { throw new Error(`Unknown action from worker: ${data.action}`); } if (data.callbackId) { - const sourceName = this.sourceName; - const targetName = data.sourceName; + const cbSourceName = this.sourceName; + const cbTargetName = data.sourceName; new Promise(function(resolve) { resolve(action(data.data)); }).then( function(result) { comObj.postMessage({ - sourceName, - targetName, + sourceName: cbSourceName, + targetName: cbTargetName, callback: CallbackKind.DATA, callbackId: data.callbackId, data: result, @@ -130,8 +130,8 @@ class MessageHandler { }, function(reason) { comObj.postMessage({ - sourceName, - targetName, + sourceName: cbSourceName, + targetName: cbTargetName, callback: CallbackKind.ERROR, callbackId: data.callbackId, reason: wrapReason(reason), diff --git a/test/add_test.js b/test/add_test.js index 067b6170f..9a2117960 100644 --- a/test/add_test.js +++ b/test/add_test.js @@ -22,9 +22,9 @@ if (!fs.existsSync(file)) { throw new Error(`PDF file does not exist '${file}'.`); } -function calculateMD5(file, callback) { +function calculateMD5(pdfFile, callback) { var hash = crypto.createHash("md5"); - var stream = fs.createReadStream(file); + var stream = fs.createReadStream(pdfFile); stream.on("data", function(data) { hash.update(data); }); diff --git a/test/driver.js b/test/driver.js index b8dff5ece..5458f3b2b 100644 --- a/test/driver.js +++ b/test/driver.js @@ -228,10 +228,10 @@ var rasterizeAnnotationLayer = (function rasterizeAnnotationLayerClosure() { for (var i = 0, ii = data.length; i < ii; i++) { images[i].src = data[i]; loadedPromises.push( - new Promise(function(resolve, reject) { - images[i].onload = resolve; + new Promise(function(resolveImage, rejectImage) { + images[i].onload = resolveImage; images[i].onerror = function(e) { - reject(new Error("Error loading image " + e)); + rejectImage(new Error("Error loading image " + e)); }; }) ); diff --git a/test/font/ttxdriver.js b/test/font/ttxdriver.js index 318c5b5cf..61c3275f2 100644 --- a/test/font/ttxdriver.js +++ b/test/font/ttxdriver.js @@ -24,10 +24,10 @@ var ttxResourcesHome = path.join(__dirname, "..", "ttx"); var nextTTXTaskId = Date.now(); -function runTtx(ttxResourcesHome, fontPath, registerOnCancel, callback) { - fs.realpath(ttxResourcesHome, function(err, ttxResourcesHome) { - var fontToolsHome = path.join(ttxResourcesHome, "fonttools-code"); - fs.realpath(fontPath, function(err, fontPath) { +function runTtx(ttxResourcesHomePath, fontPath, registerOnCancel, callback) { + fs.realpath(ttxResourcesHomePath, function(error, realTtxResourcesHomePath) { + var fontToolsHome = path.join(realTtxResourcesHomePath, "fonttools-code"); + fs.realpath(fontPath, function(errorFontPath, realFontPath) { var ttxPath = path.join("Tools", "ttx"); if (!fs.existsSync(path.join(fontToolsHome, ttxPath))) { callback("TTX was not found, please checkout PDF.js submodules"); @@ -38,7 +38,7 @@ function runTtx(ttxResourcesHome, fontPath, registerOnCancel, callback) { PYTHONDONTWRITEBYTECODE: true, }; var ttxStdioMode = "ignore"; - var ttx = spawn("python", [ttxPath, fontPath], { + var ttx = spawn("python", [ttxPath, realFontPath], { cwd: fontToolsHome, stdio: ttxStdioMode, env: ttxEnv, @@ -49,8 +49,8 @@ function runTtx(ttxResourcesHome, fontPath, registerOnCancel, callback) { callback(reason); ttx.kill(); }); - ttx.on("error", function(err) { - ttxRunError = err; + ttx.on("error", function(errorTtx) { + ttxRunError = errorTtx; callback("Unable to execute ttx"); }); ttx.on("close", function(code) { diff --git a/test/stats/statcmp.js b/test/stats/statcmp.js index 406026e75..e7b7204f4 100644 --- a/test/stats/statcmp.js +++ b/test/stats/statcmp.js @@ -37,16 +37,16 @@ function parseOptions() { function group(stats, groupBy) { var vals = []; for (var i = 0; i < stats.length; i++) { - var stat = stats[i]; + var curStat = stats[i]; var keyArr = []; for (var j = 0; j < groupBy.length; j++) { - keyArr.push(stat[groupBy[j]]); + keyArr.push(curStat[groupBy[j]]); } var key = keyArr.join(","); if (vals[key] === undefined) { vals[key] = []; } - vals[key].push(stat["time"]); + vals[key].push(curStat["time"]); } return vals; } @@ -57,13 +57,13 @@ function group(stats, groupBy) { */ function flatten(stats) { var rows = []; - stats.forEach(function(stat) { - stat["stats"].forEach(function(s) { + stats.forEach(function(curStat) { + curStat["stats"].forEach(function(s) { rows.push({ - browser: stat["browser"], - page: stat["page"], - pdf: stat["pdf"], - round: stat["round"], + browser: curStat["browser"], + page: curStat["page"], + pdf: curStat["pdf"], + round: curStat["round"], stat: s["name"], time: s["end"] - s["start"], }); diff --git a/test/test.js b/test/test.js index 38eafb4e9..5cc7bbf29 100644 --- a/test/test.js +++ b/test/test.js @@ -619,8 +619,8 @@ function refTestPostHandler(req, res) { if (pathname === "/tellMeToQuit") { // finding by path var browserPath = parsedUrl.query.path; - session = sessions.filter(function(session) { - return session.config.path === browserPath; + session = sessions.filter(function(curSession) { + return curSession.config.path === browserPath; })[0]; monitorBrowserTimeout(session, null); closeSession(session.name); @@ -689,7 +689,7 @@ function refTestPostHandler(req, res) { return true; } -function startUnitTest(url, name) { +function startUnitTest(testUrl, name) { var startTime = Date.now(); startServer(); server.hooks["POST"].push(unitTestPostHandler); @@ -712,7 +712,7 @@ function startUnitTest(url, name) { var runtime = (Date.now() - startTime) / 1000; console.log(name + " tests runtime was " + runtime.toFixed(1) + " seconds"); }; - startBrowsers(url, function(session) { + startBrowsers(testUrl, function(session) { session.numRuns = 0; session.numErrors = 0; }); @@ -784,7 +784,7 @@ function unitTestPostHandler(req, res) { return true; } -function startBrowsers(url, initSessionCallback) { +function startBrowsers(testUrl, initSessionCallback) { var browsers; if (options.browserManifestFile) { browsers = JSON.parse(fs.readFileSync(options.browserManifestFile)); @@ -801,7 +801,7 @@ function startBrowsers(url, initSessionCallback) { var browser = WebBrowser.create(b); var startUrl = getServerBaseAddress() + - url + + testUrl + "?browser=" + encodeURIComponent(b.name) + "&manifestFile=" + diff --git a/test/webbrowser.js b/test/webbrowser.js index 226423b19..93137ec87 100644 --- a/test/webbrowser.js +++ b/test/webbrowser.js @@ -26,9 +26,9 @@ var crypto = require("crypto"); var tempDirPrefix = "pdfjs_"; -function WebBrowser(name, path, headless) { +function WebBrowser(name, execPath, headless) { this.name = name; - this.path = path; + this.path = execPath; this.headless = headless; this.tmpDir = null; this.profileDir = null; @@ -197,7 +197,7 @@ WebBrowser.prototype = { // Note: First process' output it shown, the later outputs are suppressed. execAsyncNoStdin( cmdKillAll, - function checkAlive(exitCode, firstStdout) { + function checkAlive(firstExitCode, firstStdout) { execAsyncNoStdin( cmdCheckAllKilled, function(exitCode, stdout) { @@ -227,14 +227,14 @@ WebBrowser.prototype = { var firefoxResourceDir = path.join(__dirname, "resources", "firefox"); -function FirefoxBrowser(name, path, headless) { +function FirefoxBrowser(name, execPath, headless) { if (os.platform() === "darwin") { - var m = /([^.\/]+)\.app(\/?)$/.exec(path); + var m = /([^.\/]+)\.app(\/?)$/.exec(execPath); if (m) { - path += (m[2] ? "" : "/") + "Contents/MacOS/firefox"; + execPath += (m[2] ? "" : "/") + "Contents/MacOS/firefox"; } } - WebBrowser.call(this, name, path, headless); + WebBrowser.call(this, name, execPath, headless); } FirefoxBrowser.prototype = Object.create(WebBrowser.prototype); FirefoxBrowser.prototype.buildArguments = function(url) { @@ -253,15 +253,15 @@ FirefoxBrowser.prototype.setupProfileDir = function(dir) { testUtils.copySubtreeSync(firefoxResourceDir, dir); }; -function ChromiumBrowser(name, path, headless) { +function ChromiumBrowser(name, execPath, headless) { if (os.platform() === "darwin") { - var m = /([^.\/]+)\.app(\/?)$/.exec(path); + var m = /([^.\/]+)\.app(\/?)$/.exec(execPath); if (m) { - path += (m[2] ? "" : "/") + "Contents/MacOS/" + m[1]; - console.log(path); + execPath += (m[2] ? "" : "/") + "Contents/MacOS/" + m[1]; + console.log(execPath); } } - WebBrowser.call(this, name, path, headless); + WebBrowser.call(this, name, execPath, headless); } ChromiumBrowser.prototype = Object.create(WebBrowser.prototype); ChromiumBrowser.prototype.buildArguments = function(url) { @@ -291,18 +291,18 @@ ChromiumBrowser.prototype.buildArguments = function(url) { WebBrowser.create = function(desc) { var name = desc.name; - var path = fs.realpathSync(desc.path); - if (!path) { + var execPath = fs.realpathSync(desc.path); + if (!execPath) { throw new Error("Browser executable not found: " + desc.path); } if (/firefox/i.test(name)) { - return new FirefoxBrowser(name, path, desc.headless); + return new FirefoxBrowser(name, execPath, desc.headless); } if (/(chrome|chromium|opera)/i.test(name)) { - return new ChromiumBrowser(name, path, desc.headless); + return new ChromiumBrowser(name, execPath, desc.headless); } - return new WebBrowser(name, path, desc.headless); + return new WebBrowser(name, execPath, desc.headless); }; exports.WebBrowser = WebBrowser; diff --git a/test/webserver.js b/test/webserver.js index 8be2a0f4b..54f69e26e 100644 --- a/test/webserver.js +++ b/test/webserver.js @@ -283,15 +283,15 @@ WebServer.prototype = { }); } - function serveRequestedFile(filePath) { - var stream = fs.createReadStream(filePath, { flags: "rs" }); + function serveRequestedFile(reqFilePath) { + var stream = fs.createReadStream(reqFilePath, { flags: "rs" }); stream.on("error", function(error) { res.writeHead(500); res.end(); }); - var ext = path.extname(filePath).toLowerCase(); + var ext = path.extname(reqFilePath).toLowerCase(); var contentType = mimeTypes[ext] || defaultMimeType; if (!disableRangeRequests) { @@ -309,8 +309,8 @@ WebServer.prototype = { stream.pipe(res); } - function serveRequestedFileRange(filePath, start, end) { - var stream = fs.createReadStream(filePath, { + function serveRequestedFileRange(reqFilePath, start, end) { + var stream = fs.createReadStream(reqFilePath, { flags: "rs", start: start, end: end - 1, @@ -321,7 +321,7 @@ WebServer.prototype = { res.end(); }); - var ext = path.extname(filePath).toLowerCase(); + var ext = path.extname(reqFilePath).toLowerCase(); var contentType = mimeTypes[ext] || defaultMimeType; res.setHeader("Accept-Ranges", "bytes");