Open bradzacher opened 3 years ago
Bumping this because it has not been responded to yet and is a blocker for https://github.com/typescript-eslint/typescript-eslint/issues/2994
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);
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);
Quite the interesting problem!
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
.
[...] 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'
.
Bug Report
🔎 Search Terms
decorator metadata shadow same name generic (also searched in the "Domain: Decorators" tag)
🕗 Version & Regression Information
⏯ Playground Link
Playground link with relevant code
💻 Code
🙁 Actual behavior
As far as TS's unused vars logic is concerned - the above code just creates a type reference on the generic![image](https://user-images.githubusercontent.com/7462525/107127241-a2f8c800-6869-11eb-8322-daddd4bf10ca.png)
Test
:However when you look at the generated code:
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.
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 likeno-unused-vars
andconsistent-type-imports
can understand the runtime code and provide correct lints/fixes. This logic was particularly added forconsistent-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) - https://github.com/typescript-eslint/typescript-eslint/issues/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.