feat(eslint-plugin): [no-unsafe-assignment] [no-unsafe-return] add never support

[CreativeTechGuy]

#2616

This PR adds support for never as an unsafe assignment and an unsafe return. It was added as an extension to these existing rules per @bradzacher 's suggestion in this comment.

I do want to call out that I didn't want to copy every existing test and change any -> never so the tests I added were carefully chosen to highlight the key feature of adding never support (where applicable). I can see these rules being expanded in the future (maybe if the TypeScript language adds some new types) and don't want to make these rules too reliant on a specific type that it's checking for.

As mentioned in the comment linked above, these two rule updates are only part of the larger feature to warn against never in all cases. There is still an issue of passing never to a function but the appropriate rule no-unsafe-argument has not yet been created.

[typescript-eslint]

Thanks for the PR, @CreativeTechGuy!

typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community.

The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately.

Thanks again!

---

🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint. As a thank you, your profile/company logo will be added to our main README which receives thousands of unique visitors per day.

[codecov]

Codecov Report

Merging #2746 (542d037) into master (e82698c) will increase coverage by 0.00%.
The diff coverage is 96.29%.

@@           Coverage Diff           @@
##           master    #2746   +/-   ##
=======================================
  Coverage   92.79%   92.80%           
=======================================
  Files         297      297           
  Lines        9833     9845   +12     
  Branches     2762     2769    +7     
=======================================
+ Hits         9125     9137   +12     
  Misses        332      332           
  Partials      376      376           

Flag	Coverage Δ
unittest	92.80% <96.29%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
packages/eslint-plugin/src/configs/all.ts	100.00% <ø> (ø)
packages/eslint-plugin/src/util/types.ts	83.69% <91.66%> (+0.45%)	⬆️
...es/eslint-plugin/src/rules/no-unsafe-assignment.ts	81.30% <100.00%> (+0.79%)	⬆️
...ckages/eslint-plugin/src/rules/no-unsafe-return.ts	100.00% <100.00%> (ø)

[CreativeTechGuy]

After implementing this and trying it out I feel this is the wrong way to go about handling never. I believe never is a special case because often the problems are due to a variable getting scoped down and becoming never. The fact that to replicate that behavior in all cases would require 4-5 no-unsafe-* rules to be enabled feels wrong.

I'm going back to the drawing board with a no-never rule which is far simpler and more straight forward. It errors on any identifier that is found with the type never either through explicitly typing it as never (which is silly to do) or more likely a variable being scoped to become never. That one rule would handle every never case as it effectively prohibits using the type as anything other than a return type, which I feel is a good practice.

[bradzacher]

It errors on any identifier that is found with the type

This check is too expensive to realistically do.
Type information is computed lazily. The more you consume - the slower your rule will be.
Checking every single Identifier in a file will result in hundreds of checks - and will perform terribly.

This is why the no-unsafe-* rules are built the way that they are with very specific selectors and checks.

I'm going back to the drawing board with a no-never rule which is far simpler and more straight forward

Whilst never is a more specific unsafe check, I don't believe that building it out into a separate rule is the best approach.
The reason that the no-unsafe-* rules are split up is:

1. maintenance - the logic is mostly separate so it's cleaner and easier to reason about as separate rules.
2. user onboarding - the vast majority of our users have large, existing codebases and turning on a new rule is often a huge effort.

Ultimately users can enable them all at once by turning on our plugin:@typescript-eslint/recommended-requiring-type-checking config.

---

Additionally - something you may have overlooked is the generic safety checks that these rules do on top of simple checks.

These rules check things like const x: Set<string> = (new Set() as Set<any>).
Which your rule would also want to replicate.

Example of how never can be just as unsafe as any and why you need to check more than just typeof x === never:

declare function foo<T = never>(arg?: T): Set<T>;

const x = foo(); // x === Set<never>
const y = foo('a'); // y === Set<string>

function test(): Set<string> {
  return x; // returning Set<never> into a Set<string> is super unsafe but it's "compile-time safe"!
}

// Example of where things get really unsafe:
function addToStringSet(arg: Set<string>) {
    arg.add('foo');
}
function addToNumberSet(arg: Set<number>) {
    arg.add(1);
}

addToStringSet(x);
addToNumberSet(x);
// x is still of type Set<never>
// but it === new Set(['foo', 1]);
// it's assignable to any set - i.e. SUPER unsafe

repl

[CreativeTechGuy]

I clearly don't understand the considerations that go into an ESLint rule like I had thought. I'm glad you are here to check my misconceptions. There's a lot of things which I've never encountered in practice and so I never considered.

I'll close this PR to not clutter up the list as I don't think I'd be able to build a rule to the quality required. Thanks again for all of your great explanations. I learned a lot.

A suggestion for the future: Could there be performance profiling tests for each rule? It'd be great both as a user to see how much enabling each rule would cost and also as a developer so I could have more easily caught the fact that my rule was going to not meet the performance threshold. (I'll create a separate Issue for this suggestion so it can be tracked.)