Fix class name references #55262

rbuckton · 2023-08-04T01:35:51Z

This fixes our emit for references to a class name inside of a class body when down-level emit moves the member containing the reference outside of the class body. This is necessary to preserve the "double bind" semantics of class names, where references to a class name inside of a class body are not affected when the reference to the class outside of the class body is changed.

We generally emit members outside of a class body when downleveling static fields, class static blocks, and private methods/accessors.

Fixes #54607

jakebailey · 2023-08-04T01:36:57Z

src/compiler/checker.ts

-    YieldExpression,
+    YieldExpression


Do you know what you ran which did this? Was it organize imports? Something else? Hoping to get to the bottom of these so we can be consistent (or, I guess formatting will prevent that).

Probably organize imports.

Hm, just tested running organize on checker.ts and this didn't happen, even when reordering things, really odd.

I think it was a result of running Organize Imports to remove an unused import. I just checked locally and that does remove the trailing ,.

jakebailey · 2023-08-04T18:33:16Z

(Abusing this PR slightly to make sure RWC is working; this PR touches emit so it should produce a diff)

@typescript-bot test this

typescript-bot · 2023-08-04T18:33:19Z

Heya @jakebailey, I've started to run the extended test suite on this PR at 9868cdb. You can monitor the build here.

jakebailey · 2023-08-04T20:12:36Z

src/compiler/checker.ts

@@ -1070,7 +1069,7 @@ import {
    WhileStatement,
    WideningContext,
    WithStatement,
-    YieldExpression,
+    YieldExpression


Suggested change

YieldExpression

YieldExpression,

Well, in any case, probably best to leave it until we can figure out what's happening. This is of course a nit (still looking at the PR)

jakebailey

Seems correct to me though it's a little hard for me to be sure staring at the emit output 😄

jakebailey · 2023-08-04T20:14:06Z

src/compiler/types.ts

-    ClassWithBodyScopedClassBinding          = 1 << 16,  // Decorated class that contains a binding to itself inside of the class body.
-    BodyScopedClassBinding                   = 1 << 17,  // Binding to a decorated class inside of the class's body.


Should we shift the other bits down or is it no big deal to leave them empty until later?

this is a driveby cleanup, right? I don't see any removal of BodyScopedClassBinding in the rest of the PR.

All references to BodyScopedClassBinding were removed in a prior PR, but I neglected to remove the flag. I can fix up the bits used as well.

sandersn

Seems good, although I have a few questions. And the emit changes Testing123_1 -> Testing123 are unexpected to me, so worth double-checking.

sandersn · 2023-08-04T21:41:14Z

src/compiler/types.ts

-    ClassWithBodyScopedClassBinding          = 1 << 16,  // Decorated class that contains a binding to itself inside of the class body.
-    BodyScopedClassBinding                   = 1 << 17,  // Binding to a decorated class inside of the class's body.


this is a driveby cleanup, right? I don't see any removal of BodyScopedClassBinding in the rest of the PR.

sandersn · 2023-08-04T21:42:05Z

src/compiler/checker.ts

-                        break;
-                    }
-
+                while (container.kind !== SyntaxKind.SourceFile && container.parent !== declaration) {


as I read it, this while loop is semantically equivalent to the old one. Am I missing anything?

No, they are equivalent, but I feel this is a bit easier to read and reason over.

Agree, the new one is much easier to read.

sandersn · 2023-08-04T21:44:33Z

src/compiler/checker.ts

@@ -27964,38 +27963,20 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {

        let declaration = localOrExportSymbol.valueDeclaration;
        if (declaration && localOrExportSymbol.flags & SymbolFlags.Class) {
-            // Due to the emit for class decorators, any reference to the class from inside of the class body


To check my understanding: before, the _a reference was only generated for ClassExpressions and decorated classes. Now it happens for any class declaration/expression that references itself in its body. Is that correct?

We won't always generate the reference, but now we will always set the flag. It's just that the cases where we need the flag set were no longer restricted to the cases mentioned in the comment.

sandersn · 2023-08-04T21:46:18Z

tests/baselines/reference/decoratedClassExportsCommonJS1.js

@@ -24,5 +24,5 @@ let Testing123 = Testing123_1 = class Testing123 {
 exports.Testing123 = Testing123;
 Testing123.prop1 = Testing123_1.prop0;
 exports.Testing123 = Testing123 = Testing123_1 = __decorate([
-    Something({ v: () => Testing123_1 })
+    Something({ v: () => Testing123 })


I don't understand how this diff happened. I assume it's because of the first block in the checker getting deleted, but I thought the edit to the second block would cause the reference to still get generated as Testing123_1.

Testing123_1 represents the class binding inside the class body, however a class decorator is evaluated outside of the class body, so it was pointing to the wrong reference prior to this change.

Fix class name references

9868cdb

rbuckton requested review from sandersn and jakebailey August 4, 2023 01:35

typescript-bot assigned rbuckton Aug 4, 2023

typescript-bot added Author: Team For Milestone Bug PRs that fix a bug with a specific milestone labels Aug 4, 2023

jakebailey reviewed Aug 4, 2023

View reviewed changes

jakebailey approved these changes Aug 4, 2023

View reviewed changes

sandersn approved these changes Aug 4, 2023

View reviewed changes

Renumber NodeCheckFlags to close gaps

c331eb0

rbuckton merged commit b1c4dc4 into main Aug 5, 2023
18 checks passed

rbuckton deleted the fix-class-name-references branch August 5, 2023 00:36

sosnovsky mentioned this pull request Aug 29, 2023

Temp variable re-usage breaks JS when building into single file #55560

Open

h-joo mentioned this pull request Oct 10, 2023

Google's feedback for TypeScript 5.3 Beta #56053

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix class name references #55262

Fix class name references #55262

rbuckton commented Aug 4, 2023

jakebailey Aug 4, 2023

rbuckton Aug 4, 2023

jakebailey Aug 4, 2023

rbuckton Aug 4, 2023

jakebailey commented Aug 4, 2023

typescript-bot commented Aug 4, 2023 •

edited

jakebailey Aug 4, 2023

jakebailey left a comment

jakebailey Aug 4, 2023

sandersn Aug 4, 2023

rbuckton Aug 4, 2023

sandersn left a comment

sandersn Aug 4, 2023

sandersn Aug 4, 2023

rbuckton Aug 4, 2023

sandersn Aug 7, 2023

sandersn Aug 4, 2023

rbuckton Aug 4, 2023

sandersn Aug 4, 2023

rbuckton Aug 4, 2023

		ClassWithBodyScopedClassBinding = 1 << 16, // Decorated class that contains a binding to itself inside of the class body.
		BodyScopedClassBinding = 1 << 17, // Binding to a decorated class inside of the class's body.

Fix class name references #55262

Fix class name references #55262

Conversation

rbuckton commented Aug 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakebailey commented Aug 4, 2023

typescript-bot commented Aug 4, 2023 • edited

Choose a reason for hiding this comment

jakebailey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sandersn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

typescript-bot commented Aug 4, 2023 •

edited