Skip to content

Commit 30994c8

Browse files
sandersnsandersn
authored
Improve valueDeclaration for js module merges (microsoft#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, microsoft#24703, that required a change in typeFromPropertyAssignmentOutOfOrder. The test still fails for the original reason it was created, and the new bug microsoft#24703 contains a repro.
1 parent 942c42b commit 30994c8

9 files changed

Lines changed: 179 additions & 39 deletions

‎src/compiler/binder.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -236,7 +236,7 @@ namespace ts {
236236
if (symbolFlags & SymbolFlags.Value) {
237237
const { valueDeclaration } = symbol;
238238
if (!valueDeclaration ||
239-
(valueDeclaration.kind !== node.kind && valueDeclaration.kind === SyntaxKind.ModuleDeclaration)) {
239+
(valueDeclaration.kind !== node.kind && isEffectiveModuleDeclaration(valueDeclaration))) {
240240
// other kinds of value declarations take precedence over modules
241241
symbol.valueDeclaration = node;
242242
}

‎src/compiler/checker.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -921,7 +921,7 @@ namespace ts {
921921
target.flags |= source.flags;
922922
if (source.valueDeclaration &&
923923
(!target.valueDeclaration ||
924-
(target.valueDeclaration.kind === SyntaxKind.ModuleDeclaration && source.valueDeclaration.kind !== SyntaxKind.ModuleDeclaration))) {
924+
isEffectiveModuleDeclaration(target.valueDeclaration) && !isEffectiveModuleDeclaration(source.valueDeclaration))) {
925925
// other kinds of value declarations take precedence over modules
926926
target.valueDeclaration = source.valueDeclaration;
927927
}

‎src/compiler/utilities.ts‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -451,6 +451,16 @@ namespace ts {
451451
return isModuleDeclaration(node) && isStringLiteral(node.name);
452452
}
453453

454+
/**
455+
* An effective module (namespace) declaration is either
456+
* 1. An actual declaration: namespace X { ... }
457+
* 2. A Javascript declaration, which is:
458+
* An identifier in a nested property access expression: Y in `X.Y.Z = { ... }`
459+
*/
460+
export function isEffectiveModuleDeclaration(node: Node) {
461+
return isModuleDeclaration(node) || isIdentifier(node);
462+
}
463+
454464
/** Given a symbol for a module, checks that it is a shorthand ambient module. */
455465
export function isShorthandAmbientModuleSymbol(moduleSymbol: Symbol): boolean {
456466
return isShorthandAmbientModule(moduleSymbol.valueDeclaration);

‎tests/baselines/reference/typeFromPropertyAssignment24.symbols‎

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
1+
=== tests/cases/conformance/salsa/usage.js ===
2+
// note that usage is first in the compilation
3+
Outer.Inner.Message = function() {
4+
>Outer.Inner.Message : Symbol(Outer.Inner.Message, Decl(usage.js, 0, 0))
5+
>Outer.Inner : Symbol(Outer.Inner, Decl(usage.js, 1, 6), Decl(def.js, 0, 14))
6+
>Outer : Symbol(Outer, Decl(usage.js, 0, 0), Decl(def.js, 0, 3), Decl(def.js, 0, 14))
7+
>Inner : Symbol(Outer.Inner, Decl(usage.js, 1, 6), Decl(def.js, 0, 14))
8+
>Message : Symbol(Outer.Inner.Message, Decl(usage.js, 0, 0))
9+
10+
};
11+
12+
var y = new Outer.Inner()
13+
>y : Symbol(y, Decl(usage.js, 4, 3))
14+
>Outer.Inner : Symbol(Outer.Inner, Decl(usage.js, 1, 6), Decl(def.js, 0, 14))
15+
>Outer : Symbol(Outer, Decl(usage.js, 0, 0), Decl(def.js, 0, 3), Decl(def.js, 0, 14))
16+
>Inner : Symbol(Outer.Inner, Decl(usage.js, 1, 6), Decl(def.js, 0, 14))
17+
18+
y.name
19+
>y.name : Symbol(Inner.name, Decl(def.js, 1, 21))
20+
>y : Symbol(y, Decl(usage.js, 4, 3))
21+
>name : Symbol(Inner.name, Decl(def.js, 1, 21))
22+
23+
/** @type {Outer.Inner} should be instance type, not static type */
24+
var x;
25+
>x : Symbol(x, Decl(usage.js, 7, 3))
26+
27+
x.name
28+
>x.name : Symbol(Inner.name, Decl(def.js, 1, 21))
29+
>x : Symbol(x, Decl(usage.js, 7, 3))
30+
>name : Symbol(Inner.name, Decl(def.js, 1, 21))
31+
32+
=== tests/cases/conformance/salsa/def.js ===
33+
var Outer = {}
34+
>Outer : Symbol(Outer, Decl(usage.js, 0, 0), Decl(def.js, 0, 3), Decl(def.js, 0, 14))
35+
36+
Outer.Inner = class {
37+
>Outer.Inner : Symbol(Outer.Inner, Decl(usage.js, 1, 6), Decl(def.js, 0, 14))
38+
>Outer : Symbol(Outer, Decl(usage.js, 0, 0), Decl(def.js, 0, 3), Decl(def.js, 0, 14))
39+
>Inner : Symbol(Outer.Inner, Decl(usage.js, 1, 6), Decl(def.js, 0, 14))
40+
41+
name() {
42+
>name : Symbol(Inner.name, Decl(def.js, 1, 21))
43+
44+
return 'hi'
45+
}
46+
}
47+

‎tests/baselines/reference/typeFromPropertyAssignment24.types‎

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,54 @@
1+
=== tests/cases/conformance/salsa/usage.js ===
2+
// note that usage is first in the compilation
3+
Outer.Inner.Message = function() {
4+
>Outer.Inner.Message = function() {} : () => void
5+
>Outer.Inner.Message : () => void
6+
>Outer.Inner : typeof Inner
7+
>Outer : { [x: string]: any; Inner: typeof Inner; }
8+
>Inner : typeof Inner
9+
>Message : () => void
10+
>function() {} : () => void
11+
12+
};
13+
14+
var y = new Outer.Inner()
15+
>y : Inner
16+
>new Outer.Inner() : Inner
17+
>Outer.Inner : typeof Inner
18+
>Outer : { [x: string]: any; Inner: typeof Inner; }
19+
>Inner : typeof Inner
20+
21+
y.name
22+
>y.name : () => string
23+
>y : Inner
24+
>name : () => string
25+
26+
/** @type {Outer.Inner} should be instance type, not static type */
27+
var x;
28+
>x : Inner
29+
30+
x.name
31+
>x.name : () => string
32+
>x : Inner
33+
>name : () => string
34+
35+
=== tests/cases/conformance/salsa/def.js ===
36+
var Outer = {}
37+
>Outer : { [x: string]: any; Inner: typeof Inner; }
38+
>{} : { [x: string]: any; Inner: typeof Inner; }
39+
40+
Outer.Inner = class {
41+
>Outer.Inner = class { name() { return 'hi' }} : typeof Inner
42+
>Outer.Inner : typeof Inner
43+
>Outer : { [x: string]: any; Inner: typeof Inner; }
44+
>Inner : typeof Inner
45+
>class { name() { return 'hi' }} : typeof Inner
46+
47+
name() {
48+
>name : () => string
49+
50+
return 'hi'
51+
>'hi' : "hi"
52+
}
53+
}
54+

‎tests/baselines/reference/typeFromPropertyAssignmentOutOfOrder.symbols‎

Lines changed: 23 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -1,34 +1,37 @@
11
=== tests/cases/conformance/salsa/index.js ===
2-
Common.Item = class I {}
3-
>Common.Item : Symbol(Common.Item, Decl(index.js, 0, 0))
4-
>Common : Symbol(Common, Decl(index.js, 0, 0), Decl(roots.js, 0, 3))
5-
>Item : Symbol(Common.Item, Decl(index.js, 0, 0))
6-
>I : Symbol(I, Decl(index.js, 0, 13))
2+
First.Item = class I {}
3+
>First.Item : Symbol(First.Item, Decl(index.js, 0, 0))
4+
>First : Symbol(First, Decl(index.js, 0, 0), Decl(roots.js, 0, 3))
5+
>Item : Symbol(First.Item, Decl(index.js, 0, 0))
6+
>I : Symbol(I, Decl(index.js, 0, 12))
77

8-
Common.Object = class extends Common.Item {}
9-
>Common.Object : Symbol(Common.Object, Decl(index.js, 0, 24))
10-
>Common : Symbol(Common, Decl(index.js, 0, 0), Decl(roots.js, 0, 3))
11-
>Object : Symbol(Common.Object, Decl(index.js, 0, 24))
12-
>Common.Item : Symbol(Common.Item, Decl(index.js, 0, 0))
13-
>Common : Symbol(Common, Decl(index.js, 0, 0), Decl(roots.js, 0, 3))
14-
>Item : Symbol(Common.Item, Decl(index.js, 0, 0))
8+
Common.Object = class extends First.Item {}
9+
>Common.Object : Symbol(Common.Object, Decl(index.js, 0, 23))
10+
>Common : Symbol(Common, Decl(index.js, 0, 23), Decl(roots.js, 1, 3))
11+
>Object : Symbol(Common.Object, Decl(index.js, 0, 23))
12+
>First.Item : Symbol(First.Item, Decl(index.js, 0, 0))
13+
>First : Symbol(First, Decl(index.js, 0, 0), Decl(roots.js, 0, 3))
14+
>Item : Symbol(First.Item, Decl(index.js, 0, 0))
1515

1616
Workspace.Object = class extends Common.Object {}
17-
>Workspace.Object : Symbol(Workspace.Object, Decl(index.js, 1, 44))
18-
>Workspace : Symbol(Workspace, Decl(index.js, 1, 44), Decl(roots.js, 1, 3))
19-
>Object : Symbol(Workspace.Object, Decl(index.js, 1, 44))
20-
>Common.Object : Symbol(Common.Object, Decl(index.js, 0, 24))
21-
>Common : Symbol(Common, Decl(index.js, 0, 0), Decl(roots.js, 0, 3))
22-
>Object : Symbol(Common.Object, Decl(index.js, 0, 24))
17+
>Workspace.Object : Symbol(Workspace.Object, Decl(index.js, 1, 43))
18+
>Workspace : Symbol(Workspace, Decl(index.js, 1, 43), Decl(roots.js, 2, 3))
19+
>Object : Symbol(Workspace.Object, Decl(index.js, 1, 43))
20+
>Common.Object : Symbol(Common.Object, Decl(index.js, 0, 23))
21+
>Common : Symbol(Common, Decl(index.js, 0, 23), Decl(roots.js, 1, 3))
22+
>Object : Symbol(Common.Object, Decl(index.js, 0, 23))
2323

2424
/** @type {Workspace.Object} */
2525
var am;
2626
>am : Symbol(am, Decl(index.js, 6, 3))
2727

2828
=== tests/cases/conformance/salsa/roots.js ===
29+
var First = {};
30+
>First : Symbol(First, Decl(index.js, 0, 0), Decl(roots.js, 0, 3))
31+
2932
var Common = {};
30-
>Common : Symbol(Common, Decl(index.js, 0, 0), Decl(roots.js, 0, 3))
33+
>Common : Symbol(Common, Decl(index.js, 0, 23), Decl(roots.js, 1, 3))
3134

3235
var Workspace = {};
33-
>Workspace : Symbol(Workspace, Decl(index.js, 1, 44), Decl(roots.js, 1, 3))
36+
>Workspace : Symbol(Workspace, Decl(index.js, 1, 43), Decl(roots.js, 2, 3))
3437

‎tests/baselines/reference/typeFromPropertyAssignmentOutOfOrder.types‎

Lines changed: 19 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,42 +1,46 @@
11
=== tests/cases/conformance/salsa/index.js ===
2-
Common.Item = class I {}
3-
>Common.Item = class I {} : typeof I
4-
>Common.Item : typeof I
5-
>Common : typeof Common
2+
First.Item = class I {}
3+
>First.Item = class I {} : typeof I
4+
>First.Item : typeof I
5+
>First : { [x: string]: any; Item: typeof I; }
66
>Item : typeof I
77
>class I {} : typeof I
88
>I : typeof I
99

10-
Common.Object = class extends Common.Item {}
11-
>Common.Object = class extends Common.Item {} : typeof Object
10+
Common.Object = class extends First.Item {}
11+
>Common.Object = class extends First.Item {} : typeof Object
1212
>Common.Object : typeof Object
13-
>Common : typeof Common
13+
>Common : { [x: string]: any; Object: typeof Object; }
1414
>Object : typeof Object
15-
>class extends Common.Item {} : typeof Object
16-
>Common.Item : I
17-
>Common : typeof Common
15+
>class extends First.Item {} : typeof Object
16+
>First.Item : I
17+
>First : { [x: string]: any; Item: typeof I; }
1818
>Item : typeof I
1919

2020
Workspace.Object = class extends Common.Object {}
2121
>Workspace.Object = class extends Common.Object {} : typeof Object
2222
>Workspace.Object : typeof Object
23-
>Workspace : typeof Workspace
23+
>Workspace : { [x: string]: any; Object: typeof Object; }
2424
>Object : typeof Object
2525
>class extends Common.Object {} : typeof Object
2626
>Common.Object : Object
27-
>Common : typeof Common
27+
>Common : { [x: string]: any; Object: typeof Object; }
2828
>Object : typeof Object
2929

3030
/** @type {Workspace.Object} */
3131
var am;
3232
>am : Object
3333

3434
=== tests/cases/conformance/salsa/roots.js ===
35+
var First = {};
36+
>First : { [x: string]: any; Item: typeof I; }
37+
>{} : { [x: string]: any; Item: typeof I; }
38+
3539
var Common = {};
36-
>Common : typeof Common
37-
>{} : { [x: string]: any; Item: typeof I; Object: typeof Object; }
40+
>Common : { [x: string]: any; Object: typeof Object; }
41+
>{} : { [x: string]: any; Object: typeof Object; }
3842

3943
var Workspace = {};
40-
>Workspace : typeof Workspace
44+
>Workspace : { [x: string]: any; Object: typeof Object; }
4145
>{} : { [x: string]: any; Object: typeof Object; }
4246

‎tests/cases/conformance/salsa/typeFromPropertyAssignment24.ts‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
// @noEmit: true
2+
// @checkJs: true
3+
// @allowJs: true
4+
// @Filename: usage.js
5+
// note that usage is first in the compilation
6+
Outer.Inner.Message = function() {
7+
};
8+
9+
var y = new Outer.Inner()
10+
y.name
11+
/** @type {Outer.Inner} should be instance type, not static type */
12+
var x;
13+
x.name
14+
15+
// @Filename: def.js
16+
var Outer = {}
17+
Outer.Inner = class {
18+
name() {
19+
return 'hi'
20+
}
21+
}

‎tests/cases/conformance/salsa/typeFromPropertyAssignmentOutOfOrder.ts‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,14 +3,15 @@
33
// @checkJs: true
44
// @target: es3
55
// @filename: index.js
6-
Common.Item = class I {}
7-
Common.Object = class extends Common.Item {}
6+
First.Item = class I {}
7+
Common.Object = class extends First.Item {}
88

99
Workspace.Object = class extends Common.Object {}
1010

1111
/** @type {Workspace.Object} */
1212
var am;
1313

1414
// @filename: roots.js
15+
var First = {};
1516
var Common = {};
1617
var Workspace = {};

0 commit comments

Comments
 (0)