From 30994c86e457cabcddfad269c2a772fd5dc8433b Mon Sep 17 00:00:00 2001 From: Nathan Shively-Sanders <293473+sandersn@users.noreply.github.com> Date: Wed, 6 Jun 2018 11:11:15 -0700 Subject: [PATCH] Improve valueDeclaration for js module merges (#24707) Nearly everything in a merge of JS special assignments looks like a valueDeclaration. This commit ensures that intermediate "module declarations" are not used when a better valueDeclaration is available: ```js // File1.js var X = {} X.Y.Z = class { } // File2.js X.Y = {} ``` In the above example, the `Y` in `X.Y.Z = class { }` was used as the valueDeclaration for `Y` because it appeared before `X.Y = {}` in the compilation. This change exposed a bug in binding, #24703, that required a change in typeFromPropertyAssignmentOutOfOrder. The test still fails for the original reason it was created, and the new bug #24703 contains a repro. --- src/compiler/binder.ts | 2 +- src/compiler/checker.ts | 2 +- src/compiler/utilities.ts | 10 ++++ .../typeFromPropertyAssignment24.symbols | 47 ++++++++++++++++ .../typeFromPropertyAssignment24.types | 54 +++++++++++++++++++ ...peFromPropertyAssignmentOutOfOrder.symbols | 43 ++++++++------- ...typeFromPropertyAssignmentOutOfOrder.types | 38 +++++++------ .../salsa/typeFromPropertyAssignment24.ts | 21 ++++++++ .../typeFromPropertyAssignmentOutOfOrder.ts | 5 +- 9 files changed, 181 insertions(+), 41 deletions(-) create mode 100644 tests/baselines/reference/typeFromPropertyAssignment24.symbols create mode 100644 tests/baselines/reference/typeFromPropertyAssignment24.types create mode 100644 tests/cases/conformance/salsa/typeFromPropertyAssignment24.ts diff --git a/src/compiler/binder.ts b/src/compiler/binder.ts index 8b6ff01dd20..5ba78c1ad98 100644 --- a/src/compiler/binder.ts +++ b/src/compiler/binder.ts @@ -236,7 +236,7 @@ namespace ts { if (symbolFlags & SymbolFlags.Value) { const { valueDeclaration } = symbol; if (!valueDeclaration || - (valueDeclaration.kind !== node.kind && valueDeclaration.kind === SyntaxKind.ModuleDeclaration)) { + (valueDeclaration.kind !== node.kind && isEffectiveModuleDeclaration(valueDeclaration))) { // other kinds of value declarations take precedence over modules symbol.valueDeclaration = node; } diff --git a/src/compiler/checker.ts b/src/compiler/checker.ts index ca4407e9007..33d8457fe00 100644 --- a/src/compiler/checker.ts +++ b/src/compiler/checker.ts @@ -921,7 +921,7 @@ namespace ts { target.flags |= source.flags; if (source.valueDeclaration && (!target.valueDeclaration || - (target.valueDeclaration.kind === SyntaxKind.ModuleDeclaration && source.valueDeclaration.kind !== SyntaxKind.ModuleDeclaration))) { + isEffectiveModuleDeclaration(target.valueDeclaration) && !isEffectiveModuleDeclaration(source.valueDeclaration))) { // other kinds of value declarations take precedence over modules target.valueDeclaration = source.valueDeclaration; } diff --git a/src/compiler/utilities.ts b/src/compiler/utilities.ts index 0222686e8fd..8081f964ec4 100644 --- a/src/compiler/utilities.ts +++ b/src/compiler/utilities.ts @@ -451,6 +451,16 @@ namespace ts { return isModuleDeclaration(node) && isStringLiteral(node.name); } + /** + * An effective module (namespace) declaration is either + * 1. An actual declaration: namespace X { ... } + * 2. A Javascript declaration, which is: + * An identifier in a nested property access expression: Y in `X.Y.Z = { ... }` + */ + export function isEffectiveModuleDeclaration(node: Node) { + return isModuleDeclaration(node) || isIdentifier(node); + } + /** Given a symbol for a module, checks that it is a shorthand ambient module. */ export function isShorthandAmbientModuleSymbol(moduleSymbol: Symbol): boolean { return isShorthandAmbientModule(moduleSymbol.valueDeclaration); diff --git a/tests/baselines/reference/typeFromPropertyAssignment24.symbols b/tests/baselines/reference/typeFromPropertyAssignment24.symbols new file mode 100644 index 00000000000..9120b2fa171 --- /dev/null +++ b/tests/baselines/reference/typeFromPropertyAssignment24.symbols @@ -0,0 +1,47 @@ +=== tests/cases/conformance/salsa/usage.js === +// note that usage is first in the compilation +Outer.Inner.Message = function() { +>Outer.Inner.Message : Symbol(Outer.Inner.Message, Decl(usage.js, 0, 0)) +>Outer.Inner : Symbol(Outer.Inner, Decl(usage.js, 1, 6), Decl(def.js, 0, 14)) +>Outer : Symbol(Outer, Decl(usage.js, 0, 0), Decl(def.js, 0, 3), Decl(def.js, 0, 14)) +>Inner : Symbol(Outer.Inner, Decl(usage.js, 1, 6), Decl(def.js, 0, 14)) +>Message : Symbol(Outer.Inner.Message, Decl(usage.js, 0, 0)) + +}; + +var y = new Outer.Inner() +>y : Symbol(y, Decl(usage.js, 4, 3)) +>Outer.Inner : Symbol(Outer.Inner, Decl(usage.js, 1, 6), Decl(def.js, 0, 14)) +>Outer : Symbol(Outer, Decl(usage.js, 0, 0), Decl(def.js, 0, 3), Decl(def.js, 0, 14)) +>Inner : Symbol(Outer.Inner, Decl(usage.js, 1, 6), Decl(def.js, 0, 14)) + +y.name +>y.name : Symbol(Inner.name, Decl(def.js, 1, 21)) +>y : Symbol(y, Decl(usage.js, 4, 3)) +>name : Symbol(Inner.name, Decl(def.js, 1, 21)) + +/** @type {Outer.Inner} should be instance type, not static type */ +var x; +>x : Symbol(x, Decl(usage.js, 7, 3)) + +x.name +>x.name : Symbol(Inner.name, Decl(def.js, 1, 21)) +>x : Symbol(x, Decl(usage.js, 7, 3)) +>name : Symbol(Inner.name, Decl(def.js, 1, 21)) + +=== tests/cases/conformance/salsa/def.js === +var Outer = {} +>Outer : Symbol(Outer, Decl(usage.js, 0, 0), Decl(def.js, 0, 3), Decl(def.js, 0, 14)) + +Outer.Inner = class { +>Outer.Inner : Symbol(Outer.Inner, Decl(usage.js, 1, 6), Decl(def.js, 0, 14)) +>Outer : Symbol(Outer, Decl(usage.js, 0, 0), Decl(def.js, 0, 3), Decl(def.js, 0, 14)) +>Inner : Symbol(Outer.Inner, Decl(usage.js, 1, 6), Decl(def.js, 0, 14)) + + name() { +>name : Symbol(Inner.name, Decl(def.js, 1, 21)) + + return 'hi' + } +} + diff --git a/tests/baselines/reference/typeFromPropertyAssignment24.types b/tests/baselines/reference/typeFromPropertyAssignment24.types new file mode 100644 index 00000000000..5acd55568cb --- /dev/null +++ b/tests/baselines/reference/typeFromPropertyAssignment24.types @@ -0,0 +1,54 @@ +=== tests/cases/conformance/salsa/usage.js === +// note that usage is first in the compilation +Outer.Inner.Message = function() { +>Outer.Inner.Message = function() {} : () => void +>Outer.Inner.Message : () => void +>Outer.Inner : typeof Inner +>Outer : { [x: string]: any; Inner: typeof Inner; } +>Inner : typeof Inner +>Message : () => void +>function() {} : () => void + +}; + +var y = new Outer.Inner() +>y : Inner +>new Outer.Inner() : Inner +>Outer.Inner : typeof Inner +>Outer : { [x: string]: any; Inner: typeof Inner; } +>Inner : typeof Inner + +y.name +>y.name : () => string +>y : Inner +>name : () => string + +/** @type {Outer.Inner} should be instance type, not static type */ +var x; +>x : Inner + +x.name +>x.name : () => string +>x : Inner +>name : () => string + +=== tests/cases/conformance/salsa/def.js === +var Outer = {} +>Outer : { [x: string]: any; Inner: typeof Inner; } +>{} : { [x: string]: any; Inner: typeof Inner; } + +Outer.Inner = class { +>Outer.Inner = class { name() { return 'hi' }} : typeof Inner +>Outer.Inner : typeof Inner +>Outer : { [x: string]: any; Inner: typeof Inner; } +>Inner : typeof Inner +>class { name() { return 'hi' }} : typeof Inner + + name() { +>name : () => string + + return 'hi' +>'hi' : "hi" + } +} + diff --git a/tests/baselines/reference/typeFromPropertyAssignmentOutOfOrder.symbols b/tests/baselines/reference/typeFromPropertyAssignmentOutOfOrder.symbols index 59b42c24737..f31de75386a 100644 --- a/tests/baselines/reference/typeFromPropertyAssignmentOutOfOrder.symbols +++ b/tests/baselines/reference/typeFromPropertyAssignmentOutOfOrder.symbols @@ -1,34 +1,37 @@ === tests/cases/conformance/salsa/index.js === -Common.Item = class I {} ->Common.Item : Symbol(Common.Item, Decl(index.js, 0, 0)) ->Common : Symbol(Common, Decl(index.js, 0, 0), Decl(roots.js, 0, 3)) ->Item : Symbol(Common.Item, Decl(index.js, 0, 0)) ->I : Symbol(I, Decl(index.js, 0, 13)) +First.Item = class I {} +>First.Item : Symbol(First.Item, Decl(index.js, 0, 0)) +>First : Symbol(First, Decl(index.js, 0, 0), Decl(roots.js, 0, 3)) +>Item : Symbol(First.Item, Decl(index.js, 0, 0)) +>I : Symbol(I, Decl(index.js, 0, 12)) -Common.Object = class extends Common.Item {} ->Common.Object : Symbol(Common.Object, Decl(index.js, 0, 24)) ->Common : Symbol(Common, Decl(index.js, 0, 0), Decl(roots.js, 0, 3)) ->Object : Symbol(Common.Object, Decl(index.js, 0, 24)) ->Common.Item : Symbol(Common.Item, Decl(index.js, 0, 0)) ->Common : Symbol(Common, Decl(index.js, 0, 0), Decl(roots.js, 0, 3)) ->Item : Symbol(Common.Item, Decl(index.js, 0, 0)) +Common.Object = class extends First.Item {} +>Common.Object : Symbol(Common.Object, Decl(index.js, 0, 23)) +>Common : Symbol(Common, Decl(index.js, 0, 23), Decl(roots.js, 1, 3)) +>Object : Symbol(Common.Object, Decl(index.js, 0, 23)) +>First.Item : Symbol(First.Item, Decl(index.js, 0, 0)) +>First : Symbol(First, Decl(index.js, 0, 0), Decl(roots.js, 0, 3)) +>Item : Symbol(First.Item, Decl(index.js, 0, 0)) Workspace.Object = class extends Common.Object {} ->Workspace.Object : Symbol(Workspace.Object, Decl(index.js, 1, 44)) ->Workspace : Symbol(Workspace, Decl(index.js, 1, 44), Decl(roots.js, 1, 3)) ->Object : Symbol(Workspace.Object, Decl(index.js, 1, 44)) ->Common.Object : Symbol(Common.Object, Decl(index.js, 0, 24)) ->Common : Symbol(Common, Decl(index.js, 0, 0), Decl(roots.js, 0, 3)) ->Object : Symbol(Common.Object, Decl(index.js, 0, 24)) +>Workspace.Object : Symbol(Workspace.Object, Decl(index.js, 1, 43)) +>Workspace : Symbol(Workspace, Decl(index.js, 1, 43), Decl(roots.js, 2, 3)) +>Object : Symbol(Workspace.Object, Decl(index.js, 1, 43)) +>Common.Object : Symbol(Common.Object, Decl(index.js, 0, 23)) +>Common : Symbol(Common, Decl(index.js, 0, 23), Decl(roots.js, 1, 3)) +>Object : Symbol(Common.Object, Decl(index.js, 0, 23)) /** @type {Workspace.Object} */ var am; >am : Symbol(am, Decl(index.js, 6, 3)) === tests/cases/conformance/salsa/roots.js === +var First = {}; +>First : Symbol(First, Decl(index.js, 0, 0), Decl(roots.js, 0, 3)) + var Common = {}; ->Common : Symbol(Common, Decl(index.js, 0, 0), Decl(roots.js, 0, 3)) +>Common : Symbol(Common, Decl(index.js, 0, 23), Decl(roots.js, 1, 3)) var Workspace = {}; ->Workspace : Symbol(Workspace, Decl(index.js, 1, 44), Decl(roots.js, 1, 3)) +>Workspace : Symbol(Workspace, Decl(index.js, 1, 43), Decl(roots.js, 2, 3)) diff --git a/tests/baselines/reference/typeFromPropertyAssignmentOutOfOrder.types b/tests/baselines/reference/typeFromPropertyAssignmentOutOfOrder.types index c17e3034ad8..191f45e3cb1 100644 --- a/tests/baselines/reference/typeFromPropertyAssignmentOutOfOrder.types +++ b/tests/baselines/reference/typeFromPropertyAssignmentOutOfOrder.types @@ -1,30 +1,30 @@ === tests/cases/conformance/salsa/index.js === -Common.Item = class I {} ->Common.Item = class I {} : typeof I ->Common.Item : typeof I ->Common : typeof Common +First.Item = class I {} +>First.Item = class I {} : typeof I +>First.Item : typeof I +>First : { [x: string]: any; Item: typeof I; } >Item : typeof I >class I {} : typeof I >I : typeof I -Common.Object = class extends Common.Item {} ->Common.Object = class extends Common.Item {} : typeof Object +Common.Object = class extends First.Item {} +>Common.Object = class extends First.Item {} : typeof Object >Common.Object : typeof Object ->Common : typeof Common +>Common : { [x: string]: any; Object: typeof Object; } >Object : typeof Object ->class extends Common.Item {} : typeof Object ->Common.Item : I ->Common : typeof Common +>class extends First.Item {} : typeof Object +>First.Item : I +>First : { [x: string]: any; Item: typeof I; } >Item : typeof I Workspace.Object = class extends Common.Object {} >Workspace.Object = class extends Common.Object {} : typeof Object >Workspace.Object : typeof Object ->Workspace : typeof Workspace +>Workspace : { [x: string]: any; Object: typeof Object; } >Object : typeof Object >class extends Common.Object {} : typeof Object >Common.Object : Object ->Common : typeof Common +>Common : { [x: string]: any; Object: typeof Object; } >Object : typeof Object /** @type {Workspace.Object} */ @@ -32,11 +32,15 @@ var am; >am : Object === tests/cases/conformance/salsa/roots.js === -var Common = {}; ->Common : typeof Common ->{} : { [x: string]: any; Item: typeof I; Object: typeof Object; } +var First = {}; +>First : { [x: string]: any; Item: typeof I; } +>{} : { [x: string]: any; Item: typeof I; } -var Workspace = {}; ->Workspace : typeof Workspace +var Common = {}; +>Common : { [x: string]: any; Object: typeof Object; } +>{} : { [x: string]: any; Object: typeof Object; } + +var Workspace = {}; +>Workspace : { [x: string]: any; Object: typeof Object; } >{} : { [x: string]: any; Object: typeof Object; } diff --git a/tests/cases/conformance/salsa/typeFromPropertyAssignment24.ts b/tests/cases/conformance/salsa/typeFromPropertyAssignment24.ts new file mode 100644 index 00000000000..e11d9c7ac55 --- /dev/null +++ b/tests/cases/conformance/salsa/typeFromPropertyAssignment24.ts @@ -0,0 +1,21 @@ +// @noEmit: true +// @checkJs: true +// @allowJs: true +// @Filename: usage.js +// note that usage is first in the compilation +Outer.Inner.Message = function() { +}; + +var y = new Outer.Inner() +y.name +/** @type {Outer.Inner} should be instance type, not static type */ +var x; +x.name + +// @Filename: def.js +var Outer = {} +Outer.Inner = class { + name() { + return 'hi' + } +} diff --git a/tests/cases/conformance/salsa/typeFromPropertyAssignmentOutOfOrder.ts b/tests/cases/conformance/salsa/typeFromPropertyAssignmentOutOfOrder.ts index d6f97375734..061044a3476 100644 --- a/tests/cases/conformance/salsa/typeFromPropertyAssignmentOutOfOrder.ts +++ b/tests/cases/conformance/salsa/typeFromPropertyAssignmentOutOfOrder.ts @@ -3,8 +3,8 @@ // @checkJs: true // @target: es3 // @filename: index.js -Common.Item = class I {} -Common.Object = class extends Common.Item {} +First.Item = class I {} +Common.Object = class extends First.Item {} Workspace.Object = class extends Common.Object {} @@ -12,5 +12,6 @@ Workspace.Object = class extends Common.Object {} var am; // @filename: roots.js +var First = {}; var Common = {}; var Workspace = {};