Skip to content

Fix resolving entity name of namespace member after alias is merged with type #53387

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.

Sign up for GitHub

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 2 commits into from
Mar 20, 2023
Merged

Fix resolving entity name of namespace member after alias is merged with type #53387

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1bd8331
Fix resolving entity name of namespace member after alias is merged w…
andrewbranch Mar 20, 2023
6f62a98
Fix indentation
andrewbranch Mar 20, 2023
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
4 changes: 4 additions & 0 deletions src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4683,6 +4683,10 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
}
}
symbol = getMergedSymbol(getSymbol(getExportsOfSymbol(namespace), right.escapedText, meaning));
if (!symbol && (namespace.flags & SymbolFlags.Alias)) {
// `namespace` can be resolved further if there was a symbol merge with a re-export
symbol = getMergedSymbol(getSymbol(getExportsOfSymbol(resolveAlias(namespace)), right.escapedText, meaning));
Copy link
Member

@jakebailey jakebailey Mar 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Can this be simplified to just calling resolveSymbol in the original symbol = ... expression RHS above?

I feel like I fixed a bug like this recently that did something similar. That and there's getExportSymbolOfValueSymbolIfExported which I think is related and does some of this chain of calls. (I think we're somewhat inconsistent about it.)

Copy link
Member Author

Choose a reason for hiding this comment

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

No, because then we would fail to look up the property on the original value of namespace, which could very well be the one that has the export—it’s just not in this case. You could make a (somewhat more contrived) test where the alias points to a type and the local declaration is the namespace containing the export we’re looking for.

Copy link
Member

Choose a reason for hiding this comment

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

I think this also needs another condition, like

Suggested change
symbol = getMergedSymbol(getSymbol(getExportsOfSymbol(resolveAlias(namespace)), right.escapedText, meaning));
if (!getMergedSymbol(getSymbol(getExportsOfSymbol(namespace), right.escapedText, SymbolFlags.All))) {
symbol = getMergedSymbol(getSymbol(getExportsOfSymbol(resolveAlias(namespace)), right.escapedText, meaning));
}

since if that symbol exists in the original non-alias namespace (just with not the target meanings we're looking for), it should probably still block lookup in the alias target... right?

Copy link
Member Author

Choose a reason for hiding this comment

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

I’m not sure it should, but it’s worth noting that this scenario you describe would be a merge error. In the test given, the non-alias symbol meaning is the type alias Something<A>—not a namespace at all. If we change it to be a namespace and try to reason about which of the two namespaces we want to pull from:

image

Copy link
Member

Choose a reason for hiding this comment

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

Are you sure you can't manage a similar shape in JS that we don't technically error on though? Something like

// @Filename: usage.ts
import { Something } from "./prelude"
export const myValue: Something<string> = Something.of("abc")
export type MyType = Something.SubType<string>

// @Filename: Something.ts
export type Something<A> = { value: A }
export type SubType<A> = { value: A }
export declare function of<A>(value: A): Something<A>

// @Filename: prelude.js
import * as S from "./Something"
export * as Something from "./Something"
export type Something<A> = S.Something<A>;
module.exports.Something = 12;

Copy link
Member Author

Choose a reason for hiding this comment

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

In that example, we basically seem to ignore the module.exports.Something assignment, both before and after this change. But just imagine that the double-namespace example I gave was not an error, and our emit did something that would make the runtime merge work as expected. If that were possible, then the following would be true:

  1. Exports declared in multiple locations of that merged namespace would need to be checked for mergeability the same way local namespace merges are checked, so duplicate values/types couldn’t clobber each other.
  2. If two declarations of the same-named export are allowed to merge, since one is a type and one is a value, I don’t see why the local one should block the alias one.

Saying that the local should block the alias would be predicated on the runtime merge not working, but there’s already a top-level error for that.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I guess that's true; JS merges already let weird things with totally undefined behavior happen anyway, so it's not really important how they influence this.

}
if (!symbol) {
if (!ignoreErrors) {
const namespaceName = getFullyQualifiedName(namespace);
Expand Down
32 changes: 32 additions & 0 deletions tests/baselines/reference/exportTypeMergedWithExportStarAsNamespace.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
//// [tests/cases/conformance/externalModules/exportTypeMergedWithExportStarAsNamespace.ts] ////

//// [usage.ts]
import { Something } from "./prelude"
export const myValue: Something<string> = Something.of("abc")
export type MyType = Something.SubType<string>

//// [Something.ts]
export type Something<A> = { value: A }
export type SubType<A> = { value: A }
export declare function of<A>(value: A): Something<A>

//// [prelude.ts]
import * as S from "./Something"
export * as Something from "./Something"
export type Something<A> = S.Something<A>


//// [Something.js]
"use strict";
Object.defineProperty(exports, "__esModule", { value: true });
//// [prelude.js]
"use strict";
Object.defineProperty(exports, "__esModule", { value: true });
exports.Something = void 0;
exports.Something = require("./Something");
//// [usage.js]
"use strict";
Object.defineProperty(exports, "__esModule", { value: true });
exports.myValue = void 0;
var prelude_1 = require("./prelude");
exports.myValue = prelude_1.Something.of("abc");
51 changes: 51 additions & 0 deletions tests/baselines/reference/exportTypeMergedWithExportStarAsNamespace.symbols
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
=== tests/cases/conformance/externalModules/usage.ts ===
import { Something } from "./prelude"
>Something : Symbol(Something, Decl(usage.ts, 0, 8))

export const myValue: Something<string> = Something.of("abc")
>myValue : Symbol(myValue, Decl(usage.ts, 1, 12))
>Something : Symbol(Something, Decl(usage.ts, 0, 8))
>Something.of : Symbol(of, Decl(Something.ts, 1, 37))
>Something : Symbol(Something, Decl(usage.ts, 0, 8))
>of : Symbol(of, Decl(Something.ts, 1, 37))

export type MyType = Something.SubType<string>
>MyType : Symbol(MyType, Decl(usage.ts, 1, 61))
>Something : Symbol(Something, Decl(usage.ts, 0, 8))
>SubType : Symbol(SubType, Decl(Something.ts, 0, 39))

=== tests/cases/conformance/externalModules/Something.ts ===
export type Something<A> = { value: A }
>Something : Symbol(Something, Decl(Something.ts, 0, 0))
>A : Symbol(A, Decl(Something.ts, 0, 22))
>value : Symbol(value, Decl(Something.ts, 0, 28))
>A : Symbol(A, Decl(Something.ts, 0, 22))

export type SubType<A> = { value: A }
>SubType : Symbol(SubType, Decl(Something.ts, 0, 39))
>A : Symbol(A, Decl(Something.ts, 1, 20))
>value : Symbol(value, Decl(Something.ts, 1, 26))
>A : Symbol(A, Decl(Something.ts, 1, 20))

export declare function of<A>(value: A): Something<A>
>of : Symbol(of, Decl(Something.ts, 1, 37))
>A : Symbol(A, Decl(Something.ts, 2, 27))
>value : Symbol(value, Decl(Something.ts, 2, 30))
>A : Symbol(A, Decl(Something.ts, 2, 27))
>Something : Symbol(Something, Decl(Something.ts, 0, 0))
>A : Symbol(A, Decl(Something.ts, 2, 27))

=== tests/cases/conformance/externalModules/prelude.ts ===
import * as S from "./Something"
>S : Symbol(S, Decl(prelude.ts, 0, 6))

export * as Something from "./Something"
>Something : Symbol(Something, Decl(prelude.ts, 1, 6), Decl(prelude.ts, 1, 40))

export type Something<A> = S.Something<A>
>Something : Symbol(Something, Decl(prelude.ts, 1, 6), Decl(prelude.ts, 1, 40))
>A : Symbol(A, Decl(prelude.ts, 2, 22))
>S : Symbol(S, Decl(prelude.ts, 0, 6))
>Something : Symbol(S.Something, Decl(Something.ts, 0, 0))
>A : Symbol(A, Decl(prelude.ts, 2, 22))

40 changes: 40 additions & 0 deletions tests/baselines/reference/exportTypeMergedWithExportStarAsNamespace.types
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
=== tests/cases/conformance/externalModules/usage.ts ===
import { Something } from "./prelude"
>Something : typeof import("tests/cases/conformance/externalModules/Something")

export const myValue: Something<string> = Something.of("abc")
>myValue : Something<string>
>Something.of("abc") : import("tests/cases/conformance/externalModules/Something").Something<string>
>Something.of : <A>(value: A) => import("tests/cases/conformance/externalModules/Something").Something<A>
>Something : typeof import("tests/cases/conformance/externalModules/Something")
>of : <A>(value: A) => import("tests/cases/conformance/externalModules/Something").Something<A>
>"abc" : "abc"

export type MyType = Something.SubType<string>
>MyType : { value: string; }
>Something : any

=== tests/cases/conformance/externalModules/Something.ts ===
export type Something<A> = { value: A }
>Something : Something<A>
>value : A

export type SubType<A> = { value: A }
>SubType : SubType<A>
>value : A

export declare function of<A>(value: A): Something<A>
>of : <A>(value: A) => Something<A>
>value : A

=== tests/cases/conformance/externalModules/prelude.ts ===
import * as S from "./Something"
>S : typeof S

export * as Something from "./Something"
>Something : typeof S

export type Something<A> = S.Something<A>
>Something : Something<A>
>S : any

14 changes: 14 additions & 0 deletions tests/cases/conformance/externalModules/exportTypeMergedWithExportStarAsNamespace.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
// @Filename: usage.ts
import { Something } from "./prelude"
export const myValue: Something<string> = Something.of("abc")
export type MyType = Something.SubType<string>

// @Filename: Something.ts
export type Something<A> = { value: A }
export type SubType<A> = { value: A }
export declare function of<A>(value: A): Something<A>

// @Filename: prelude.ts
import * as S from "./Something"
export * as Something from "./Something"
export type Something<A> = S.Something<A>