Skip to content
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.

Sign up for GitHub

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

Bind toplevel this assignments as global declarations #22891

Closed
wants to merge 5 commits into from
Closed

Bind toplevel this assignments as global declarations #22891

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a0dc554
Bind toplevel this assignments as global decls
sandersn Mar 26, 2018
e58da91
Improve toplevel this-assignment test
sandersn Mar 27, 2018
5831e80
Bind+check toplevel this in commonjs
sandersn Mar 27, 2018
59e2617
Merge branch 'master' into bind-toplevel-this
sandersn Mar 27, 2018
2050027
Don't create new types for toplevel every time
sandersn Mar 28, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/compiler/binder.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2362,6 +2362,11 @@ namespace ts {
const symbolTable = hasModifier(thisContainer, ModifierFlags.Static) ? containingClass.symbol.exports : containingClass.symbol.members;
declareSymbol(symbolTable, containingClass.symbol, node, SymbolFlags.Property, SymbolFlags.None, /*isReplaceableByMethod*/ true);
break;
case SyntaxKind.SourceFile:
// this.foo assignment in a source file
// Bind this property in the global namespace
declareSymbolAndAddToSymbolTable(node, SymbolFlags.FunctionScopedVariable, SymbolFlags.FunctionScopedVariableExcludes);
Copy link

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link

@Jessidhia Jessidhia Mar 27, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

break;

default:
Debug.fail(Debug.showSyntaxKind(thisContainer));
Expand Down
10 changes: 10 additions & 0 deletions tests/baselines/reference/topLevelThisAssignment.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
//// [a.js]
this.a = 10;
this.a;
a;


//// [output.js]
this.a = 10;
this.a;
a;
8 changes: 8 additions & 0 deletions tests/baselines/reference/topLevelThisAssignment.symbols
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
=== tests/cases/conformance/salsa/a.js ===
this.a = 10;
>a : Symbol(a, Decl(a.js, 0, 0))

this.a;
a;
>a : Symbol(a, Decl(a.js, 0, 0))

16 changes: 16 additions & 0 deletions tests/baselines/reference/topLevelThisAssignment.types
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
=== tests/cases/conformance/salsa/a.js ===
this.a = 10;
>this.a = 10 : 10
>this.a : any
>this : any
>a : any
>10 : 10

this.a;
>this.a : any
>this : any
>a : any

a;
>a : number

6 changes: 6 additions & 0 deletions tests/cases/conformance/salsa/topLevelThisAssignment.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
// @out: output.js
// @allowJs: true
// @Filename: a.js
this.a = 10;
this.a;
a;