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.

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

Fix binder performance regression #31142

Merged
merged 1 commit into from Apr 29, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
18 changes: 15 additions & 3 deletions src/compiler/checker.ts
Expand Up @@ -94,8 +94,6 @@ namespace ts {

const globalThisSymbol = createSymbol(SymbolFlags.Module, "globalThis" as __String, CheckFlags.Readonly);
globalThisSymbol.exports = globals;
globalThisSymbol.valueDeclaration = createNode(SyntaxKind.Identifier) as Identifier;
(globalThisSymbol.valueDeclaration as Identifier).escapedText = "globalThis" as __String;
globals.set(globalThisSymbol.escapedName, globalThisSymbol);

const argumentsSymbol = createSymbol(SymbolFlags.Property, "arguments" as __String);
Expand Down Expand Up @@ -926,7 +924,12 @@ namespace ts {
recordMergedSymbol(target, source);
}
else if (target.flags & SymbolFlags.NamespaceModule) {
error(getNameOfDeclaration(source.declarations[0]), Diagnostics.Cannot_augment_module_0_with_value_exports_because_it_resolves_to_a_non_module_entity, symbolToString(target));
// Do not report an error when merging `var globalThis` with the built-in `globalThis`,
// as we will already report a "Declaration name conflicts..." error, and this error
// won't make much sense.
if (target !== globalThisSymbol) {
error(getNameOfDeclaration(source.declarations[0]), Diagnostics.Cannot_augment_module_0_with_value_exports_because_it_resolves_to_a_non_module_entity, symbolToString(target));
}
}
else { // error
const isEitherEnum = !!(target.flags & SymbolFlags.Enum || source.flags & SymbolFlags.Enum);
Expand Down Expand Up @@ -30456,6 +30459,14 @@ namespace ts {
continue;
}
if (!isExternalOrCommonJsModule(file)) {
// It is an error for a non-external-module (i.e. script) to declare its own `globalThis`.
// We can't use `builtinGlobals` for this due to synthetic expando-namespace generation in JS files.
const fileGlobalThisSymbol = file.locals!.get("globalThis" as __String);
if (fileGlobalThisSymbol) {
for (const declaration of fileGlobalThisSymbol.declarations) {
diagnostics.add(createDiagnosticForNode(declaration, Diagnostics.Declaration_name_conflicts_with_built_in_global_identifier_0, "globalThis"));
}
}
mergeSymbolTable(globals, file.locals!);
}
if (file.jsGlobalAugmentations) {
Expand Down Expand Up @@ -30501,6 +30512,7 @@ namespace ts {
getSymbolLinks(undefinedSymbol).type = undefinedWideningType;
getSymbolLinks(argumentsSymbol).type = getGlobalType("IArguments" as __String, /*arity*/ 0, /*reportErrors*/ true);
getSymbolLinks(unknownSymbol).type = errorType;
getSymbolLinks(globalThisSymbol).type = createObjectType(ObjectFlags.Anonymous, globalThisSymbol);

// Initialize special types
globalArrayType = getGlobalType("Array" as __String, /*arity*/ 1, /*reportErrors*/ true);
Expand Down
7 changes: 7 additions & 0 deletions tests/baselines/reference/globalThisCollision.errors.txt
@@ -0,0 +1,7 @@
tests/cases/conformance/es2019/globalThisCollision.js(1,5): error TS2397: Declaration name conflicts with built-in global identifier 'globalThis'.


==== tests/cases/conformance/es2019/globalThisCollision.js (1 errors) ====
var globalThis;
Copy link
Member

Choose a reason for hiding this comment

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

nitpit: an even better test would be var globalThis = window or var globalThis = this since that's the likely polyfill code we'll see in JS.

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 think its necessary since that doesn't effect the outcome of the test, this is purely testing that var globalThis in the global scope results in our collision error message.

~~~~~~~~~~
!!! error TS2397: Declaration name conflicts with built-in global identifier 'globalThis'.
4 changes: 4 additions & 0 deletions tests/baselines/reference/globalThisCollision.symbols
@@ -0,0 +1,4 @@
=== tests/cases/conformance/es2019/globalThisCollision.js ===
var globalThis;
>globalThis : Symbol(globalThis, Decl(globalThisCollision.js, 0, 3))

4 changes: 4 additions & 0 deletions tests/baselines/reference/globalThisCollision.types
@@ -0,0 +1,4 @@
=== tests/cases/conformance/es2019/globalThisCollision.js ===
var globalThis;
>globalThis : any

5 changes: 5 additions & 0 deletions tests/cases/conformance/es2019/globalThisCollision.ts
@@ -0,0 +1,5 @@
// @allowJs: true
// @checkJs: true
// @noEmit: true
// @filename: globalThisCollision.js
var globalThis;