Skip to content

In JS declaration emit, move imports painted in nested contexts to the root private context #39818

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
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
23 changes: 16 additions & 7 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5889,7 +5889,7 @@ namespace ts {
const enclosingDeclaration = context.enclosingDeclaration!;
let results: Statement[] = [];
const visitedSymbols = new Set<number>();
let deferredPrivates: ESMap<SymbolId, Symbol> | undefined;
const deferredPrivatesStack: ESMap<SymbolId, Symbol>[] = [];
const oldcontext = context;
context = {
...oldcontext,
Expand Down Expand Up @@ -6100,9 +6100,8 @@ namespace ts {
}

function visitSymbolTable(symbolTable: SymbolTable, suppressNewPrivateContext?: boolean, propertyAsAlias?: boolean) {
const oldDeferredPrivates = deferredPrivates;
if (!suppressNewPrivateContext) {
deferredPrivates = new Map();
deferredPrivatesStack.push(new Map());
}
symbolTable.forEach((symbol: Symbol) => {
serializeSymbol(symbol, /*isPrivate*/ false, !!propertyAsAlias);
Expand All @@ -6111,11 +6110,11 @@ namespace ts {
// deferredPrivates will be filled up by visiting the symbol table
// And will continue to iterate as elements are added while visited `deferredPrivates`
// (As that's how a map iterator is defined to work)
deferredPrivates!.forEach((symbol: Symbol) => {
deferredPrivatesStack[deferredPrivatesStack.length - 1].forEach((symbol: Symbol) => {
serializeSymbol(symbol, /*isPrivate*/ true, !!propertyAsAlias);
});
deferredPrivatesStack.pop();
}
deferredPrivates = oldDeferredPrivates;
}

function serializeSymbol(symbol: Symbol, isPrivate: boolean, propertyAsAlias: boolean) {
Expand Down Expand Up @@ -6299,9 +6298,19 @@ namespace ts {

function includePrivateSymbol(symbol: Symbol) {
if (some(symbol.declarations, isParameterDeclaration)) return;
Debug.assertIsDefined(deferredPrivates);
Debug.assertIsDefined(deferredPrivatesStack[deferredPrivatesStack.length - 1]);
getUnusedName(unescapeLeadingUnderscores(symbol.escapedName), symbol); // Call to cache unique name for symbol
deferredPrivates.set(getSymbolId(symbol), symbol);
// Blanket moving (import) aliases into the root private context should work, since imports are not valid within namespaces
// (so they must have been in the root to begin with if they were real imports) cjs `require` aliases (an upcoming feature)
// will throw a wrench in this, since those may have been nested, but we'll need to synthesize them in the outer scope
Copy link
Member

Choose a reason for hiding this comment

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

can you add a test for the commonjs case so I'll be forced to fix it before merging that PR?

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 mean, a copy of jsDeclarationsImportAliasExposedWithinNamespace.ts but with cjs requires instead of import and module.exports instead of export should do; i'll add it, but you'll still need to watch it (since it may do... nothing).

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've added jsDeclarationsImportAliasExposedWithinNamespaceCjs.ts - it currently has errors and unexpected output (due to the nested typedefs not resolving correctly with cjs exports).

Copy link
Member

Choose a reason for hiding this comment

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

Thanks!

// anyway, as that's the only place the import they translate to is valid. In such a case, we might need to use a unique name
// for the moved import; which hopefully the above `getUnusedName` call should produce.
const isExternalImportAlias = !!(symbol.flags & SymbolFlags.Alias) && !some(symbol.declarations, d =>
!!findAncestor(d, isExportDeclaration) ||
isNamespaceExport(d) ||
(isImportEqualsDeclaration(d) && !isExternalModuleReference(d.moduleReference))
Copy link
Member

Choose a reason for hiding this comment

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

this last line looks for import x = y, not import x = require('y'), right? I didn't think these declarations needed to be moved, so that seems backwards. But maybe I'm reading it backward.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, but that's correct; the logic goes "An alias is an external import alias if it is not an import to any of an export (or any kind) or an alias to a local reference."

);
deferredPrivatesStack[isExternalImportAlias ? 0 : (deferredPrivatesStack.length - 1)].set(getSymbolId(symbol), symbol);
}

function isExportingScope(enclosingDeclaration: Node) {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
//// [tests/cases/conformance/jsdoc/declarations/jsDeclarationsImportAliasExposedWithinNamespace.ts] ////

//// [file.js]
/**
* @namespace myTypes
* @global
* @type {Object<string,*>}
*/
const myTypes = {
// SOME PROPS HERE
};

/** @typedef {string|RegExp|Array<string|RegExp>} myTypes.typeA */

/**
* @typedef myTypes.typeB
* @property {myTypes.typeA} prop1 - Prop 1.
* @property {string} prop2 - Prop 2.
*/

/** @typedef {myTypes.typeB|Function} myTypes.typeC */

export {myTypes};
//// [file2.js]
import {myTypes} from './file.js';

/**
* @namespace testFnTypes
* @global
* @type {Object<string,*>}
*/
const testFnTypes = {
// SOME PROPS HERE
};

/** @typedef {boolean|myTypes.typeC} testFnTypes.input */

/**
* @function testFn
* @description A test function.
* @param {testFnTypes.input} input - Input.
* @returns {number|null} Result.
*/
function testFn(input) {
if (typeof input === 'number') {
return 2 * input;
} else {
return null;
}
}

export {testFn, testFnTypes};



//// [file.d.ts]
/**
* @namespace myTypes
* @global
* @type {Object<string,*>}
*/
export const myTypes: {
[x: string]: any;
};
export namespace myTypes {
type typeA = string | RegExp | (string | RegExp)[];
type typeB = {
/**
* - Prop 1.
*/
prop1: string | RegExp | (string | RegExp)[];
/**
* - Prop 2.
*/
prop2: string;
};
type typeC = Function | typeB;
}
//// [file2.d.ts]
/** @typedef {boolean|myTypes.typeC} testFnTypes.input */
/**
* @function testFn
* @description A test function.
* @param {testFnTypes.input} input - Input.
* @returns {number|null} Result.
*/
export function testFn(input: boolean | Function | myTypes.typeB): number | null;
/**
* @namespace testFnTypes
* @global
* @type {Object<string,*>}
*/
export const testFnTypes: {
[x: string]: any;
};
export namespace testFnTypes {
type input = boolean | Function | myTypes.typeB;
}
import { myTypes } from "./file.js";
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
=== tests/cases/conformance/jsdoc/declarations/file.js ===
/**
* @namespace myTypes
* @global
* @type {Object<string,*>}
*/
const myTypes = {
>myTypes : Symbol(myTypes, Decl(file.js, 5, 5), Decl(file.js, 9, 50), Decl(file.js, 12, 12), Decl(file.js, 17, 38))

// SOME PROPS HERE
};

/** @typedef {string|RegExp|Array<string|RegExp>} myTypes.typeA */

/**
* @typedef myTypes.typeB
* @property {myTypes.typeA} prop1 - Prop 1.
* @property {string} prop2 - Prop 2.
*/

/** @typedef {myTypes.typeB|Function} myTypes.typeC */

export {myTypes};
>myTypes : Symbol(myTypes, Decl(file.js, 19, 8))

=== tests/cases/conformance/jsdoc/declarations/file2.js ===
import {myTypes} from './file.js';
>myTypes : Symbol(myTypes, Decl(file2.js, 0, 8))

/**
* @namespace testFnTypes
* @global
* @type {Object<string,*>}
*/
const testFnTypes = {
>testFnTypes : Symbol(testFnTypes, Decl(file2.js, 7, 5), Decl(file2.js, 11, 37))

// SOME PROPS HERE
};

/** @typedef {boolean|myTypes.typeC} testFnTypes.input */

/**
* @function testFn
* @description A test function.
* @param {testFnTypes.input} input - Input.
* @returns {number|null} Result.
*/
function testFn(input) {
>testFn : Symbol(testFn, Decl(file2.js, 9, 2))
>input : Symbol(input, Decl(file2.js, 19, 16))

if (typeof input === 'number') {
>input : Symbol(input, Decl(file2.js, 19, 16))

return 2 * input;
>input : Symbol(input, Decl(file2.js, 19, 16))

} else {
return null;
}
}

export {testFn, testFnTypes};
>testFn : Symbol(testFn, Decl(file2.js, 27, 8))
>testFnTypes : Symbol(testFnTypes, Decl(file2.js, 27, 15))

Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
=== tests/cases/conformance/jsdoc/declarations/file.js ===
/**
* @namespace myTypes
* @global
* @type {Object<string,*>}
*/
const myTypes = {
>myTypes : { [x: string]: any; }
>{ // SOME PROPS HERE} : {}

// SOME PROPS HERE
};

/** @typedef {string|RegExp|Array<string|RegExp>} myTypes.typeA */

/**
* @typedef myTypes.typeB
* @property {myTypes.typeA} prop1 - Prop 1.
* @property {string} prop2 - Prop 2.
*/

/** @typedef {myTypes.typeB|Function} myTypes.typeC */

export {myTypes};
>myTypes : { [x: string]: any; }

=== tests/cases/conformance/jsdoc/declarations/file2.js ===
import {myTypes} from './file.js';
>myTypes : { [x: string]: any; }

/**
* @namespace testFnTypes
* @global
* @type {Object<string,*>}
*/
const testFnTypes = {
>testFnTypes : { [x: string]: any; }
>{ // SOME PROPS HERE} : {}

// SOME PROPS HERE
};

/** @typedef {boolean|myTypes.typeC} testFnTypes.input */

/**
* @function testFn
* @description A test function.
* @param {testFnTypes.input} input - Input.
* @returns {number|null} Result.
*/
function testFn(input) {
>testFn : (input: testFnTypes.input) => number | null
>input : boolean | Function | myTypes.typeB

if (typeof input === 'number') {
>typeof input === 'number' : boolean
>typeof input : "string" | "number" | "bigint" | "boolean" | "symbol" | "undefined" | "object" | "function"
>input : boolean | Function | myTypes.typeB
>'number' : "number"

return 2 * input;
>2 * input : number
>2 : 2
>input : never

} else {
return null;
>null : null
}
}

export {testFn, testFnTypes};
>testFn : (input: boolean | Function | myTypes.typeB) => number
>testFnTypes : { [x: string]: any; }

Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
tests/cases/conformance/jsdoc/declarations/file2.js(12,31): error TS2694: Namespace 'myTypes' has no exported member 'typeC'.


==== tests/cases/conformance/jsdoc/declarations/file2.js (1 errors) ====
const {myTypes} = require('./file.js');

/**
* @namespace testFnTypes
* @global
* @type {Object<string,*>}
*/
const testFnTypes = {
// SOME PROPS HERE
};

/** @typedef {boolean|myTypes.typeC} testFnTypes.input */
~~~~~
!!! error TS2694: Namespace 'myTypes' has no exported member 'typeC'.

/**
* @function testFn
* @description A test function.
* @param {testFnTypes.input} input - Input.
* @returns {number|null} Result.
*/
function testFn(input) {
if (typeof input === 'number') {
return 2 * input;
} else {
return null;
}
}

module.exports = {testFn, testFnTypes};
==== tests/cases/conformance/jsdoc/declarations/file.js (0 errors) ====
/**
* @namespace myTypes
* @global
* @type {Object<string,*>}
*/
const myTypes = {
// SOME PROPS HERE
};

/** @typedef {string|RegExp|Array<string|RegExp>} myTypes.typeA */

/**
* @typedef myTypes.typeB
* @property {myTypes.typeA} prop1 - Prop 1.
* @property {string} prop2 - Prop 2.
*/

/** @typedef {myTypes.typeB|Function} myTypes.typeC */

exports.myTypes = myTypes;
Loading