[decorator metadata] implicit runtime reference created

[bradzacher]

Bug Report

🔎 Search Terms

decorator metadata shadow same name generic
(also searched in the "Domain: Decorators" tag)

🕗 Version & Regression Information

• This is the behavior in every version I tried, and I reviewed the FAQ for entries about decorators

⏯ Playground Link

Playground link with relevant code

💻 Code

import { Test } from './Test';

declare function deco(..._param: any): any;

export class Clazz {
  @deco
  method<Test>(Test: Test) {

  }
}

🙁 Actual behavior

As far as TS's unused vars logic is concerned - the above code just creates a type reference on the generic Test:

However when you look at the generated code:

__decorate([
    deco,
    __metadata("design:type", Function),
    __metadata("design:paramtypes", [typeof (_a = typeof Test !== "undefined" && Test) === "function" ? _a : Object]),
    __metadata("design:returntype", void 0)
], Clazz.prototype, "method", null);

The decorator actually creates an implicit value reference on the import Test.

Which means if you want to satisfy the noUnusedLocals error, you will (unknowingly) change the runtime behaviour.

🙂 Expected behavior

I think this is the intended behaviour? TBH I'm not entirely sure.
I haven't found any docs about how it's supposed to work.
I think it should not be generating any runtime references.

__decorate([
    deco,
    __metadata("design:type", Function),
    __metadata("design:paramtypes", [Object]),
    __metadata("design:returntype", void 0)
], Clazz.prototype, "method", null);

For context, in @typescript-eslint we have a scope analyser which also attempts to understand the runtime value references created by emitDecoratorMetadata. This understanding means lint rules like no-unused-vars and consistent-type-imports can understand the runtime code and provide correct lints/fixes. This logic was particularly added for consistent-type-imports, which broke people's code due to it not understanding that decorators created implicit value references.

A user presented a bug due to an (incorrect reference) - typescript-eslint/typescript-eslint#2994.
Whilst investigating the fix, I want to ensure it's fixed correctly for all cases, including the one presented above.

If it's entirely intentional that this creates a value reference - then I'll make the referencer work the same way.

[vegerot]

Bumping this because it has not been responded to yet and is a blocker for typescript-eslint/typescript-eslint#2994

[bradzacher]

This bug still appears in TS4.9, though the situation has improved in TS4.9.

In <TS4.9, TS would actually emit broken code:

import { Component } from 'react';

declare function deco(..._param: any): any;

export class Clazz {
  @deco
  method<Component>(Component: Component) {

  }
}

//// emits ////

/* ~~ snip helpers ~~ */
export class Clazz {
    method(Component) {
    }
}
__decorate([
    deco,
    __metadata("design:type", Function),
    __metadata("design:paramtypes", [Component]),
    __metadata("design:returntype", void 0)
], Clazz.prototype, "method", null);

playground

Note that on the 3rd to last line TS emitted __metadata("design:paramtypes", [Component]), but TS did not emit the react import that it believed was unused - leading to completely broken code (Uncaught ReferenceError: Component is not defined).

As of TS4.9 however TS does emit working code:

__decorate([
    deco,
    __metadata("design:type", Function),
    __metadata("design:paramtypes", [typeof (_a = typeof Component !== "undefined" && Component) === "function" ? _a : Object]),
    __metadata("design:returntype", void 0)
], Clazz.prototype, "method", null);

It guards against the non-existence of Component.

However this is still a problem if, for example, you have a top-level (or global) variable of the same name defined in a way that TS won't scrub:

function Component() {}

declare function deco(..._param: any): any;

export class Clazz {
  @deco
  method<Component>(Component: Component) {

  }
}

//// emits ////

__decorate([
    deco,
    __metadata("design:type", Function),
    __metadata("design:paramtypes", [typeof (_a = typeof Component !== "undefined" && Component) === "function" ? _a : Object]),
    __metadata("design:returntype", void 0)
], Clazz.prototype, "method", null);

playground

Quite the interesting problem!

[rbuckton]

At present, this is the expected behavior. If the compiler is confident that Test in value-space points to a constructor that produces a Test type, then it will emit an unconditional Test reference. If the compiler is confident that Test in the value-space does not point to a constructor that produces a Test type, it will emit an unconditional Object reference. If Test comes from an import and the compiler is unable to determine its runtime value at all, either due to an unresolved import or the presence of --isolatedModules, it emits the conditional reference to Test. In the conditional reference case, we perform a best effort runtime check to verify that Test exists and is potentially a constructor (i.e., typeof Test === "function"), but that is the limit of what we can verify at runtime.

Any broad change to be stricter when to use Object over the conditional check is likely to break existing projects, so there's likely no broad change we could make here. I also do not believe we're likely to add additional flags to tweak --emitDecoratorMetadata at this time.

If you want to ensure that Object is reported, you can use a type-only import (i.e., import { type Test } ...). If you want to check for this more broadly, I'd suggest using either --verbatimModuleSyntax or --importsNotUsedAsValues error to find any imports in your project that only are used at types, but aren't marked with type.

[rbuckton]

[...] so there's likely no broad change we could make here. [...]

Correction, the only safe change I could see us make here would be to report an error when we can't confidently detect whether the name of a type points to a value. Currently we don't report an error for this specific case as we would usually report an error elsewhere if you are pointing to an unresolved reference. I would need a more detailed example case than the one provided, however, as the example in the OP doesn't indicate what's actually found at './Test'.