typescript-eslint / typescript-eslint

:sparkles: Monorepo for all the tooling which enables ESLint to support TypeScript
https://typescript-eslint.io
Other
15.28k stars 2.73k forks source link

[no-explicit-any] Option to disable rule for generic constraints #642

Closed anilanar closed 5 years ago

anilanar commented 5 years ago

Repro

{
  "rules": {
    "@typescript-eslint/no-explicit-any": ["error", { "ignoreGenericConstraints": true }]
  }
}
type IsString<T extends any> = T extends string ? true : false;

type CodomainOf<T extends Record<string, any>> = T extends Record<string, infer R> ? R : never;

Expected Result No errors for explicit any if they are used in generic constraints.

Actual Result There is no such option to disable errors/warnings for explicit any usage in generic constraints.

j-f1 commented 5 years ago

Does unknown have the same behavior here?

bradzacher commented 5 years ago
- type IsString<T extends any> = T extends string ? true : false;
+ type IsString<T> = T extends string ? true : false;

Nitpicking, but extends any is a useless qualifier.

- type CodomainOf<T extends Record<string, any>> = T extends Record<string, infer R> ? R : never;
+ type CodomainOf<T extends Record<string, unknown>> = T extends Record<string, infer R> ? R : never;

@j-f1 is correct, you can use unknown instead of any.

bradzacher commented 5 years ago

I'm going to close and reject this for the following reasons:

milesj commented 5 years ago

@bradzacher While I usually agree, unknown does not work if the generic has a constraint. For example:

Screen Shot 2019-07-28 at 23 00 08
anilanar commented 5 years ago

@milesjs In that case, you replace unknown with Context.

any is needed when defining a generic constraint with a type that has a contravariant parameter. Once such example is Function and its args type which is contravariant against Function. There are many such cases like that because it’s a generic property of type systems. e.g. key of record/dictionary/map types, any interface whose generic parameter is used in contravariant position etc.

anilanar commented 5 years ago

tsc already knows and keeps track of what is covariant and what is contravariant, so it may be better if this would be done in typescript.

milesj commented 5 years ago

@anilanar Context cant be used here either, because it causes the 'Context' is assignable to the constraint of type 'Ctx', but 'Context' could be instantiated with a different subtype of constraint 'Ctx'. error.

bradzacher commented 5 years ago

could you share more about your setup? It's hard to provide advice from a screenshot... Maybe a better solution would be to infer the context as a generic?

export default function milesjFunc<T extends Context = Context>(
  titleOrWorkUnit: unknown,
  action?: Action<T, Input, Output>,
  scope?: unknown,
)
milesj commented 5 years ago

Probably, but that's adding far more generics all over the place for this one use case. https://github.com/milesj/boost/blob/master/packages/pipeline/src/createWorkUnit.ts#L15

I'm usually anti-any, but in cases like this it would be nice. But I'm also not a fan adding eslint line disables anywhere.

bradzacher commented 5 years ago

Investigating this more - you should be able to achieve this as follows:

function createWorkUnit<Input, Output = Input, TCtx extends Context = Context>(
  titleOrWorkUnit: string | WorkUnit<any, Input, Output>,
  action?: Action<TCtx, Input, Output>,
  scope?: unknown,
): WorkUnit<any, Input, Output>;

In usage you will never have to instantiate the generic, as its type will be inferred for you automatically based on usage:

createWorkUnit('foo'); // TCtx === Context

class FooContext extends Context {}
createWorkUnit( // TCtx === FooContext
  'foo',
  (context: FooContext, value: string, runner: any) => value,
);

repl


The problem with adding this option is that it opens a door very widely - there's no granularity. There's no easy way to detect if it's a "valid" use case, which means that all of a sudden every single generic type can have anys freely, without checks - which as I mentioned is very much against the spirit of the rule.

The point of the rule is to not use any, as there is almost always a better alternative. Very rarely should you actually have to reach for it. Some things will take a bit of work to do type-safely, but it is doable.

The point of eslint disables is to clearly document that you are working around the eslint rule for a reason. Which is exactly the case here. IMO this is completely valid code, and I wouldn't complain if I saw this in a PR:

function createWorkUnit<Input, Output = Input>(
  titleOrWorkUnit: string | WorkUnit<any, Input, Output>,
  // No point introducing a generic for this, as the type of the context doesn't matter to this function
  // eslint-disable-next-line @typescript-eslint/no-explicit-any
  action?: Action<any, Input, Output>,
  scope?: unknown,
): WorkUnit<any, Input, Output>;
milesj commented 5 years ago

I use constraints in many of my generics, so having to add additional generics to solve this ESLint rule is completely overkill. I'll just turn it off.

Thanks for the feedback.

mmun commented 5 years ago

I think my use case is a common one: constraining a generic type to be an array. Is there a way that avoids any?

export function useDeps<T extends any[]>(deps: T): T {
  const depsRef = useRef<T>();

  if (!isEqualArray(depsRef.current, deps)) {
    depsRef.current = deps;
  }

  return depsRef.current;
}
bradzacher commented 5 years ago

Use unknown[] instead.

anilanar commented 5 years ago

You cannot because its contravariant.

bradzacher commented 5 years ago

Seems to work fine though? What cases am I missing?

declare function isEqualArray<T extends readonly unknown[]>(arg1: T, arg2: T): boolean;
declare function useRef<T>(): {current: T};

export function useDeps<T extends unknown[]>(deps: T): T {
  const depsRef = useRef<T>();

  if (!isEqualArray(depsRef.current, deps)) {
    depsRef.current = deps;
  }

  return depsRef.current;
}

const x1 = useDeps([1,2,3]); // typeof x1 === number[]
const x2 = useDeps(['a', 'b', 'c'] as string[]); // typeof x2 === string[]
const x3 = useDeps([1,2,3] as const); // error: type 'readonly [1, 2, 3]' is 'readonly' and cannot be assigned to the mutable type 'unknown[]'

// for bonus points you can allow tuples by adding readonly

export function useDepsTuple<T extends readonly unknown[]>(deps: T): T {
  const depsRef = useRef<T>();

  if (!isEqualArray(depsRef.current, deps)) {
    depsRef.current = deps;
  }

  return depsRef.current;
}

const x4 = useDepsTuple([1,2,3]); // typeof x4 === number[]
const x5 = useDepsTuple(['a', 'b', 'c'] as string[]); // typeof x5 === string[]
const x6 = useDepsTuple([1,2,3] as const); // typeof x6 === readonly [1,2,3]

repl

mmun commented 5 years ago

@bradzacher Thanks for taking the time to respond with that example. You're right. I had a fundamental misunderstanding of unknown.

If anyone else stumbles on this issue, check out When to use never and unknown in TypeScript.

TL;DR:

milesj commented 5 years ago

The way I explain unknown to others, is that unknown is a type-safe alternative to any, and can replace any in most scenarios.

bradzacher commented 5 years ago

@milesj - an interesting thing to note that I hadn't thought about (thanks @mmun) - you can actually use never in place of any when you want to give a type to a generic which has a constraint (i.e. when you can't use unknown).

function createWorkUnit<Input, Output = Input>(
  titleOrWorkUnit: string | WorkUnit<never, Input, Output>,
  action?: Action<never, Input, Output>,
  scope?: unknown,
): WorkUnit<never, Input, Output>;

repl

milesj commented 5 years ago

@bradzacher Oh wow, never would of though about that either. I'll give that shot. For a few of those scenarios, I've been using {} which has done the job also.