Fix emit for leading 'var' declarations for synthesized namespaces #17631
Conversation
There was a problem hiding this comment.
After another review it seems this is not a complete solution as it could be possible to end up with an error at runtime if you merge a class and a namespace, where you synthesize an updated namespace during a transformation:
class C { }
namespace C { ... }If you replace namespace C with an updated node, then the ES2015 emit might end up as:
class C { }
var C; // error: cannot redeclare block-scoped binding.
(function (C) { ... })(C || (C = {}));
src/compiler/transformers/ts.ts
Outdated
| function isFirstEmittedDeclarationInScope(node: Node) { | ||
| function isPotentiallyFirstEmittedDeclarationInScope(node: Node) { | ||
| // If the node has a named symbol, then we have enough knowledge to determine | ||
| // whether a prior declaration has been emitted. |
There was a problem hiding this comment.
We should consider using getParseTreeNode here and get the symbol of the parse tree node. If there is no parse tree node, or the node does not have a symbol, we should fall back to the text of the declaration's name.
|
I've changed the implementation to just never use symbols; we have |
src/compiler/transformers/ts.ts
Outdated
| } | ||
| } | ||
|
|
||
| /** | ||
| * Determines whether a declaration is the first declaration with the same name emitted | ||
| * in the current scope. | ||
| * Determines whether a declaration is *could* be the first declaration with |
src/compiler/transformers/ts.ts
Outdated
| } | ||
|
|
||
| function declaredNameInScope(node: FunctionDeclaration | ClassDeclaration | ModuleDeclaration | EnumDeclaration): __String { | ||
| if (node.name.kind !== SyntaxKind.Identifier) { |
There was a problem hiding this comment.
Debug.assertNode(node.name, isIdentifier)
src/compiler/transformers/ts.ts
Outdated
| @@ -2746,7 +2760,7 @@ namespace ts { | |||
| return createNotEmittedStatement(node); | |||
| } | |||
|
|
|||
| Debug.assert(isIdentifier(node.name), "TypeScript module should have an Identifier name."); | |||
| Debug.assert(isIdentifier(node.name), "A TypeScript namespace should have an Identifier name."); | |||
There was a problem hiding this comment.
Debug.assertNode(node, isIdentifier)
DanielRosenwasser
changed the title
Down-leveling a namespace to ES involved an optimization within individual files to avoid emitting multiple
vardeclarations by grabbing the namespace's symbol's name and checking whether a prior symbol with a given name already had avardeclaration emitted. However, the check was not conservative enough to account for synthetic nodes that don't have symbols.With this PR, we unconditionally emit thevardeclaration for namespaces that have no symbols.With this PR, we just use local identifiers to keep track of declarations in the current scope.
Fixes #17596.