Skip to content

Don't latebind global JS property assignments #61593

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

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

@Copilot 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
Preview

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 is powered by AI, so mistakes are possible. Review output carefully before use.

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 rbuckton and iisaduan 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