microsoft / TypeScript

TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
https://www.typescriptlang.org
Apache License 2.0
100.19k stars 12.38k forks source link

Parameter decorators not checked as thoroughly as invocations. #43132

Open rbuckton opened 3 years ago

rbuckton commented 3 years ago

This is more of a meta issue that covers the following existing issues, but with some additional context:

The above issues reference various problems with decorators and decorator type checking. The following are some examples of issues I'd like to address:

Constructor parameter decorators receive undefined for propertyKey

The definition of ParameterDecorator indicates the propertyKey is string | symbol, however constructor parameters receive undefined. Receiving undefined is an intentional behavior, as it allows the decorator author to detect they are decorating a constructor's parameter. However, this is not correctly noted in the definition of ParameterDecorator. That is the crux of #33260.

However, there is more that we should be doing here, as we do not accurately check the type that should be passed for the propertyKey argument:

declare function P(target: Function, propertyKey: undefined, parameterIndex: number): void;

class C {
  constructor(@P x: any) {} // ok (correct)
  method(@P x: any) {} // ok (should be an error)
}

P(C, undefined, 0); // ok (correct)
P(C.prototype.method, "method", 0); // error (correct)

The converse should also be addressed. If the propertyKey only allows string | symbol, then the decorator should be an error when used on a constructor parameter:

declare function P(target: Function, propertyKey: string | symbol, parameterIndex: number): void;

class C {
  constructor(@P x: any) {} // ok (should be an error)
  method(@P x: any) {} // ok (correct)
}

P(C, undefined, 0); // error (correct)
P(C.prototype.method, "method", 0); // ok (correct)

Parameter decorators do not check target type

In this case, we are not correctly checking the type of the target parameter:

declare function P(target: new (...args: any[]) => any, propertyKey: string | symbol | undefined, parameterIndex: number): void;

class C {
  constructor(@P x: any) {} // ok (correct)
  method(@P x: any) {} // ok (should be an error)
}

P(C, undefined, 0); // ok (correct)
P(C.prototype.method, "method", 0); // error (correct)

And the same issue occurs when target is a function type:

declare function P(target: (...args: any[]) => any, propertyKey: string | symbol | undefined, parameterIndex: number): void;

class C {
  constructor(@P x: any) {} // ok (should be an error)
  method(@P x: any) {} // ok (correct)
}

P(C, undefined, 0); // error (correct)
P(C.prototype.method, "method", 0); // ok (correct)

Constraining the type of a parameter

Constraining the type of a parameter is difficult (but not impossible) now that we have string literal types:


type WithConstructorParameter<
  F extends (new (...args: A) => any),
  I extends number,
  T,
  A extends any[] = ConstructorParameters<F>
> = F & (new (...args: ReplaceArg<A, I, T>) => any);

type ReplaceArg<A extends any[], I extends number, T> = { [P in keyof A]: P extends `${I}` ? T : A[P] };

However, we again do not seem to check this correctly in decorators:

type Expected = { x: number };

declare function P<F extends (new (...args: any[]) => any), I extends number>(
  target: WithConstructorParameter<F, I, Expected>,
  propertyKey: undefined,
  parameterIndex: I
): void;

class C {
  constructor(@P x: { y: number }) {} // ok (should be error)
}

P(C, undefined, 0); // error (correct), because `{ y: number }` is not assignable to `{ x: number }`

This is not a new issue, its likely existed since decorators were added.

sorgloomer commented 2 years ago

Distilled down the inference example to a subjectively simpler one:

declare function decorator1(target: MyClass, prop: 'method1', index: 0)  : void;
declare function decorator2(target: string , prop: any      , index: any): void;
declare function decorator3(target: any    , prop: 'wrong'  , index: any): void;
declare function decorator4(target: any    , prop: any      , index: 99) : void;

class MyClass {
     method1(@decorator1 foo: number) {}
     //      ^^^^^^^^^^^ should pass, now `Argument of type 'number' is not assignable to parameter of type '0'`
     method2(@decorator2 foo: number) {}
     //      ^^^^^^^^^^^ should fail, now passes
     method3(@decorator3 foo: number) {}
     //      ^^^^^^^^^^^ should fail, now passes
     method4(@decorator4 foo: number) {}
     //      ^^^^^^^^^^^ should fail, now fails but for the wrong reason, message is the same as #1
}

decorator1(MyClass.prototype, 'method1', 0); // correct, no type errors
//
decorator2(MyClass.prototype, 'method1', 0); // correct, type error
//         ^^^^^^^^^^^^^^^^^
decorator3(MyClass.prototype, 'method1', 0); // correct, type error
//                            ^^^^^^^^^
decorator4(MyClass.prototype, 'method1', 0); // correct, type error
//                                       ^

Playground

sorgloomer commented 1 year ago

It seems inference for decorators shipped in TypeScript 5, all my above testcases work as expected in TypeScript 5.