Skip to content

Commit

Permalink
Chore: avoid handling rules instances in config-validator (#9364)
Browse files Browse the repository at this point in the history 
This is a modified version of #9277, which was merged but then reverted later for performance reasons. The performance issues from #9177 have been fixed (`Linter` no longer creates a new map of rules whenever it lints text).

The goal of this change is to progress towards being able to remove the `rules` property from `Linter` instances. Unfortunately, this isn't possible yet, since there are a few other modules that rely on reading the list of defined rules.
  • Loading branch information
@not-an-aardvark
not-an-aardvark committed Nov 10, 2017
1 parent fe5ac7e commit 4def876
Show file tree
Hide file tree
Showing 6 changed files with 130 additions and 112 deletions.
10 changes: 7 additions & 3 deletions lib/cli-engine.js
View file
Expand Up @@ -409,9 +409,13 @@ class CLIEngine {
});
}

Object.keys(this.options.rules || {}).forEach(name => {
validator.validateRuleOptions(name, this.options.rules[name], "CLI", this.linter.rules);
});
if (this.options.rules && Object.keys(this.options.rules).length) {
const loadedRules = this.linter.getRules();

Object.keys(this.options.rules).forEach(name => {
validator.validateRuleOptions(loadedRules.get(name), name, this.options.rules[name], "CLI");
});
}

this.config = new Config(this.options, this.linter);
}
Expand Down
4 changes: 3 additions & 1 deletion lib/config/config-file.js
View file
Expand Up @@ -501,8 +501,10 @@ function loadFromDisk(resolvedPath, configContext) {
}
}

const ruleMap = configContext.linterContext.getRules();

// validate the configuration before continuing
validator.validate(config, resolvedPath.configFullName, configContext.linterContext.rules, configContext.linterContext.environments);
validator.validate(config, resolvedPath.configFullName, ruleMap.get.bind(ruleMap), configContext.linterContext.environments);

/*
* If an `extends` property is defined, it represents a configuration file to use as
Expand Down
52 changes: 26 additions & 26 deletions lib/config/config-validator.js
View file
Expand Up @@ -14,9 +14,7 @@ const ajv = require("../util/ajv"),
configSchema = require("../../conf/config-schema.js"),
util = require("util");

const validators = {
rules: Object.create(null)
};
const ruleValidators = new WeakMap();

//------------------------------------------------------------------------------
// Private
Expand All @@ -25,13 +23,11 @@ let validateSchema;

/**
* Gets a complete options schema for a rule.
* @param {string} id The rule's unique name.
* @param {Rules} rulesContext Rule context
* @param {{create: Function, schema: (Array|null)}} rule A new-style rule object
* @returns {Object} JSON Schema for the rule's options.
*/
function getRuleOptionsSchema(id, rulesContext) {
const rule = rulesContext.get(id),
schema = rule && rule.schema || rule && rule.meta && rule.meta.schema;
function getRuleOptionsSchema(rule) {
const schema = rule.schema || rule.meta && rule.meta.schema;

// Given a tuple of schemas, insert warning level at the beginning
if (Array.isArray(schema)) {
Expand Down Expand Up @@ -72,19 +68,20 @@ function validateRuleSeverity(options) {

/**
* Validates the non-severity options passed to a rule, based on its schema.
* @param {string} id The rule's unique name
* @param {{create: Function}} rule The rule to validate
* @param {array} localOptions The options for the rule, excluding severity
* @param {Rules} rulesContext Rule context
* @returns {void}
*/
function validateRuleSchema(id, localOptions, rulesContext) {
const schema = getRuleOptionsSchema(id, rulesContext);
function validateRuleSchema(rule, localOptions) {
if (!ruleValidators.has(rule)) {
const schema = getRuleOptionsSchema(rule);

if (!validators.rules[id] && schema) {
validators.rules[id] = ajv.compile(schema);
if (schema) {
ruleValidators.set(rule, ajv.compile(schema));
}
}

const validateRule = validators.rules[id];
const validateRule = ruleValidators.get(rule);

if (validateRule) {
validateRule(localOptions);
Expand All @@ -98,21 +95,24 @@ function validateRuleSchema(id, localOptions, rulesContext) {

/**
* Validates a rule's options against its schema.
* @param {string} id The rule's unique name.
* @param {{create: Function}|null} rule The rule that the config is being validated for
* @param {string} ruleId The rule's unique name.
* @param {array|number} options The given options for the rule.
* @param {string} source The name of the configuration source to report in any errors.
* @param {Rules} rulesContext Rule context
* @returns {void}
*/
function validateRuleOptions(id, options, source, rulesContext) {
function validateRuleOptions(rule, ruleId, options, source) {
if (!rule) {
return;
}
try {
const severity = validateRuleSeverity(options);

if (severity !== 0 && !(typeof severity === "string" && severity.toLowerCase() === "off")) {
validateRuleSchema(id, Array.isArray(options) ? options.slice(1) : [], rulesContext);
validateRuleSchema(rule, Array.isArray(options) ? options.slice(1) : []);
}
} catch (err) {
throw new Error(`${source}:\n\tConfiguration for rule "${id}" is invalid:\n${err.message}`);
throw new Error(`${source}:\n\tConfiguration for rule "${ruleId}" is invalid:\n${err.message}`);
}
}

Expand Down Expand Up @@ -143,16 +143,16 @@ function validateEnvironment(environment, source, envContext) {
* Validates a rules config object
* @param {Object} rulesConfig The rules config object to validate.
* @param {string} source The name of the configuration source to report in any errors.
* @param {Rules} rulesContext Rule context
* @param {function(string): {create: Function}} ruleMapper A mapper function from strings to loaded rules
* @returns {void}
*/
function validateRules(rulesConfig, source, rulesContext) {
function validateRules(rulesConfig, source, ruleMapper) {
if (!rulesConfig) {
return;
}

Object.keys(rulesConfig).forEach(id => {
validateRuleOptions(id, rulesConfig[id], source, rulesContext);
validateRuleOptions(ruleMapper(id), id, rulesConfig[id], source);
});
}

Expand Down Expand Up @@ -223,13 +223,13 @@ function validateConfigSchema(config, source) {
* Validates an entire config object.
* @param {Object} config The config object to validate.
* @param {string} source The name of the configuration source to report in any errors.
* @param {Rules} rulesContext The rules context
* @param {function(string): {create: Function}} ruleMapper A mapper function from rule IDs to defined rules
* @param {Environments} envContext The env context
* @returns {void}
*/
function validate(config, source, rulesContext, envContext) {
function validate(config, source, ruleMapper, envContext) {
validateConfigSchema(config, source);
validateRules(config.rules, source, rulesContext);
validateRules(config.rules, source, ruleMapper);
validateEnvironment(config.env, source, envContext);
}

Expand Down
8 changes: 4 additions & 4 deletions lib/linter.js
View file
Expand Up @@ -278,7 +278,7 @@ function createDisableDirectives(type, loc, value) {
* @param {string} filename The file being checked.
* @param {ASTNode} ast The top node of the AST.
* @param {Object} config The existing configuration data.
* @param {Linter} linterContext Linter context object
* @param {function(string): {create: Function}} ruleMapper A map from rule IDs to defined rules
* @returns {{
* config: Object,
* problems: Problem[],
Expand All @@ -291,7 +291,7 @@ function createDisableDirectives(type, loc, value) {
* }} Modified config object, along with any problems encountered
* while parsing config comments
*/
function modifyConfigsFromComments(filename, ast, config, linterContext) {
function modifyConfigsFromComments(filename, ast, config, ruleMapper) {

const commentConfig = {
exported: {},
Expand Down Expand Up @@ -337,7 +337,7 @@ function modifyConfigsFromComments(filename, ast, config, linterContext) {
Object.keys(parseResult.config).forEach(name => {
const ruleValue = parseResult.config[name];

validator.validateRuleOptions(name, ruleValue, `${filename} line ${comment.loc.start.line}`, linterContext.rules);
validator.validateRuleOptions(ruleMapper(name), name, ruleValue, `${filename} line ${comment.loc.start.line}`);
commentRules[name] = ruleValue;
});
} else {
Expand Down Expand Up @@ -806,7 +806,7 @@ module.exports = class Linter {

// parse global comments and modify config
if (allowInlineConfig !== false) {
const modifyConfigResult = modifyConfigsFromComments(filename, sourceCode.ast, config, this);
const modifyConfigResult = modifyConfigsFromComments(filename, sourceCode.ast, config, ruleId => this.rules.get(ruleId));

config = modifyConfigResult.config;
modifyConfigResult.problems.forEach(problem => problems.push(problem));
Expand Down
57 changes: 30 additions & 27 deletions lib/testers/rule-tester.js
View file
Expand Up @@ -271,6 +271,23 @@ class RuleTester {
].concat(scenarioErrors).join("\n"));
}


linter.defineRule(ruleName, Object.assign({}, rule, {

// Create a wrapper rule that freezes the `context` properties.
create(context) {
freezeDeeply(context.options);
freezeDeeply(context.settings);
freezeDeeply(context.parserOptions);

return (typeof rule === "function" ? rule : rule.create)(context);
}
}));

linter.defineRules(this.rules);

const ruleMap = linter.getRules();

/**
* Run the rule for the given item
* @param {string|Object} item Item to run the rule against
Expand Down Expand Up @@ -313,20 +330,22 @@ class RuleTester {
config.rules[ruleName] = 1;
}

linter.defineRule(ruleName, Object.assign({}, rule, {
const schema = validator.getRuleOptionsSchema(rule);

// Create a wrapper rule that freezes the `context` properties.
create(context) {
freezeDeeply(context.options);
freezeDeeply(context.settings);
freezeDeeply(context.parserOptions);

return (typeof rule === "function" ? rule : rule.create)(context);
/*
* Setup AST getters.
* The goal is to check whether or not AST was modified when
* running the rule under test.
*/
linter.defineRule("rule-tester/validate-ast", () => ({
Program(node) {
beforeAST = cloneDeeplyExcludesParent(node);
},
"Program:exit"(node) {
afterAST = node;
}
}));

const schema = validator.getRuleOptionsSchema(ruleName, linter.rules);

if (schema) {
ajv.validateSchema(schema);

Expand All @@ -341,21 +360,7 @@ class RuleTester {
}
}

validator.validate(config, "rule-tester", linter.rules, new Environments());

/*
* Setup AST getters.
* The goal is to check whether or not AST was modified when
* running the rule under test.
*/
linter.defineRule("rule-tester/validate-ast", () => ({
Program(node) {
beforeAST = cloneDeeplyExcludesParent(node);
},
"Program:exit"(node) {
afterAST = node;
}
}));
validator.validate(config, "rule-tester", ruleMap.get.bind(ruleMap), new Environments());

return {
messages: linter.verify(code, config, filename, true),
Expand Down Expand Up @@ -532,7 +537,6 @@ class RuleTester {
RuleTester.describe("valid", () => {
test.valid.forEach(valid => {
RuleTester.it(typeof valid === "object" ? valid.code : valid, () => {
linter.defineRules(this.rules);
testValidTemplate(valid);
});
});
Expand All @@ -541,7 +545,6 @@ class RuleTester {
RuleTester.describe("invalid", () => {
test.invalid.forEach(invalid => {
RuleTester.it(invalid.code, () => {
linter.defineRules(this.rules);
testInvalidTemplate(invalid);
});
});
Expand Down

0 comments on commit 4def876

Please sign in to comment.