Skip to content

Commit d28672d

Browse files
weswighamweswigham
authored
Fix alias naming and structure bugs in js declarations (microsoft#34786)
* Fix alias naming and structure bugs in js declarations * Add another test case and change condition for ns merge to require signature or export content * Fix typo in comment
1 parent 87cc8c4 commit d28672d

11 files changed

+857
-81
lines changed

‎src/compiler/checker.ts

Lines changed: 53 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -4968,7 +4968,7 @@ namespace ts {
49684968
const oldcontext = context;
49694969
context = {
49704970
...oldcontext,
4971-
usedSymbolNames: mapMap(symbolTable, (_symbol, name) => [unescapeLeadingUnderscores(name), true]),
4971+
usedSymbolNames: createMap(),
49724972
remappedSymbolNames: createMap(),
49734973
tracker: {
49744974
...oldcontext.tracker,
@@ -4992,6 +4992,10 @@ namespace ts {
49924992
context.usedSymbolNames!.set(name, true);
49934993
});
49944994
}
4995+
forEachEntry(symbolTable, (symbol, name) => {
4996+
const baseName = unescapeLeadingUnderscores(name);
4997+
void getInternalSymbolName(symbol, baseName); // Called to cache values into `usedSymbolNames` and `remappedSymbolNames`
4998+
});
49954999
let addingDeclare = !bundled;
49965000
const exportEquals = symbolTable.get(InternalSymbolName.ExportEquals);
49975001
if (exportEquals && symbolTable.size > 1 && exportEquals.flags & SymbolFlags.Alias) {
@@ -5204,7 +5208,11 @@ namespace ts {
52045208
isPrivate = true;
52055209
}
52065210
const modifierFlags = (!isPrivate ? ModifierFlags.Export : 0) | (isDefault && !needsPostExportDefault ? ModifierFlags.Default : 0);
5207-
if (symbol.flags & SymbolFlags.Function) {
5211+
const isConstMergedWithNS = symbol.flags & SymbolFlags.Module &&
5212+
symbol.flags & (SymbolFlags.BlockScopedVariable | SymbolFlags.FunctionScopedVariable | SymbolFlags.Property) &&
5213+
symbol.escapedName !== InternalSymbolName.ExportEquals;
5214+
const isConstMergedWithNSPrintableAsSignatureMerge = isConstMergedWithNS && isTypeRepresentableAsFunctionNamespaceMerge(getTypeOfSymbol(symbol), symbol);
5215+
if (symbol.flags & SymbolFlags.Function || isConstMergedWithNSPrintableAsSignatureMerge) {
52085216
serializeAsFunctionNamespaceMerge(getTypeOfSymbol(symbol), symbol, getInternalSymbolName(symbol, symbolName), modifierFlags);
52095217
}
52105218
if (symbol.flags & SymbolFlags.TypeAlias) {
@@ -5215,7 +5223,8 @@ namespace ts {
52155223
if (symbol.flags & (SymbolFlags.BlockScopedVariable | SymbolFlags.FunctionScopedVariable | SymbolFlags.Property)
52165224
&& symbol.escapedName !== InternalSymbolName.ExportEquals
52175225
&& !(symbol.flags & SymbolFlags.Prototype)
5218-
&& !(symbol.flags & SymbolFlags.Class)) {
5226+
&& !(symbol.flags & SymbolFlags.Class)
5227+
&& !isConstMergedWithNSPrintableAsSignatureMerge) {
52195228
serializeVariableOrProperty(symbol, symbolName, isPrivate, needsPostExportDefault, propertyAsAlias, modifierFlags);
52205229
}
52215230
if (symbol.flags & SymbolFlags.Enum) {
@@ -5232,7 +5241,7 @@ namespace ts {
52325241
serializeAsClass(symbol, getInternalSymbolName(symbol, symbolName), modifierFlags);
52335242
}
52345243
}
5235-
if (symbol.flags & (SymbolFlags.ValueModule | SymbolFlags.NamespaceModule)) {
5244+
if ((symbol.flags & (SymbolFlags.ValueModule | SymbolFlags.NamespaceModule) && (!isConstMergedWithNS || isTypeOnlyNamespace(symbol))) || isConstMergedWithNSPrintableAsSignatureMerge) {
52365245
serializeModule(symbol, symbolName, modifierFlags);
52375246
}
52385247
if (symbol.flags & SymbolFlags.Interface) {
@@ -5259,7 +5268,9 @@ namespace ts {
52595268
}
52605269

52615270
function includePrivateSymbol(symbol: Symbol) {
5271+
if (some(symbol.declarations, isParameterDeclaration)) return;
52625272
Debug.assertDefined(deferredPrivates);
5273+
getUnusedName(unescapeLeadingUnderscores(symbol.escapedName), symbol); // Call to cache unique name for symbol
52635274
deferredPrivates!.set("" + getSymbolId(symbol), symbol);
52645275
}
52655276

@@ -5333,8 +5344,16 @@ namespace ts {
53335344
), modifierFlags);
53345345
}
53355346

5347+
function getNamespaceMembersForSerialization(symbol: Symbol) {
5348+
return !symbol.exports ? [] : filter(arrayFrom((symbol.exports).values()), p => !((p.flags & SymbolFlags.Prototype) || (p.escapedName === "prototype")));
5349+
}
5350+
5351+
function isTypeOnlyNamespace(symbol: Symbol) {
5352+
return every(getNamespaceMembersForSerialization(symbol), m => !(resolveSymbol(m).flags & SymbolFlags.Value));
5353+
}
5354+
53365355
function serializeModule(symbol: Symbol, symbolName: string, modifierFlags: ModifierFlags) {
5337-
const members = !symbol.exports ? [] : filter(arrayFrom((symbol.exports).values()), p => !((p.flags & SymbolFlags.Prototype) || (p.escapedName === "prototype")));
5356+
const members = getNamespaceMembersForSerialization(symbol);
53385357
// Split NS members up by declaration - members whose parent symbol is the ns symbol vs those whose is not (but were added in later via merging)
53395358
const locationMap = arrayToMultiMap(members, m => m.parent && m.parent === symbol ? "real" : "merged");
53405359
const realMembers = locationMap.get("real") || emptyArray;
@@ -5344,18 +5363,21 @@ namespace ts {
53445363
// so we don't even have placeholders to fill in.
53455364
if (length(realMembers)) {
53465365
const localName = getInternalSymbolName(symbol, symbolName);
5347-
serializeAsNamespaceDeclaration(realMembers, localName, modifierFlags, !!(symbol.flags & SymbolFlags.Function));
5366+
serializeAsNamespaceDeclaration(realMembers, localName, modifierFlags, !!(symbol.flags & (SymbolFlags.Function | SymbolFlags.Assignment)));
53485367
}
53495368
if (length(mergedMembers)) {
53505369
const localName = getInternalSymbolName(symbol, symbolName);
5351-
forEach(mergedMembers, includePrivateSymbol);
53525370
const nsBody = createModuleBlock([createExportDeclaration(
53535371
/*decorators*/ undefined,
53545372
/*modifiers*/ undefined,
53555373
createNamedExports(map(filter(mergedMembers, n => n.escapedName !== InternalSymbolName.ExportEquals), s => {
53565374
const name = unescapeLeadingUnderscores(s.escapedName);
53575375
const localName = getInternalSymbolName(s, name);
5358-
return createExportSpecifier(name === localName ? undefined : localName, name);
5376+
const aliasDecl = s.declarations && getDeclarationOfAliasSymbol(s);
5377+
const target = aliasDecl && getTargetOfAliasDeclaration(aliasDecl, /*dontRecursivelyResolve*/ true);
5378+
includePrivateSymbol(target || s);
5379+
const targetName = target ? getInternalSymbolName(target, unescapeLeadingUnderscores(target.escapedName)) : localName;
5380+
return createExportSpecifier(name === targetName ? undefined : targetName, name);
53595381
}))
53605382
)]);
53615383
addResult(createModuleDeclaration(
@@ -5630,6 +5652,7 @@ namespace ts {
56305652
serializeMaybeAliasAssignment(symbol);
56315653
break;
56325654
case SyntaxKind.BinaryExpression:
5655+
case SyntaxKind.PropertyAccessExpression:
56335656
// Could be best encoded as though an export specifier or as though an export assignment
56345657
// If name is default or export=, do an export assignment
56355658
// Otherwise do an export specifier
@@ -5640,10 +5663,6 @@ namespace ts {
56405663
serializeExportSpecifier(localName, targetName);
56415664
}
56425665
break;
5643-
case SyntaxKind.PropertyAccessExpression:
5644-
// A PAE alias is _always_ going to exist as an append to a top-level export, where our top level
5645-
// handling should always be sufficient to encode the export action itself
5646-
break;
56475666
default:
56485667
return Debug.failBadSyntaxKind(node, "Unhandled alias declaration kind in symbol serializer!");
56495668
}
@@ -5672,7 +5691,8 @@ namespace ts {
56725691
const aliasDecl = symbol.declarations && getDeclarationOfAliasSymbol(symbol);
56735692
// serialize what the alias points to, preserve the declaration's initializer
56745693
const target = aliasDecl && getTargetOfAliasDeclaration(aliasDecl, /*dontRecursivelyResolve*/ true);
5675-
if (target) {
5694+
// If the target resolves and resolves to a thing defined in this file, emit as an alias, otherwise emit as a const
5695+
if (target && length(target.declarations) && some(target.declarations, d => getSourceFileOfNode(d) === getSourceFileOfNode(enclosingDeclaration))) {
56765696
// In case `target` refers to a namespace member, look at the declaration and serialize the leftmost symbol in it
56775697
// eg, `namespace A { export class B {} }; exports = A.B;`
56785698
// Technically, this is all that's required in the case where the assignment is an entity name expression
@@ -5758,6 +5778,7 @@ namespace ts {
57585778
return getObjectFlags(typeToSerialize) & (ObjectFlags.Anonymous | ObjectFlags.Mapped) &&
57595779
!getIndexInfoOfType(typeToSerialize, IndexKind.String) &&
57605780
!getIndexInfoOfType(typeToSerialize, IndexKind.Number) &&
5781+
!!(length(getPropertiesOfType(typeToSerialize)) || length(getSignaturesOfType(typeToSerialize, SignatureKind.Call))) &&
57615782
!length(getSignaturesOfType(typeToSerialize, SignatureKind.Construct)) && // TODO: could probably serialize as function + ns + class, now that that's OK
57625783
!getDeclarationWithTypeAnnotation(hostSymbol) &&
57635784
!(typeToSerialize.symbol && some(typeToSerialize.symbol.declarations, d => getSourceFileOfNode(d) !== ctxSrc)) &&
@@ -6112,11 +6133,8 @@ namespace ts {
61126133
return context.remappedSymbolNames!.get("" + getSymbolId(symbol))!;
61136134
}
61146135
}
6115-
if (input === InternalSymbolName.Default) {
6116-
input = "_default";
6117-
}
6118-
else if (input === InternalSymbolName.ExportEquals) {
6119-
input = "_exports";
6136+
if (symbol) {
6137+
input = getNameCandidateWorker(symbol, input);
61206138
}
61216139
let i = 0;
61226140
const original = input;
@@ -6131,17 +6149,29 @@ namespace ts {
61316149
return input;
61326150
}
61336151

6134-
function getInternalSymbolName(symbol: Symbol, localName: string) {
6135-
if (context.remappedSymbolNames!.has("" + getSymbolId(symbol))) {
6136-
return context.remappedSymbolNames!.get("" + getSymbolId(symbol))!;
6137-
}
6152+
function getNameCandidateWorker(symbol: Symbol, localName: string) {
61386153
if (localName === InternalSymbolName.Default || localName === InternalSymbolName.Class || localName === InternalSymbolName.Function) {
61396154
const flags = context.flags;
61406155
context.flags |= NodeBuilderFlags.InInitialEntityName;
61416156
const nameCandidate = getNameOfSymbolAsWritten(symbol, context);
61426157
context.flags = flags;
6143-
localName = isIdentifierText(nameCandidate, languageVersion) && !isStringANonContextualKeyword(nameCandidate) ? nameCandidate : getUnusedName(`_default`, symbol);
6158+
localName = nameCandidate.length > 0 && isSingleOrDoubleQuote(nameCandidate.charCodeAt(0)) ? stripQuotes(nameCandidate) : nameCandidate;
6159+
}
6160+
if (localName === InternalSymbolName.Default) {
6161+
localName = "_default";
6162+
}
6163+
else if (localName === InternalSymbolName.ExportEquals) {
6164+
localName = "_exports";
6165+
}
6166+
localName = isIdentifierText(localName, languageVersion) && !isStringANonContextualKeyword(localName) ? localName : "_" + localName.replace(/[^a-zA-Z0-9]/g, "_");
6167+
return localName;
6168+
}
6169+
6170+
function getInternalSymbolName(symbol: Symbol, localName: string) {
6171+
if (context.remappedSymbolNames!.has("" + getSymbolId(symbol))) {
6172+
return context.remappedSymbolNames!.get("" + getSymbolId(symbol))!;
61446173
}
6174+
localName = getNameCandidateWorker(symbol, localName);
61456175
// The result of this is going to be used as the symbol's name - lock it in, so `getUnusedName` will also pick it up
61466176
context.remappedSymbolNames!.set("" + getSymbolId(symbol), localName);
61476177
return localName;

‎tests/baselines/reference/jsDeclarationsExportAssignedClassExpressionShadowing.js

Lines changed: 2 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,4 @@
11
//// [index.js]
2-
// TODO: Fixup
32
class A {
43
member = new Q();
54
}
@@ -15,7 +14,6 @@ module.exports.Another = Q;
1514

1615

1716
//// [index.js]
18-
// TODO: Fixup
1917
var A = /** @class */ (function () {
2018
function A() {
2119
this.member = new Q();
@@ -43,44 +41,11 @@ declare class Q {
4341
x: A;
4442
}
4543
declare namespace Q {
46-
export { Another };
44+
export { Q_1 as Another };
4745
}
4846
declare class A {
4947
member: Q;
5048
}
51-
declare var Another: typeof Q;
52-
declare class Q {
49+
declare class Q_1 {
5350
x: number;
5451
}
55-
56-
57-
//// [DtsFileErrors]
58-
59-
60-
out/index.d.ts(2,15): error TS2300: Duplicate identifier 'Q'.
61-
out/index.d.ts(5,19): error TS2300: Duplicate identifier 'Q'.
62-
out/index.d.ts(12,15): error TS2300: Duplicate identifier 'Q'.
63-
64-
65-
==== ./out/index.d.ts (3 errors) ====
66-
export = Q;
67-
declare class Q {
68-
~
69-
!!! error TS2300: Duplicate identifier 'Q'.
70-
x: A;
71-
}
72-
declare namespace Q {
73-
~
74-
!!! error TS2300: Duplicate identifier 'Q'.
75-
export { Another };
76-
}
77-
declare class A {
78-
member: Q;
79-
}
80-
declare var Another: typeof Q;
81-
declare class Q {
82-
~
83-
!!! error TS2300: Duplicate identifier 'Q'.
84-
x: number;
85-
}
86-

‎tests/baselines/reference/jsDeclarationsExportAssignedClassExpressionShadowing.symbols

Lines changed: 14 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,37 +1,36 @@
11
=== tests/cases/conformance/jsdoc/declarations/index.js ===
2-
// TODO: Fixup
32
class A {
43
>A : Symbol(A, Decl(index.js, 0, 0))
54

65
member = new Q();
7-
>member : Symbol(A.member, Decl(index.js, 1, 9))
8-
>Q : Symbol(Q, Decl(index.js, 3, 1))
6+
>member : Symbol(A.member, Decl(index.js, 0, 9))
7+
>Q : Symbol(Q, Decl(index.js, 2, 1))
98
}
109
class Q {
11-
>Q : Symbol(Q, Decl(index.js, 3, 1))
10+
>Q : Symbol(Q, Decl(index.js, 2, 1))
1211

1312
x = 42;
14-
>x : Symbol(Q.x, Decl(index.js, 4, 9))
13+
>x : Symbol(Q.x, Decl(index.js, 3, 9))
1514
}
1615
module.exports = class Q {
1716
>module.exports : Symbol("tests/cases/conformance/jsdoc/declarations/index", Decl(index.js, 0, 0))
18-
>module : Symbol(export=, Decl(index.js, 6, 1))
19-
>exports : Symbol(export=, Decl(index.js, 6, 1))
20-
>Q : Symbol(Q, Decl(index.js, 7, 16))
17+
>module : Symbol(export=, Decl(index.js, 5, 1))
18+
>exports : Symbol(export=, Decl(index.js, 5, 1))
19+
>Q : Symbol(Q, Decl(index.js, 6, 16))
2120

2221
constructor() {
2322
this.x = new A();
24-
>this.x : Symbol(Q.x, Decl(index.js, 8, 19))
25-
>this : Symbol(Q, Decl(index.js, 7, 16))
26-
>x : Symbol(Q.x, Decl(index.js, 8, 19))
23+
>this.x : Symbol(Q.x, Decl(index.js, 7, 19))
24+
>this : Symbol(Q, Decl(index.js, 6, 16))
25+
>x : Symbol(Q.x, Decl(index.js, 7, 19))
2726
>A : Symbol(A, Decl(index.js, 0, 0))
2827
}
2928
}
3029
module.exports.Another = Q;
3130
>module.exports.Another : Symbol(Another)
32-
>module.exports : Symbol(Another, Decl(index.js, 11, 1))
33-
>module : Symbol(module, Decl(index.js, 6, 1))
31+
>module.exports : Symbol(Another, Decl(index.js, 10, 1))
32+
>module : Symbol(module, Decl(index.js, 5, 1))
3433
>exports : Symbol("tests/cases/conformance/jsdoc/declarations/index", Decl(index.js, 0, 0))
35-
>Another : Symbol(Another, Decl(index.js, 11, 1))
36-
>Q : Symbol(Q, Decl(index.js, 3, 1))
34+
>Another : Symbol(Another, Decl(index.js, 10, 1))
35+
>Q : Symbol(Q, Decl(index.js, 2, 1))
3736

‎tests/baselines/reference/jsDeclarationsExportAssignedClassExpressionShadowing.types

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,4 @@
11
=== tests/cases/conformance/jsdoc/declarations/index.js ===
2-
// TODO: Fixup
32
class A {
43
>A : A
54

‎tests/baselines/reference/jsDeclarationsFunctionsCjs.js

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -121,8 +121,7 @@ export namespace b {
121121
}
122122
export function c(): void;
123123
export namespace c {
124-
class Cls {
125-
}
124+
export { Cls };
126125
}
127126
export function d(a: number, b: number): string;
128127
export function e<T, U>(a: T, b: U): T & U;
@@ -159,3 +158,6 @@ export function i(): void;
159158
export function ii(): void;
160159
export function jj(): void;
161160
export function j(): void;
161+
declare class Cls {
162+
}
163+
export {};

0 commit comments

Comments
 (0)