Use of decorator factories as decorators is not flagged as an error

[mhegazy]

As mentioned in #2249 (comment), consider this:

// decorator factory
function F(options?: {optional: boolean}) {
     // returns a property decorator
    return (target , propertyKey): void => {
        // decorator implementation
    }
}

class C {
    @F()    // OK, used as a factory
    prop1; 

    @F   // No error
    prop2; 
}

The problem here is that the compiler compares the signature of the decorator factory F against declare 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.

[JsonFreeman]

Oh, it's the bivariance again. Ideally the signature of F would not be assignable to PropertyDecorator because Object is not assignable to {optional: boolean}. But because of signature bivariance we allow this.

The original issue raised in #2249 is different though, because the type of F's parameter is any.

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.

[mhegazy]

@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 (target, propetyKey) ).

[JsonFreeman]

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.

[mhegazy]

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.

[rbuckton]

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 getEffectiveCallArguments that we later replace with globalTemplateStringsArrayType when we call checkApplicableSignature.

To make the same approach work for decorators, I'm looking into adding a getSyntheticArgumentType which will encompass both the TemplateStringsArray argument for a tagged template expression, and the arguments for a decorator based on its target. The result is much more reliable and flexible than the current approach, and resolves some other small issues with typing decorators today.

[JsonFreeman]

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 instantiateSignatureInContextOfin the compiler), but it does the whole call, not just the type argument inference.

[rbuckton]

I've created a PR for this issue, based on my proposed resolution above.