Chore: refactor directive comment processing (#10007)

Author: not-an-aardvark

lib/linter.js

@@ -179,12 +179,12 @@ function parseListConfig(string) {
  * scope.
  * @param {Scope} globalScope The global scope.
  * @param {Object} config The existing configuration data.
+ * @param {{exportedVariables: Object, enabledGlobals: Object}} commentDirectives Directives from comment configuration
  * @param {Environments} envContext Env context
  * @returns {void}
  */
-function addDeclaredGlobals(globalScope, config, envContext) {
+function addDeclaredGlobals(globalScope, config, commentDirectives, envContext) {
  const declaredGlobals = {},
- exportedGlobals = {},
  explicitGlobals = {},
  builtin = envContext.get("builtin");
 
@@ -199,9 +199,8 @@ function addDeclaredGlobals(globalScope, config, envContext) {
  }
  });
 
- Object.assign(exportedGlobals, config.exported);
  Object.assign(declaredGlobals, config.globals);
- Object.assign(explicitGlobals, config.astGlobals);
+ Object.assign(explicitGlobals, commentDirectives.enabledGlobals);
 
  Object.keys(declaredGlobals).forEach(name => {
  let variable = globalScope.set.get(name);
@@ -229,7 +228,7 @@ function addDeclaredGlobals(globalScope, config, envContext) {
  });
 
  // mark all exported variables as such
- Object.keys(exportedGlobals).forEach(name => {
+ Object.keys(commentDirectives.exportedVariables).forEach(name => {
  const variable = globalScope.set.get(name);
 
  if (variable) {
@@ -283,108 +282,90 @@ function createDisableDirectives(type, loc, value) {
  * where reporting is disabled or enabled and merges them with reporting config.
  * @param {string} filename The file being checked.
  * @param {ASTNode} ast The top node of the AST.
- * @param {Object} config The existing configuration data.
  * @param {function(string): {create: Function}} ruleMapper A map from rule IDs to defined rules
- * @returns {{config: Object, problems: Problem[], disableDirectives: DisableDirective[]}}
- * Modified config object, along with any problems encountered while parsing config comments
+ * @returns {{configuredRules: Object, enabledGlobals: Object, exportedVariables: Object, problems: Problem[], disableDirectives: DisableDirective[]}}
+ * A collection of the directive comments that were found, along with any problems that occurred when parsing
  */
-function modifyConfigsFromComments(filename, ast, config, ruleMapper) {
-
- const commentConfig = {
- exported: {},
- astGlobals: {},
- rules: {},
- env: {}
- };
- const commentRules = {};
+function getDirectiveComments(filename, ast, ruleMapper) {
+ const configuredRules = {};
+ const enabledGlobals = {};
+ const exportedVariables = {};
  const problems = [];
  const disableDirectives = [];
 
  ast.comments.filter(token => token.type !== "Shebang").forEach(comment => {
+ const trimmedCommentText = comment.value.trim();
+ const match = /^(eslint(-\w+){0,3}|exported|globals?)(\s|$)/.exec(trimmedCommentText);
 
- let value = comment.value.trim();
- const match = /^(eslint(-\w+){0,3}|exported|globals?)(\s|$)/.exec(value);
-
- if (match) {
- value = value.slice(match.index + match[1].length);
- if (comment.type === "Block") {
- switch (match[1]) {
- case "exported":
- Object.assign(commentConfig.exported, parseBooleanConfig(value, comment));
- break;
-
- case "globals":
- case "global":
- Object.assign(commentConfig.astGlobals, parseBooleanConfig(value, comment));
- break;
-
- case "eslint-disable":
- [].push.apply(disableDirectives, createDisableDirectives("disable", comment.loc.start, value));
- break;
-
- case "eslint-disable-line":
- if (comment.loc.start.line === comment.loc.end.line) {
- [].push.apply(disableDirectives, createDisableDirectives("disable-line", comment.loc.start, value));
- }
- break;
-
- case "eslint-disable-next-line":
- if (comment.loc.start.line === comment.loc.end.line) {
- [].push.apply(disableDirectives, createDisableDirectives("disable-next-line", comment.loc.start, value));
- }
- break;
-
- case "eslint-enable":
- [].push.apply(disableDirectives, createDisableDirectives("enable", comment.loc.start, value));
- break;
-
- case "eslint": {
- const parseResult = parseJsonConfig(value, comment.loc);
-
- if (parseResult.success) {
- Object.keys(parseResult.config).forEach(name => {
- const ruleValue = parseResult.config[name];
-
- try {
- validator.validateRuleOptions(ruleMapper(name), name, ruleValue);
- } catch (err) {
- problems.push({
- ruleId: name,
- severity: 2,
- source: null,
- message: err.message,
- line: comment.loc.start.line,
- column: comment.loc.start.column + 1,
- endLine: comment.loc.end.line,
- endColumn: comment.loc.end.column + 1,
- nodeType: null
- });
- }
- commentRules[name] = ruleValue;
- });
- } else {
- problems.push(parseResult.error);
- }
+ if (!match) {
+ return;
+ }
 
- break;
+ const directiveValue = trimmedCommentText.slice(match.index + match[1].length);
+
+ if (/^eslint-disable-(next-)?line$/.test(match[1]) && comment.loc.start.line === comment.loc.end.line) {
+ const directiveType = match[1].slice("eslint-".length);
+
+ [].push.apply(disableDirectives, createDisableDirectives(directiveType, comment.loc.start, directiveValue));
+ } else if (comment.type === "Block") {
+ switch (match[1]) {
+ case "exported":
+ Object.assign(exportedVariables, parseBooleanConfig(directiveValue, comment));
+ break;
+
+ case "globals":
+ case "global":
+ Object.assign(enabledGlobals, parseBooleanConfig(directiveValue, comment));
+ break;
+
+ case "eslint-disable":
+ [].push.apply(disableDirectives, createDisableDirectives("disable", comment.loc.start, directiveValue));
+ break;
+
+ case "eslint-enable":
+ [].push.apply(disableDirectives, createDisableDirectives("enable", comment.loc.start, directiveValue));
+ break;
+
+ case "eslint": {
+ const parseResult = parseJsonConfig(directiveValue, comment.loc);
+
+ if (parseResult.success) {
+ Object.keys(parseResult.config).forEach(name => {
+ const ruleValue = parseResult.config[name];
+
+ try {
+ validator.validateRuleOptions(ruleMapper(name), name, ruleValue);
+ } catch (err) {
+ problems.push({
+ ruleId: name,
+ severity: 2,
+ source: null,
+ message: err.message,
+ line: comment.loc.start.line,
+ column: comment.loc.start.column + 1,
+ endLine: comment.loc.end.line,
+ endColumn: comment.loc.end.column + 1,
+ nodeType: null
+ });
+ }
+ configuredRules[name] = ruleValue;
+ });
+ } else {
+ problems.push(parseResult.error);
  }
 
- // no default
- }
- } else { // comment.type === "Line"
- if (match[1] === "eslint-disable-line") {
- [].push.apply(disableDirectives, createDisableDirectives("disable-line", comment.loc.start, value));
- } else if (match[1] === "eslint-disable-next-line") {
- [].push.apply(disableDirectives, createDisableDirectives("disable-next-line", comment.loc.start, value));
+ break;
  }
+
+ // no default
  }
  }
  });
 
- Object.assign(commentConfig.rules, commentRules);
-
  return {
- config: ConfigOps.merge(config, commentConfig),
+ configuredRules,
+ enabledGlobals,
+ exportedVariables,
  problems,
  disableDirectives
  };
@@ -775,10 +756,11 @@ module.exports = class Linter {
  // evaluate arguments
  if (typeof filenameOrOptions === "object") {
  providedFilename = filenameOrOptions.filename;
- allowInlineConfig = filenameOrOptions.allowInlineConfig;
+ allowInlineConfig = filenameOrOptions.allowInlineConfig !== false;
  reportUnusedDisableDirectives = filenameOrOptions.reportUnusedDisableDirectives;
  } else {
  providedFilename = filenameOrOptions;
+ allowInlineConfig = true;
  }
 
  if (typeof textOrSourceCode === "string") {
@@ -861,24 +843,23 @@ module.exports = class Linter {
  }
  }
 
- const problems = [];
  const sourceCode = lastSourceCodes.get(this);
- let disableDirectives;
+ const commentDirectives = allowInlineConfig
+ ? getDirectiveComments(filename, sourceCode.ast, ruleId => this.rules.get(ruleId))
+ : { configuredRules: {}, enabledGlobals: {}, exportedVariables: {}, problems: [], disableDirectives: [] };
 
- // parse global comments and modify config
- if (allowInlineConfig !== false) {
- const modifyConfigResult = modifyConfigsFromComments(filename, sourceCode.ast, config, ruleId => this.rules.get(ruleId));
+ const configuredRules = Object.assign({}, config.rules, commentDirectives.configuredRules);
 
- config = modifyConfigResult.config;
- modifyConfigResult.problems.forEach(problem => problems.push(problem));
- disableDirectives = modifyConfigResult.disableDirectives;
- } else {
- disableDirectives = [];
- }
+ // augment global scope with declared global variables
+ addDeclaredGlobals(
+ sourceCode.scopeManager.scopes[0],
+ config,
+ { exportedVariables: commentDirectives.exportedVariables, enabledGlobals: commentDirectives.enabledGlobals },
+ this.environments
+ );
 
  const emitter = createEmitter();
  const traverser = new Traverser();
- const scopeManager = sourceCode.scopeManager;
 
  /*
  * Create a frozen object with the ruleContext properties and methods that are shared by all rules.
@@ -890,11 +871,11 @@ module.exports = class Linter {
  Object.create(BASE_TRAVERSAL_CONTEXT),
  {
  getAncestors: () => traverser.parents(),
- getDeclaredVariables: scopeManager.getDeclaredVariables.bind(scopeManager),
+ getDeclaredVariables: sourceCode.scopeManager.getDeclaredVariables.bind(sourceCode.scopeManager),
  getFilename: () => filename,
- getScope: () => getScope(scopeManager, traverser.current(), config.parserOptions.ecmaVersion),
+ getScope: () => getScope(sourceCode.scopeManager, traverser.current(), config.parserOptions.ecmaVersion),
  getSourceCode: () => sourceCode,
- markVariableAsUsed: name => markVariableAsUsed(scopeManager, traverser.current(), config.parserOptions, name),
+ markVariableAsUsed: name => markVariableAsUsed(sourceCode.scopeManager, traverser.current(), config.parserOptions, name),
  parserOptions: config.parserOptions,
  parserPath: config.parser,
  parserServices: sourceCode.parserServices,
@@ -916,9 +897,11 @@ module.exports = class Linter {
  )
  );
 
+ const lintingProblems = [];
+
  // enable appropriate rules
- Object.keys(config.rules).forEach(ruleId => {
- const severity = ConfigOps.getRuleSeverity(config.rules[ruleId]);
+ Object.keys(configuredRules).forEach(ruleId => {
+ const severity = ConfigOps.getRuleSeverity(configuredRules[ruleId]);
 
  if (severity === 0) {
  return;
@@ -932,7 +915,7 @@ module.exports = class Linter {
  Object.create(sharedTraversalContext),
  {
  id: ruleId,
- options: getRuleOptions(config.rules[ruleId]),
+ options: getRuleOptions(configuredRules[ruleId]),
  report() {
 
  /*
@@ -953,7 +936,7 @@ module.exports = class Linter {
  if (problem.fix && rule.meta && !rule.meta.fixable) {
  throw new Error("Fixable rules should export a `meta.fixable` property.");
  }
- problems.push(problem);
+ lintingProblems.push(problem);
 
  /*
  * This is used to avoid breaking rules that used monkeypatch Linter, and relied on
@@ -995,9 +978,6 @@ module.exports = class Linter {
  }
  });
 
- // augment global scope with declared global variables
- addDeclaredGlobals(scopeManager.scopes[0], config, this.environments);
-
  const eventGenerator = new CodePathAnalyzer(new NodeEventGenerator(emitter));
 
  /*
@@ -1018,8 +998,10 @@ module.exports = class Linter {
  });
 
  return applyDisableDirectives({
- directives: disableDirectives,
- problems: problems.sort((problemA, problemB) => problemA.line - problemB.line || problemA.column - problemB.column),
+ directives: commentDirectives.disableDirectives,
+ problems: lintingProblems
+ .concat(commentDirectives.problems)
+ .sort((problemA, problemB) => problemA.line - problemB.line || problemA.column - problemB.column),
  reportUnusedDisableDirectives
  });
  }