Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rule proposal: Disallow implicit casts from/to any #306

Closed
emlai opened this issue Feb 21, 2019 · 10 comments
Closed

Rule proposal: Disallow implicit casts from/to any #306

emlai opened this issue Feb 21, 2019 · 10 comments
Labels
enhancement: new plugin rule New rule request for eslint-plugin package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin

Comments

@emlai
Copy link

emlai commented Feb 21, 2019

In my experience a frequent source of errors in TypeScript is implicit conversion from/to any.

noImplicitAny and no-explicit-any do help, but sometimes third-party libraries return e.g. any[] that still needs to be cast. For example:

  const result = await database.query('SELECT * FROM foo')
  doSomethingWith(result.rows) // result.rows has type `any[]`

I propose to add a rule that flags the above implicit conversion from result.rows to whatever doSomethingWith parameter type is, and requires the user to cast it explicitly:

  doSomethingWith(result.rows as Foo[])

Now if the doSomethingWith parameter type changes, result.rows is not silently cast to a possibly wrong type.

@emlai emlai added package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for maintainers to take a look labels Feb 21, 2019
@bradzacher bradzacher added enhancement: new plugin rule New rule request for eslint-plugin and removed triage Waiting for maintainers to take a look labels Feb 23, 2019
@bradzacher
Copy link
Member

bradzacher commented Feb 23, 2019

I'm not sure how much value this has, at least from your example.
IMO this creates a "brittle" state in your code where you'll have to manually update an (unnecessary) typecast if the function signature ever changes.

If you've got some more examples to help justify it, I'd love to see them.


Just as a side note:

If you have control of the typings for the function (doubt you do, but as an FYI anyways), you can accomplish this with the unknown type:

declare var database: {
    query(q: string): {
        rows: unknown[],
    },
}

type Foo = string;
function doSomethingWith(rows: Foo[]) {}

const result = database.query('SELECT * FROM foo');
doSomethingWith(result.rows); /*
↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑
Argument of type 'unknown[]' is not assignable to parameter of type 'string[]'.
  Type 'unknown' is not assignable to type 'string'.
*/
doSomethingWith(result.rows as string[]); // no error

repl

@emlai
Copy link
Author

emlai commented Feb 24, 2019

IMO this creates a "brittle" state in your code where you'll have to manually update an (unnecessary) typecast if the function signature ever changes.

I think you specifically do want to manually check the call site if the function signature changes, because result.rows might no longer have the correct type for the function.

Yeah if you control the typings you can just use unknown instead of any everywhere. This rule would give you the benefits of unknown when using third-party libraries that still use any.

@ThomasdenH
Copy link
Contributor

Do you mean the no-unsafe-any rule?

@ThomasdenH
Copy link
Contributor

The unknown keyword is always preferred in a strict codebase. However, since many libraries do not yet use unknown for various reasons, the rule is essential for a statically checked project.

For example, the signature for JSON.parse should look like this: (in: string) => unknown. Instead, it looks like this: (in: string): any. This means that the following crashing code is allowed by Typescript:

let a: { property: { subproperty: string } };
a = JSON.parse("{}");
console.log(a.property.subproperty);

Playground

The proposal to add this option in Typescript was closed, as the unknown type fixes this eventually, and the no-unsafe-any lint covers the other cases.

@mohsen1
Copy link
Contributor

mohsen1 commented Feb 25, 2019

The TypeScript team decided this is not useful as a strict flag. But I support this as an ESLint rule

@emlai
Copy link
Author

emlai commented Feb 25, 2019

no-unsafe-any seems to allow implicit conversions from any[] to T[]. I would like the lint rule to also catch those.

@ThomasdenH
Copy link
Contributor

And preferably there would be a better error message that gives a hint about what coercion is happening.

@bradzacher
Copy link
Member

I've written up a more detailed proposal #306 which covers more cases.
I'm closing this in favour of that.

@bradenhs
Copy link

bradenhs commented Aug 1, 2019

I've written up a more detailed proposal #306 which covers more cases.
I'm closing this in favour of that.

#306 is this issue

@bradzacher
Copy link
Member

#306 is this issue

image

I meant - #791

@typescript-eslint typescript-eslint locked as resolved and limited conversation to collaborators Feb 21, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement: new plugin rule New rule request for eslint-plugin package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin
Projects
None yet
Development

No branches or pull requests

5 participants