Skip to content

Fixed declaration emit for expando properties on function declarations declared using element access expressions #55183

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.

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

Conversation

Andarist
Copy link
Contributor

fixes #54811

@typescript-bot typescript-bot added the For Backlog Bug PRs that fix a backlog bug label Jul 28, 2023
@sandersn sandersn requested review from weswigham and sandersn August 11, 2023 21:55
Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

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

Looks good: I had one question, one name change, and one test case request.

const c = "C"

export function decl () {}
decl["B"] = 'foo'
Copy link
Member

Choose a reason for hiding this comment

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

what does the BinaryExpression case look like? If there's not a test for it here or somewhere else, please add one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's the decl2 case. This is the baseline diff without the isBinaryExpression check:

baseline diff
diff --git a/tests/baselines/reference/declarationEmitLateBoundAssignments.js b/tests/baselines/reference/declarationEmitLateBoundAssignments.js
index b76db45754..763511ec0d 100644
--- a/tests/baselines/reference/declarationEmitLateBoundAssignments.js
+++ b/tests/baselines/reference/declarationEmitLateBoundAssignments.js
@@ -38,5 +38,4 @@ const a = foo[dashStrMem];
 export declare function foo(): void;
 export declare namespace foo {
     var bar: number;
-    var strMemName: string;
 }
diff --git a/tests/baselines/reference/declarationEmitLateBoundAssignments2.js b/tests/baselines/reference/declarationEmitLateBoundAssignments2.js
index 4ec90c2606..b2d3efc2a8 100644
--- a/tests/baselines/reference/declarationEmitLateBoundAssignments2.js
+++ b/tests/baselines/reference/declarationEmitLateBoundAssignments2.js
@@ -37,9 +37,6 @@ export declare namespace decl {
     var B: string;
 }
 export declare function decl2(): void;
-export declare namespace decl2 {
-    var C: number;
-}
 export declare const arrow: {
     (): void;
     B: string;

@Andarist Andarist requested a review from sandersn August 14, 2023 19:30
Copy link
Member

@weswigham weswigham left a comment

Choose a reason for hiding this comment

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

Can we just add some test cases for expando element access properties with non-identifier names (numbers, strings with spaces, unique symbols)? This looks good, I just wanna make sure we don't try and emit { a b: string } or something equally silly.

Comment on lines +131 to +146
export declare function decl3(): void;
export declare namespace decl3 { }
export declare function decl4(): void;
export declare namespace decl4 { }
export declare function decl5(): void;
export declare namespace decl5 { }
export declare function decl6(): void;
export declare namespace decl6 { }
export declare function decl7(): void;
export declare namespace decl7 { }
export declare function decl8(): void;
export declare namespace decl8 { }
export declare function decl9(): void;
export declare namespace decl9 { }
export declare function decl10(): void;
export declare namespace decl10 { }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

those don't work because function declarations are emitted as namespaces and non-identifiers can't be exported from namespaces, see: #55190

I could make an effort to avoid those empty namespaces. The issue itself stands though. I think it's a slight problem that function declarations and function expressions have such a subtle and silent (!) difference in their capabilities to emit expando members

@Andarist
Copy link
Contributor Author

Can we just add some test cases for expando element access properties with non-identifier names (numbers, strings with spaces, unique symbols)?

Nice, this helped me find the issue in isStringNamed. I didn't add any tests related to symbols because I'm pretty sure that symbols are already covered OK (I fixed myself a couple of issues related to expando properties with symbol names, such as #54726 and #55357 )

@Andarist Andarist requested a review from weswigham August 14, 2023 22:31
Copy link
Member

@weswigham weswigham left a comment

Choose a reason for hiding this comment

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

Sorry for the delay on coming back to this PR~

@weswigham weswigham merged commit 4c34a18 into microsoft:main Sep 13, 2023
@Andarist Andarist deleted the fix/element-access-late-bound-assignments-on-fn-decl branch September 13, 2023 19:21
snovader pushed a commit to EG-A-S/TypeScript that referenced this pull request Sep 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Backlog Bug PRs that fix a backlog bug
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Missing computed properties on expando functions
4 participants