Skip to content

Fix values and types merging in JS module exports #37896

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Apr 24, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 0 additions & 10 deletions src/compiler/binder.ts
Original file line number Diff line number Diff line change
Expand Up @@ -320,16 +320,6 @@ namespace ts {
}
}

function setValueDeclaration(symbol: Symbol, node: Declaration): void {
const { valueDeclaration } = symbol;
if (!valueDeclaration ||
(isAssignmentDeclaration(valueDeclaration) && !isAssignmentDeclaration(node)) ||
(valueDeclaration.kind !== node.kind && isEffectiveModuleDeclaration(valueDeclaration))) {
// other kinds of value declarations take precedence over modules and assignment declarations
symbol.valueDeclaration = node;
}
}

// Should not be called on a declaration with a computed property name,
// unless it is a well known Symbol.
function getDeclarationName(node: Declaration): __String | undefined {
Expand Down
45 changes: 34 additions & 11 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1115,12 +1115,8 @@ namespace ts {
target.constEnumOnlyModule = false;
}
target.flags |= source.flags;
if (source.valueDeclaration &&
(!target.valueDeclaration ||
isAssignmentDeclaration(target.valueDeclaration) && !isAssignmentDeclaration(source.valueDeclaration) ||
isEffectiveModuleDeclaration(target.valueDeclaration) && !isEffectiveModuleDeclaration(source.valueDeclaration))) {
// other kinds of value declarations take precedence over modules and assignment declarations
target.valueDeclaration = source.valueDeclaration;
if (source.valueDeclaration) {
setValueDeclaration(target, source.valueDeclaration);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this need a fix in the binder too? It probably only applies to code that is js-only (assignment declarations) and ts-only (ambient declarations) so 99.9% of the time it'll be in different files, but for the 0.01%, we do understand ambient declarations in .js as well -- we just put an error on them.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, how would you end up with an ambient declaration in a JavaScript file? Off the top of my head I didn’t think that was possible.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, we'll parse and understand this in JS:

declare function f(): void
f()

Just with lots of errors saying "please don't use this".

More practically, I think it would be better if this code and the check inside declareSymbol in the binder were the same -- if they haven't already diverged too far.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic in those two places was identical, so it was safe and easy to move to a utility. That said, I couldn’t actually trigger the same crash by writing ambient code in a JS file, I think because an ambient module declaration is only a module augmentation if the file is an ES module, and if it’s an ES module, module.exports is ignored.

addRange(target.declarations, source.declarations);
if (source.members) {
Expand Down Expand Up @@ -7724,11 +7720,38 @@ namespace ts {
resolvedSymbol.exports = createSymbolTable();
}
(resolvedSymbol || symbol).exports!.forEach((s, name) => {
if (members.has(name)) {
const exportedMember = exportedType.members.get(name)!;
const union = createSymbol(s.flags | exportedMember.flags, name);
union.type = getUnionType([getTypeOfSymbol(s), getTypeOfSymbol(exportedMember)]);
members.set(name, union);
const exportedMember = members.get(name)!;
if (exportedMember && exportedMember !== s) {
if (s.flags & SymbolFlags.Value) {
// If the member has an additional value-like declaration, union the types from the two declarations,
// but issue an error if they occurred in two different files. The purpose is to support a JS file with
// a pattern like:
//
// module.exports = { a: true };
// module.exports.a = 3;
//
// but we may have a JS file with `module.exports = { a: true }` along with a TypeScript module augmentation
// declaring an `export const a: number`. In that case, we issue a duplicate identifier error, because
// it's unclear what that's supposed to mean, so it's probably a mistake.
if (getSourceFileOfNode(s.valueDeclaration) !== getSourceFileOfNode(exportedMember.valueDeclaration)) {
const unescapedName = unescapeLeadingUnderscores(s.escapedName);
const exportedMemberName = tryCast(exportedMember.valueDeclaration, isNamedDeclaration)?.name || exportedMember.valueDeclaration;
addRelatedInfo(
error(s.valueDeclaration, Diagnostics.Duplicate_identifier_0, unescapedName),
createDiagnosticForNode(exportedMemberName, Diagnostics._0_was_also_declared_here, unescapedName));
addRelatedInfo(
error(exportedMemberName, Diagnostics.Duplicate_identifier_0, unescapedName),
createDiagnosticForNode(s.valueDeclaration, Diagnostics._0_was_also_declared_here, unescapedName));
}
const union = createSymbol(s.flags | exportedMember.flags, name);
union.type = getUnionType([getTypeOfSymbol(s), getTypeOfSymbol(exportedMember)]);
union.valueDeclaration = exportedMember.valueDeclaration;
union.declarations = concatenate(exportedMember.declarations, s.declarations);
members.set(name, union);
}
else {
members.set(name, mergeSymbol(s, exportedMember));
}
}
else {
members.set(name, s);
Expand Down
11 changes: 11 additions & 0 deletions src/compiler/utilities.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2314,6 +2314,17 @@ namespace ts {
!!getJSDocTypeTag(expr.parent);
}

export function setValueDeclaration(symbol: Symbol, node: Declaration): void {
const { valueDeclaration } = symbol;
if (!valueDeclaration ||
!(node.flags & NodeFlags.Ambient && !(valueDeclaration.flags & NodeFlags.Ambient)) &&
(isAssignmentDeclaration(valueDeclaration) && !isAssignmentDeclaration(node)) ||
(valueDeclaration.kind !== node.kind && isEffectiveModuleDeclaration(valueDeclaration))) {
// other kinds of value declarations take precedence over modules and assignment declarations
symbol.valueDeclaration = node;
}
}

export function isFunctionSymbol(symbol: Symbol | undefined) {
if (!symbol || !symbol.valueDeclaration) {
return false;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ module.exports = Foo;
>Foo : Symbol(Foo, Decl(cls.js, 4, 2))

module.exports.Strings = Strings;
>module.exports.Strings : Symbol(Strings)
>module.exports.Strings : Symbol(Strings, Decl(cls.js, 6, 21))
>module.exports : Symbol(Strings, Decl(cls.js, 6, 21))
>module : Symbol(module, Decl(cls.js, 5, 24))
>exports : Symbol("tests/cases/conformance/jsdoc/declarations/cls", Decl(cls.js, 0, 0))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ module.exports = Handler;
>Handler : Symbol(Handler, Decl(source.js, 0, 0), Decl(source.js, 7, 1))

module.exports.Strings = Strings
>module.exports.Strings : Symbol(Strings)
>module.exports.Strings : Symbol(Strings, Decl(source.js, 14, 25))
>module.exports : Symbol(Strings, Decl(source.js, 14, 25))
>module : Symbol(module, Decl(source.js, 12, 1))
>exports : Symbol("tests/cases/conformance/jsdoc/declarations/source", Decl(source.js, 0, 0))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ module.exports = m.default;
>default : Symbol(default, Decl(exporter.js, 0, 22))

module.exports.memberName = "thing";
>module.exports.memberName : Symbol(memberName)
>module.exports.memberName : Symbol(memberName, Decl(index.js, 2, 27))
>module.exports : Symbol(memberName, Decl(index.js, 2, 27))
>module : Symbol(module, Decl(index.js, 0, 32))
>exports : Symbol("tests/cases/conformance/jsdoc/declarations/index", Decl(index.js, 0, 0))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ module.exports = class {
}
}
module.exports.Sub = class {
>module.exports.Sub : Symbol(Sub)
>module.exports.Sub : Symbol(Sub, Decl(index.js, 7, 1))
>module.exports : Symbol(Sub, Decl(index.js, 7, 1))
>module : Symbol(module, Decl(index.js, 0, 0), Decl(index.js, 10, 27))
>exports : Symbol("tests/cases/conformance/jsdoc/declarations/index", Decl(index.js, 0, 0))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ module.exports = class Q {
}
}
module.exports.Another = Q;
>module.exports.Another : Symbol(Another)
>module.exports.Another : Symbol(Another, Decl(index.js, 10, 1))
>module.exports : Symbol(Another, Decl(index.js, 10, 1))
>module : Symbol(module, Decl(index.js, 5, 1))
>exports : Symbol("tests/cases/conformance/jsdoc/declarations/index", Decl(index.js, 0, 0))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ module.exports = Foo;
>Foo : Symbol(Foo, Decl(cls.js, 3, 2))

module.exports.Strings = Strings;
>module.exports.Strings : Symbol(Strings)
>module.exports.Strings : Symbol(Strings, Decl(cls.js, 5, 21))
>module.exports : Symbol(Strings, Decl(cls.js, 5, 21))
>module : Symbol(module, Decl(cls.js, 4, 12))
>exports : Symbol("tests/cases/conformance/jsdoc/declarations/cls", Decl(cls.js, 0, 0))
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
=== /test.js ===
class Abcde {
>Abcde : Symbol(Abcde, Decl(test.js, 0, 0))

/** @type {string} */
x;
>x : Symbol(Abcde.x, Decl(test.js, 0, 13))
}

module.exports = {
>module.exports : Symbol("/test", Decl(test.js, 0, 0))
>module : Symbol("/test.js", Decl(test.js, 3, 1), Decl(index.ts, 0, 31))
>exports : Symbol("/test.js", Decl(test.js, 3, 1), Decl(index.ts, 0, 31))

Abcde
>Abcde : Symbol(Abcde, Decl(test.js, 5, 18))

};

=== /index.ts ===
import { Abcde } from "./test";
>Abcde : Symbol(Abcde, Decl(index.ts, 0, 8))

declare module "./test" {
>"./test" : Symbol("/test.js", Decl(test.js, 3, 1), Decl(index.ts, 0, 31))

interface Abcde { b: string }
>Abcde : Symbol(Abcde, Decl(index.ts, 2, 25), Decl(test.js, 5, 18))
>b : Symbol(Abcde.b, Decl(index.ts, 3, 19))
}

new Abcde().x;
>new Abcde().x : Symbol(Abcde.x, Decl(test.js, 0, 13))
>Abcde : Symbol(Abcde, Decl(index.ts, 0, 8))
>x : Symbol(Abcde.x, Decl(test.js, 0, 13))

// Bug: the type meaning from /test.js does not
// propagate through the object literal export.
const x: Abcde = { b: "" };
>x : Symbol(x, Decl(index.ts, 10, 5))
>Abcde : Symbol(Abcde, Decl(index.ts, 0, 8))
>b : Symbol(b, Decl(index.ts, 10, 18))

Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
=== /test.js ===
class Abcde {
>Abcde : Abcde

/** @type {string} */
x;
>x : string
}

module.exports = {
>module.exports = { Abcde} : { Abcde: typeof Abcde; }
>module.exports : { Abcde: typeof Abcde; }
>module : { "\"/test\"": { Abcde: typeof Abcde; }; }
>exports : { Abcde: typeof Abcde; }
>{ Abcde} : { Abcde: typeof Abcde; }

Abcde
>Abcde : typeof Abcde

};

=== /index.ts ===
import { Abcde } from "./test";
>Abcde : typeof Abcde

declare module "./test" {
>"./test" : { Abcde: typeof Abcde; }

interface Abcde { b: string }
>b : string
}

new Abcde().x;
>new Abcde().x : string
>new Abcde() : Abcde
>Abcde : typeof Abcde
>x : string

// Bug: the type meaning from /test.js does not
// propagate through the object literal export.
const x: Abcde = { b: "" };
>x : Abcde
>{ b: "" } : { b: string; }
>b : string
>"" : ""

Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
/index.ts(4,16): error TS2300: Duplicate identifier 'a'.
/index.ts(7,3): error TS2339: Property 'toFixed' does not exist on type 'string'.
/test.js(2,3): error TS2300: Duplicate identifier 'a'.


==== /test.js (1 errors) ====
module.exports = {
a: "ok"
~
!!! error TS2300: Duplicate identifier 'a'.
!!! related TS6203 /index.ts:4:16: 'a' was also declared here.
};

==== /index.ts (2 errors) ====
import { a } from "./test";

declare module "./test" {
export const a: number;
~
!!! error TS2300: Duplicate identifier 'a'.
!!! related TS6203 /test.js:2:3: 'a' was also declared here.
}

a.toFixed();
~~~~~~~
!!! error TS2339: Property 'toFixed' does not exist on type 'string'.

Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
=== /test.js ===
module.exports = {
>module.exports : Symbol("/test", Decl(test.js, 0, 0))
>module : Symbol("/test.js", Decl(test.js, 0, 0), Decl(index.ts, 0, 27))
>exports : Symbol("/test.js", Decl(test.js, 0, 0), Decl(index.ts, 0, 27))

a: "ok"
>a : Symbol(a, Decl(test.js, 0, 18))

};

=== /index.ts ===
import { a } from "./test";
>a : Symbol(a, Decl(index.ts, 0, 8))

declare module "./test" {
>"./test" : Symbol("/test.js", Decl(test.js, 0, 0), Decl(index.ts, 0, 27))

export const a: number;
>a : Symbol(a, Decl(index.ts, 3, 14))
}

a.toFixed();
>a : Symbol(a, Decl(index.ts, 0, 8))

Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
=== /test.js ===
module.exports = {
>module.exports = { a: "ok"} : { a: string | number; }
>module.exports : { a: string | number; }
>module : { "\"/test\"": { a: string | number; }; }
>exports : { a: string | number; }
>{ a: "ok"} : { a: string; }

a: "ok"
>a : string
>"ok" : "ok"

};

=== /index.ts ===
import { a } from "./test";
>a : string

declare module "./test" {
>"./test" : { a: string | number; }

export const a: number;
>a : number
}

a.toFixed();
>a.toFixed() : any
>a.toFixed : any
>a : string
>toFixed : any

2 changes: 1 addition & 1 deletion tests/baselines/reference/moduleExportAlias2.symbols
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ exports = module.exports = C
>C : Symbol(C, Decl(semver.js, 2, 22))

exports.f = n => n + 1
>exports.f : Symbol(f)
>exports.f : Symbol(f, Decl(semver.js, 1, 28))
>exports : Symbol(f, Decl(semver.js, 1, 28))
>f : Symbol(f, Decl(semver.js, 1, 28))
>n : Symbol(n, Decl(semver.js, 2, 11))
Expand Down
2 changes: 1 addition & 1 deletion tests/baselines/reference/moduleExportAlias4.symbols
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ module.exports = class C {}
>C : Symbol(C, Decl(bug24024.js, 2, 16))

module.exports.D = class D { }
>module.exports.D : Symbol(D)
>module.exports.D : Symbol(D, Decl(bug24024.js, 2, 27))
>module.exports : Symbol(D, Decl(bug24024.js, 2, 27))
>module : Symbol(module, Decl(bug24024.js, 1, 31))
>exports : Symbol("tests/cases/conformance/salsa/bug24024", Decl(bug24024.js, 0, 0))
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
=== tests/cases/conformance/salsa/bug28014.js ===
exports.version = 1
>exports.version : Symbol(version)
>exports.version : Symbol(version, Decl(bug28014.js, 0, 0))
>exports : Symbol(version, Decl(bug28014.js, 0, 0))
>version : Symbol(version, Decl(bug28014.js, 0, 0))

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ module.exports = axios // both assignments should be ok
>axios : Symbol(axios, Decl(axios.js, 0, 3))

module.exports.default = axios
>module.exports.default : Symbol(default)
>module.exports.default : Symbol(default, Decl(axios.js, 1, 22))
>module.exports : Symbol(default, Decl(axios.js, 1, 22))
>module : Symbol(module, Decl(axios.js, 0, 14))
>exports : Symbol("tests/cases/conformance/salsa/axios", Decl(axios.js, 0, 0))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,14 @@ mod1.justExport.toFixed()
>toFixed : Symbol(Number.toFixed, Decl(lib.es5.d.ts, --, --))

mod1.bothBefore.toFixed() // error, 'toFixed' not on 'string | number'
>mod1.bothBefore : Symbol(bothBefore)
>mod1.bothBefore : Symbol(bothBefore, Decl(mod1.js, 3, 18), Decl(mod1.js, 0, 0))
>mod1 : Symbol(mod1, Decl(a.js, 1, 3))
>bothBefore : Symbol(bothBefore)
>bothBefore : Symbol(bothBefore, Decl(mod1.js, 3, 18), Decl(mod1.js, 0, 0))

mod1.bothAfter.toFixed() // error, 'toFixed' not on 'string | number'
>mod1.bothAfter : Symbol(bothAfter)
>mod1.bothAfter : Symbol(bothAfter, Decl(mod1.js, 4, 18), Decl(mod1.js, 6, 1))
>mod1 : Symbol(mod1, Decl(a.js, 1, 3))
>bothAfter : Symbol(bothAfter)
>bothAfter : Symbol(bothAfter, Decl(mod1.js, 4, 18), Decl(mod1.js, 6, 1))

mod1.justProperty.length
>mod1.justProperty.length : Symbol(String.length, Decl(lib.es5.d.ts, --, --))
Expand All @@ -41,7 +41,7 @@ declare function require(name: string): any;
=== tests/cases/conformance/salsa/mod1.js ===
/// <reference path='./requires.d.ts' />
module.exports.bothBefore = 'string'
>module.exports.bothBefore : Symbol(bothBefore)
>module.exports.bothBefore : Symbol(bothBefore, Decl(mod1.js, 3, 18), Decl(mod1.js, 0, 0))
>module.exports : Symbol(bothBefore, Decl(mod1.js, 0, 0))
>module : Symbol(module, Decl(mod1.js, 0, 0))
>exports : Symbol("tests/cases/conformance/salsa/mod1", Decl(mod1.js, 0, 0))
Expand All @@ -62,7 +62,7 @@ module.exports = {
>bothAfter : Symbol(bothAfter, Decl(mod1.js, 4, 18))
}
module.exports.bothAfter = 'string'
>module.exports.bothAfter : Symbol(bothAfter)
>module.exports.bothAfter : Symbol(bothAfter, Decl(mod1.js, 4, 18), Decl(mod1.js, 6, 1))
>module.exports : Symbol(bothAfter, Decl(mod1.js, 6, 1))
>module : Symbol(module, Decl(mod1.js, 0, 0))
>exports : Symbol("tests/cases/conformance/salsa/mod1", Decl(mod1.js, 0, 0))
Expand Down
Loading