Skip to content

Parameter decorators not checked as thoroughly as invocations. #43132

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

Open
rbuckton opened this issue Mar 8, 2021 · 2 comments
Open

Parameter decorators not checked as thoroughly as invocations. #43132

rbuckton opened this issue Mar 8, 2021 · 2 comments
Labels
Meta-Issue An issue about the team, or the direction of TypeScript

Comments

@rbuckton
Copy link
Member

rbuckton commented Mar 8, 2021

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.

@RyanCavanaugh RyanCavanaugh added the Meta-Issue An issue about the team, or the direction of TypeScript label Mar 9, 2021
@sorgloomer
Copy link

sorgloomer commented Jan 24, 2022

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
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Meta-Issue An issue about the team, or the direction of TypeScript
Projects
None yet
Development

No branches or pull requests

3 participants