Skip to content

Commit

Permalink
Breaking: make no-redeclare stricter (fixes #11370, fixes #11405) (#1…
Browse files Browse the repository at this point in the history 
…1509)
  • Loading branch information
@mysticatea
mysticatea committed Apr 25, 2019
1 parent ed675a6 commit 20364cc
Show file tree
Hide file tree
Showing 13 changed files with 474 additions and 148 deletions.
7 changes: 7 additions & 0 deletions docs/developer-guide/working-with-rules.md
View file
Expand Up @@ -169,6 +169,13 @@ This method returns the scope which has the following types:
**※2** Only if the `for` statement defines the iteration variable as a block-scoped variable (E.g., `for (let i = 0;;) {}`).<br>
**※3** The scope of the closest ancestor node which has own scope. If the closest ancestor node has multiple scopes then it chooses the innermost scope (E.g., the `Program` node has a `global` scope and a `module` scope if `Program#sourceType` is `"module"`. The innermost scope is the `module` scope.).

The returned value is a [`Scope` object](scope-manager-interface.md) defined by the `eslint-scope` package. The `Variable` objects of global variables have some additional properties.

* `variable.writeable` (`boolean | undefined`) ... If `true`, this global variable can be assigned arbitrary value. If `false`, this global variable is read-only.
* `variable.eslintExplicitGlobal` (`boolean | undefined`) ... If `true`, this global variable was defined by a `/* globals */` directive comment in the source code file.
* `variable.eslintExplicitGlobalComments` (`Comment[] | undefined`) ... The array of `/* globals */` directive comments which defined this global variable in the source code file. This property is `undefined` if there are no `/* globals */` directive comments.
* `variable.eslintImplicitGlobalSetting` (`"readonly" | "writable" | undefined`) ... The configured value in config files. This can be different from `variable.writeable` if there are `/* globals */` directive comments.

### context.report()

The main method you'll use is `context.report()`, which publishes a warning or error (depending on the configuration being used). This method accepts a single argument, which is an object containing the following properties:
Expand Down
2 changes: 1 addition & 1 deletion docs/rules/no-redeclare.md
View file
Expand Up @@ -27,7 +27,7 @@ a = 10;

## Options

This rule takes one optional argument, an object with a boolean property `"builtinGlobals"`. It defaults to `false`.
This rule takes one optional argument, an object with a boolean property `"builtinGlobals"`. It defaults to `true`.
If set to `true`, this rule also checks redeclaration of built-in globals, such as `Object`, `Array`, `Number`...

### builtinGlobals
Expand Down
4 changes: 2 additions & 2 deletions lib/config/config-ops.js
View file
Expand Up @@ -385,14 +385,14 @@ module.exports = {
case "true":
case "writeable":
case "writable":
return "writeable";
return "writable";

case null:
case false:
case "false":
case "readable":
case "readonly":
return "readable";
return "readonly";

default:
throw new Error(`'${configuredValue}' is not a valid configuration for a global (use 'readonly', 'writable', or 'off')`);
Expand Down
77 changes: 42 additions & 35 deletions lib/linter.js
View file
Expand Up @@ -70,39 +70,41 @@ const commentParser = new ConfigCommentParser();
* @param {{exportedVariables: Object, enabledGlobals: Object}} commentDirectives Directives from comment configuration
* @returns {void}
*/
function addDeclaredGlobals(globalScope, configGlobals, commentDirectives) {
const mergedGlobalsInfo = Object.assign(
{},
function addDeclaredGlobals(globalScope, configGlobals, { exportedVariables, enabledGlobals }) {

// Define configured global variables.
for (const id of new Set([...Object.keys(configGlobals), ...Object.keys(enabledGlobals)])) {

/*
* `ConfigOps.normalizeConfigGlobal` will throw an error if a configured global value is invalid. However, these errors would
* typically be caught when validating a config anyway (validity for inline global comments is checked separately).
*/
lodash.mapValues(configGlobals, value => ({ sourceComment: null, value: ConfigOps.normalizeConfigGlobal(value) })),
lodash.mapValues(commentDirectives.enabledGlobals, ({ comment, value }) => ({ sourceComment: comment, value }))
);
const configValue = configGlobals[id] === void 0 ? void 0 : ConfigOps.normalizeConfigGlobal(configGlobals[id]);
const commentValue = enabledGlobals[id] && enabledGlobals[id].value;
const value = commentValue || configValue;
const sourceComments = enabledGlobals[id] && enabledGlobals[id].comments;

Object.keys(mergedGlobalsInfo)
.filter(name => mergedGlobalsInfo[name].value !== "off")
.forEach(name => {
let variable = globalScope.set.get(name);

if (!variable) {
variable = new eslintScope.Variable(name, globalScope);
if (mergedGlobalsInfo[name].sourceComment === null) {
variable.eslintExplicitGlobal = false;
} else {
variable.eslintExplicitGlobal = true;
variable.eslintExplicitGlobalComment = mergedGlobalsInfo[name].sourceComment;
}
globalScope.variables.push(variable);
globalScope.set.set(name, variable);
}
variable.writeable = (mergedGlobalsInfo[name].value === "writeable");
});
if (value === "off") {
continue;
}

let variable = globalScope.set.get(id);

if (!variable) {
variable = new eslintScope.Variable(id, globalScope);

globalScope.variables.push(variable);
globalScope.set.set(id, variable);
}

variable.eslintImplicitGlobalSetting = configValue;
variable.eslintExplicitGlobal = sourceComments !== void 0;
variable.eslintExplicitGlobalComments = sourceComments;
variable.writeable = (value === "writable");
}

// mark all exported variables as such
Object.keys(commentDirectives.exportedVariables).forEach(name => {
Object.keys(exportedVariables).forEach(name => {
const variable = globalScope.set.get(name);

if (variable) {
Expand Down Expand Up @@ -157,7 +159,7 @@ function createDisableDirectives(type, loc, value) {
* @param {string} filename The file being checked.
* @param {ASTNode} ast The top node of the AST.
* @param {function(string): {create: Function}} ruleMapper A map from rule IDs to defined rules
* @returns {{configuredRules: Object, enabledGlobals: Object, exportedVariables: Object, problems: Problem[], disableDirectives: DisableDirective[]}}
* @returns {{configuredRules: Object, enabledGlobals: {value:string,comment:Token}[], exportedVariables: Object, problems: Problem[], disableDirectives: DisableDirective[]}}
* A collection of the directive comments that were found, along with any problems that occurred when parsing
*/
function getDirectiveComments(filename, ast, ruleMapper) {
Expand Down Expand Up @@ -201,11 +203,8 @@ function getDirectiveComments(filename, ast, ruleMapper) {
break;

case "globals":
case "global": {
const updatedGlobals = commentParser.parseStringConfig(directiveValue, comment);

Object.keys(updatedGlobals).forEach(globalName => {
const { value } = updatedGlobals[globalName];
case "global":
for (const [id, { value }] of Object.entries(commentParser.parseStringConfig(directiveValue, comment))) {
let normalizedValue;

try {
Expand All @@ -221,13 +220,21 @@ function getDirectiveComments(filename, ast, ruleMapper) {
endColumn: comment.loc.end.column + 1,
nodeType: null
});
return;
continue;
}

enabledGlobals[globalName] = { comment, value: normalizedValue };
});
if (enabledGlobals[id]) {
enabledGlobals[id].comments.push(comment);
enabledGlobals[id].value = normalizedValue;
} else {
enabledGlobals[id] = {
comments: [comment],
value: normalizedValue
};
}
}
break;
}

case "eslint-disable":
disableDirectives.push(...createDisableDirectives("disable", comment.loc.start, directiveValue));
break;
Expand Down
150 changes: 109 additions & 41 deletions lib/rules/no-redeclare.js
View file
Expand Up @@ -5,6 +5,12 @@

"use strict";

//------------------------------------------------------------------------------
// Requirements
//------------------------------------------------------------------------------

const astUtils = require("../util/ast-utils");

//------------------------------------------------------------------------------
// Rule Definition
//------------------------------------------------------------------------------
Expand All @@ -20,11 +26,17 @@ module.exports = {
url: "https://eslint.org/docs/rules/no-redeclare"
},

messages: {
redeclared: "'{{id}}' is already defined.",
redeclaredAsBuiltin: "'{{id}}' is already defined as a built-in global variable.",
redeclaredBySyntax: "'{{id}}' is already defined by a variable declaration."
},

schema: [
{
type: "object",
properties: {
builtinGlobals: { type: "boolean", default: false }
builtinGlobals: { type: "boolean", default: true }
},
additionalProperties: false
}
Expand All @@ -33,72 +45,128 @@ module.exports = {

create(context) {
const options = {
builtinGlobals: context.options[0] && context.options[0].builtinGlobals
builtinGlobals: Boolean(
context.options.length === 0 ||
context.options[0].builtinGlobals
)
};
const sourceCode = context.getSourceCode();

/**
* Find variables in a given scope and flag redeclared ones.
* @param {Scope} scope - An eslint-scope scope object.
* @returns {void}
* @private
* Iterate declarations of a given variable.
* @param {escope.variable} variable The variable object to iterate declarations.
* @returns {IterableIterator<{type:string,node:ASTNode,loc:SourceLocation}>} The declarations.
*/
function findVariablesInScope(scope) {
scope.variables.forEach(variable => {
const hasBuiltin = options.builtinGlobals && "writeable" in variable;
const count = (hasBuiltin ? 1 : 0) + variable.identifiers.length;
function *iterateDeclarations(variable) {
if (options.builtinGlobals && (
variable.eslintImplicitGlobalSetting === "readonly" ||
variable.eslintImplicitGlobalSetting === "writable"
)) {
yield { type: "builtin" };
}

if (count >= 2) {
variable.identifiers.sort((a, b) => a.range[1] - b.range[1]);
for (const id of variable.identifiers) {
yield { type: "syntax", node: id, loc: id.loc };
}

for (let i = (hasBuiltin ? 0 : 1), l = variable.identifiers.length; i < l; i++) {
context.report({ node: variable.identifiers[i], message: "'{{a}}' is already defined.", data: { a: variable.name } });
}
if (variable.eslintExplicitGlobalComments) {
for (const comment of variable.eslintExplicitGlobalComments) {
yield {
type: "comment",
node: comment,
loc: astUtils.getNameLocationInGlobalDirectiveComment(
sourceCode,
comment,
variable.name
)
};
}
});

}
}

/**
* Find variables in the current scope.
* @param {ASTNode} node - The Program node.
* Find variables in a given scope and flag redeclared ones.
* @param {Scope} scope - An eslint-scope scope object.
* @returns {void}
* @private
*/
function checkForGlobal(node) {
const scope = context.getScope(),
parserOptions = context.parserOptions,
ecmaFeatures = parserOptions.ecmaFeatures || {};

// Nodejs env or modules has a special scope.
if (ecmaFeatures.globalReturn || node.sourceType === "module") {
findVariablesInScope(scope.childScopes[0]);
} else {
findVariablesInScope(scope);
function findVariablesInScope(scope) {
for (const variable of scope.variables) {
const [
declaration,
...extraDeclarations
] = iterateDeclarations(variable);

if (extraDeclarations.length === 0) {
continue;
}

/*
* If the type of a declaration is different from the type of
* the first declaration, it shows the location of the first
* declaration.
*/
const detailMessageId = declaration.type === "builtin"
? "redeclaredAsBuiltin"
: "redeclaredBySyntax";
const data = { id: variable.name };

// Report extra declarations.
for (const { type, node, loc } of extraDeclarations) {
const messageId = type === declaration.type
? "redeclared"
: detailMessageId;

context.report({ node, loc, messageId, data });
}
}
}

/**
* Find variables in the current scope.
* @param {ASTNode} node The node of the current scope.
* @returns {void}
* @private
*/
function checkForBlock() {
findVariablesInScope(context.getScope());
}
function checkForBlock(node) {
const scope = context.getScope();

if (context.parserOptions.ecmaVersion >= 6) {
return {
Program: checkForGlobal,
BlockStatement: checkForBlock,
SwitchStatement: checkForBlock
};
/*
* In ES5, some node type such as `BlockStatement` doesn't have that scope.
* `scope.block` is a different node in such a case.
*/
if (scope.block === node) {
findVariablesInScope(scope);
}
}

return {
Program: checkForGlobal,
Program() {
const scope = context.getScope();

findVariablesInScope(scope);

// Node.js or ES modules has a special scope.
if (
scope.type === "global" &&
scope.childScopes[0] &&

// The special scope's block is the Program node.
scope.block === scope.childScopes[0].block
) {
findVariablesInScope(scope.childScopes[0]);
}
},

FunctionDeclaration: checkForBlock,
FunctionExpression: checkForBlock,
ArrowFunctionExpression: checkForBlock
};
ArrowFunctionExpression: checkForBlock,

BlockStatement: checkForBlock,
ForStatement: checkForBlock,
ForInStatement: checkForBlock,
ForOfStatement: checkForBlock,
SwitchStatement: checkForBlock
};
}
};

0 comments on commit 20364cc

Please sign in to comment.