Refine no-unnecessary-type-assertion rule to not flag necessary assertions #3120
Conversation
…tions Type assertions prevent a type from being widened in a couple of situations, such as in an object literal or the inferred return type of a function.
|
I think the test failure is unrelated; looks like a timeout. |
| type Bar = 'bar'; | ||
| const data = { | ||
| x: 'foo' as 'foo', | ||
| y: 'bar' as Bar, |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
That makes sense, thank you for the detailed explanation
| @@ -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) && | |||
There was a problem hiding this comment.
you can use isObjectType(castType) to avoid the assertion in the next line
| type Bar = 'bar'; | ||
| const data = { | ||
| x: 'foo' as 'foo', | ||
| y: 'bar' as Bar, |
There was a problem hiding this comment.
That makes sense, thank you for the detailed explanation
|
Related TypeScript PR that would make this change unnecessary: microsoft/TypeScript#17785 Let's see if it lands. |
|
That won't solve all of the cases, plus it appears the goal is to put it behind a flag. I'd prefer to land this and adjust if necessary after that lands + is in a current release of TS + that flag is included in |
That's what I was planning to do. I just wanted to leave this open for a few days, so others can have a look, too. |
…tions (palantir#3120) * Refine no-unnecessary-type-assertion rule to not flag necessary assertions Type assertions prevent a type from being widened in a couple of situations, such as in an object literal or the inferred return type of a function. * Fix lint errors. * Use isObjectType to avoid cast
Type assertions prevent a type from being widened in a couple of
situations, such as in an object literal or the inferred return type of
a function.