Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix use-before-init error when targeting ES2022 #55028

Merged
merged 7 commits into from
Jul 28, 2023
Merged

fix use-before-init error when targeting ES2022 #55028

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
f0315b9
fix use-before-init error when targeting ES2022
Zzzen Jul 15, 2023
b2c1019
remove redundant check of emit target
Zzzen Jul 20, 2023
a01de8f
add emitStandardClassFields flag
Zzzen Jul 21, 2023
f6c010a
add emitStandardClassFields flag
Zzzen Jul 21, 2023
a2dabce
simplify tests
Zzzen Jul 21, 2023
8a7c753
clean code
Zzzen Jul 21, 2023
66b8263
remove unused baselines
Zzzen Jul 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 10 additions & 10 deletions src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2782,8 +2782,8 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
return !isPropertyImmediatelyReferencedWithinDeclaration(declaration, usage, /*stopAtAnyPropertyDeclaration*/ false);
}
else if (isParameterPropertyDeclaration(declaration, declaration.parent)) {
// 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 useDefineForClassFields when bar is a parameter property
return !(useDefineForClassFields
Copy link
Member

@jakebailey jakebailey Jul 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  • isBlockScopedNameDeclaredBeforeUse
  • checkAndReportErrorForInvalidInitializer
  • getFirstTransformableStaticClassElement

And these might be wrong and would be fixed like in this PR:

  • checkConstructorDeclarationDiagnostics

@sandersn

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There of course may be a nuance I'm totally missing here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I completely agree with you. The checks for the emit target are unnecessary and should be removed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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;
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost, but when { useDefineForClassFields: true, target: "ES2017" }, getUseDefineForClassFields is true but emitStandardClassFields would be false.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(emitStandardClassFields would be compilerOptions.useDefineForClassFields !== false && getEmitScriptTarget(compilerOptions) >= ScriptTarget.ES2022)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

&& getContainingClass(declaration) === getContainingClass(usage)
&& isUsedInFunctionOrInstanceProperty(usage, declaration));
}
Expand Down Expand Up @@ -2814,7 +2814,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
return true;
}
if (isUsedInFunctionOrInstanceProperty(usage, declaration)) {
if (getEmitScriptTarget(compilerOptions) >= ScriptTarget.ES2022 && useDefineForClassFields
if (useDefineForClassFields
&& getContainingClass(declaration)
&& (isPropertyDeclaration(declaration) || isParameterPropertyDeclaration(declaration, declaration.parent))) {
return !isPropertyImmediatelyReferencedWithinDeclaration(declaration, usage, /*stopAtAnyPropertyDeclaration*/ true);
Expand Down Expand Up @@ -2971,7 +2971,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
case SyntaxKind.PropertyDeclaration:
// static properties in classes introduce temporary variables
if (hasStaticModifier(node)) {
return target < ScriptTarget.ESNext || !useDefineForClassFields;
return !useDefineForClassFields;
}
return requiresScopeChangeWorker((node as PropertyDeclaration).name);
default:
Expand Down Expand Up @@ -3389,10 +3389,10 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
// 1. When result is undefined, after checking for a missing "this."
// 2. When result is defined
function checkAndReportErrorForInvalidInitializer() {
if (propertyWithInvalidInitializer && !(useDefineForClassFields && getEmitScriptTarget(compilerOptions) >= ScriptTarget.ES2022)) {
if (propertyWithInvalidInitializer && !useDefineForClassFields) {
// We have a match, but the reference occurred within a property initializer and the identifier also binds
// to a local variable in the constructor where the code will be emitted. Note that this is actually allowed
// with ESNext+useDefineForClassFields because the scope semantics are different.
// with useDefineForClassFields because the scope semantics are different.
error(errorLocation,
errorLocation && propertyWithInvalidInitializer.type && textRangeContainsPositionInclusive(propertyWithInvalidInitializer.type, errorLocation.pos)
? Diagnostics.Type_of_instance_member_variable_0_cannot_reference_identifier_1_declared_in_the_constructor
Expand Down Expand Up @@ -31739,7 +31739,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))) {
diagnosticMessage = error(right, Diagnostics.Property_0_is_used_before_its_initialization, declarationName);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

else if (valueDeclaration.kind === SyntaxKind.ClassDeclaration &&
Expand Down Expand Up @@ -38429,7 +38429,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
case "length":
case "caller":
case "arguments":
if (compilerOptions.useDefineForClassFields) {
if (useDefineForClassFields) {
break;
}
// fall through
Expand Down Expand Up @@ -38634,7 +38634,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
// or the containing class declares instance member variables with initializers.

const superCallShouldBeRootLevel =
(getEmitScriptTarget(compilerOptions) !== ScriptTarget.ESNext || !useDefineForClassFields) &&
(!useDefineForClassFields) &&
(some((node.parent as ClassDeclaration).members, isInstancePropertyWithInitializerOrPrivateIdentifierProperty) ||
some(node.parameters, p => hasSyntacticModifier(p, ModifierFlags.ParameterPropertyModifier)));

Expand Down Expand Up @@ -42900,7 +42900,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
!legacyDecorators && languageVersion < ScriptTarget.ESNext &&
classOrConstructorParameterIsDecorated(/*useLegacyDecorators*/ false, node);
const willTransformPrivateElementsOrClassStaticBlocks = languageVersion <= ScriptTarget.ES2022;
const willTransformInitializers = !useDefineForClassFields || languageVersion < ScriptTarget.ES2022;
const willTransformInitializers = !useDefineForClassFields;
if (willTransformStaticElementsOfDecoratedClass || willTransformPrivateElementsOrClassStaticBlocks) {
for (const member of node.members) {
if (willTransformStaticElementsOfDecoratedClass && classElementOrClassElementParameterIsDecorated(/*useLegacyDecorators*/ false, member, node)) {
Expand Down
6 changes: 5 additions & 1 deletion tests/baselines/reference/assignParameterPropertyToPropertyDeclarationES2022.errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,10 @@ assignParameterPropertyToPropertyDeclarationES2022.ts(2,16): error TS2729: Prope
assignParameterPropertyToPropertyDeclarationES2022.ts(3,16): error TS2729: Property 'foo' is used before its initialization.
assignParameterPropertyToPropertyDeclarationES2022.ts(6,19): error TS2729: Property 'm3' is used before its initialization.
assignParameterPropertyToPropertyDeclarationES2022.ts(12,17): error TS2729: Property 'baz' is used before its initialization.
assignParameterPropertyToPropertyDeclarationES2022.ts(13,16): error TS2729: Property 'foo' is used before its initialization.


==== assignParameterPropertyToPropertyDeclarationES2022.ts (4 errors) ====
==== assignParameterPropertyToPropertyDeclarationES2022.ts (5 errors) ====
class C {
qux = this.bar // should error
~~~
Expand All @@ -30,6 +31,9 @@ assignParameterPropertyToPropertyDeclarationES2022.ts(12,17): error TS2729: Prop
!!! error TS2729: Property 'baz' is used before its initialization.
!!! related TS2728 assignParameterPropertyToPropertyDeclarationES2022.ts:13:5: 'baz' is declared here.
baz = this.foo; // should error
~~~
!!! error TS2729: Property 'foo' is used before its initialization.
!!! related TS2728 assignParameterPropertyToPropertyDeclarationES2022.ts:11:17: 'foo' is declared here.
Comment on lines 33 to +36
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems good!

quid = this.baz // ok
m2() {
this.foo // ok
Expand Down
12 changes: 0 additions & 12 deletions ...nce/classMemberInitializerScoping2(target=es2017,usedefineforclassfields=true).errors.txt
View file
Open in desktop

This file was deleted.

4 changes: 2 additions & 2 deletions ...eference/classMemberInitializerScoping2(target=es2017,usedefineforclassfields=true).types
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,8 @@ class C {
>C : C

p = x
>p : any
>x : any
>p : number
>x : 1

constructor(x: string) { }
>x : string
Expand Down
43 changes: 43 additions & 0 deletions tests/baselines/reference/defineProperty(target=es5).errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
defineProperty.ts(3,14): error TS2729: Property 'y' is used before its initialization.
defineProperty.ts(10,14): error TS2729: Property 'y' is used before its initialization.
defineProperty.ts(18,14): error TS2729: Property 'ka' is used before its initialization.
defineProperty.ts(22,15): error TS2729: Property 'ka' is used before its initialization.


==== defineProperty.ts (4 errors) ====
var x: "p" = "p"
class A {
a = this.y
~
!!! error TS2729: Property 'y' is used before its initialization.
!!! related TS2728 defineProperty.ts:9:17: 'y' is declared here.
b
public c;
["computed"] = 13
;[x] = 14
m() { }
constructor(public readonly y: number) { }
z = this.y
~
!!! error TS2729: Property 'y' is used before its initialization.
!!! related TS2728 defineProperty.ts:9:17: 'y' is declared here.
declare notEmitted;
}
class B {
public a;
}
class C extends B {
declare public a;
z = this.ka
~~
!!! error TS2729: Property 'ka' is used before its initialization.
!!! related TS2728 defineProperty.ts:19:17: 'ka' is declared here.
constructor(public ka: number) {
super()
}
ki = this.ka
~~
!!! error TS2729: Property 'ka' is used before its initialization.
!!! related TS2728 defineProperty.ts:19:17: 'ka' is declared here.
}

22 changes: 22 additions & 0 deletions tests/baselines/reference/initializationOrdering1(target=es2021).errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
initializationOrdering1.ts(11,16): error TS2729: Property 'facade' is used before its initialization.


==== initializationOrdering1.ts (1 errors) ====
class Helper {
create(): boolean {
return true
}
}

export class Broken {
constructor(readonly facade: Helper) {
console.log(this.bug)
}
bug = this.facade.create()
~~~~~~
!!! error TS2729: Property 'facade' is used before its initialization.
!!! related TS2728 initializationOrdering1.ts:8:17: 'facade' is declared here.

}

new Broken(new Helper)
43 changes: 43 additions & 0 deletions tests/baselines/reference/initializationOrdering1(target=es2021).js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
//// [tests/cases/conformance/classes/propertyMemberDeclarations/initializationOrdering1.ts] ////

//// [initializationOrdering1.ts]
class Helper {
create(): boolean {
return true
}
}

export class Broken {
constructor(readonly facade: Helper) {
console.log(this.bug)
}
bug = this.facade.create()

}

new Broken(new Helper)

//// [initializationOrdering1.js]
class Helper {
create() {
return true;
}
}
export class Broken {
constructor(facade) {
Object.defineProperty(this, "facade", {
enumerable: true,
configurable: true,
writable: true,
value: facade
});
Object.defineProperty(this, "bug", {
enumerable: true,
configurable: true,
writable: true,
value: this.facade.create()
});
console.log(this.bug);
}
}
new Broken(new Helper);
42 changes: 42 additions & 0 deletions tests/baselines/reference/initializationOrdering1(target=es2021).symbols
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
//// [tests/cases/conformance/classes/propertyMemberDeclarations/initializationOrdering1.ts] ////

=== initializationOrdering1.ts ===
class Helper {
>Helper : Symbol(Helper, Decl(initializationOrdering1.ts, 0, 0))

create(): boolean {
>create : Symbol(Helper.create, Decl(initializationOrdering1.ts, 0, 14))

return true
}
}

export class Broken {
>Broken : Symbol(Broken, Decl(initializationOrdering1.ts, 4, 1))

constructor(readonly facade: Helper) {
>facade : Symbol(Broken.facade, Decl(initializationOrdering1.ts, 7, 16))
>Helper : Symbol(Helper, Decl(initializationOrdering1.ts, 0, 0))

console.log(this.bug)
>console.log : Symbol(Console.log, Decl(lib.dom.d.ts, --, --))
>console : Symbol(console, Decl(lib.dom.d.ts, --, --))
>log : Symbol(Console.log, Decl(lib.dom.d.ts, --, --))
>this.bug : Symbol(Broken.bug, Decl(initializationOrdering1.ts, 9, 5))
>this : Symbol(Broken, Decl(initializationOrdering1.ts, 4, 1))
>bug : Symbol(Broken.bug, Decl(initializationOrdering1.ts, 9, 5))
}
bug = this.facade.create()
>bug : Symbol(Broken.bug, Decl(initializationOrdering1.ts, 9, 5))
>this.facade.create : Symbol(Helper.create, Decl(initializationOrdering1.ts, 0, 14))
>this.facade : Symbol(Broken.facade, Decl(initializationOrdering1.ts, 7, 16))
>this : Symbol(Broken, Decl(initializationOrdering1.ts, 4, 1))
>facade : Symbol(Broken.facade, Decl(initializationOrdering1.ts, 7, 16))
>create : Symbol(Helper.create, Decl(initializationOrdering1.ts, 0, 14))

}

new Broken(new Helper)
>Broken : Symbol(Broken, Decl(initializationOrdering1.ts, 4, 1))
>Helper : Symbol(Helper, Decl(initializationOrdering1.ts, 0, 0))

46 changes: 46 additions & 0 deletions tests/baselines/reference/initializationOrdering1(target=es2021).types
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
//// [tests/cases/conformance/classes/propertyMemberDeclarations/initializationOrdering1.ts] ////

=== initializationOrdering1.ts ===
class Helper {
>Helper : Helper

create(): boolean {
>create : () => boolean

return true
>true : true
}
}

export class Broken {
>Broken : Broken

constructor(readonly facade: Helper) {
>facade : Helper

console.log(this.bug)
>console.log(this.bug) : void
>console.log : (...data: any[]) => void
>console : Console
>log : (...data: any[]) => void
>this.bug : boolean
>this : this
>bug : boolean
}
bug = this.facade.create()
>bug : boolean
>this.facade.create() : boolean
>this.facade.create : () => boolean
>this.facade : Helper
>this : this
>facade : Helper
>create : () => boolean

}

new Broken(new Helper)
>new Broken(new Helper) : Broken
>Broken : typeof Broken
>new Helper : Helper
>Helper : typeof Helper

22 changes: 22 additions & 0 deletions tests/baselines/reference/initializationOrdering1(target=es2022).errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
initializationOrdering1.ts(11,16): error TS2729: Property 'facade' is used before its initialization.


==== initializationOrdering1.ts (1 errors) ====
class Helper {
create(): boolean {
return true
}
}

export class Broken {
constructor(readonly facade: Helper) {
console.log(this.bug)
}
bug = this.facade.create()
~~~~~~
!!! error TS2729: Property 'facade' is used before its initialization.
!!! related TS2728 initializationOrdering1.ts:8:17: 'facade' is declared here.

}

new Broken(new Helper)
34 changes: 34 additions & 0 deletions tests/baselines/reference/initializationOrdering1(target=es2022).js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
//// [tests/cases/conformance/classes/propertyMemberDeclarations/initializationOrdering1.ts] ////

//// [initializationOrdering1.ts]
class Helper {
create(): boolean {
return true
}
}

export class Broken {
constructor(readonly facade: Helper) {
console.log(this.bug)
}
bug = this.facade.create()

}

new Broken(new Helper)

//// [initializationOrdering1.js]
class Helper {
create() {
return true;
}
}
export class Broken {
facade;
constructor(facade) {
this.facade = facade;
console.log(this.bug);
}
bug = this.facade.create();
}
new Broken(new Helper);
Loading