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

merged 3 commits into from
Aug 17, 2017

Conversation

calebegg
Copy link
Contributor

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.

…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.
@calebegg
Copy link
Contributor Author

I think the test failure is unrelated; looks like a timeout.

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

Copy link
Contributor

@ajafff ajafff left a 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) &&
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!

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.

That makes sense, thank you for the detailed explanation

@ajafff
Copy link
Contributor

ajafff commented Aug 14, 2017

Related TypeScript PR that would make this change unnecessary: microsoft/TypeScript#17785

Let's see if it lands.

@calebegg
Copy link
Contributor Author

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 --strict. How does that sound?

@ajafff
Copy link
Contributor

ajafff commented Aug 17, 2017

I'd prefer to land this and adjust if necessary after that lands + is in a current release of TS

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.

@ajafff ajafff merged commit c51e1bd into palantir:master Aug 17, 2017
HyphnKnight pushed a commit to HyphnKnight/tslint that referenced this pull request Apr 9, 2018
…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
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants