Skip to content

Conversation

@sandersn
Copy link
Member

Late binding assumes a parent symbol. Currently, the binder does not check whether an assignment declaration has a parent symbol exists before creating a symbol for late binding. The easiest place to encounter this is with window assignments, because global (script) files have no symbol, unlike module files:

const X = "X"
window[X] = () => 1

It is possible to special-case window for late binding similar to the way that it's done for normal binding, or even any top-level assignment declaration in a global file. But I don't think there's enough value. For now, this PR prevents binding entirely for a dynamic name when there is no parent symbol.

Fixes #61583

Late binding assumes a parent symbol. Currently, the binder does not
check whether an assignment declaration has a parent symbol exists
before creating a symbol for late binding. The easiest place to
encounter this is with `window` assignments, because global (script)
files have no symbol, unlike module files:

```js
const X = "X"
window[X] = () => 1
```

It is possible to special-case `window` for late binding similar to the
way that it's done for normal binding. But I don't think there's enough
value. For now, this PR prevents binding entirely for a dynamic name when
there is no parent symbol.
Copilot AI review requested due to automatic review settings April 18, 2025 15:15
@github-project-automation github-project-automation bot moved this to Not started in PR Backlog Apr 18, 2025
@typescript-bot typescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Apr 18, 2025
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes an issue with latebound global JavaScript property assignments by preventing binding for dynamic names when no parent symbol is present.

  • Added an early return in the binder when parentSymbol is undefined in dynamic name assignments
  • Updated the test case to verify that global assignments to window properties using dynamic names are not latebound

Reviewed Changes

Copilot reviewed 2 out of 4 changed files in this pull request and generated 1 comment.

File Description
tests/cases/conformance/salsa/lateboundWindowToplevelAssignment.ts Added a test to verify that global dynamic assignments (e.g. window[...] = …) do not create a symbol
src/compiler/binder.ts Added a check for a missing parent symbol before binding dynamic names to prevent late binding in global files
Files not reviewed (2)
  • tests/baselines/reference/lateboundWindowToplevelAssignment.symbols: Language not supported
  • tests/baselines/reference/lateboundWindowToplevelAssignment.types: Language not supported

Comment on lines 3434 to +3437
else if (hasDynamicName(node)) {
if (!parentSymbol) {
return;
}
Copy link

Copilot AI Apr 18, 2025

Choose a reason for hiding this comment

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

The added check prevents binding for dynamic property assignments when no parent symbol is present, avoiding unintended symbol creation in global files. Consider adding an inline comment here to clarify why binding is skipped in this scenario for future maintainability.

Suggested change
else if (hasDynamicName(node)) {
if (!parentSymbol) {
return;
}
else if (hasDynamicName(node)) {
// Skip binding for dynamic property assignments when no parent symbol is present.
// This prevents unintended symbol creation in global files, ensuring correctness.
if (!parentSymbol) {
return;
}

Copilot uses AI. Check for mistakes.
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't really agree, but I would change my mind if a human spoke up saying that they do.

@sandersn sandersn requested review from iisaduan and rbuckton April 18, 2025 15:16

window[UPDATE_MARKER_FUNC]();
>window[UPDATE_MARKER_FUNC]() : error
>window[UPDATE_MARKER_FUNC] : error
Copy link
Member Author

Choose a reason for hiding this comment

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

Having type error shows that binding didn't happen.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug

Projects

Status: Not started

Development

Successfully merging this pull request may close these issues.

Internal error in tsc: "TypeError: Cannot read properties of undefined (reading 'flags')"

2 participants