Skip to content

Set parents of augmented module exports #59609

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 4 commits into from
Aug 15, 2024
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
25 changes: 22 additions & 3 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2683,7 +2683,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
}
if (source.exports) {
if (!target.exports) target.exports = createSymbolTable();
mergeSymbolTable(target.exports, source.exports, unidirectional);
mergeSymbolTable(target.exports, source.exports, unidirectional, target);
Copy link
Member

Choose a reason for hiding this comment

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

do the symbols in target.exports already have parent set to target?

}
if (!unidirectional) {
recordMergedSymbol(target, source);
Expand Down Expand Up @@ -2772,10 +2772,29 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
return combined;
}

function mergeSymbolTable(target: SymbolTable, source: SymbolTable, unidirectional = false) {
function mergeSymbolTable(target: SymbolTable, source: SymbolTable, unidirectional = false, mergedParent?: Symbol) {
source.forEach((sourceSymbol, id) => {
const targetSymbol = target.get(id);
target.set(id, targetSymbol ? mergeSymbol(targetSymbol, sourceSymbol, unidirectional) : getMergedSymbol(sourceSymbol));
const merged = targetSymbol ? mergeSymbol(targetSymbol, sourceSymbol, unidirectional) : getMergedSymbol(sourceSymbol);
Copy link
Member

Choose a reason for hiding this comment

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

I can't remember: can targetSymbol or sourceSymbol be returned from mergeSymbol or getMergedSymbol? Or is the result always a fresh transient symbol?

The reason I'm asking is that I want to know what happens if we already have an existing parent. Is it guaranteed to be the same as parent? Related to it in some way?

if (mergedParent && targetSymbol) {
Copy link
Member

Choose a reason for hiding this comment

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

from discussion with @iisaduan -- do we know which merged symbols don't have parents? Is it related to whether sourceSymbol (or target) has a parent? Isabel tracked it down to exports lists=no parent; exported interface=parent. Is there something missing in the binder when parents are set?

// If a merge was performed on the target symbol, set its parent to the merged parent that initiated the merge
// of its exports. Otherwise, `merged` came only from `sourceSymbol` and can keep its parent:
//
// // a.ts
// export interface A { x: number; }
//
// // b.ts
// declare module "./a" {
// interface A { y: number; }
// interface B {}
// }
//
// When merging the module augmentation into a.ts, the symbol for `A` will itself be merged, so its parent
// should be the merged module symbol. But the symbol for `B` has only one declaration, so its parent should
// be the module augmentation symbol, which contains its only declaration.
merged.parent = mergedParent;
}
target.set(id, merged);
});
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
/// <reference path="fourslash.ts" />
// @module: nodenext

// @Filename: /node_modules/@sapphire/pieces/index.d.ts
//// interface Container {
//// stores: unknown;
//// }
////
//// declare class Piece {
//// container: Container;
//// }
////
//// export { Piece, type Container };

// @FileName: /augmentation.ts
//// declare module "@sapphire/pieces" {
//// interface Container {
//// client: unknown;
//// }
//// export { Container };
//// }

// @Filename: /index.ts
//// import { Piece } from "@sapphire/pieces";
//// class FullPiece extends Piece {
//// /*1*/
//// }

const preferences = {
includeCompletionsWithClassMemberSnippets: true,
includeCompletionsWithInsertText: true,
};

verify.completions({
marker: "1",
includes: [
{
name: "container",
insertText: "container: Container;",
filterText: "container",
hasAction: true,
source: "ClassMemberSnippet/",
},
],
preferences,
isNewIdentifierLocation: true,
});

verify.applyCodeActionFromCompletion("1", {
name: "container",
source: "ClassMemberSnippet/",
description: `Includes imports of types referenced by 'container'`,
newFileContent: `import { Container, Piece } from "@sapphire/pieces";
class FullPiece extends Piece {

}`,
preferences,
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
/// <reference path="fourslash.ts" />
// @module: nodenext

// @Filename: /node_modules/@sapphire/pieces/index.d.ts
//// interface Container {
//// stores: unknown;
//// }
////
//// declare class Piece {
//// get container(): Container;
//// }
////
//// declare class AliasPiece extends Piece {}
////
//// export { AliasPiece, type Container };

// @Filename: /node_modules/@sapphire/framework/index.d.ts
//// import { AliasPiece } from "@sapphire/pieces";
////
//// declare class Command extends AliasPiece {}
////
//// declare module "@sapphire/pieces" {
//// interface Container {
//// client: unknown;
//// }
//// }
////
//// export { Command };

// @Filename: /index.ts
//// import "@sapphire/pieces";
//// import { Command } from "@sapphire/framework";
//// class PingCommand extends Command {
//// /*1*/
//// }

const preferences = {
includeCompletionsWithClassMemberSnippets: true,
includeCompletionsWithInsertText: true,
};

verify.completions({
marker: "1",
includes: [
{
name: "container",
insertText: "get container(): Container {\n}",
filterText: "container",
hasAction: true,
source: "ClassMemberSnippet/",
},
],
preferences,
isNewIdentifierLocation: true,
});

verify.applyCodeActionFromCompletion("1", {
name: "container",
source: "ClassMemberSnippet/",
description: `Includes imports of types referenced by 'container'`,
newFileContent: `import "@sapphire/pieces";
import { Command } from "@sapphire/framework";
import { Container } from "@sapphire/pieces";
class PingCommand extends Command {

}`,
preferences,
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
/// <reference path="fourslash.ts" />
// @module: nodenext

// @Filename: /node_modules/@sapphire/pieces/index.d.ts
//// export interface Container {
//// stores: unknown;
//// }
////
//// declare class Piece {
//// container: Container;
//// }
////
//// export { Piece };

// @FileName: /augmentation.ts
//// declare module "@sapphire/pieces" {
//// interface Container {
//// client: unknown;
//// }
//// }

// @Filename: /index.ts
//// import { Piece } from "@sapphire/pieces";
//// class FullPiece extends Piece {
//// /*1*/
//// }

const preferences = {
includeCompletionsWithClassMemberSnippets: true,
includeCompletionsWithInsertText: true,
};

verify.completions({
marker: "1",
includes: [
{
name: "container",
insertText: "container: Container;",
filterText: "container",
hasAction: true,
source: "ClassMemberSnippet/",
},
],
preferences,
isNewIdentifierLocation: true,
});

verify.applyCodeActionFromCompletion("1", {
name: "container",
source: "ClassMemberSnippet/",
description: `Includes imports of types referenced by 'container'`,
newFileContent: `import { Container, Piece } from "@sapphire/pieces";
class FullPiece extends Piece {

}`,
preferences,
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
/// <reference path="fourslash.ts" />

// @module: nodenext

// @Filename: /node_modules/@sapphire/pieces/index.d.ts
//// interface Container {
//// stores: unknown;
//// }
////
//// declare class Piece {
//// get container(): Container;
//// }
////
//// export { Piece as Alias, type Container };

// @Filename: /node_modules/@sapphire/framework/index.d.ts
//// import { Alias } from "@sapphire/pieces";
////
//// declare class Command extends Alias {}
////
//// declare module "@sapphire/pieces" {
//// interface Container {
//// client: unknown;
//// }
//// }
////
//// export { Command as CommandAlias };

// @Filename: /index.ts
//// import "@sapphire/pieces";
//// import { CommandAlias } from "@sapphire/framework";
//// class PingCommand extends CommandAlias {
//// /*1*/
//// }

const preferences = {
includeCompletionsWithClassMemberSnippets: true,
includeCompletionsWithInsertText: true,
};

verify.completions({
marker: "1",
includes: [
{
name: "container",
insertText: "get container(): Container {\n}",
filterText: "container",
hasAction: true,
source: "ClassMemberSnippet/",
},
],
preferences,
isNewIdentifierLocation: true,
});

verify.applyCodeActionFromCompletion("1", {
name: "container",
source: "ClassMemberSnippet/",
description: `Includes imports of types referenced by 'container'`,
newFileContent: `import "@sapphire/pieces";
import { CommandAlias } from "@sapphire/framework";
import { Container } from "@sapphire/pieces";
class PingCommand extends CommandAlias {

}`,
preferences,
});