Skip to content
This repository has been archived by the owner on Mar 25, 2021. It is now read-only.

Refine no-unnecessary-type-assertion rule to not flag necessary assertions #3120

Merged
merged 3 commits into from
Aug 17, 2017
Merged

Refine no-unnecessary-type-assertion rule to not flag necessary assertions #3120

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3730704
Refine no-unnecessary-type-assertion rule to not flag necessary asser…
calebegg Aug 11, 2017
04ac2d4
Fix lint errors.
calebegg Aug 11, 2017
f17e6bf
Use isObjectType to avoid cast
calebegg Aug 14, 2017
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
10 changes: 10 additions & 0 deletions src/rules/noUnnecessaryTypeAssertionRule.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
* limitations under the License.
*/

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

Expand Down Expand Up @@ -65,6 +66,15 @@ class Walker extends Lint.AbstractWalker<void> {
return;
}

if (node.kind !== ts.SyntaxKind.NonNullExpression &&
(isTypeFlagSet(castType, ts.TypeFlags.Literal) ||
isTypeFlagSet(castType, ts.TypeFlags.Object) &&
Copy link
Contributor

Choose a reason for hiding this comment

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

you can use isObjectType(castType) to avoid the assertion in the next line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, thanks!

isObjectFlagSet(castType as ts.ObjectType, ts.ObjectFlags.Tuple))) {
// 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 = this.checker.getTypeAtLocation(node.expression);
if (uncastType === castType) {
this.addFailureAtNode(node, Rule.FAILURE_STRING, node.kind === ts.SyntaxKind.TypeAssertionExpression
Expand Down
10 changes: 10 additions & 0 deletions test/rules/no-unnecessary-type-assertion/test.ts.fix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,3 +69,13 @@ function func(aOrB: TypeA|TypeB) {
let z = aOrB;
}
}

// Expecting no warning for these assertions as they are not unnecessary.

type Bar = 'bar';
const data = {
x: 'foo' as 'foo',
y: 'bar' as Bar,
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't get why you need this behavior. A type annotation would also work

const data = {x: 'foo', y: Bar} = {x: 'foo', y: bar}

Same goes for the arrow function below: just add the return type:

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

I guess it's useful if the assertion is only a little part of a bigger object where you don't want to write the whole type?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a relatively common pattern in our very large codebase where we're trying to turn on this rule. I don't think there's anything so wrong with the pattern that it would make sense to ban it. It keeps you from having to repeat the keys, at least.

Skimming through my list of breakages, I also see:

const FOO = {
  bar: 'a' as 'a',
  baz: 'stuff',
  // ... many more properties without 'as'

i.e., not all properties are cast like this. For those I would hate to have to make it so much more verbose.

I also see instances of:

for (const foo of ['bar' as 'bar', 'baz' as 'baz']) { /* ... */ }

Also, a search turned up a match in tslint itself:
https://github.com/palantir/tslint/blob/master/src/linter.ts#L110

Copy link
Contributor

Choose a reason for hiding this comment

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

That makes sense, thank you for the detailed explanation

}

[1, 2, 3, 4, 5].map(x => [x, 'A' + x] as [number, string]);
10 changes: 10 additions & 0 deletions test/rules/no-unnecessary-type-assertion/test.ts.lint
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,3 +86,13 @@ function func(aOrB: TypeA|TypeB) {
~~~~~~~~~~~ [This assertion is unnecessary since it does not change the type of the expression.]
}
}

// Expecting no warning for these assertions as they are not unnecessary.

type Bar = 'bar';
const data = {
x: 'foo' as 'foo',
y: 'bar' as Bar,
}

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