Bind toplevel this assignments as global declarations #22891

sandersn · 2018-03-26T23:18:52Z

Previously, this-assignments at the top level of a source file did not bind anything. The source file case was not handled in bindThisPropertyAssignment, however, so when #22643 added an assert that it handled every case, files like this started crashing on the assert:

this.a = 10

With the new commits, this fix correctly binds references to a and this.a in both script files and commonjs modules. Binding in Typescript files is unchanged.

Checking also understands toplevel this better in Javascript files. Now, this: { [s: string]: any } & globals. That means this.a will have the correct type, but also this.NaN. Checking in Typescript files is unchanged because I don't want to encourage this usage there.

Improves the fix for #22888

ghost · 2018-03-26T23:40:18Z

src/compiler/binder.ts

+                case SyntaxKind.SourceFile:
+                    // this.foo assignment in a source file
+                    // Bind this property in the global namespace
+                    declareSymbolAndAddToSymbolTable(node, SymbolFlags.FunctionScopedVariable, SymbolFlags.FunctionScopedVariableExcludes);


The following test doesn't work perfectly:

// @out: output.js // @allowJs: true // @Filename: a.js this.a = 10; a; // @Filename: /b.js this.a; a;

In /b.js, this.a will be any, while a is still number.
I don't see how this ends up in globals? declareSourceFileMember seems to put it in the source file locals, but does that really point to the globals instead?

this seems half working, we either acknowledge that this at the top level is the global scope, and mirror that both for declarations and look ups, or we do not do either.

in other words, if this.a = 10 is a declaration of a global a, then should var a = 10; and both should be visible on this.a.

For this bug fix, i would say just avoid the assert by handling the SourceFile case.

Note that top level this in a commonjs file is the initial exports object.

$ echo 'this.foo = "bar"' > a.js $ node -p 'require("./a")' { foo: 'bar' }

this is only global in a traditional namespace-style file.

Binding

@andy-ms the single file case works the same way. file.locals declarations on a script (not module) source file will be available in the global namespace. As @Kovensky points out, we should put the declaration on file.exports when the source file is a module.

Type checking

@mhegazy Regarding the type checking, I guess you want us to start treating this as { [s: string]: any, a: number } instead of any? For the module case, we can treat it as the type of exports, which is actually the source file, I think. I guess we can look up members on top level this by looking them up in file.locals or file.exports, and just returning any if the member is not found.

Short term fix

@mhegazy It’s easy enough to no-op source files — do you think a short-term fix is worth it here? I guess the customer that @sheetalkamat was working with would benefit. I’ll create a separate PR with just the no-op and keep this one open for the complete fix.

Make sure that the global declaration is visible in other files

sandersn · 2018-03-27T22:57:00Z

See revised comments above; this now binds and checks toplevel this.

src/compiler/binder.ts

ghost · 2018-03-27T23:37:03Z

src/compiler/checker.ts

+                if (isSourceFile(container)) {
+                    // look up in the source file's locals or exports
+                    const parent = getSymbolOfNode(container);
+                    return createAnonymousType(parent, container.commonJsModuleIndicator ? parent.exports : globals, emptyArray, emptyArray, createIndexInfo(anyType, /*isReadonly*/ false), undefined);


createAnonymousType looks expensive if we call it on every this expression not in a class/function. Don't we already get a type for the source file when doing import a = require("source-file");?
Also, we should maintain a type for globals; that would fix #14052. But until then createAnonymousType(globals) will be really expensive if there are 1000 globals, and there might be! Might be better to leave that case as a // TODO: GH#14052 and leave as any for now.

I am also not sure i understand the need for the indexer.. why not just return a "global" type?

@mhegazy Do you want an error on any unknown toplevel this access in Javascript? I don't think we should be stricter than Typescript here.

@andy-ms I'll take a look and see what I can do. A global type seems like a good idea.

I don't think the current code is a real performance problem, though, for a couple of reasons:

It only happens a couple of times per project, even in large code bases like chrome-devtools-frontend. Compare to checkObjectLiteral, which creates a fresh anonymous type for every object literal in a code base.

createAnonymousType just saves a pointer to the globals symbol table (which does in fact have hundreds of entries just from JS built-ins). The property symbols themselves cache their own resolved type if it's requested.

1. Add a global type. This is only accessible via toplevel `this` in javascript scripts, but could be the type of other values later, and also given a typename later. 2. Use the type of the containing file in the export case.

sandersn · 2018-03-29T16:25:49Z

Note that this PR creates a global type but doesn't give it a type name, and only makes it accessible via this in a js file. We can add a type name and additional values that have the type when the "global" proposal in TC39 decides on a value name. And we can define behaviour for both Typescript and Javascript.

sandersn · 2018-03-30T20:33:15Z

After the design meeting, we decided to hold this change until the "global" proposal on TC39 finds a good syntax. Then we'll use that as the type and value name, both in Typescript and in Javascript.

typescript-bot · 2018-07-25T18:31:17Z

Thanks for your contribution. This PR has not been updated in a while and cannot be automatically merged at the time being. For housekeeping purposes we are closing stale PRs. If you'd still like to continue working on this PR, please leave a message and one of the maintainers can reopen it.

Bind toplevel this assignments as global decls

a0dc554

sandersn requested review from mhegazy and a user March 26, 2018 23:18

sandersn changed the title ~~Bind toplevel this assignments as global decls~~ Bind toplevel this assignments as global declarations Mar 26, 2018

ghost reviewed Mar 26, 2018

View reviewed changes

sandersn added 3 commits March 27, 2018 11:01

Improve toplevel this-assignment test

e58da91

Make sure that the global declaration is visible in other files

Bind+check toplevel this in commonjs

5831e80

Merge branch 'master' into bind-toplevel-this

59e2617

ghost suggested changes Mar 27, 2018

View reviewed changes

Don't create new types for toplevel every time

2050027

1. Add a global type. This is only accessible via toplevel `this` in javascript scripts, but could be the type of other values later, and also given a typename later. 2. Use the type of the containing file in the export case.

mhegazy mentioned this pull request Mar 31, 2018

Design Meeting Notes, 3/31/2018 #23045

Closed

This was referenced Apr 17, 2018

Add create-react-app and puppeteer user tests #23471

Merged

Add axios' source to user tests #23490

Merged

typescript-bot closed this Jul 25, 2018

sandersn mentioned this pull request Jan 9, 2019

Add globalThis #29332

Merged

jakebailey deleted the bind-toplevel-this branch November 7, 2022 17:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bind toplevel this assignments as global declarations #22891

Bind toplevel this assignments as global declarations #22891

sandersn commented Mar 26, 2018 •

edited

Loading

ghost Mar 26, 2018

mhegazy Mar 26, 2018

Jessidhia Mar 27, 2018 •

edited

Loading

sandersn Mar 27, 2018

sandersn commented Mar 27, 2018

ghost Mar 27, 2018 •

edited by ghost

Loading

mhegazy Mar 28, 2018

sandersn Mar 28, 2018 •

edited

Loading

sandersn commented Mar 29, 2018 •

edited

Loading

sandersn commented Mar 30, 2018 •

edited

Loading

typescript-bot commented Jul 25, 2018

Bind toplevel this assignments as global declarations #22891

Bind toplevel this assignments as global declarations #22891

Conversation

sandersn commented Mar 26, 2018 • edited Loading

ghost Mar 26, 2018

Choose a reason for hiding this comment

mhegazy Mar 26, 2018

Choose a reason for hiding this comment

Jessidhia Mar 27, 2018 • edited Loading

Choose a reason for hiding this comment

sandersn Mar 27, 2018

Choose a reason for hiding this comment

Binding

Type checking

Short term fix

sandersn commented Mar 27, 2018

ghost Mar 27, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

mhegazy Mar 28, 2018

Choose a reason for hiding this comment

sandersn Mar 28, 2018 • edited Loading

Choose a reason for hiding this comment

sandersn commented Mar 29, 2018 • edited Loading

sandersn commented Mar 30, 2018 • edited Loading

typescript-bot commented Jul 25, 2018

sandersn commented Mar 26, 2018 •

edited

Loading

Jessidhia Mar 27, 2018 •

edited

Loading

ghost Mar 27, 2018 •

edited by ghost

Loading

sandersn Mar 28, 2018 •

edited

Loading

sandersn commented Mar 29, 2018 •

edited

Loading

sandersn commented Mar 30, 2018 •

edited

Loading