From 4cd5590208188beb2e91afa96fe5c0627d3fe895 Mon Sep 17 00:00:00 2001 From: Brad Zacher Date: Thu, 9 May 2019 09:32:28 -0700 Subject: [PATCH] feat(eslint-plugin): Add better non-null handling [no-unnecessary-type-assertion] (#478) --- .eslintrc.json | 1 + .../eslint-plugin/src/rules/member-naming.ts | 8 +- .../src/rules/no-extraneous-class.ts | 4 +- .../rules/no-unnecessary-type-assertion.ts | 233 ++++++++++++------ packages/eslint-plugin/src/util/types.ts | 54 +++- .../no-unnecessary-type-assertion.test.ts | 158 ++++++++++++ 6 files changed, 381 insertions(+), 77 deletions(-) diff --git a/.eslintrc.json b/.eslintrc.json index 818b90dfab5..2039fcec10e 100644 --- a/.eslintrc.json +++ b/.eslintrc.json @@ -8,6 +8,7 @@ "extends": ["eslint:recommended", "plugin:@typescript-eslint/recommended"], "rules": { "comma-dangle": ["error", "always-multiline"], + "curly": ["error", "all"], "no-mixed-operators": "error", "no-console": "off", "no-undef": "off", diff --git a/packages/eslint-plugin/src/rules/member-naming.ts b/packages/eslint-plugin/src/rules/member-naming.ts index 1e6fcdd226e..c0252331f79 100644 --- a/packages/eslint-plugin/src/rules/member-naming.ts +++ b/packages/eslint-plugin/src/rules/member-naming.ts @@ -76,9 +76,13 @@ export default util.createRule({ const convention = conventions[accessibility]; const method = node as TSESTree.MethodDefinition; - if (method.kind === 'constructor') return; + if (method.kind === 'constructor') { + return; + } - if (!convention || convention.test(name)) return; + if (!convention || convention.test(name)) { + return; + } context.report({ node: node.key, diff --git a/packages/eslint-plugin/src/rules/no-extraneous-class.ts b/packages/eslint-plugin/src/rules/no-extraneous-class.ts index 4799f211e7d..461b8c6efa8 100644 --- a/packages/eslint-plugin/src/rules/no-extraneous-class.ts +++ b/packages/eslint-plugin/src/rules/no-extraneous-class.ts @@ -96,7 +96,9 @@ export default util.createRule({ onlyStatic = false; } } - if (!(onlyStatic || onlyConstructor)) break; + if (!(onlyStatic || onlyConstructor)) { + break; + } } if (onlyConstructor) { diff --git a/packages/eslint-plugin/src/rules/no-unnecessary-type-assertion.ts b/packages/eslint-plugin/src/rules/no-unnecessary-type-assertion.ts index ca86b6c9ca6..1e78c75338d 100644 --- a/packages/eslint-plugin/src/rules/no-unnecessary-type-assertion.ts +++ b/packages/eslint-plugin/src/rules/no-unnecessary-type-assertion.ts @@ -1,5 +1,15 @@ import { TSESTree } from '@typescript-eslint/typescript-estree'; -import * as tsutils from 'tsutils'; +import { + isCallExpression, + isNewExpression, + isObjectType, + isObjectFlagSet, + isParameterDeclaration, + isPropertyDeclaration, + isStrictCompilerOptionEnabled, + isTypeFlagSet, + isVariableDeclaration, +} from 'tsutils'; import ts from 'typescript'; import * as util from '../util'; @@ -8,7 +18,7 @@ type Options = [ typesToIgnore?: string[]; } ]; -type MessageIds = 'unnecessaryAssertion'; +type MessageIds = 'contextuallyUnnecessary' | 'unnecessaryAssertion'; export default util.createRule({ name: 'no-unnecessary-type-assertion', @@ -24,6 +34,8 @@ export default util.createRule({ messages: { unnecessaryAssertion: 'This assertion is unnecessary since it does not change the type of the expression.', + contextuallyUnnecessary: + 'This assertion is unnecessary since the receiver accepts the original type of the expression.', }, schema: [ { @@ -44,6 +56,8 @@ export default util.createRule({ create(context, [options]) { const sourceCode = context.getSourceCode(); const parserServices = util.getParserServices(context); + const checker = parserServices.program.getTypeChecker(); + const compilerOptions = parserServices.program.getCompilerOptions(); /** * Sometimes tuple types don't have ObjectFlags.Tuple set, like when they're being matched against an inferred type. @@ -76,91 +90,170 @@ export default util.createRule({ return true; } - function checkNonNullAssertion( - node: TSESTree.Node, + /** + * Returns the contextual type of a given node. + * Contextual type is the type of the target the node is going into. + * i.e. the type of a called function's parameter, or the defined type of a variable declaration + */ + function getContextualType( checker: ts.TypeChecker, - ): void { - const originalNode = parserServices.esTreeNodeToTSNodeMap.get< - ts.NonNullExpression - >(node); - const type = checker.getTypeAtLocation(originalNode.expression); - - if (type === checker.getNonNullableType(type)) { - context.report({ - node, - messageId: 'unnecessaryAssertion', - fix(fixer) { - return fixer.removeRange([ - originalNode.expression.end, - originalNode.end, - ]); - }, - }); + node: ts.Expression, + ): ts.Type | undefined { + const parent = node.parent; + if (!parent) { + return; } - } - function verifyCast( - node: TSESTree.TSTypeAssertion | TSESTree.TSAsExpression, - checker: ts.TypeChecker, - ): void { - if ( - options && - options.typesToIgnore && - options.typesToIgnore.indexOf( - sourceCode.getText(node.typeAnnotation), - ) !== -1 + if (isCallExpression(parent) || isNewExpression(parent)) { + if (node === parent.expression) { + // is the callee, so has no contextual type + return; + } + } else if ( + isVariableDeclaration(parent) || + isPropertyDeclaration(parent) || + isParameterDeclaration(parent) + ) { + return parent.type + ? checker.getTypeFromTypeNode(parent.type) + : undefined; + } else if ( + ![ts.SyntaxKind.TemplateSpan, ts.SyntaxKind.JsxExpression].includes( + parent.kind, + ) ) { + // parent is not something we know we can get the contextual type of return; } + // TODO - support return statement checking - const originalNode = parserServices.esTreeNodeToTSNodeMap.get< - ts.AssertionExpression - >(node); - const castType = checker.getTypeAtLocation(originalNode); + return checker.getContextualType(node); + } + /** + * Returns true if there's a chance the variable has been used before a value has been assigned to it + */ + function isPossiblyUsedBeforeAssigned(node: ts.Expression): boolean { + const declaration = util.getDeclaration(checker, node); if ( - tsutils.isTypeFlagSet(castType, ts.TypeFlags.Literal) || - (tsutils.isObjectType(castType) && - (tsutils.isObjectFlagSet(castType, ts.ObjectFlags.Tuple) || - couldBeTupleType(castType))) + // non-strict mode doesn't care about used before assigned errors + isStrictCompilerOptionEnabled(compilerOptions, 'strictNullChecks') && + // ignore class properties as they are compile time guarded + // also ignore function arguments as they can't be used before defined + isVariableDeclaration(declaration) && + // is it `const x!: number` + declaration.initializer === undefined && + declaration.exclamationToken === undefined && + declaration.type !== undefined ) { - // It's not always safe to remove a cast to a literal type or tuple - // type, as those types are sometimes widened without the cast. - return; + // check if the defined variable type has changed since assignment + const declarationType = checker.getTypeFromTypeNode(declaration.type); + const type = util.getConstrainedTypeAtLocation(checker, node); + if (declarationType === type) { + // possibly used before assigned, so just skip it + // better to false negative and skip it, than false postiive and fix to compile erroring code + // + // no better way to figure this out right now + // https://github.com/Microsoft/TypeScript/issues/31124 + return true; + } } + return false; + } + + return { + TSNonNullExpression(node) { + const originalNode = parserServices.esTreeNodeToTSNodeMap.get< + ts.NonNullExpression + >(node); + const type = util.getConstrainedTypeAtLocation( + checker, + originalNode.expression, + ); + + if (!util.isNullableType(type)) { + if (isPossiblyUsedBeforeAssigned(originalNode.expression)) { + return; + } - const uncastType = checker.getTypeAtLocation(originalNode.expression); - - if (uncastType === castType) { - context.report({ - node, - messageId: 'unnecessaryAssertion', - fix(fixer) { - return originalNode.kind === ts.SyntaxKind.TypeAssertionExpression - ? fixer.removeRange([ - originalNode.getStart(), - originalNode.expression.getStart(), - ]) - : fixer.removeRange([ + context.report({ + node, + messageId: 'unnecessaryAssertion', + fix(fixer) { + return fixer.removeRange([ + originalNode.expression.end, + originalNode.end, + ]); + }, + }); + } else { + // we know it's a nullable type + // so figure out if the variable is used in a place that accepts nullable types + const contextualType = getContextualType(checker, originalNode); + if (contextualType && util.isNullableType(contextualType)) { + context.report({ + node, + messageId: 'contextuallyUnnecessary', + fix(fixer) { + return fixer.removeRange([ originalNode.expression.end, originalNode.end, ]); - }, - }); - } - } + }, + }); + } + } + }, + 'TSAsExpression, TSTypeAssertion'( + node: TSESTree.TSTypeAssertion | TSESTree.TSAsExpression, + ): void { + if ( + options && + options.typesToIgnore && + options.typesToIgnore.indexOf( + sourceCode.getText(node.typeAnnotation), + ) !== -1 + ) { + return; + } - const checker = parserServices.program.getTypeChecker(); + const originalNode = parserServices.esTreeNodeToTSNodeMap.get< + ts.AssertionExpression + >(node); + const castType = checker.getTypeAtLocation(originalNode); - return { - TSNonNullExpression(node) { - checkNonNullAssertion(node, checker); - }, - TSTypeAssertion(node) { - verifyCast(node, checker); - }, - TSAsExpression(node) { - verifyCast(node, checker); + if ( + isTypeFlagSet(castType, ts.TypeFlags.Literal) || + (isObjectType(castType) && + (isObjectFlagSet(castType, ts.ObjectFlags.Tuple) || + couldBeTupleType(castType))) + ) { + // It's not always safe to remove a cast to a literal type or tuple + // type, as those types are sometimes widened without the cast. + return; + } + + const uncastType = checker.getTypeAtLocation(originalNode.expression); + + if (uncastType === castType) { + context.report({ + node, + messageId: 'unnecessaryAssertion', + fix(fixer) { + return originalNode.kind === ts.SyntaxKind.TypeAssertionExpression + ? fixer.removeRange([ + originalNode.getStart(), + originalNode.expression.getStart(), + ]) + : fixer.removeRange([ + originalNode.expression.end, + originalNode.end, + ]); + }, + }); + } + + // TODO - add contextually unnecessary check for this }, }; }, diff --git a/packages/eslint-plugin/src/util/types.ts b/packages/eslint-plugin/src/util/types.ts index 4e5d455926a..866ff80a45e 100644 --- a/packages/eslint-plugin/src/util/types.ts +++ b/packages/eslint-plugin/src/util/types.ts @@ -1,4 +1,9 @@ -import * as tsutils from 'tsutils'; +import { + isTypeFlagSet, + isTypeReference, + isUnionOrIntersectionType, + unionTypeParts, +} from 'tsutils'; import ts from 'typescript'; /** @@ -10,11 +15,11 @@ export function containsTypeByName( type: ts.Type, allowedNames: Set, ): boolean { - if (tsutils.isTypeFlagSet(type, ts.TypeFlags.Any | ts.TypeFlags.Unknown)) { + if (isTypeFlagSet(type, ts.TypeFlags.Any | ts.TypeFlags.Unknown)) { return true; } - if (tsutils.isTypeReference(type)) { + if (isTypeReference(type)) { type = type.target; } @@ -25,7 +30,7 @@ export function containsTypeByName( return true; } - if (tsutils.isUnionOrIntersectionType(type)) { + if (isUnionOrIntersectionType(type)) { return type.types.some(t => containsTypeByName(t, allowedNames)); } @@ -35,3 +40,44 @@ export function containsTypeByName( bases.some(t => containsTypeByName(t, allowedNames)) ); } + +/** + * Resolves the given node's type. Will resolve to the type's generic constraint, if it has one. + */ +export function getConstrainedTypeAtLocation( + checker: ts.TypeChecker, + node: ts.Node, +): ts.Type { + const nodeType = checker.getTypeAtLocation(node); + const constrained = checker.getBaseConstraintOfType(nodeType); + + return constrained || nodeType; +} + +/** + * Checks if the given type is (or accepts) nullable + * @param isReceiver true if the type is a receiving type (i.e. the type of a called function's parameter) + */ +export function isNullableType(type: ts.Type, isReceiver?: boolean): boolean { + let flags: ts.TypeFlags = 0; + for (const t of unionTypeParts(type)) { + flags |= t.flags; + } + + flags = + isReceiver && flags & (ts.TypeFlags.Any | ts.TypeFlags.Unknown) + ? -1 + : flags; + + return (flags & (ts.TypeFlags.Null | ts.TypeFlags.Undefined)) !== 0; +} + +/** + * Gets the declaration for the given variable + */ +export function getDeclaration( + checker: ts.TypeChecker, + node: ts.Expression, +): ts.Declaration { + return checker.getSymbolAtLocation(node)!.declarations![0]; +} diff --git a/packages/eslint-plugin/tests/rules/no-unnecessary-type-assertion.test.ts b/packages/eslint-plugin/tests/rules/no-unnecessary-type-assertion.test.ts index 6d5e76bbffa..21428ba7256 100644 --- a/packages/eslint-plugin/tests/rules/no-unnecessary-type-assertion.test.ts +++ b/packages/eslint-plugin/tests/rules/no-unnecessary-type-assertion.test.ts @@ -55,6 +55,40 @@ type Foo = number; const foo = (3 + 5);`, options: [{ typesToIgnore: ['Foo'] }], }, + // https://github.com/typescript-eslint/typescript-eslint/issues/453 + // the ol' use-before-assign-is-okay-trust-me assertion + ` +let bar: number +bar! + 1 + `, + ` +let bar: undefined | number +bar! + 1 + `, + ` +let bar: number, baz: number +bar! + 1 + `, + ` +function foo(bar: T) { + return bar! +} + `, + ` +declare function nonNull(s: string); +let s: string | null = null; +nonNull(s!); + `, + ` +const x: number | null = null; +const y: number = x!; + `, + ` +const x: number | null = null; +class Foo { + prop: number = x!; +} + `, ], invalid: [ @@ -116,5 +150,129 @@ const foo = (3 + 5);`, }, ], }, + // https://github.com/typescript-eslint/typescript-eslint/issues/453 + { + code: ` +let bar: number = 1 +bar! + 1 + `, + output: ` +let bar: number = 1 +bar + 1 + `, + errors: [ + { + messageId: 'unnecessaryAssertion', + line: 3, + }, + ], + }, + { + // definite declaration operator + code: ` +let bar!: number +bar! + 1 + `, + output: ` +let bar!: number +bar + 1 + `, + errors: [ + { + messageId: 'unnecessaryAssertion', + line: 3, + }, + ], + }, + { + code: ` +let bar: number | undefined +bar = 1; +bar! + 1 + `, + output: ` +let bar: number | undefined +bar = 1; +bar + 1 + `, + errors: [ + { + messageId: 'unnecessaryAssertion', + line: 4, + }, + ], + }, + { + code: ` +function foo(bar: T) { + return bar! +} + `, + output: ` +function foo(bar: T) { + return bar +} + `, + errors: [ + { + messageId: 'unnecessaryAssertion', + line: 3, + }, + ], + }, + { + code: ` +declare function nonNull(s: string | null); +let s: string | null = null; +nonNull(s!); + `, + output: ` +declare function nonNull(s: string | null); +let s: string | null = null; +nonNull(s); + `, + errors: [ + { + messageId: 'contextuallyUnnecessary', + line: 4, + }, + ], + }, + { + code: ` +const x: number | null = null; +const y: number | null = x!; + `, + output: ` +const x: number | null = null; +const y: number | null = x; + `, + errors: [ + { + messageId: 'contextuallyUnnecessary', + line: 3, + }, + ], + }, + { + code: ` +const x: number | null = null; +class Foo { + prop: number | null = x!; +} + `, + output: ` +const x: number | null = null; +class Foo { + prop: number | null = x; +} + `, + errors: [ + { + messageId: 'contextuallyUnnecessary', + line: 4, + }, + ], + }, ], });