Skip to content

Breaking: Add support for TypeScript rules #197

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Oct 11, 2021
Merged

Breaking: Add support for TypeScript rules #197

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
# eslint-plugin-eslint-plugin ![CI](https://github.com/not-an-aardvark/eslint-plugin-eslint-plugin/workflows/CI/badge.svg) [![NPM version](https://img.shields.io/npm/v/eslint-plugin-eslint-plugin.svg?style=flat)](https://npmjs.org/package/eslint-plugin-eslint-plugin)

An ESLint plugin for linting ESLint plugins
An ESLint plugin for linting ESLint plugins. Rules written in CJS, ESM, and TypeScript are all supported.

## Installation

Expand Down
57 changes: 40 additions & 17 deletions lib/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,24 @@ function isRuleTesterConstruction (node) {
const INTERESTING_RULE_KEYS = new Set(['create', 'meta']);

/**
* Helper for `getRuleInfo`. Handles ESM rules.
* Collect properties from an object that have interesting key names into a new object
* @param {Node[]} properties
* @param {Set<String>} interestingKeys
* @returns Object
*/
function collectInterestingProperties (properties, interestingKeys) {
// eslint-disable-next-line unicorn/prefer-object-from-entries
return properties.reduce((parsedProps, prop) => {
const keyValue = module.exports.getKeyName(prop);
if (interestingKeys.has(keyValue)) {
parsedProps[keyValue] = prop.value;
}
return parsedProps;
}, {});
}

/**
* Helper for `getRuleInfo`. Handles ESM and TypeScript rules.
*/
function getRuleExportsESM (ast) {
return ast.body
Expand All @@ -95,16 +112,29 @@ function getRuleExportsESM (ast) {
// eslint-disable-next-line unicorn/prefer-object-from-entries
.reduce((currentExports, node) => {
if (node.type === 'ObjectExpression') {
// eslint-disable-next-line unicorn/prefer-object-from-entries
return node.properties.reduce((parsedProps, prop) => {
const keyValue = module.exports.getKeyName(prop);
if (INTERESTING_RULE_KEYS.has(keyValue)) {
parsedProps[keyValue] = prop.value;
}
return parsedProps;
}, {});
// Check `export default { create() {}, meta: {} }`
return collectInterestingProperties(node.properties, INTERESTING_RULE_KEYS);
} else if (isNormalFunctionExpression(node)) {
// Check `export default function() {}`
return { create: node, meta: null, isNewStyle: false };
} else if (
node.type === 'CallExpression' &&
node.typeParameters &&
node.typeParameters.params.length === 2 && // Expecting: <Options, MessageIds>
Copy link
Member

@bradzacher bradzacher Oct 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note that it's not required that you declare type parameters.

In general - we are lazy and avoid declaring the type arguments if there are no options (example).

If there are no options then TS can infer [] as the options type, and it can infer the messageIds from the object literal definition.

Ideally as well you wouldn't ever need to declare two parameters... but that's a problem with TS (there's no way to tell TS "use this explicit generic and infer the second generic) :(

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bradzacher based on what you're telling me here, I'll remove the check for the type parameters to avoid false negatives. Let me know if there are any other common TS rule formats to support or other cues we can use to more-accurately detect TypeScript rules.

node.arguments.length === 1 &&
node.arguments[0].type === 'ObjectExpression' &&
// Check various TypeScript rule helper formats.
(
// createESLintRule({ ... })
node.callee.type === 'Identifier' ||
// util.createRule({ ... })
(node.callee.type === 'MemberExpression' && node.callee.object.type === 'Identifier' && node.callee.property.type === 'Identifier') ||
// ESLintUtils.RuleCreator(docsUrl)({ ... })
(node.callee.type === 'CallExpression' && node.callee.callee.type === 'MemberExpression' && node.callee.callee.object.type === 'Identifier' && node.callee.callee.property.type === 'Identifier')
)
) {
Copy link
Contributor

@aladdin-add aladdin-add Oct 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it seems likely to cause some false positives. I was wondering if we can support a new setting like createrFunc: ["createESLintRule"]. so it would not check other function calls - no false positives, and maybe faster.

Copy link
Member Author

@bmish bmish Oct 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requiring the user to have to configure a global .eslintrc.js setting for all these rules to work is a high barrier-to-entry that I would like to avoid. Perhaps as an optional setting for users with a custom util function, but not required.

Right now, I do believe the chance of false positives is actually very low, given:

  1. We're looking for a specific default-exported function call structure including parameterized types like this: export default [specific-function-call-structure]<Options, MessageIds>({ /* single parameter with object with relevant meta/create keys */ }). If we don't find those relevant keys inside this exact structure, we assume it's not a rule.
  2. We're only linting ESLint plugins, and if needed, a user can choose to only enable the eslint-plugin/rules config for their rules/ directory.
  3. Our getRuleInfo util function currently considers any default export with an object with relevant keys or a function definition to be a potential rule, so if anything, this TypeScript rule check has less chance of false positives than the existing function-style rule check.
  4. Given that we immediately check for this parameterized type structure <Options, MessageIds> and bail out if we don't see that, this new TypeScript rule check should not hurt performance at all either.

That said, I'm more than happy to try to make this TypeScript rule check more strict to further reduce the potential for false positives if we discover any additional import/type/other information we can cue off of. Let me know if you have ideas. But as is, I do believe that the existing checks are more-than-sufficient, and that we could treat any improvements as bug fixes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well, it also makes sense!

// Check `export default someTypeScriptHelper({ create() {}, meta: {} });
return collectInterestingProperties(node.arguments[0].properties, INTERESTING_RULE_KEYS);
}
return currentExports;
}, {});
Expand Down Expand Up @@ -136,14 +166,7 @@ function getRuleExportsCJS (ast) {
} else if (node.right.type === 'ObjectExpression') {
// Check `module.exports = { create: function () {}, meta: {} }`

// eslint-disable-next-line unicorn/prefer-object-from-entries
return node.right.properties.reduce((parsedProps, prop) => {
const keyValue = module.exports.getKeyName(prop);
if (INTERESTING_RULE_KEYS.has(keyValue)) {
parsedProps[keyValue] = prop.value;
}
return parsedProps;
}, {});
return collectInterestingProperties(node.right.properties, INTERESTING_RULE_KEYS);
}
return {};
} else if (
Expand Down
4 changes: 3 additions & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
"statements": 99
},
"devDependencies": {
"@typescript-eslint/parser": "^4.32.0",
"chai": "^4.1.0",
"dirty-chai": "^2.0.1",
"eslint": "^7.9.0",
Expand All @@ -55,7 +56,8 @@
"mocha": "^9.1.2",
"npm-run-all": "^4.1.5",
"nyc": "^15.1.0",
"release-it": "^14.9.0"
"release-it": "^14.9.0",
"typescript": "^4.4.3"
},
"peerDependencies": {
"eslint": ">=6.0.0"
Expand Down
27 changes: 27 additions & 0 deletions tests/lib/rules/require-meta-docs-description.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,3 +241,30 @@ ruleTester.run('require-meta-docs-description', rule, {
},
],
});

const ruleTesterTypeScript = new RuleTester({
parserOptions: { sourceType: 'module' },
parser: require.resolve('@typescript-eslint/parser'),
});
ruleTesterTypeScript.run('require-meta-docs-description (TypeScript)', rule, {
valid: [
`
export default createESLintRule<Options, MessageIds>({
meta: { docs: { description: 'disallow unused variables' } },
create(context) {}
});
`,
],
invalid: [
{
code: `
export default createESLintRule<Options, MessageIds>({
meta: {},
create(context) {}
});
`,
output: null,
errors: [{ messageId: 'missing', type: 'ObjectExpression' }],
},
],
});
115 changes: 111 additions & 4 deletions tests/lib/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,11 @@ const eslintScope = require('eslint-scope');
const estraverse = require('estraverse');
const assert = require('chai').assert;
const utils = require('../../lib/utils');
const typescriptEslintParser = require('@typescript-eslint/parser');

describe('utils', () => {
describe('getRuleInfo', () => {
describe('the file does not have a valid rule', () => {
describe('the file does not have a valid rule (CJS)', () => {
[
'',
'module.exports;',
Expand All @@ -25,6 +26,11 @@ describe('utils', () => {
'module.exports = { create: foo }',
'module.exports = { create: function* foo() {} }',
'module.exports = { create: async function foo() {} }',

// Correct TypeScript helper structure but missing parameterized types (note: we don't support CJS for TypeScript rules):
'module.exports = createESLintRule({ create() {}, meta: {} });',
'module.exports = util.createRule({ create() {}, meta: {} });',
'module.exports = ESLintUtils.RuleCreator(docsUrl)({ create() {}, meta: {} });',
].forEach(noRuleCase => {
it(`returns null for ${noRuleCase}`, () => {
const ast = espree.parse(noRuleCase, { ecmaVersion: 8, range: true });
Expand All @@ -39,6 +45,19 @@ describe('utils', () => {
'export const foo = { create() {} }',
'export default { foo: {} }',
'const foo = {}; export default foo',

// Incorrect TypeScript helper structure:
'export default foo()({ create() {}, meta: {} });',
'export default foo().bar({ create() {}, meta: {} });',
'export default foo.bar.baz({ create() {}, meta: {} });',
'export default foo(123);',
'export default foo.bar(123);',
'export default foo.bar()(123);',

// Correct TypeScript helper structure but missing parameterized types:
'export default createESLintRule({ create() {}, meta: {} });',
'export default util.createRule({ create() {}, meta: {} });',
'export default ESLintUtils.RuleCreator(docsUrl)({ create() {}, meta: {} });',
].forEach(noRuleCase => {
it(`returns null for ${noRuleCase}`, () => {
const ast = espree.parse(noRuleCase, { ecmaVersion: 8, range: true, sourceType: 'module' });
Expand All @@ -47,9 +66,80 @@ describe('utils', () => {
});
});

describe('the file has a valid rule', () => {
describe('the file does not have a valid rule (TypeScript + TypeScript parser + ESM)', () => {
[
// Incorrect TypeScript helper structure:
'export default foo()<Options, MessageIds>({ create() {}, meta: {} });',
'export default foo().bar<Options, MessageIds>({ create() {}, meta: {} });',
'export default foo.bar.baz<Options, MessageIds>({ create() {}, meta: {} });',
'export default foo<Options, MessageIds>(123);',
'export default foo.bar<Options, MessageIds>(123);',
'export default foo.bar()<Options, MessageIds>(123);',

// Correct TypeScript helper structure but missing parameterized types:
'export default createESLintRule({ create() {}, meta: {} });',
'export default createESLintRule<>({ create() {}, meta: {} });',
'export default createESLintRule<OnlyOneType>({ create() {}, meta: {} });',
'export default util.createRule({ create() {}, meta: {} });',
'export default ESLintUtils.RuleCreator(docsUrl)({ create() {}, meta: {} });',
].forEach(noRuleCase => {
it(`returns null for ${noRuleCase}`, () => {
const ast = typescriptEslintParser.parse(noRuleCase, { ecmaVersion: 8, range: true, sourceType: 'module' });
assert.isNull(utils.getRuleInfo({ ast }), 'Expected no rule to be found');
});
});
});

describe('the file does not have a valid rule (TypeScript + TypeScript parser + CJS)', () => {
[
// Correct TypeScript helper structure but missing parameterized types (note: we don't support CJS for TypeScript rules):
'module.exports = createESLintRule<Options, MessageIds>({ create() {}, meta: {} });',
'module.exports = util.createRule<Options, MessageIds>({ create() {}, meta: {} });',
'module.exports = ESLintUtils.RuleCreator(docsUrl)<Options, MessageIds>({ create() {}, meta: {} });',
].forEach(noRuleCase => {
it(`returns null for ${noRuleCase}`, () => {
const ast = typescriptEslintParser.parse(noRuleCase, { range: true, sourceType: 'script' });
assert.isNull(utils.getRuleInfo({ ast }), 'Expected no rule to be found');
});
});
});

describe('the file has a valid rule (TypeScript + TypeScript parser + ESM)', () => {
const CASES = {
// Util function only
'export default createESLintRule<Options, MessageIds>({ create() {}, meta: {} });': {
create: { type: 'FunctionExpression' },
meta: { type: 'ObjectExpression' },
isNewStyle: true,
},
// Util function from util object
'export default util.createRule<Options, MessageIds>({ create() {}, meta: {} });': {
create: { type: 'FunctionExpression' },
meta: { type: 'ObjectExpression' },
isNewStyle: true,
},
// Util function from util object with additional doc URL argument
'export default ESLintUtils.RuleCreator(docsUrl)<Options, MessageIds>({ create() {}, meta: {} });': {
create: { type: 'FunctionExpression' },
meta: { type: 'ObjectExpression' },
isNewStyle: true,
},
};

Object.keys(CASES).forEach(ruleSource => {
it(ruleSource, () => {
const ast = typescriptEslintParser.parse(ruleSource, { ecmaVersion: 6, range: true, sourceType: 'module' });
const ruleInfo = utils.getRuleInfo({ ast });
assert(
lodash.isMatch(ruleInfo, CASES[ruleSource]),
`Expected \n${inspect(ruleInfo)}\nto match\n${inspect(CASES[ruleSource])}`
);
});
});
});

describe('the file has a valid rule (CJS)', () => {
const CASES = {
// CJS
'module.exports = { create: function foo() {} };': {
create: { type: 'FunctionExpression', id: { name: 'foo' } }, // (This property will actually contain the AST node.)
meta: null,
Expand Down Expand Up @@ -125,7 +215,22 @@ describe('utils', () => {
meta: null,
isNewStyle: false,
},
};

Object.keys(CASES).forEach(ruleSource => {
it(ruleSource, () => {
const ast = espree.parse(ruleSource, { ecmaVersion: 6, range: true, sourceType: 'script' });
const ruleInfo = utils.getRuleInfo({ ast });
assert(
lodash.isMatch(ruleInfo, CASES[ruleSource]),
`Expected \n${inspect(ruleInfo)}\nto match\n${inspect(CASES[ruleSource])}`
);
});
});
});

describe('the file has a valid rule (ESM)', () => {
const CASES = {
// ESM (object style)
'export default { create() {} }': {
create: { type: 'FunctionExpression' },
Expand Down Expand Up @@ -153,15 +258,17 @@ describe('utils', () => {

Object.keys(CASES).forEach(ruleSource => {
it(ruleSource, () => {
const ast = espree.parse(ruleSource, { ecmaVersion: 6, range: true, sourceType: ruleSource.startsWith('export default') ? 'module' : 'script' });
const ast = espree.parse(ruleSource, { ecmaVersion: 6, range: true, sourceType: 'module' });
const ruleInfo = utils.getRuleInfo({ ast });
assert(
lodash.isMatch(ruleInfo, CASES[ruleSource]),
`Expected \n${inspect(ruleInfo)}\nto match\n${inspect(CASES[ruleSource])}`
);
});
});
});

describe('the file has a valid rule (different scope options)', () => {
for (const scopeOptions of [
{ ignoreEval: true, ecmaVersion: 6, sourceType: 'script', nodejsScope: true },
{ ignoreEval: true, ecmaVersion: 6, sourceType: 'script' },
Expand Down