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

Type loss when using generic decorators #17795

Closed
oleg-codaio opened this issue Aug 15, 2017 · 8 comments
Closed

Type loss when using generic decorators #17795

oleg-codaio opened this issue Aug 15, 2017 · 8 comments
Labels
Question An issue which isn't directly actionable in code

Comments

@oleg-codaio
Copy link

oleg-codaio commented Aug 15, 2017

TypeScript Version: 2.4.1

The following code aims to restrict the decorator decorate to members of a class inheriting from Base. However, it seems that K ends up only including members in Base, not in the inherited class. (This is a minimal reproducible example for other cases, e.g., restricting the decorator to methods within subclasses of Base of a certain return type.)

Code

abstract class Base {
  base() { return 1; };
}

type ProtoOf<T> = Pick<T, keyof T>;

function decorate<T extends Base, K extends keyof ProtoOf<T>, F extends T[K]>() {
  return (proto: ProtoOf<T>, propertyKey: K, descriptor: TypedPropertyDescriptor<F>) => {
    // Do stuff.
  };
}

class Test extends Base {
  @decorate()
  bar(): boolean {
    return false;
  }
}

Expected behavior:
No errors when applying @decorate() to bar().

Actual behavior:
Error: [ts] Argument of type '"bar"' is not assignable to parameter of type '"base"'.

@aluanhaddad
Copy link
Contributor

aluanhaddad commented Aug 16, 2017

You can work around it by returning a generic decorator from your decorator factory.

function decorate() {
  return <T extends Base, K extends keyof T, F extends T[K]>
    (proto: ProtoOf<T>, propertyKey: K, descriptor: TypedPropertyDescriptor<F>) => {
    // Do stuff.
  };
}

class Test extends Base {
  @decorate() bar(): boolean {
    return false;
  }
}

I think this behavior is correct since it is equivalent to writing

function decorate<T extends Base, K extends keyof ProtoOf<T>, F extends T[K]>() {
  return (proto: ProtoOf<T>, propertyKey: K, descriptor: TypedPropertyDescriptor<F>) => {
    // Do stuff.
  };
}

const decorator = decorate();

class Test extends Base {
  @decorator bar(): boolean {
    return false;
  }
}

@oleg-codaio
Copy link
Author

@aluanhaddad thanks for the update! Your solution does address the issue in my earlier example, though I guess the actual problem I was having had to do with decorator arguments:

function decorate<T extends Base>(property: keyof T) {
  return <U extends T, K extends keyof U, F extends U[K]>
    (proto: ProtoOf<U>, propertyKey: K, descriptor: TypedPropertyDescriptor<F>) => {
    // Do stuff.
  };
}

class Test extends Base {
  @decorate('foo') bar(): boolean {
    return false;
  }

  foo(): boolean { return false; }
}

So the decorator works on bar() fine now, but is failing with this error: [ts] Argument of type '"foo"' is not assignable to parameter of type '"base"'. Basically, is there a good way of having the decorator parameters be generic w.r.t. the decorated class?

@oleg-codaio
Copy link
Author

On another note, if bar() is marked private, then the original error returns - it seems that in this case, the decorator is only able to access public properties? My gut feeling is this is a limitation we'd have to deal with.

@mhegazy
Copy link
Contributor

mhegazy commented Aug 22, 2017

Just nothing that this function decorate<T extends Base>(property: keyof T) does not have any place to infer T, you can not infer a type from a name of one of its properties. and remember decorate is a factory that returns a function that will be used to decorate. so it is equivalent to decorate(property: keyof Base), which means you can only decorate properties that have the same name as ones in Base.

So @aluanhaddad's suggestion seems like the correct solution here.

@mhegazy mhegazy added the Question An issue which isn't directly actionable in code label Aug 22, 2017
@oleg-codaio
Copy link
Author

Your explanation makes sense. Though the issue still stands that it doesn't seem currently possible to have decorate properties work off the decorated class. It would be great if there was a way to give "context" to decorator properties as to what exact object they're decorating.

@aluanhaddad
Copy link
Contributor

aluanhaddad commented Aug 23, 2017

@vaskevich I'm not sure if I understand what you are trying to achieve correctly, but you can capture a string literal type parameter when the decorator factory is applied and then subsequently validate that this property exists on the class with the decorated method.

So, going back to your example, you can validate that a foo member exists on the decorated class and we can even place constraints on its type. For example, in the following, Test must have a callable member foo that has the same return type as the decorated member.

(Warning these types are pretty hard to read and I experienced several language service crashes in VS code due to recursion while working them out.)

type ProtoOf<T> = Pick<T, keyof T>;

function decorate<CK extends string>(property: CK) {

  return <
    T extends Base & {[P in CK]: G},
    K extends keyof T,
    F extends T[K] & G,
    G extends  ((...args: {}[]) => R),
    R>(
      proto: ProtoOf<T> & {[P in CK]: (...args: {}[]) => R},
      propertyKey: K,
      descriptor: TypedPropertyDescriptor<F>) => {
    // Do stuff.
  };
}

class Test extends Base {
  @decorate('foo') bar(): boolean {
    return false;
  }

  foo(): boolean {return false;}
}

The way this works is by capturing a type for the argument to the factory and using that argument to define the expected shape of the object that will be decorated. The declaration of G and the intersection type used to describe the target of the decorator was an experiment that seemed to work. The intent was that if we change foo to return a type not assignable to the return type of bar, we will get an error at the decorator application site.

Note that the the declaration of T is provided, as in my previous example, by the decorator and not the decorator factory.

@mhegazy
Copy link
Contributor

mhegazy commented Sep 6, 2017

Automatically closing this issue for housekeeping purposes. The issue labels indicate that it is unactionable at the moment or has already been addressed.

@mhegazy mhegazy closed this as completed Sep 6, 2017
@oleg-codaio
Copy link
Author

Thanks for the replies - I haven't been able to take a look at this again yet, but will open a new issue if there's a specific defect here (which doesn't seem to be the case).

@microsoft microsoft locked and limited conversation to collaborators Jun 14, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Question An issue which isn't directly actionable in code
Projects
None yet
Development

No branches or pull requests

3 participants