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

New set of rules: no-unsafe-* #791

Closed
bradzacher opened this issue Aug 1, 2019 · 35 comments · Fixed by #1643, #1644, #1647, #1694 or #3256
Closed

New set of rules: no-unsafe-* #791

bradzacher opened this issue Aug 1, 2019 · 35 comments · Fixed by #1643, #1644, #1647, #1694 or #3256
Assignees
Labels
enhancement: new plugin rule New rule request for eslint-plugin has pr there is a PR raised to close this package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin

Comments

@bradzacher
Copy link
Member

bradzacher commented Aug 1, 2019

These rules are all implemented - one final case remains to be implemented: no-unsafe-argument to catch when you're passing any into an argument. (See #791 (comment))


Create a rule which uses type information to determine when you're inadvertently breaking type safety by using an any, potentially without knowing.

Often libraries (or even the typescript defs themselves) can have weak(/lazy) types which return any. If you're not careful, you can inadvertently introduce anys within your codebase, leading to bugs that aren't caught by the compiler.

Examples:

const foo = Object.create(null);
//    ^^^ error: variable declaration implicitly typed as any
const bar = x.prop;
//    ^^^ error: variable declaration implicitly typed as any
const baz = (1 as any);
//    ^^^ error: variable declaration implicitly typed as any
const buzz = [];
//    ^^^^ error: variable declaration implicitly typed as any[]

let bam: any = 1; // no error, as it's **explicitly** any

   function foo(arg: string) {
// ^^^^^^^^^^^^^^^^^^^^^^^^^ error: function return type is implicitly any
     if (someCondition) {
       return 'str';
     }

     return bam;
//   ^^^^^^^^^^^  error: return statement implicitly returns any
   }

foo(x.prop);
//  ^^^^^^ error: passed function argument implicitly typed as any

for (const fizz of bar) {}
//   ^^^^^^^^^^ error: variable declaration implicitly typed as any
for (let i = foo; i < 10; i += 1) {}
//       ^ error: variable declaration implicitly typed as any

// variable decls, returns, arguments, assignment expressions, ifs... 

Should also check assignments:

    bam = foo;
//  ^^^^^^^^^^ error: assignment expression implicitly uses as any
const clazz = new Clazz();
      clazz.foo = foo;
//    ^^^^^^^^^^^^^^^^ error: assignment expression implicitly uses as any
    foo += foo;
//  ^^^^^^^^^^^ error: assignment expression implicitly uses any
    foo++;
//  ^^^^^^ error: update expression implicitly uses any

Maybe also check things boolean things like if/while/etc?

if (foo) {}
//  ^^^ error: if condition implicitly typed as any
while (foo) {}
//     ^^^ error: while condition implicitly typed as any
do { } while (foo)
//            ^^^ error: do...while condition implicitly typed as any
switch (foo) {
//      ^^^ error: switch discriminate implicitly typed as any
    case bar:
//       ^^^ error: case test implicitly typed as any
      break;
}

Should also provide an option to go strict with property access to help with finding the source of an any via property access):

type Obj = {
  foo: {
    [k: string]: any;
    bar: {
      baz: string;
    };
  };
};

declare const x: Obj;

const y = x.foo.bam.baz;
//        ^^^^^^^^^ error: property access implicitly returns any
//    ^ error: variable declaration implicitly typed as any

const z: string = x.foo.bam.baz;
//                ^^^^^^^^^ error: property access implicitly returns any

const { baz } = x.foo.bam;
//              ^^^^^^^^^ error: property access implicitly returns any
//      ^^^ error: destructured variable implicitly typed as any
@bradzacher bradzacher added package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin enhancement: new plugin rule New rule request for eslint-plugin labels Aug 1, 2019
@octogonz
Copy link
Contributor

octogonz commented Aug 2, 2019

Will this name be confused with the tsconfig.json setting noImplicitAny?

The spirit is similar, but the meaning is different, and it might be confusing in conversation. "Hey Pete, I tried what you said but I keep getting no implicit any errors!" [much puzzlement later] "Ohhhhhhhh you meant the lint rule!"

Maybe no-inferred-any instead?

@bradzacher
Copy link
Member Author

Definitely a valid concern. We can look at naming it something different. This was the first thing that came to my mind because of concepts of "implicitly returning any", "implicitly typing a variable as any", etc.

inferred might be a better word, yeah. Part of me is cautious of using inferred because typescript has the infer operator. I'll have a think about the name more before implementation.

@ThomasdenH
Copy link
Contributor

In tslint this was called no-unsafe-any. Is that an option?

@bradzacher
Copy link
Member Author

Having a quick look at the tslint docs, it doesn't seem like it's a 1:1 - it looks like the tslint rule only handle usage of a variable of type any.

But it's definitely a valid idea for a name, as long as we're clear that it's not a 1:1 replacement for the tslint rule.

(as an aside, I hate that the tslint docs don't include examples.. I'm always left wondering exactly what their rules cover).

@bradenhs
Copy link

no-silent-any is another thought. Really I'd like a rule like this to warn me when I think I have type safety but actually don't. In situations where I've explicitly specified any I'd prefer not to be bothered. Its areas where any silently creeps into the type system that I'd be concerned about. What exactly that would look like in terms of code examples I'm not 100% sure. I imagine it would be difficult to determine when the presence of any was intentional or not.

@fabb
Copy link

fabb commented Aug 22, 2019

no-unsafe-any has saved me from quite some mistakes... Could I achieve the same safety with this plugin + the no-any rule?

@ThomasdenH
Copy link
Contributor

Having a quick look at the tslint docs, it doesn't seem like it's a 1:1 - it looks like the tslint rule only handle usage of a variable of type any.

I think the simplest description is that any is treated like unknown. That means it can be passed around and used as long as the operations are valid for every type.

@fabb
Copy link

fabb commented Aug 22, 2019

Yeah, I wish the TypeScript compiler would just have an option to treat any as unknown.

@fabb
Copy link

fabb commented Aug 22, 2019

Seems like the TypeScript maintainers already decided not to 😢
microsoft/TypeScript#24737 (comment)

@fabb
Copy link

fabb commented Aug 24, 2019

Related:
microsoft/TypeScript#27265
microsoft/TypeScript#26188

@felixfbecker
Copy link

felixfbecker commented Aug 27, 2019

I would love to have this rule as this is a big blind spot in type safety of large codebases.

One specific use case I have a lot is that caught exceptions in TypeScript are of type any, but really should be of type unknown. I would like to use this rule to make sure errors are first type guarded before accessing any properties from them.

Even if TypeScript did this for lib.d.ts (which is very difficult with BC), it would still exist in many typings of libraries, so a rule is very useful.

@fabb
Copy link

fabb commented Nov 9, 2019

Would this also cause an error?

(1 as any).foo();

I would hope so, even though it is not an implicit any.

@bradzacher
Copy link
Member Author

bradzacher commented Nov 9, 2019

Hard to say - using an explicit cast might be a way to say the rule should ignore this expression.
It would be caught by no-explicit-any, regardless.

And any usages of the return value would be flagged by this rule:

const bar = (something as any).foo();
bar.baz // no-implicit-any error - accessing property on any type

function baz() {
  return (something as any).foo(); // no-implicit-any error - returning any type
}
// etc

@jsilveira2
Copy link

Hi, nothing has been defined so far? Is there any palative solution until topic is resolved?

@bradzacher bradzacher added the has pr there is a PR raised to close this label Dec 23, 2019
@danielnixon
Copy link
Contributor

Hi, nothing has been defined so far? Is there any palative solution until topic is resolved?

https://github.com/plantain-00/type-coverage is useful for spotting anys.

@k290
Copy link

k290 commented Feb 14, 2020

This would be handy. We made the switch from tslint to eslint to only find this unavailable :(. Now we have bugs creeping into our codebase due to unsafe usages of any. Although we try to make people remember to use unknown wherever possible but its a big team.

@fabb
Copy link

fabb commented Feb 14, 2020

@k290 in the mean time, i use the tslint plugin for eslint to run only this exact rule together with my eslint rules.

@bradzacher
Copy link
Member Author

@k290 have you looked at our no-explicit-any rule? This has an optional fixer to fix any with unknown.

Also there's our ban-types rule if there are other types which can resolve to any.

I get that any can leak in from library types, but hopefully most libraries define decent types now a days.

@jrnail23
Copy link

@bradzacher, for me the biggest risk is typically when I'm using FP style, piping/composing functions (i.e., using lodash's flow/pipe functions). It's often not quite clear that one or more of the results being passed to the next function is actually any. That's frequently due to inadequate typing for libs like ramda or lodash/fp, but it's really easy for stuff like that to sneak in and destroy any illusions of type safety I may have.
Unfortunately, there are still a lot of scenarios where we can't quite depend on libraries' typing just yet.

I've resorted to @fabb's approach of turning on VS Code's tslint extension solely for its no-implicit-any in addition to my typescript-eslint setup, just to help me identify such problems, but obviously it would be great if typescript-eslint could handle that instead.

Speaking to @danielnixon's suggestion of using type-coverage, I haven't found that tool very useful, as it really only seems to find explicit usages of any (which no-explicit-any does just fine). I stumbled upon that package while looking for something that could actually provide type coverage info, like Flow does. The fact that TypeScript doesn't have such a feature is maddening.

@bradzacher
Copy link
Member Author

of turning on VS Code's tslint extension

You should instead try our eslint plugin, @typescript-eslint/eslint-plugin-tslint, which will let you run tslint at eslint time.

The fact that TypeScript doesn't have such a feature is maddening

It's all open source. Features are just a PR away!

@jrnail23
Copy link

@bradzacher, I didn't know about eslint-plugin-tslint. Thanks for the tip!

I really wish I had the time to dive into a type coverage feature... I guess that's the other side of that OSS coin -- projects are always at the mercy of (potential) contributors' schedule demands.

@k290
Copy link

k290 commented Feb 15, 2020

Thanks @bradzacher , the rules you posted further up will help us.

@felixfbecker
Copy link

The approach to have one rule for each possible expression is interesting, but I feel like there's going to be a large number of expressions not covered. Excluding the rules already added in above PRs:

  • Passing any to function parameter fn(anyVar)
  • Using any in arithmetic expression anyVar * 2
  • Using any in template string ${anyVar}
  • Using any as template string tag
  • Using any as member name obj[anyVar]
  • Using any in assignment foo = anyVar
  • ...

Should/can we really have a rule for every single one for these cases?

@bradzacher
Copy link
Member Author

bradzacher commented Mar 3, 2020

Using any as template string tag

Using any as member name

I think most, if not all projects use noImplicitAny, so this would be a rare case, but easy to handle.

Note also that arrays do not have the same check applied to them 😢.

const a = [1,2,3];
const b = a[1 as any]; // no ts error
//          ^^^^^^^^ no-unsafe-member-expression

const x = { prop: 1 };
const y = x[1 as any]; // Element implicitly has an 'any' type because expression of type 'any' can't be used to index type '{ prop: number; }'.
//          ^^^^^^^^ no-unsafe-member-expression

Passing any to function parameter

  • I was planning on adding another rule, no-unsafe-argument to handle this.

Using any in template string

This is covered by restrict-template-expressions

Using any in assignment

  • I was planning on adding another rule, no-unsafe-assignment to handle this.
    It would handle both const x = ..., x = ..., pattern default values, and argument default values. (feat(eslint-plugin): add rule no-unsafe-assignment #1694)
    Considering also handling x += 1, etc. Probably should.

Using any in arithmetic expression

At this point, this should be implicitly handled by any of the previous rules.

  • const x = 1 * any caught by no-unsafe-assignment
  • return 1 * any caught by no-unsafe-return
  • fn(1 * any) caught by no-unsafe-argument
  • (1 * any).toString() caught by no-unsafe-call
  • ...

@bradzacher bradzacher reopened this Mar 3, 2020
@bradzacher
Copy link
Member Author

(reopening for the aforementioned missing cases and extra rules)

@scvnathan

This comment has been minimized.

@bradzacher

This comment has been minimized.

@bradzacher
Copy link
Member Author

Using any in arithmetic expression

At this point, this should be implicitly handled by any of the previous rules.

Note from investigation - I realise I'm very wrong. Typescript knows that it doesn't matter what the type of the operands for most binary expressions (-*/|^&); the result will be a number, because NaN is a number.
+ is treated differently, because + has operator overloading and does different things, so if there is an any in a + binary expression, it will return any.

So a new rule, no-unsafe-binary-expression would be a good addition to the suite.


Also for clarity as I merge #1968 in here.
I don't think a separate rule for unsafe arguments is the correct path. I think it would be better as part of the no-unsafe-call rule. I think that it seems more logical from a user's POV.

@mmkal

This comment has been minimized.

@bradzacher

This comment has been minimized.

@mmkal

This comment has been minimized.

@bradzacher

This comment has been minimized.

@David-Else

This comment has been minimized.

@bradzacher

This comment has been minimized.

@bradzacher bradzacher self-assigned this Aug 24, 2020
bradzacher added a commit that referenced this issue Apr 1, 2021
Fixes #791

This adds a new rule which just checks the passing of arguments.
This case is a lot more complex than most cases due to spread arguments.
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.