Remove EndOfDeclarationMarker and MergeDeclarationMarker nodes
#53901
Conversation
typescript-bot
added
Author: Team
For Uncommitted Bug
|
Can you give an example of what semantic change this PR has? I'm having trouble coming up with something concrete before/after where you could access the undecorated class. |
|
An easier example would be downlevel emit for class static fields: export class C {
static x = 1;
}// before:
// target: es2017
// module: commonjs
class C {
}
C.x = 1;
exports.C = C;
// target: es2017
// module: esnext
class C {
}
C.x = 1;
export { C };
// after:
// target: es2017
// module: commonjs
class C {
}
exports.C = C;
C.x = 1;
// target: es2017
// module: esnext
class C {
}
C.x = 1;
export { C };The difference here is the ordering of The only way you can see the uninitialized (or undecorated) class is if you have a cycle in your imports and run code during initialization that references the exported version of the class: // file.ts
import { C as C2 } from "./file";
export class C {
static x = C2.y;
static y = 2;
}This isn't much different than the same code without the circular import, so it's not much of a footgun. |
|
It's important to note that // a.ts
let x = 1;
export { x };
// b.ts
export { x };
let x = 1; |
|
Thanks for explaining! If it comes to it, how difficult is the change to make it fully semantically equivalent? I know you want this to unblock I know esbuild's output is resilient to this, though I think that's because it defines module exports via |
ESBuild isn't conformant with the spec either, since |
|
Sure, but in that case, would it just be |
Not difficult, but it requires an IIFE. We intentionally did not do that because we preferred emit like: class C {}
C.x = 1;over let C = (() => {
class C {}
C.x = 1;
return C;
})();and I'm pretty sure that opinion hasn't changed much, at least for static fields. |
For ESBuild? No, it would be in TDZ until |
It's |
Note though, that while it may be "not difficult", it is compounded for each new class feature, i.e.: let C = (() => {
let C = (() => {
class C {}
C.x = 1;
return C;
})();
C = __decorate(C, [dec]);
return C;
})(); |
I stand corrected on that count. But there are still three observable states:
|
|
Right, and the way to get into the "partially initialized" state would be to try and use the class inside itself such that you get it before But yeah, it's effectively no better than what this PR does in that case. |
Essentially, yes, which is why I said it would be exceedingly rare to end up in that state accidentally. |
jakebailey
left a comment
jakebailey
left a comment
There was a problem hiding this comment.
Given the discussion, this seems okay to me, though one of the other reviewers probably need to take a look.
|
@typescript-bot perf test |
|
Heya @rbuckton, I've started to run the parallelized Definitely Typed test suite on this PR at 4aad347. You can monitor the build here. Update: The results are in! |
|
Heya @rbuckton, I've started to run the diff-based user code test suite on this PR at 4aad347. You can monitor the build here. Update: The results are in! |
|
Heya @rbuckton, I've started to run the perf test suite on this PR at 4aad347. You can monitor the build here. Update: The results are in! |
|
@rbuckton Here are the results of running the user test suite comparing There were infrastructure failures potentially unrelated to your change:
Otherwise... Everything looks good! |
weswigham
left a comment
weswigham
left a comment
There was a problem hiding this comment.
Bit weird that you could observe the non-decorated class downlevel in some cases now, but if we're willing to accept that (and it seems like we are), this looks fine.
| PartiallyEmittedExpression, | ||
| CommaListExpression, | ||
| MergeDeclarationMarker, | ||
| EndOfDeclarationMarker, |
There was a problem hiding this comment.
Should we leave these in but toss a @deprecated and maybe a comment saying they're unused, at least until such a time were we direly need to reclaim the numbers? It might just be a lil' odd if we reuse these syntax kind numbers in the future. It'd reduce the API changes to zero, anyway.
There was a problem hiding this comment.
Is that necessary? The types they represented were /** @internal */, and the number values for SyntaxKind change often.
As I said up-thread, you can already observe them when using |
|
Hey @rbuckton, the results of running the DT tests are ready. |
|
@rbuckton Here they are:
CompilerComparison Report - main..53901
System
Hosts
Scenarios
TSServerComparison Report - main..53901
System
Hosts
Scenarios
StartupComparison Report - main..53901
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
These nodes were added to attempt to preserve the emit behavior of an older version of the TypeScript compiler prior to the introduction of transforms. We're a long way away from TS 1.x at this point, and the complexity these nodes add aren't worth preserving specific emit as long as the resulting emit is compatible.
One thing of note is that in some cases we used to defer the emit of an export for a class until after static initializers or decorators have run. While that was intended to be consistent with native imports and exports when downleveling to CommonJS/System, our actual ES module emit fails to preserve this semantic. The only way to preserve this semantic consistently would be to wrap all downlevel classes that have statics/legacy decorators in IIFEs. However, IIFEs reduce readability and we've tried to avoid them unless absolutely necessary.
While it is technically feasible to access an undecorated class via an import with this change to CJS/System emit, it would be an exceedingly rare occurrence to do so accidentally. I think the slight non-conformance of the emit is worth the simplification of the AST and reduction in complexity for class-related transforms. Removing these marker nodes will also unblock the
usingdeclarations work.If we find that we do want to adopt 100% semantic conformance for these exports, we can revisit wrapping such exported classes in IIFEs like we do for the new ES decorators emit.