Skip to content

Commit

Permalink
feat(eslint-plugin): Add better non-null handling [no-unnecessary-typ…
Browse files Browse the repository at this point in the history 
…e-assertion] (#478)
  • Loading branch information
@bradzacher
bradzacher committed May 9, 2019
1 parent 298d66c commit 4cd5590
Show file tree
Hide file tree
Showing 6 changed files with 381 additions and 77 deletions.
1 change: 1 addition & 0 deletions .eslintrc.json
View file
Expand Up @@ -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",
Expand Down
8 changes: 6 additions & 2 deletions packages/eslint-plugin/src/rules/member-naming.ts
View file
Expand Up @@ -76,9 +76,13 @@ export default util.createRule<Options, MessageIds>({
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,
Expand Down
4 changes: 3 additions & 1 deletion packages/eslint-plugin/src/rules/no-extraneous-class.ts
View file
Expand Up @@ -96,7 +96,9 @@ export default util.createRule<Options, MessageIds>({
onlyStatic = false;
}
}
if (!(onlyStatic || onlyConstructor)) break;
if (!(onlyStatic || onlyConstructor)) {
break;
}
}

if (onlyConstructor) {
Expand Down
233 changes: 163 additions & 70 deletions packages/eslint-plugin/src/rules/no-unnecessary-type-assertion.ts
View file
@@ -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';

Expand All @@ -8,7 +18,7 @@ type Options = [
typesToIgnore?: string[];
}
];
type MessageIds = 'unnecessaryAssertion';
type MessageIds = 'contextuallyUnnecessary' | 'unnecessaryAssertion';

export default util.createRule<Options, MessageIds>({
name: 'no-unnecessary-type-assertion',
Expand All @@ -24,6 +34,8 @@ export default util.createRule<Options, MessageIds>({
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: [
{
Expand All @@ -44,6 +56,8 @@ export default util.createRule<Options, MessageIds>({
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.
Expand Down Expand Up @@ -76,91 +90,170 @@ export default util.createRule<Options, MessageIds>({
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
},
};
},
Expand Down
54 changes: 50 additions & 4 deletions packages/eslint-plugin/src/util/types.ts
View file
@@ -1,4 +1,9 @@
import * as tsutils from 'tsutils';
import {
isTypeFlagSet,
isTypeReference,
isUnionOrIntersectionType,
unionTypeParts,
} from 'tsutils';
import ts from 'typescript';

/**
Expand All @@ -10,11 +15,11 @@ export function containsTypeByName(
type: ts.Type,
allowedNames: Set<string>,
): 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;
}

Expand All @@ -25,7 +30,7 @@ export function containsTypeByName(
return true;
}

if (tsutils.isUnionOrIntersectionType(type)) {
if (isUnionOrIntersectionType(type)) {
return type.types.some(t => containsTypeByName(t, allowedNames));
}

Expand All @@ -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];
}

0 comments on commit 4cd5590

Please sign in to comment.