Rule proposal: no-unnecessary-type-annotation

[mohsen1]

This can be a very complicated rule so I thought I open an issue discussing it before starting to work on it.

The idea is, when a type is inferred we should not have extra annotation.

Use Cases

There are many places in TypeScript that we can have extraneous type annotation. This list is probably not complete:

Variable declaration

var a: string = "foo";
//     ^__ unnecessary type annotation 

Function arguments of a typed function

type Fn = (a: string) => void;

let f: Fn = (a: string) => {
  //            ^__ unnecessary type annotation 
}

Return type annotation

class C {
  method(): string {
  //        ^__ unnecessary type annotation 
   return 'str'
  }
}

[bradzacher]

This already exists: no-unnecessary-type-assertion

[mohsen1]

I believe that's a different rule. What I'm suggesting is avoiding extraneous type annotation not assertion.

[bradzacher]

I see.
I can see some of this functionality living in other rules.
I.e.
The return type annotation part could fit into explicit-function-return-type

The variable declaration part fits into no-unnecessary-type-assertion.

We don't have anything specific for function arguments yet though.

[mohsen1]

I think it makes sense to split this into multiple rules. The most useful one for React is to warn against extra typing React.SFC functions:

const Component: React.SFC<{foo: string}> =
  ({ foo }: { foo: string; children: React.ReactNode }): React.ReactElement<any> | null  => (<div />);

The parameter and return type annotations are redundant.

I suggest we make a specific rule for this. Not sure how to name it.

question
In case the return type is narrower, it's valid to provide the return type. Should we not enforce the return type? Should we warn if the return type is a specific or wider than inferred type?

[bradzacher]

The reason I say that the return type piece belongs in explicit-function-return-type is because of #149 - people want that same functionality in the rule.

It seems weird to put the functionality to check for an unnecessary return type into a new rule - this would create a possible state where two rules can conflict: you would have explicit-function-return-type telling you to include an annotation, and this new rule telling you not to have an annotation.

That would suggest that we should duplicate logic into explicit-function-return-type, and the new rule. However this would put you into a state where you can get the double reporting for the exact same error (telling you that there's an unnecessary return type annotation).

Potentially it's worth renaming the rule entirely (to something like function-type-annotation) so that we can include all of these features in it.

[mohsen1]

okay, that makes sense. So I think to make this suggested rule's scope even narrower we can focus on parameters of a typed function expression (not function declarations) and leave return types to explicit-function-return-type.

We can name it no-unnecessary-parameter-types where it can work for function expressions. It can enforce parameter type in "callback" paradigm too:

fs.readFile('file' , (err: NodeJS.ErrnoException) => {})
//                         ^__ unnecessary type annotation

[ljharb]

In general, what I'd want is a rule that errors when there's an explicit type that, once removed, results in an identical inferred type - in any context.

[bbugh]

I came here actually looking for the opposite rule - ensuring that all of my inferred types are explicitly typed. Normally I prefer to allow TypeScript to infer the types, but when working on open source libraries, I find it a lot more useful to be explicitly typed. People who come along after me know the intent more clearly, and reading code on pull requests/github is sure easier when the types are explicit. I was hoping that typescript-eslint could enforce this automatically.

[JoshuaKGoldberg]

We can name it no-unnecessary-parameter-types where
I came here actually looking for the opposite rule

How about a name like inferable-parameter-types, then?

[thewilkybarkid]

In general, what I'd want is a rule that errors when there's an explicit type that, once removed, results in an identical inferred type - in any context.

We'd find this useful. Example is a factory function for a Koa middleware:

export default (): Middleware => (
  async (context: Context, next: Next): Promise<void> => {
    ...

The context, next and return types are redundant as the Middleware return type results on safe inference:

export default (): Middleware => (
  async (context, next) => {
    ...

[JoshuaKGoldberg]

@mohsen1 are you still interested in working on this? If not, I can!

[bradzacher]

Just as a note - there is a large problem with implementing this rule - namely that TS has no API to say "give me the type of this thing before annotation".

You can (in a way) do this manually by comparing the type of the thing and the type of the annotation.

There are a number of cases where TS doesn't do a good job of reporting the "actual" type of an expression (examples from no-unnecessary-type-assertion #2248, #1282)

One such example is when a generic is used in a return type position. In this case, TS will infer the type of the generic based on the contextual destination of the value.

declare function foo<T = number>(): T;
const x: string = foo();

The type of x is string. TS will use this type to contextually infer the type of T to be string. This is encoded directly into the type of the call expression.

What this means is that if you inspect the type of the variable, you'll get string, and you inspect the return type of the call, you'll get string. So the rule will report. But removing the explicit annotation will cause TS to lose its contextual information, so instead it will infer T to be number.

This is a bit of an anti-pattern in TS - generics shouldn't ever just be used in return value locations, but there are seemingly valid patterns where it can happen as well.
(eg declare function bar<T = number>(arg?: T): T; will do the same thing when you provide no argument).

I'm not arguing against this rule, I'm just dumping some knowledge that will need to be encoded in the rule's logic (/ included in the docs).

[ark120202]

TypeScript has similar logic in the extract to constant refactor: https://github.com/microsoft/TypeScript/blob/master/src/services/refactors/extractSymbol.ts#L1098-L1100. It is using isContextSensitive to check if annotation is required, however it doesn't seem to correctly handle your example. I have opened a microsoft/TypeScript#40423, so if it would be resolved this rule could use the same approach.

[JoshuaKGoldberg]

I actually have a somewhat workable prototype of this working here: https://github.com/JoshuaKGoldberg/TypeStat/blob/166ab6c118ca611b0e6e37a73ebb9a225dad4b45/src/mutators/builtIn/fixNoInferableTypes/index.ts

[bradzacher]

@JoshuaKGoldberg - your implementation isn't much different to the no-inferrable-types, with the addition of using the private isTypeAssignableTo checker method.

[mohsen1]

Two years after writing this I came to understand writing type annotations is helpful to TypeScript's performance. no-inferrable-types does this enforcement anyways but I suggest turning it off in large projects.

[jp7837]

@mohsen1 can you expand on how it helps TS performance? Compilation, type detection in VS Code, etc?

Also, I thought this issue was tracking expanding the rule from primitives to potentially all types when the language service can infer it. If not, I can open a new issue for that assuming it's possible.