-
Notifications
You must be signed in to change notification settings - Fork 12.2k
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
Use of decorator factories as decorators is not flagged as an error #3246
Comments
Oh, it's the bivariance again. Ideally the signature of F would not be assignable to PropertyDecorator because Object is not assignable to The original issue raised in #2249 is different though, because the type of F's parameter is What is the requirement for the return type of a property decorator? Could we require a property decorator to be void, or is that too drastic? That would fix this because F returns a function. |
@JsonFreeman yes you are correct they are two different issues. the core is that just doing the assignablity check is not enough to catch common errors of using the decorator vs. the factory. @rbuckton is looking into treating it as a call expression instead to make it an error to pass extra arguments (i.e. as if we are calling F with |
Yeah that is another possibility, but it breaks down if you have a decorator factory with two parameters... It seems like a fix that happens to work in this case, but doesn't really solve the overall problem of a decorator factory being confused with a decorator. That is why I think enforcing a void return might make more sense. I also think this highlights an inconsistency between assignability and call resolution. Assignability allows "calling with too many arguments", but call resolution does not. I suppose the reason for the inconsistency is that calling something with too many arguments is totally useless, but assigning something with fewer arguments is useful if the assignment target is trying to be maximally generous to the source with its parameters. |
the return type can not always be void, e.g. in the case of class, and method decorators. we talked about two way assignability (as a substitute to identity), but this seems to be too restrictive. |
I'm going the same route we went with tagged template expressions and resolving a decorator as a call. For tagged template expressions, we create a synthetic argument in To make the same approach work for decorators, I'm looking into adding a |
Ok, yes. In general calling something is more reliable than checking assignability. You get better arity checking, you get parameter contravariance, and you even get type argument inference for the type parameters of the decorator 😃 It is a bit like contextual signature instantiation (which is |
I've created a PR for this issue, based on my proposed resolution above. |
As mentioned in #2249 (comment), consider this:
The problem here is that the compiler compares the signature of the decorator factory
F
againstdeclare type PropertyDecorator = (target: Object, propertyKey: string | symbol) => void;
which ends up to be assignable. I think we need a stronger check here to make sure we catch these issue.The text was updated successfully, but these errors were encountered: