Skip to content

Commit

Permalink
no-unnecessary-type-assertion: extract checking of NonNullExpression (p…
Browse files Browse the repository at this point in the history 
…alantir#3468)
  • Loading branch information
@ajafff @HyphnKnight
ajafff authored and HyphnKnight committed Apr 9, 2018
1 parent d99c15a commit b744dec
Show file tree
Hide file tree
Showing 3 changed files with 94 additions and 35 deletions.
63 changes: 45 additions & 18 deletions src/rules/noUnnecessaryTypeAssertionRule.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
* limitations under the License.
*/

import { isAssertionExpression, isObjectFlagSet, isObjectType, isTypeFlagSet } from "tsutils";
import { isObjectFlagSet, isObjectType, isTypeFlagSet } from "tsutils";
import * as ts from "typescript";
import * as Lint from "../index";

Expand Down Expand Up @@ -54,10 +54,12 @@ class Walker extends Lint.AbstractWalker<string[]> {
public walk(sourceFile: ts.SourceFile) {
const cb = (node: ts.Node): void => {
switch (node.kind) {
case ts.SyntaxKind.TypeAssertionExpression:
case ts.SyntaxKind.NonNullExpression:
this.checkNonNullAssertion(node as ts.NonNullExpression);
break;
case ts.SyntaxKind.TypeAssertionExpression:
case ts.SyntaxKind.AsExpression:
this.verifyCast(node as ts.TypeAssertion | ts.NonNullExpression | ts.AsExpression);
this.verifyCast(node as ts.AssertionExpression);
}

return ts.forEachChild(node, cb);
Expand All @@ -66,24 +68,21 @@ class Walker extends Lint.AbstractWalker<string[]> {
return ts.forEachChild(sourceFile, cb);
}

private verifyCast(node: ts.TypeAssertion | ts.NonNullExpression | ts.AsExpression) {
if (isAssertionExpression(node) && this.options.indexOf(node.type.getText(this.sourceFile)) !== -1) {
return;
private checkNonNullAssertion(node: ts.NonNullExpression) {
const type = this.checker.getTypeAtLocation(node.expression);
if (type === this.checker.getNonNullableType(type)) {
this.addFailureAtNode(node, Rule.FAILURE_STRING, Lint.Replacement.deleteFromTo(node.expression.end, node.end));
}
const castType = this.checker.getTypeAtLocation(node);
if (castType === undefined) {
}

private verifyCast(node: ts.AssertionExpression) {
if (this.options.indexOf(node.type.getText(this.sourceFile)) !== -1) {
return;
}
const castType = this.checker.getTypeAtLocation(node);

if (node.kind !== ts.SyntaxKind.NonNullExpression &&
(isTypeFlagSet(castType, ts.TypeFlags.Literal) ||
isObjectType(castType) &&
isObjectFlagSet(castType, ts.ObjectFlags.Tuple)) ||
// Sometimes tuple types don't have ObjectFlags.Tuple set, like when
// they're being matched against an inferred type. So, in addition,
// check if any properties are numbers, which implies that this is
// likely a tuple type.
(castType.getProperties().some((symbol) => !isNaN(Number(symbol.name))))) {
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.
Expand All @@ -94,7 +93,35 @@ class Walker extends Lint.AbstractWalker<string[]> {
if (uncastType === castType) {
this.addFailureAtNode(node, Rule.FAILURE_STRING, node.kind === ts.SyntaxKind.TypeAssertionExpression
? Lint.Replacement.deleteFromTo(node.getStart(), node.expression.getStart())
: Lint.Replacement.deleteFromTo(node.expression.getEnd(), node.getEnd()));
: Lint.Replacement.deleteFromTo(node.expression.end, node.end));
}
}
}

/**
* Sometimes tuple types don't have ObjectFlags.Tuple set, like when they're being matched against an inferred type.
* So, in addition, check if there are integer properties 0..n and no other numeric keys
*/
function couldBeTupleType(type: ts.ObjectType): boolean {
const properties = type.getProperties();
if (properties.length === 0) {
return false;
}
let i = 0;
for (; i < properties.length; ++i) {
const name = properties[i].name;
if (String(i) !== name) {
if (i === 0) {
// if there are no integer properties, this is not a tuple
return false;
}
break;
}
}
for (; i < properties.length; ++i) {
if (String(+properties[i].name) === properties[i].name) {
return false; // if there are any other numeric properties, this is not a tuple
}
}
return true;
}
14 changes: 14 additions & 0 deletions test/rules/no-unnecessary-type-assertion/test.ts.fix
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,13 @@ const nonNullString: string;
const nullableString: string|undefined;
let anyType: any;
type AnyDuringMigration = any;
let tuple: [number, number] = [1, 2];

// non-null
let a = nonNullStringLiteral;
let b = nonNullString;
let c = nullableString!;
tuple;

// as
let d = nonNullStringLiteral as string;
Expand Down Expand Up @@ -83,3 +85,15 @@ const data = {

[1, 2, 3, 4, 5].map(x => [x, 'A' + x] as [number, string]);
let x: Array<[number, string]> = [1, 2, 3, 4, 5].map(x => [x, 'A' + x] as [number, string]);

interface NotATuple {
0: number,
0.5: number,
2: number,
}

declare const notATuple: NotATuple;
notATuple;

unknownName;

52 changes: 35 additions & 17 deletions test/rules/no-unnecessary-type-assertion/test.ts.lint
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,41 +3,44 @@ const nonNullString: string;
const nullableString: string|undefined;
let anyType: any;
type AnyDuringMigration = any;
let tuple: [number, number] = [1, 2];

// non-null
let a = nonNullStringLiteral!;
~~~~~~~~~~~~~~~~~~~~~ [This assertion is unnecessary since it does not change the type of the expression.]
~~~~~~~~~~~~~~~~~~~~~ [0]
let b = nonNullString!;
~~~~~~~~~~~~~~ [This assertion is unnecessary since it does not change the type of the expression.]
~~~~~~~~~~~~~~ [0]
let c = nullableString!;
tuple!;
~~~~~~ [0]

// as
let d = nonNullStringLiteral as string;
let e = nonNullString as string;
~~~~~~~~~~~~~~~~~~~~~~~ [This assertion is unnecessary since it does not change the type of the expression.]
~~~~~~~~~~~~~~~~~~~~~~~ [0]
let f = nullableString as string;

// type assertion
let g = <string>nonNullStringLiteral;
let h = <string>nonNullString;
~~~~~~~~~~~~~~~~~~~~~ [This assertion is unnecessary since it does not change the type of the expression.]
~~~~~~~~~~~~~~~~~~~~~ [0]
let i = <string>nullableString;

// complex inner expression
let j = (nonNullString + nonNullStringLiteral)!;
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [This assertion is unnecessary since it does not change the type of the expression.]
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [0]
let k = (nonNullString + nonNullStringLiteral) as string;
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [This assertion is unnecessary since it does not change the type of the expression.]
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [0]
let l = <string>(nonNullString + nonNullStringLiteral);
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [This assertion is unnecessary since it does not change the type of the expression.]
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [0]
let m = nonNullString.trim()!;
~~~~~~~~~~~~~~~~~~~~~ [This assertion is unnecessary since it does not change the type of the expression.]
~~~~~~~~~~~~~~~~~~~~~ [0]
let n = nonNullString.trim() as string;
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [This assertion is unnecessary since it does not change the type of the expression.]
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [0]
let o = <string>nonNullString.trim();
~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [This assertion is unnecessary since it does not change the type of the expression.]
~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [0]
let p = nonNullString!.trim();
~~~~~~~~~~~~~~ [This assertion is unnecessary since it does not change the type of the expression.]
~~~~~~~~~~~~~~ [0]

// custom types
interface Iface1 {
Expand All @@ -51,10 +54,10 @@ const value1: Iface1 = {prop: 'test'};
const value2: Iface2 = {prop: 'test'};

let q = <Iface1>value1;
~~~~~~~~~~~~~~ [This assertion is unnecessary since it does not change the type of the expression.]
~~~~~~~~~~~~~~ [0]
let r = <Iface2>value1;
let s = value2 as Iface2;
~~~~~~~~~~~~~~~~ [This assertion is unnecessary since it does not change the type of the expression.]
~~~~~~~~~~~~~~~~ [0]
let t = value2 as Iface1;
let aa = anyType as AnyDuringMigration;

Expand All @@ -75,18 +78,18 @@ function func(aOrB: TypeA|TypeB) {

if (aOrB.kind === 'a') {
let w = aOrB as TypeA;
~~~~~~~~~~~~~ [This assertion is unnecessary since it does not change the type of the expression.]
~~~~~~~~~~~~~ [0]
} else {
let x = <TypeB>aOrB;
~~~~~~~~~~~ [This assertion is unnecessary since it does not change the type of the expression.]
~~~~~~~~~~~ [0]
}

if (isB(aOrB)) {
let y = aOrB as TypeB;
~~~~~~~~~~~~~ [This assertion is unnecessary since it does not change the type of the expression.]
~~~~~~~~~~~~~ [0]
} else {
let z = <TypeA>aOrB;
~~~~~~~~~~~ [This assertion is unnecessary since it does not change the type of the expression.]
~~~~~~~~~~~ [0]
}
}

Expand All @@ -100,3 +103,18 @@ const data = {

[1, 2, 3, 4, 5].map(x => [x, 'A' + x] as [number, string]);
let x: Array<[number, string]> = [1, 2, 3, 4, 5].map(x => [x, 'A' + x] as [number, string]);

interface NotATuple {
0: number,
0.5: number,
2: number,
}

declare const notATuple: NotATuple;
<NotATuple>notATuple;
~~~~~~~~~~~~~~~~~~~~ [0]

unknownName!;
~~~~~~~~~~~~ [0]

[0]: This assertion is unnecessary since it does not change the type of the expression.

0 comments on commit b744dec

Please sign in to comment.