Correctly merge JS decls

Turns out merging was incorrect even for non-nested declarations, but
tests didn't catch it before.

src/compiler/binder.ts

@@ -2403,12 +2403,7 @@ namespace ts {
                 return lookupSymbolForNameWorker(container, node.escapedText);
             }
             else {
-                let symbol = lookupSymbolForPropertyAccess(node.expression);
-                symbol = symbol && isDeclarationOfJavascriptContainerExpression(symbol.valueDeclaration) ? (symbol.valueDeclaration as VariableDeclaration).initializer.symbol :
-                    symbol && isDeclarationOfDefaultedJavascriptContainerExpression(symbol.valueDeclaration) ? ((symbol.valueDeclaration as VariableDeclaration).initializer as BinaryExpression).right.symbol :
-                    symbol && isAssignmentOfDefaultedJavascriptContainerExpression(symbol.valueDeclaration.parent) ? (((symbol.valueDeclaration.parent as BinaryExpression).right as BinaryExpression).right as BinaryExpression).symbol :
-                    symbol && isAssignmentOfJavascriptContainerExpression(symbol.valueDeclaration) ? ((symbol.valueDeclaration.parent as BinaryExpression).right as BinaryExpression).symbol :
-                    symbol;
+                const symbol = follow(lookupSymbolForPropertyAccess(node.expression));
                 return symbol && symbol.exports && symbol.exports.get(node.name.escapedText);
             }
         }
@@ -2417,11 +2412,7 @@ namespace ts {
             // Look up the property in the local scope, since property assignments should follow the declaration
             const symbol = lookupSymbolForPropertyAccess(name);
             // TODO: Should be able to structure this with less duplication
-            let targetSymbol = symbol && isDeclarationOfJavascriptContainerExpression(symbol.valueDeclaration) ? (symbol.valueDeclaration as VariableDeclaration).initializer.symbol :
-                symbol && isDeclarationOfDefaultedJavascriptContainerExpression(symbol.valueDeclaration) ? ((symbol.valueDeclaration as VariableDeclaration).initializer as BinaryExpression).right.symbol :
-                symbol && isAssignmentOfDefaultedJavascriptContainerExpression(symbol.valueDeclaration.parent) ? (((symbol.valueDeclaration.parent as BinaryExpression).right as BinaryExpression).right as BinaryExpression).symbol :
-                symbol && isAssignmentOfJavascriptContainerExpression(symbol.valueDeclaration) ? ((symbol.valueDeclaration.parent as BinaryExpression).right as BinaryExpression).symbol :
-                symbol;
+            let targetSymbol = follow(symbol);
             Debug.assert(propertyAccess.parent.kind === SyntaxKind.BinaryExpression ||
                          propertyAccess.parent.kind === SyntaxKind.ExpressionStatement ||
                          propertyAccess.parent.kind === SyntaxKind.PropertyAccessExpression);
@@ -2430,24 +2421,40 @@ namespace ts {
                 const initializerKind = (propertyAccess.parent as BinaryExpression).right.kind;
                 isLegalPosition = (initializerKind === SyntaxKind.ClassExpression || initializerKind === SyntaxKind.FunctionExpression) &&
                     propertyAccess.parent.parent.parent.kind === SyntaxKind.SourceFile;
+                if (propertyAccess.parent.parent.parent.kind === SyntaxKind.SourceFile && initializerKind === SyntaxKind.BinaryExpression && (((propertyAccess.parent as BinaryExpression).right as BinaryExpression).right.kind === SyntaxKind.ClassExpression || ((propertyAccess.parent as BinaryExpression).right as BinaryExpression).right.kind === SyntaxKind.FunctionExpression)) {
+                    isLegalPosition = true;
+                }
             }
             else {
                 isLegalPosition = propertyAccess.parent.parent.kind === SyntaxKind.SourceFile;
             }
             if (!isPrototypeProperty && (!targetSymbol || !(targetSymbol.flags & SymbolFlags.Namespace)) && isLegalPosition) {
-                const identifier = isIdentifier(propertyAccess.expression) ? propertyAccess.expression : isIdentifier(propertyAccess.expression.expression) && propertyAccess.expression.expression;
-                Debug.assert(identifier !== undefined);
                 const flags = SymbolFlags.Module | SymbolFlags.JSContainer;
                 const excludeFlags = SymbolFlags.ValueModuleExcludes & ~SymbolFlags.JSContainer;
-                if (targetSymbol) {
-                    // TODO: Not sure this is correct for nested declarations -- maybe this should be added to targetSymbol
-                    // Note: add declaration to original symbol, not the special-syntax's symbol, so that namespaces work for type lookup
-                    addDeclarationToSymbol(symbol, identifier, flags);
-                }
-                else {
-                    // TODO: Not sure this branch is correct for nested declarations
-                    targetSymbol = declareSymbol(container.locals, /*parent*/ undefined, identifier, flags, excludeFlags);
-                }
+                // const startTargetSymbol = !!targetSymbol;
+                // hm. This is only needed to make namespaced access of types workable. Namespaced access of *values* doesn't work now either, so something is wrong.
+                // (Note: for the non-nested case, at least, addDeclarationToSymbol is only needed for things that could be further namespaces, because it
+                // makes the intermediate namespace. However, I think something like it is needed for *all* nested assignments, in case their intermediate namespaces don't exist)
+                iterateEntityNameExpression(propertyAccess.expression, (id, originalSymbol, available) => {
+                    if (targetSymbol) {
+                        if (available) {
+                            // Note: add declaration to original symbol, not the special-syntax's symbol, so that namespaces work for type lookup
+                            addDeclarationToSymbol(originalSymbol, id, flags);
+                            // TODO: Why can't I overwrite targetSymbol here? I'm having trouble tracking targetSymbol's state.
+                            return originalSymbol;
+                        }
+                        else {
+                            originalSymbol.exports = originalSymbol.exports || createSymbolTable();
+                            targetSymbol = declareSymbol(originalSymbol.exports, originalSymbol, id, flags, excludeFlags);
+                            return targetSymbol;
+                        }
+                    }
+                    else {
+                        Debug.assert(!available);
+                        targetSymbol = declareSymbol(container.locals, /*parent*/ undefined, id, flags, excludeFlags);
+                        return targetSymbol;
+                    }
+                });
             }
             if (!targetSymbol || !(targetSymbol.flags & (SymbolFlags.Function | SymbolFlags.Class | SymbolFlags.NamespaceModule | SymbolFlags.ObjectLiteral))) {
                 return;
@@ -2462,6 +2469,20 @@ namespace ts {
             declareSymbol(symbolTable, targetSymbol, propertyAccess, SymbolFlags.Property, SymbolFlags.PropertyExcludes);
         }
 
+        function iterateEntityNameExpression(e: EntityNameExpression, action: (e: Identifier, originalSymbol: Symbol, available: boolean) => Symbol): Symbol {
+            if (isIdentifier(e)) {
+                const s = lookupSymbolForPropertyAccess(e);
+                return action(e, s, !!s);
+            }
+            else {
+                const s = follow(iterateEntityNameExpression(e.expression, action));
+                Debug.assert(!!s, "lost the chant");
+                Debug.assert(!!s.exports, `${s.escapedName} has no exports???`);
+                const t = s.exports.get(e.name.escapedText);
+                return action(e.name, t || s, s.exports.has(e.name.escapedText));
+            }
+        }
+
         function bindCallExpression(node: CallExpression) {
             // We're only inspecting call expressions to detect CommonJS modules, so we can skip
             // this check if we've already seen the module indicator

src/compiler/checker.ts

@@ -835,7 +835,7 @@ namespace ts {
 
         function mergeSymbol(target: Symbol, source: Symbol) {
             if (!(target.flags & getExcludedSymbolFlags(source.flags)) ||
-                source.flags & SymbolFlags.JSContainer || target.flags & SymbolFlags.JSContainer) {
+                (source.flags | target.flags) & SymbolFlags.JSContainer) {
                 // Javascript static-property-assignment declarations always merge, even though they are also values
                 if (source.flags & SymbolFlags.ValueModule && target.flags & SymbolFlags.ValueModule && target.constEnumOnlyModule && !source.constEnumOnlyModule) {
                     // reset flag when merging instantiated module into value module that has only const enums
@@ -857,6 +857,14 @@ namespace ts {
                     if (!target.exports) target.exports = createSymbolTable();
                     mergeSymbolTable(target.exports, source.exports);
                 }
+                if ((source.flags | target.flags) & SymbolFlags.JSContainer) {
+                    const fs = follow(source);
+                    const ft = follow(target);
+                    if (fs !== source || ft !== target) {
+                    // also follow the source's valueDeclaration and merge its symbol
+                        mergeSymbol(follow(target), follow(source));
+                    }
+                }
                 recordMergedSymbol(target, source);
             }
             else if (target.flags & SymbolFlags.NamespaceModule) {
@@ -14752,14 +14760,12 @@ namespace ts {
             // Grammar checking
             checkGrammarObjectLiteralExpression(node, inDestructuringPattern);
 
-            let propertiesTable = createSymbolTable();
+            let propertiesTable: SymbolTable;
             let propertiesArray: Symbol[] = [];
             let spread: Type = emptyObjectType;
             let propagatedFlags: TypeFlags = TypeFlags.FreshLiteral;
 
             const isInJSFile = isInJavaScriptFile(node);
-            // TODO: Might need to skip if isDeclarationOfDefaultedJavascriptContainerExpression(node.parent.parent) instead of just checking node.properties.length > 0
-            // (actually, it would be better not to skip contextual typing at all, but to do that I need to avoid the checking loop another way)
             const contextualType = getApparentTypeOfContextualType(node);
             const contextualTypeHasPattern = contextualType && contextualType.pattern &&
                 (contextualType.pattern.kind === SyntaxKind.ObjectBindingPattern || contextualType.pattern.kind === SyntaxKind.ObjectLiteralExpression);
@@ -14768,11 +14774,15 @@ namespace ts {
             let patternWithComputedProperties = false;
             let hasComputedStringProperty = false;
             let hasComputedNumberProperty = false;
-            // TODO: This seems like the wrong way to put the assignment-properties into the type (especially the manual call to mergeSymbolTable)
-            if (isInJSFile && node.symbol && node.symbol.exports) {
-                mergeSymbolTable(propertiesTable, node.symbol.exports);
-                node.symbol.exports.forEach(symbol => propertiesArray.push(symbol));
+            // TODO: This seems like it might be wrong, or at least should come earlier
+            // (maybe check SymbolFlags.JSContainer? This currently misses normal declarations like `var my = {}`, but shouldn't)
+            if (isInJSFile && node.symbol && node.symbol.exports && node.properties.length === 0) {
+                let symbol = node.symbol;
+                propertiesTable = symbol.exports;
+                symbol.exports.forEach(symbol => propertiesArray.push(getMergedSymbol(symbol)));
+                return createObjectLiteralType();
             }
+            propertiesTable = createSymbolTable();
 
             let offset = 0;
             for (let i = 0; i < node.properties.length; i++) {

src/compiler/utilities.ts

@@ -1486,6 +1486,14 @@ namespace ts {
         return false;
     }
 
+    export function follow(symbol: Symbol) {
+        return symbol && isDeclarationOfJavascriptContainerExpression(symbol.valueDeclaration) ? (symbol.valueDeclaration as VariableDeclaration).initializer.symbol :
+            symbol && isDeclarationOfDefaultedJavascriptContainerExpression(symbol.valueDeclaration) ? ((symbol.valueDeclaration as VariableDeclaration).initializer as BinaryExpression).right.symbol :
+            symbol && symbol.valueDeclaration && isAssignmentOfDefaultedJavascriptContainerExpression(symbol.valueDeclaration.parent) ? (((symbol.valueDeclaration.parent as BinaryExpression).right as BinaryExpression).right as BinaryExpression).symbol :
+            symbol && isAssignmentOfJavascriptContainerExpression(symbol.valueDeclaration) ? ((symbol.valueDeclaration.parent as BinaryExpression).right as BinaryExpression).symbol :
+            symbol;
+    }
+
     /**
      * Returns true if the node is a variable declaration whose initializer is a function or class expression, or an empty object literal.
      * This function does not test if the node is in a JavaScript file or not.