fix use-before-init error when targeting ES2022 #55028
Conversation
| // foo = this.bar is illegal in esnext+useDefineForClassFields when bar is a parameter property | ||
| return !(getEmitScriptTarget(compilerOptions) === ScriptTarget.ESNext && useDefineForClassFields | ||
| // foo = this.bar is illegal in es2022+useDefineForClassFields when bar is a parameter property | ||
| return !(getEmitScriptTarget(compilerOptions) >= ScriptTarget.ES2022 && useDefineForClassFields |
There was a problem hiding this comment.
Actually, I'm pretty sure that this should just be useDefineForClassFields and not check the script target. It seems like getUseDefineForClassFields already has the check for the script target and may have been missed in #42663.
Honestly, I suspect that there are other uses of the useDefineForClassFields which get this wrong or are at least redundant. Skimming, these may be redundant:
isBlockScopedNameDeclaredBeforeUsecheckAndReportErrorForInvalidInitializergetFirstTransformableStaticClassElement
And these might be wrong and would be fixed like in this PR:
checkConstructorDeclarationDiagnostics
There was a problem hiding this comment.
There of course may be a nuance I'm totally missing here.
There was a problem hiding this comment.
I completely agree with you. The checks for the emit target are unnecessary and should be removed.
There was a problem hiding this comment.
The emitted code for "useDefineForClassFields": true pre-class fields runs OK here, though: it's a series of Object.defineProperty calls in the constructor that replace what were previously assignments.
This is true first two uses of useDefineForClassFields you point out are the same way. You need both ES2022+ and useDefineForClassFields to emit ES standard class fields, and those--barring inheritance--are the things with different semantics from before.
Maybe it would be a good idea to introduce a new variable emitStandardClassFields that combines ES2022+ and useDefineForClassFields. Actually, useDefineForClassFields is combined with the ES2022 check often enough that it's likely that var emitStandardClassFields should replace var useDefinedForClassFields.
Either way, all the (===ESNext) occurrences need to change to (>=ES2022) now that class fields have been published in ES2022.
There was a problem hiding this comment.
Maybe it would be a good idea to introduce a new variable
emitStandardClassFieldsthat combines ES2022+ and useDefineForClassFields. Actually, useDefineForClassFields is combined with the ES2022 check often enough that it's likely thatvar emitStandardClassFieldsshould replacevar useDefinedForClassFields.
I'm confused; isn't that what the useDefineForClassFields variable is after #42663?
var useDefineForClassFields = getUseDefineForClassFields(compilerOptions);
// ...
export function getUseDefineForClassFields(compilerOptions: CompilerOptions): boolean {
return compilerOptions.useDefineForClassFields === undefined ? getEmitScriptTarget(compilerOptions) >= ScriptTarget.ES2022 : compilerOptions.useDefineForClassFields;
}
There was a problem hiding this comment.
Almost, but when { useDefineForClassFields: true, target: "ES2017" }, getUseDefineForClassFields is true but emitStandardClassFields would be false.
There was a problem hiding this comment.
(emitStandardClassFields would be compilerOptions.useDefineForClassFields !== false && getEmitScriptTarget(compilerOptions) >= ScriptTarget.ES2022)
There was a problem hiding this comment.
Quite the matrix of options 😄
Well, in any case, pulling these out to a consistent variable that does the right thing would be super helpful; it'd be nice to not have all of these es version comparisons that we can accidentally miss.
| baz = this.foo; // should error | ||
| ~~~ | ||
| !!! error TS2729: Property 'foo' is used before its initialization. | ||
| !!! related TS2728 assignParameterPropertyToPropertyDeclarationES2022.ts:11:17: 'foo' is declared here. |
| class C { | ||
| p = x | ||
| ~ | ||
| !!! error TS2301: Initializer of instance member variable 'p' cannot reference identifier 'x' declared in the constructor. |
There was a problem hiding this comment.
This test has the combo of es2017/esnext and useDefineForClassFields=true/false. Before, es2017 always errored, and esnext only errored when useDefineForClassFields=false.
Now, es2017 with useDefineForClassFields=false also no longer errors.
Is that correct? I'm not totally sure. @sandersn
| // @target: esnext, es2021, es2022 | ||
| // @useDefineForClassFields: true |
There was a problem hiding this comment.
| // @target: esnext, es2021, es2022 | |
| // @useDefineForClassFields: true | |
| // @target: esnext, es2021, es2022 | |
| // @useDefineForClassFields: false, true |
This should work and let you eliminate that other file, and make the baselines more clear, I think.
|
@typescript-bot test this |
|
Heya @jakebailey, I've started to run the extended test suite on this PR at b2c1019. You can monitor the build here. |
|
Heya @jakebailey, I've started to run the diff-based top-repos suite on this PR at b2c1019. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the diff-based user code test suite on this PR at b2c1019. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the tarball bundle task on this PR at b2c1019. You can monitor the build here. |
|
Hey @jakebailey, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running There is also a playground for this build and an npm module you can use via |
|
@jakebailey Here are the results of running the user test suite comparing There were infrastructure failures potentially unrelated to your change:
Otherwise... Something interesting changed - please have a look. Details
|
There was a problem hiding this comment.
As far as I can tell, tsc almost worked correctly before this PR: it provides an error for ES2022 standard class fields and does not when they're downleveled via Object.defineProperty or simple assignment:
The only problem is that (===ESNext) needs to change to (>=ES2022) everywhere in the code.
I also suggested that var useDefineForForClassFields should maybe include the ES2022 check so that it's used consistently everywhere.
| // foo = this.bar is illegal in esnext+useDefineForClassFields when bar is a parameter property | ||
| return !(getEmitScriptTarget(compilerOptions) === ScriptTarget.ESNext && useDefineForClassFields | ||
| // foo = this.bar is illegal in es2022+useDefineForClassFields when bar is a parameter property | ||
| return !(getEmitScriptTarget(compilerOptions) >= ScriptTarget.ES2022 && useDefineForClassFields |
There was a problem hiding this comment.
The emitted code for "useDefineForClassFields": true pre-class fields runs OK here, though: it's a series of Object.defineProperty calls in the constructor that replace what were previously assignments.
This is true first two uses of useDefineForClassFields you point out are the same way. You need both ES2022+ and useDefineForClassFields to emit ES standard class fields, and those--barring inheritance--are the things with different semantics from before.
Maybe it would be a good idea to introduce a new variable emitStandardClassFields that combines ES2022+ and useDefineForClassFields. Actually, useDefineForClassFields is combined with the ES2022 check often enough that it's likely that var emitStandardClassFields should replace var useDefinedForClassFields.
Either way, all the (===ESNext) occurrences need to change to (>=ES2022) now that class fields have been published in ES2022.
|
@jakebailey Here are the results of running the top-repos suite comparing Everything looks good! |
|
@typescript-bot test this |
|
Heya @jakebailey, I've started to run the diff-based top-repos suite on this PR at 66b8263. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the extended test suite on this PR at 66b8263. You can monitor the build here. |
|
Heya @jakebailey, I've started to run the diff-based user code test suite on this PR at 66b8263. You can monitor the build here. Update: The results are in! |
|
@jakebailey Here are the results of running the user test suite comparing There were infrastructure failures potentially unrelated to your change:
Otherwise... Something interesting changed - please have a look. Details
|
|
@jakebailey Here are the results of running the top-repos suite comparing Everything looks good! |
There was a problem hiding this comment.
I wish we didn't still need useDefineForClassFields, but I checked the 3 remaining uses and they're all still (reasonably) correct.
Even more wishful thinking: someday it would be nice to drop --useDefineForClassFields, but I think a few big projects may still use it for some time.
Edit: After reading @rbuckton 's comment, I additionally wish that parameter properties could be deprecated. =)
| @@ -31739,7 +31741,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |||
| && !(isAccessExpression(node) && isAccessExpression(node.expression)) | |||
| && !isBlockScopedNameDeclaredBeforeUse(valueDeclaration, right) | |||
| && !(isMethodDeclaration(valueDeclaration) && getCombinedModifierFlagsCached(valueDeclaration) & ModifierFlags.Static) | |||
| && (compilerOptions.useDefineForClassFields || !isPropertyDeclaredInAncestorClass(prop))) { | |||
| && (useDefineForClassFields || !isPropertyDeclaredInAncestorClass(prop))) { | |||
There was a problem hiding this comment.
useDefineForClassFields is incomplete here; it errors when the property is used before its initialisation even if there's a base property. But this program always prints 1 when targeting both es2017 and es2022, and when useDefineForClassFields is true or false:
class Base {
x = 1
}
class Ugh extends Base {
direct = this.x
x = 2
}
// should print 1 not undefined or 2
console.log(new Ugh().direct)Removing it entirely shows two related test cases: redeclaredProperty and redefinedPararameterProperty [sic]. Here are test cases that should still error:
class Base {
x = 1
}
class Ugh extends Base {
x;
direct = this.x
}
// should print undefined not 1 or 2 with [[Define]]
console.log(new Ugh().direct)and
class Base {
x = 1
}
class Ugh extends Base {
direct = this.x
constructor(public x) { }
}
// should print undefined not 1 or 2 with standard class fields.
console.log(new Ugh().direct)Confusingly, the first case should error whenever useDefineForClassFields is on, but the second should only error when emitStandardClassFields is on.
After typing all that up, I think this is a separate problem. I'll file a separate bug for it.
Edit: Thinking about it a bit more, even if the first example works, it's bad code! You shouldn't rely on lexical order of property initialisation in order to reference a base property's value. I'll let somebody else file the bug, and if nobody has a problem with it, we can leave it as it is.
|
I'm still of the opinion the underlying issue is with the emit, which I mentioned in the linked bug. I've been meaning to circle back on that after finishing the class fields emit-related changes for ES Decorators following 5.1, but haven't had time yet. I suppose it's fine to issue an error for now in lieu of actually fixing the emit issue, but I do think we should take the time to fix the emit issue in the future and remove this restriction. If we ever plan to fix this, it would probably be a good idea to either not mark this as fixing #50971, or leave it as fixing #50971 and create a new issue that recommends an emit fix so that we don't lose track of the issue. |
|
@rbuckton Not sure I can agree with the proposed emit in the linked comment - moving the initializer from a classfield into the constructor observably changes runtime behavior of JS code which violates design goals, and in a way that can’t be justified by inaccurate downleveling ( IMO if I write an initializer on a classfield, ECMAScript says that runs before the constructor, so I just need to accept that behavior is not going to play well with parameter properties. I’d find it even more confusing if the timing of initialization changed depending on whether I have any parameter props or not. |
|
@fatcerberus let's have that discussion on a new bug: #55132 @rbuckton Are you OK to merge this? It is at least a fix for an existing restriction, even if that restriction is incorrect. |
Fixes #50971