GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — Skip ambient modules in globalThis by sandersn · Pull Request #48938 · microsoft/TypeScript

Conversation

Nathan Shively-Sanders (sandersn) commented May 3, 2022

Copy link

Copy Markdown

Member

Previously, globalThis mistakenly included ambient modules, even though these are not values:

declare module "ambientModule" {
  export type typ = 1
  export var val: typ
}
type Oops = (typeof globalThis)[\"ambientModule\"]

This PR adds ambient modules to the kinds of things that are skipped when constructing globalThis' properties, along with block-scoped variables.

Fixes #48147

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          Skip ambient modules in globalThis

cc39d91

Previously, globalThis mistakenly included ambient modules, even though
these are not values:

```ts
declare module "ambientModule" {
  export type typ = 1
  export var val: typ
}
type Oops = (typeof globalThis)[\"ambientModule\"]
```

This PR adds ambient modules to the kinds of things that are skipped
when constructing `globalThis`' properties, along with block-scoped
variables.

TypeScript Bot (typescript-bot) assigned Nathan Shively-Sanders (sandersn)

May 3, 2022

TypeScript Bot (typescript-bot) added Author: Team For Milestone Bug PRs that fix a bug with a specific milestone labels

May 3, 2022

Nathan Shively-Sanders (sandersn) requested review from Gabriela Araujo Britto (gabritto) and Wesley Wigham (weswigham)

May 3, 2022 17:02

Gabriela Araujo Britto (gabritto) reviewed

May 3, 2022

View reviewed changes

Comment thread

src/compiler/checker.ts Outdated

		const varsOnly = new Map<string, Symbol>() as SymbolTable;
		members.forEach(p => {
		if (!(p.flags & SymbolFlags.BlockScoped)) {
		if (!(p.flags & SymbolFlags.BlockScoped) && !(p.flags & SymbolFlags.ValueModule && some(p.declarations, isAmbientModule))) {

Gabriela Araujo Britto (gabritto) May 3, 2022

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

some questions:

what's this ValueModule symbol flag/what is a value module?
is it precise enough to check if some declaration is an ambient module? what if some of the other declarations are not ambient modules? (that question may be answered by the one above)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Nathan Shively-Sanders (sandersn) May 3, 2022

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

A value module is a namespace OR declare module "foo" with values inside it. A namespace module is a namespace that has only types inside it. (Both kinds of module can contain other namespaces; any nested value turns the parent namespaces into value modules.)
I think the binder only allows ambient modules to merge with ambient modules? I'll have to check.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Nathan Shively-Sanders (sandersn) May 3, 2022

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Hm, the excludes are the same as other value modules: Value & ~(Function | Class | RegularEnum | ValueModule)

I'll see if I can name a function, class or enum "foo", but I don't think I can.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Nathan Shively-Sanders (sandersn) May 3, 2022

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

So, effectively only other ambient modules can merge with ambient modules.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Wesley Wigham (weswigham) May 3, 2022

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I'll see if I can name a function, class or enum "foo", but I don't think I can.

You can if you use a js merge, eg,

globalThis['"module"'] = function () {}

or similar.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

gabritto reacted with eyes emoji

All reactions

👀 1 reaction

Gabriela Araujo Britto (gabritto) reviewed

May 3, 2022

View reviewed changes

Comment thread

tests/cases/conformance/es2019/globalThisAmbientModules.ts Outdated


		type GlobalThis = keyof typeof globalThis
		// should error
		const bad2: GlobalThis = 'ambientModule'

Gabriela Araujo Britto (gabritto) May 3, 2022

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I'm not sure I understand what's being tested here, because the use of quotes makes it confusing for me.
const bad2: GlobalThis = 'ambientModule' should error with or without this bug fix, no? Because the ambient module is "ambientModule", with the quotes. Or am I misunderstanding something?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Nathan Shively-Sanders (sandersn) May 3, 2022

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

This was the original test case, which was already confusing. I should probably just leave it out: it turns out that keyof turns "\"foo\"" into just "foo". But it's not relevant to whether "\"foo\"" should be there in the first place.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Wesley Wigham (weswigham) reviewed

May 3, 2022

View reviewed changes

Comment thread

src/compiler/checker.ts Outdated

		const varsOnly = new Map<string, Symbol>() as SymbolTable;
		members.forEach(p => {
		if (!(p.flags & SymbolFlags.BlockScoped)) {
		if (!(p.flags & SymbolFlags.BlockScoped) && !(p.flags & SymbolFlags.ValueModule && some(p.declarations, isAmbientModule))) {

Wesley Wigham (weswigham) May 3, 2022

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

some? What if you make a global named "module" that ?merges? with the ambient module named "module"? (Is such a merge even possible?)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Nathan Shively-Sanders (sandersn) May 4, 2022 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

How do you make a global named "module"?

Edit: Ah, you give an example above. I guess every then?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          Skip only modules with every declaration ambient

ba8e45c

The modules are required to have at least one declaration so that our
treatment of `globalThis` stays the same, and
`globalThis.globalThis.globalThis` remains legal.

Gabriela Araujo Britto (gabritto) approved these changes

May 4, 2022

View reviewed changes

Wesley Wigham (weswigham) approved these changes

May 4, 2022

View reviewed changes

Nathan Shively-Sanders (sandersn) merged commit 46e8306 into main

May 5, 2022

Nathan Shively-Sanders (sandersn) deleted the skip-ambient-modules-in-globalThis branch

May 5, 2022 16:33

Microsoft (microsoft) locked as resolved and limited conversation to collaborators

Oct 22, 2025

Reviewers

Wesley Wigham (weswigham)

weswigham approved these changes

Gabriela Araujo Britto (gabritto)

gabritto approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

Nathan Shively-Sanders (sandersn)

Labels

Author: Team For Milestone Bug

PRs that fix a bug with a specific milestone

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Ambient external module name incorrectly appears in keyof typeof globalThis

Uh oh!

There was an error while loading. Please reload this page.

4 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

Conversation

Nathan Shively-Sanders (sandersn) commented May 3, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Nathan Shively-Sanders (sandersn) May 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Nathan Shively-Sanders (sandersn) May 4, 2022 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.