Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Breaking: make no-redeclare stricter (fixes #11370, fixes #11405) #11509

Merged
merged 17 commits into from Apr 25, 2019
Merged
Changes from 11 commits
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

@@ -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 that [`eslint-scope` package defined](scope-manager-interface.md). The `Variable` objects of global variables have some additional properties.
This conversation was marked as resolved by mysticatea

This comment has been minimized.

Copy link
@platinumazure

platinumazure Mar 28, 2019

Member

Possible wording improvement:

Suggested change
The returned value is a `Scope` object that [`eslint-scope` package defined](scope-manager-interface.md). The `Variable` objects of global variables have some additional properties.
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:
@@ -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
@@ -387,18 +387,18 @@ 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";

// Fallback to minimize compatibility impact
default:
return "writeable";
return "writable";
}
}
};
@@ -70,34 +70,36 @@ 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(
{},
lodash.mapValues(configGlobals, value => ({ sourceComment: null, value: ConfigOps.normalizeConfigGlobal(value) })),
lodash.mapValues(commentDirectives.enabledGlobals, ({ comment, value }) => ({ sourceComment: comment, value: ConfigOps.normalizeConfigGlobal(value) }))
);
function addDeclaredGlobals(globalScope, configGlobals, { exportedVariables, enabledGlobals }) {

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");
});
// Define configured global variables.
for (const id of new Set([...Object.keys(configGlobals), ...Object.keys(enabledGlobals)])) {
const configValue = configGlobals[id] === void 0 ? void 0 : ConfigOps.normalizeConfigGlobal(configGlobals[id]);
const commentValue = enabledGlobals[id] && ConfigOps.normalizeConfigGlobal(enabledGlobals[id].value);
const value = commentValue || configValue;
const sourceComments = enabledGlobals[id] && enabledGlobals[id].comments;

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) {
@@ -152,7 +154,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) {
@@ -197,7 +199,14 @@ function getDirectiveComments(filename, ast, ruleMapper) {

case "globals":
case "global":
Object.assign(enabledGlobals, commentParser.parseStringConfig(directiveValue, comment));
for (const [id, { value }] of Object.entries(commentParser.parseStringConfig(directiveValue, comment))) {
if (enabledGlobals[id]) {
enabledGlobals[id].comments.push(comment);
enabledGlobals[id].value = value;
} else {
enabledGlobals[id] = { comments: [comment], value };
}
}
break;

case "eslint-disable":
@@ -5,6 +5,12 @@

"use strict";

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

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

//------------------------------------------------------------------------------
// Rule Definition
//------------------------------------------------------------------------------
@@ -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
}
@@ -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
};
}
};
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.