From 3e7e2a26202bea562386a6fee069ababb91c0398 Mon Sep 17 00:00:00 2001 From: Brad Zacher Date: Tue, 28 Apr 2020 10:23:34 -0700 Subject: [PATCH 1/2] feat(experimental-utils): expose our RuleTester extension There are a few things we've built in (like automatically adding a filename) that others might find value in. Plus I want to use it in the internal plugin. --- .../tests/RuleTester.ts | 26 ++--- packages/eslint-plugin/tests/RuleTester.ts | 100 +--------------- .../src/eslint-utils/RuleTester.ts | 109 ++++++++++++++++++ .../src/eslint-utils/index.ts | 1 + 4 files changed, 120 insertions(+), 116 deletions(-) create mode 100644 packages/experimental-utils/src/eslint-utils/RuleTester.ts diff --git a/packages/eslint-plugin-internal/tests/RuleTester.ts b/packages/eslint-plugin-internal/tests/RuleTester.ts index daef7ec9a8b..3b7d3afc554 100644 --- a/packages/eslint-plugin-internal/tests/RuleTester.ts +++ b/packages/eslint-plugin-internal/tests/RuleTester.ts @@ -1,22 +1,10 @@ -import { TSESLint, ESLintUtils } from '@typescript-eslint/experimental-utils'; +import { ESLintUtils } from '@typescript-eslint/experimental-utils'; +import path from 'path'; -const { batchedSingleLineTests } = ESLintUtils; - -const parser = '@typescript-eslint/parser'; - -type RuleTesterConfig = Omit & { - parser: typeof parser; -}; -class RuleTester extends TSESLint.RuleTester { - // as of eslint 6 you have to provide an absolute path to the parser - // but that's not as clean to type, this saves us trying to manually enforce - // that contributors require.resolve everything - constructor(options: RuleTesterConfig) { - super({ - ...options, - parser: require.resolve(options.parser), - }); - } +function getFixturesRootDir(): string { + return path.join(__dirname, 'fixtures'); } -export { RuleTester, batchedSingleLineTests }; +const { batchedSingleLineTests, RuleTester } = ESLintUtils; + +export { RuleTester, batchedSingleLineTests, getFixturesRootDir }; diff --git a/packages/eslint-plugin/tests/RuleTester.ts b/packages/eslint-plugin/tests/RuleTester.ts index 1774200db32..dad454369c1 100644 --- a/packages/eslint-plugin/tests/RuleTester.ts +++ b/packages/eslint-plugin/tests/RuleTester.ts @@ -1,104 +1,10 @@ -import { TSESLint, ESLintUtils } from '@typescript-eslint/experimental-utils'; -import { clearCaches } from '@typescript-eslint/parser'; +import { ESLintUtils } from '@typescript-eslint/experimental-utils'; import * as path from 'path'; -const parser = '@typescript-eslint/parser'; - -type RuleTesterConfig = Omit & { - parser: typeof parser; -}; -class RuleTester extends TSESLint.RuleTester { - // as of eslint 6 you have to provide an absolute path to the parser - // but that's not as clean to type, this saves us trying to manually enforce - // that contributors require.resolve everything - constructor(private readonly options: RuleTesterConfig) { - super({ - ...options, - parser: require.resolve(options.parser), - }); - } - private getFilename(options?: TSESLint.ParserOptions): string { - if (options) { - const filename = `file.ts${ - options.ecmaFeatures && options.ecmaFeatures.jsx ? 'x' : '' - }`; - if (options.project) { - return path.join(getFixturesRootDir(), filename); - } - - return filename; - } else if (this.options.parserOptions) { - return this.getFilename(this.options.parserOptions); - } - - return 'file.ts'; - } - - // as of eslint 6 you have to provide an absolute path to the parser - // If you don't do that at the test level, the test will fail somewhat cryptically... - // This is a lot more explicit - run>( - name: string, - rule: TSESLint.RuleModule, - tests: TSESLint.RunTests, - ): void { - const errorMessage = `Do not set the parser at the test level unless you want to use a parser other than ${parser}`; - - // standardize the valid tests as objects - tests.valid = tests.valid.map(test => { - if (typeof test === 'string') { - return { - code: test, - }; - } - return test; - }); - - tests.valid.forEach(test => { - if (typeof test !== 'string') { - if (test.parser === parser) { - throw new Error(errorMessage); - } - if (!test.filename) { - test.filename = this.getFilename(test.parserOptions); - } - } - }); - tests.invalid.forEach(test => { - if (test.parser === parser) { - throw new Error(errorMessage); - } - if (!test.filename) { - test.filename = this.getFilename(test.parserOptions); - } - }); - - super.run(name, rule, tests); - } -} - function getFixturesRootDir(): string { - return path.join(process.cwd(), 'tests/fixtures/'); + return path.join(__dirname, 'fixtures'); } -const { batchedSingleLineTests } = ESLintUtils; - -// make sure that the parser doesn't hold onto file handles between tests -// on linux (i.e. our CI env), there can be very a limited number of watch handles available -afterAll(() => { - clearCaches(); -}); - -/** - * Simple no-op tag to mark code samples as "should not format with prettier" - * for the internal/plugin-test-formatting lint rule - */ -function noFormat(strings: TemplateStringsArray, ...keys: string[]): string { - const lastIndex = strings.length - 1; - return ( - strings.slice(0, lastIndex).reduce((p, s, i) => p + s + keys[i], '') + - strings[lastIndex] - ); -} +const { batchedSingleLineTests, RuleTester, noFormat } = ESLintUtils; export { batchedSingleLineTests, getFixturesRootDir, noFormat, RuleTester }; diff --git a/packages/experimental-utils/src/eslint-utils/RuleTester.ts b/packages/experimental-utils/src/eslint-utils/RuleTester.ts new file mode 100644 index 00000000000..07b3a4cf0cf --- /dev/null +++ b/packages/experimental-utils/src/eslint-utils/RuleTester.ts @@ -0,0 +1,109 @@ +import * as TSESLint from '../ts-eslint'; +import * as path from 'path'; + +const parser = '@typescript-eslint/parser'; + +type RuleTesterConfig = Omit & { + parser: typeof parser; +}; + +class RuleTester extends TSESLint.RuleTester { + // as of eslint 6 you have to provide an absolute path to the parser + // but that's not as clean to type, this saves us trying to manually enforce + // that contributors require.resolve everything + constructor(private readonly options: RuleTesterConfig) { + super({ + ...options, + parser: require.resolve(options.parser), + }); + + // make sure that the parser doesn't hold onto file handles between tests + // on linux (i.e. our CI env), there can be very a limited number of watch handles available + afterAll(() => { + try { + // instead of creating a hard dependency, just use a soft require + // a bit weird, but if they're using this tooling, it'll be installed + require(parser).clearCaches(); + } catch { + // ignored + } + }); + } + private getFilename(options?: TSESLint.ParserOptions): string { + if (options) { + const filename = `file.ts${ + options.ecmaFeatures && options.ecmaFeatures.jsx ? 'x' : '' + }`; + if (options.project) { + return path.join( + options.tsconfigRootDir != null + ? options.tsconfigRootDir + : process.cwd(), + filename, + ); + } + + return filename; + } else if (this.options.parserOptions) { + return this.getFilename(this.options.parserOptions); + } + + return 'file.ts'; + } + + // as of eslint 6 you have to provide an absolute path to the parser + // If you don't do that at the test level, the test will fail somewhat cryptically... + // This is a lot more explicit + run>( + name: string, + rule: TSESLint.RuleModule, + tests: TSESLint.RunTests, + ): void { + const errorMessage = `Do not set the parser at the test level unless you want to use a parser other than ${parser}`; + + // standardize the valid tests as objects + tests.valid = tests.valid.map(test => { + if (typeof test === 'string') { + return { + code: test, + }; + } + return test; + }); + + tests.valid.forEach(test => { + if (typeof test !== 'string') { + if (test.parser === parser) { + throw new Error(errorMessage); + } + if (!test.filename) { + test.filename = this.getFilename(test.parserOptions); + } + } + }); + tests.invalid.forEach(test => { + if (test.parser === parser) { + throw new Error(errorMessage); + } + if (!test.filename) { + test.filename = this.getFilename(test.parserOptions); + } + }); + + super.run(name, rule, tests); + } +} + +/** + * Simple no-op tag to mark code samples as "should not format with prettier" + * for the internal/plugin-test-formatting lint rule + */ +function noFormat(strings: TemplateStringsArray, ...keys: string[]): string { + const lastIndex = strings.length - 1; + return ( + strings.slice(0, lastIndex).reduce((p, s, i) => p + s + keys[i], '') + + strings[lastIndex] + ); +} + +export { noFormat, RuleTester }; diff --git a/packages/experimental-utils/src/eslint-utils/index.ts b/packages/experimental-utils/src/eslint-utils/index.ts index 72977de2003..60452e9aca4 100644 --- a/packages/experimental-utils/src/eslint-utils/index.ts +++ b/packages/experimental-utils/src/eslint-utils/index.ts @@ -2,4 +2,5 @@ export * from './applyDefault'; export * from './batchedSingleLineTests'; export * from './getParserServices'; export * from './RuleCreator'; +export * from './RuleTester'; export * from './deepMerge'; From 78f09422f1955ce4f0702d053f345e5f59ec3a4e Mon Sep 17 00:00:00 2001 From: Brad Zacher Date: Tue, 28 Apr 2020 10:24:46 -0700 Subject: [PATCH 2/2] feat(eslint-plugin-internal): add rule no-poorly-typed-ts-props Simple lint rule to ban usage of properties that introduce bugs. https://github.com/microsoft/TypeScript/issues/24706 --- .../eslint-plugin-internal/src/rules/index.ts | 2 + .../src/rules/no-poorly-typed-ts-props.ts | 110 ++++++++++++++++++ .../tests/fixtures/file.ts | 0 .../tests/fixtures/tsconfig.json | 12 ++ .../rules/no-poorly-typed-ts-props.test.ts | 94 +++++++++++++++ 5 files changed, 218 insertions(+) create mode 100644 packages/eslint-plugin-internal/src/rules/no-poorly-typed-ts-props.ts create mode 100644 packages/eslint-plugin-internal/tests/fixtures/file.ts create mode 100644 packages/eslint-plugin-internal/tests/fixtures/tsconfig.json create mode 100644 packages/eslint-plugin-internal/tests/rules/no-poorly-typed-ts-props.test.ts diff --git a/packages/eslint-plugin-internal/src/rules/index.ts b/packages/eslint-plugin-internal/src/rules/index.ts index eb8d8efe852..17a0a192bf8 100644 --- a/packages/eslint-plugin-internal/src/rules/index.ts +++ b/packages/eslint-plugin-internal/src/rules/index.ts @@ -1,9 +1,11 @@ +import noPoorlyTypedTsProps from './no-poorly-typed-ts-props'; import noTypescriptDefaultImport from './no-typescript-default-import'; import noTypescriptEstreeImport from './no-typescript-estree-import'; import pluginTestFormatting from './plugin-test-formatting'; import preferASTTypesEnum from './prefer-ast-types-enum'; export default { + 'no-poorly-typed-ts-props': noPoorlyTypedTsProps, 'no-typescript-default-import': noTypescriptDefaultImport, 'no-typescript-estree-import': noTypescriptEstreeImport, 'plugin-test-formatting': pluginTestFormatting, diff --git a/packages/eslint-plugin-internal/src/rules/no-poorly-typed-ts-props.ts b/packages/eslint-plugin-internal/src/rules/no-poorly-typed-ts-props.ts new file mode 100644 index 00000000000..a1485e2435f --- /dev/null +++ b/packages/eslint-plugin-internal/src/rules/no-poorly-typed-ts-props.ts @@ -0,0 +1,110 @@ +import { + TSESTree, + ESLintUtils, + TSESLint, +} from '@typescript-eslint/experimental-utils'; +import { createRule } from '../util'; + +/* +TypeScript declares some bad types for certain properties. +See: https://github.com/microsoft/TypeScript/issues/24706 + +This rule simply warns against using them, as using them will likely introduce type safety holes. +*/ + +const BANNED_PROPERTIES = [ + // { + // type: 'Node', + // property: 'parent', + // fixWith: null, + // }, + { + type: 'Symbol', + property: 'declarations', + fixWith: 'getDeclarations()', + }, + { + type: 'Type', + property: 'symbol', + fixWith: 'getSymbol()', + }, +]; + +export default createRule({ + name: 'no-poorly-typed-ts-props', + meta: { + type: 'problem', + docs: { + description: + "Enforces rules don't use TS API properties with known bad type definitions", + category: 'Possible Errors', + recommended: 'error', + requiresTypeChecking: true, + }, + fixable: 'code', + schema: [], + messages: { + doNotUse: 'Do not use {{type}}.{{property}} because it is poorly typed.', + doNotUseWithFixer: + 'Do not use {{type}}.{{property}} because it is poorly typed. Use {{type}}.{{fixWith}} instead.', + suggestedFix: 'Use {{type}}.{{fixWith}} instead.', + }, + }, + defaultOptions: [], + create(context) { + const { program, esTreeNodeToTSNodeMap } = ESLintUtils.getParserServices( + context, + ); + const checker = program.getTypeChecker(); + + return { + ':matches(MemberExpression, OptionalMemberExpression)[computed = false]'( + node: + | TSESTree.MemberExpressionNonComputedName + | TSESTree.OptionalMemberExpressionNonComputedName, + ): void { + for (const banned of BANNED_PROPERTIES) { + if (node.property.name !== banned.property) { + continue; + } + + // make sure the type name matches + const tsObjectNode = esTreeNodeToTSNodeMap.get(node.object); + const objectType = checker.getTypeAtLocation(tsObjectNode); + const objectSymbol = objectType.getSymbol(); + if (objectSymbol?.getName() !== banned.type) { + continue; + } + + const tsNode = esTreeNodeToTSNodeMap.get(node.property); + const symbol = checker.getSymbolAtLocation(tsNode); + const decls = symbol?.getDeclarations(); + const isFromTs = decls?.some(decl => + decl.getSourceFile().fileName.includes('/node_modules/typescript/'), + ); + if (isFromTs !== true) { + continue; + } + + return context.report({ + node, + messageId: banned.fixWith ? 'doNotUseWithFixer' : 'doNotUse', + data: banned, + suggest: [ + { + messageId: 'suggestedFix', + fix(fixer): TSESLint.RuleFix | null { + if (banned.fixWith == null) { + return null; + } + + return fixer.replaceText(node.property, banned.fixWith); + }, + }, + ], + }); + } + }, + }; + }, +}); diff --git a/packages/eslint-plugin-internal/tests/fixtures/file.ts b/packages/eslint-plugin-internal/tests/fixtures/file.ts new file mode 100644 index 00000000000..e69de29bb2d diff --git a/packages/eslint-plugin-internal/tests/fixtures/tsconfig.json b/packages/eslint-plugin-internal/tests/fixtures/tsconfig.json new file mode 100644 index 00000000000..7e9126b848c --- /dev/null +++ b/packages/eslint-plugin-internal/tests/fixtures/tsconfig.json @@ -0,0 +1,12 @@ +{ + "compilerOptions": { + "jsx": "preserve", + "target": "es5", + "module": "commonjs", + "strict": true, + "esModuleInterop": true, + "lib": ["es2015", "es2017", "esnext"], + "experimentalDecorators": true + }, + "include": ["file.ts"] +} diff --git a/packages/eslint-plugin-internal/tests/rules/no-poorly-typed-ts-props.test.ts b/packages/eslint-plugin-internal/tests/rules/no-poorly-typed-ts-props.test.ts new file mode 100644 index 00000000000..63b1f8d5726 --- /dev/null +++ b/packages/eslint-plugin-internal/tests/rules/no-poorly-typed-ts-props.test.ts @@ -0,0 +1,94 @@ +import rule from '../../src/rules/no-poorly-typed-ts-props'; +import { RuleTester, getFixturesRootDir } from '../RuleTester'; + +const ruleTester = new RuleTester({ + parser: '@typescript-eslint/parser', + parserOptions: { + project: './tsconfig.json', + tsconfigRootDir: getFixturesRootDir(), + sourceType: 'module', + }, +}); + +ruleTester.run('no-poorly-typed-ts-props', rule, { + valid: [ + ` +declare const foo: { declarations: string[] }; +foo.declarations.map(decl => console.log(decl)); + `, + ` +declare const bar: Symbol; +bar.declarations.map(decl => console.log(decl)); + `, + ` +declare const baz: Type; +baz.symbol.name; + `, + ], + invalid: [ + { + code: ` +import ts from 'typescript'; +declare const thing: ts.Symbol; +thing.declarations.map(decl => {}); + `.trimRight(), + errors: [ + { + messageId: 'doNotUseWithFixer', + data: { + type: 'Symbol', + property: 'declarations', + fixWith: 'getDeclarations()', + }, + line: 4, + suggestions: [ + { + messageId: 'suggestedFix', + data: { + type: 'Symbol', + fixWith: 'getDeclarations()', + }, + output: ` +import ts from 'typescript'; +declare const thing: ts.Symbol; +thing.getDeclarations().map(decl => {}); + `.trimRight(), + }, + ], + }, + ], + }, + { + code: ` +import ts from 'typescript'; +declare const thing: ts.Type; +thing.symbol; + `.trimRight(), + errors: [ + { + messageId: 'doNotUseWithFixer', + data: { + type: 'Type', + property: 'symbol', + fixWith: 'getSymbol()', + }, + line: 4, + suggestions: [ + { + messageId: 'suggestedFix', + data: { + type: 'Type', + fixWith: 'getSymbol()', + }, + output: ` +import ts from 'typescript'; +declare const thing: ts.Type; +thing.getSymbol(); + `.trimRight(), + }, + ], + }, + ], + }, + ], +});