-
Notifications
You must be signed in to change notification settings - Fork 889
Refine no-unnecessary-type-assertion rule to not flag necessary assertions #3120
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.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added one suggestion below
@@ -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.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, thanks!
type Bar = 'bar'; | ||
const data = { | ||
x: 'foo' as 'foo', | ||
y: 'bar' as Bar, |
There was a problem hiding this comment.
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
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.