plantain-00 / type-coverage

A CLI tool to check type coverage for typescript code
MIT License
1.21k stars 43 forks source link

Shouldn't flag safe `as` type assertions #55

Closed danielnixon closed 4 years ago

danielnixon commented 4 years ago

Version(if relevant): 2.8.1

Code(if relevant):

Same situation as #52 and #54, but this one might be more controversial.

There are some "safe" as type assertions that probably shouldn't be flagged as untyped.

const foo = 1 as number; // useless but not unsafe
const foo = 1 as number & PositiveNumber; // intersection (tagged) type
const bar = foo as number; // throwing away the type tag. `number` is supertype of `number & PositiveNumber` so this is safe too
const foo = 1 as const;
const bar = foo as number; // widening type by throwing away the knowledge of number literal

Expected:

These safe examples are not flagged as untyped.

Actual:

These safe examples are flagged as untyped.

danielnixon commented 4 years ago

I recommend relaxing this rule as much as humanly possible for the "safe" cases, because what amount to false positives will discourage use of strict mode, which we want to encourage!

plantain-00 commented 4 years ago

Widen type seems not safe:

const foo = "aaa" as unknown
const bar = foo as number // <- here
danielnixon commented 4 years ago

Yep that's basically a downcast and definitely not safe. Flagging that as untyped is legit. 👍

FWIW here are the test cases for an ESLint I'm writing with very similar goals: https://github.com/danielnixon/eslint-plugin-total-functions/blob/master/src/rules/no-unsafe-type-assertion.test.ts

danielnixon commented 4 years ago

Somewhat related: https://github.com/microsoft/TypeScript/issues/7481

plantain-00 commented 4 years ago

I think no enough gain for the complexity. And I think compared to const foo = "aaa" as unknown as number, const foo: number = "aaa" as unknown is safer, typescript can find the error. Your proposed cases can be rewritten by that or as const, for example:

const foo = 42 as number
const foo = 42 as const

const foo = 'foo' as 'foo' | 'bar'
const foo: 'foo' | 'bar' = 'foo'

type Foo = { readonly foo: string | undefined }
const foo = {} as Foo
const foo: Foo = {}
danielnixon commented 4 years ago

I think no enough gain for the complexity.

This is a fair point.

And I think compared to const foo = "aaa" as unknown as number, const foo: number = "aaa" as unknown is safer, typescript can find the error.

This is a fair point too, but in my thinking string as unknown would be considered safe (so would not be flagged as untyped) but unknown as number would definitely be considered unsafe (so would be flagged as untyped).

plantain-00 commented 4 years ago

but in my thinking string as unknown would be considered safe (so would not be flagged as untyped) but unknown as number would definitely be considered unsafe (so would be flagged as untyped).

That's what's going on.

danielnixon commented 4 years ago

This looks like it's covered by https://github.com/plantain-00/type-coverage/commit/b76b2a36c5abc13959a3a04ef69811a526d3362a (nice work) so closing this issue now.

danielnixon commented 4 years ago

Although... https://github.com/microsoft/TypeScript/pull/33263/files#r335215697

plantain-00 commented 4 years ago

I checked whether isTypeAssignableTo exists, I think it's OK if I releasing a quick fix if isTypeAssignableTo breaks.

danielnixon commented 4 years ago

A few (maybe) interesting cases that are considered unsafe according to isTypeAssignableTo's assignability rules but are arguably safe:

type Foo = { readonly foo: string };
const foo = {foo: 'foo', bar: 'bar'} as Foo;
type Foo = { readonly foo: undefined };
const foo = {} as Foo;
type Foo = { readonly foo: string | undefined };
const foo = {} as Foo;

I don't think I feel strongly about whether these should be considered typed or untyped (safe or unsafe) but sharing just in case there's a way (and a desire) to relax the rules even further.

The counterargument that isTypeAssignableTo is gospel truth is hard to argue with.