Skip to content

Inline *_base declaration in d.ts files #59550

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

Open
6 tasks done
CraigMacomber opened this issue Aug 7, 2024 · 5 comments Β· May be fixed by #61087
Open
6 tasks done

Inline *_base declaration in d.ts files #59550

CraigMacomber opened this issue Aug 7, 2024 · 5 comments Β· May be fixed by #61087
Labels
Experimentation Needed Someone needs to try this out to see what happens Suggestion An idea for TypeScript
Milestone

Comments

@CraigMacomber
Copy link

CraigMacomber commented Aug 7, 2024

πŸ” Search Terms

Anonymous class, _base, base, d.ts, api-extractor, recursive

βœ… Viability Checklist

⭐ Suggestion

Currently classes which extends anonomous classes compile to d.ts files with two declarations:

class A extends class {} {}

Generates:

declare const A_base: {
    new (): {};
};
declare class A extends A_base {
}

playground link

My suggestion is for it to generate something like this instead:

declare class A extends ({} as {
    new (): {};
}) {
}

More generally replace:

declare const A_base: DEFINITION_OF_BASE_HERE;
declare class A extends A_base {
}

with:

declare class A extends ({} as DEFINITION_OF_BASE_HERE) {
}

πŸ“ƒ Motivating Example

When exporting classes with anonymous bases (for example classes with bases generated using functions, for example in fluid-framework's tree schema system, it would be nice if the generated d.ts file better matched the original source, so tools (like TypeScript and API-Extractor) behave more similarly to how they would if run on the source instead of on the d.ts. This would make the d.ts better serve as a concise summary of the type information of the original code.

πŸ’» Use Cases

  1. What do you want to use this for?
    I know of two cases this would help:

    1. API-Extractor: it would fix [api-extractor] Extending anonymous classes errors that _base class inserted by compiler is not exported.Β rushstack#4429 by removing this odd case from d.ts files.
    2. Some recursive types compile without error when the base in inline, but not when its split into a separate variable. Sometimes this even ends up being compilation order dependent (similar to Recursive types can depend on presence and source location of unused type declarationΒ #55758 ) and can result in failing incremental builds and working clean builds. Making the d.ts declare it inline when the original source files does helps make the d.ts more aligned with the non-d.ts type checking for recursive types avoiding introducing additional such issues specific to the d.ts. I haven't extracted a minimal repro for this, but there are some not so minimal examples (and a workaround) in tree: Add Recursive ArrayNode export workaroundΒ FluidFramework#22122.
  2. What shortcomings exist with current approaches?
    Currently the behavior is confusing since most TS developers don't think about the differences between the d.ts files and the original source, so having to do workarounds to make them stay aligned (like manually exporting the base to make API-Extractor happy, or ensuring that if the base is not inline, that the code still type checked for the recursive case) is confusing and unintuitive.

  3. What workarounds are you using in the meantime?
    For API extractor, manually export the base type under a different name.
    For the recursive type issue, export carefully crafted usage before the declaration which happens to cause it to compile with the split declaration.

@CraigMacomber
Copy link
Author

I hit this again. It makes it really annoying for users of Fluid Framework's SharedTree to use API-Extractor since doing so requires really annoying workarounds to export schema.

A schema definition like export class JsonArray extends sf.arrayRecursive("array", JsonUnion) {} has to become

/**
 * Do not use. Exists only as a workaround for {@link https://github.com/microsoft/TypeScript/issues/59550} and {@link https://github.com/microsoft/rushstack/issues/4429}.
 * @system @alpha
 */
export const _APIExtractorWorkaroundJsonArrayBase = sf.arrayRecursive("array", JsonUnion);

/**
 * Docs here....
 * @alpha
 */
export class JsonArray extends _APIExtractorWorkaroundJsonArrayBase {}

Having to do this for every shared tree schema makes a real mess schema files and the package API surface.
It also exposes customers of SharedTree and API-Extractor to an implementation detail of d.ts generation which they really shouldn't have to know, but if they don't know, they will have a hard time knowing what to do about an error like (ae-forgotten-export) The symbol "JsonArray_base" needs to be exported by the entry point alpha.d.ts when that's a symbol they never defined. THis is annoying for me who already knows exactly that's going on and how to work around it: I imagine for someone with no idea about the _base types in d.ts files, it would be quite confusing.

@octogonz
Copy link

octogonz commented Nov 8, 2024

Also, in general it is difficult to detect these _base declarations, as they can have other names such as _base_1. For example:

example.ts

class A_base {}
class A extends class {} {}

example.d.ts

declare class A_base {
}
declare const A_base_1: {
    new (): {};
};
declare class A extends A_base_1 {
}

The fundamental challenge is that, by design, API Extractor processes .d.ts files and does not consider their .ts inputs. I agree that, this case, emitting an algebraic type might make the .d.ts file actually easier to interpret.

@CraigMacomber
Copy link
Author

I hit this again. I'd love to see my suggested fix implemented: I'm not aware of any downsides to it.

I think the code that needs to change is

if (extendsClause && !isEntityNameExpression(extendsClause.expression) && extendsClause.expression.kind !== SyntaxKind.NullKeyword) {
// We must add a temporary declaration for the extends clause expression
const oldId = input.name ? unescapeLeadingUnderscores(input.name.escapedText) : "default";
const newId = factory.createUniqueName(`${oldId}_base`, GeneratedIdentifierFlags.Optimistic);
getSymbolAccessibilityDiagnostic = () => ({
diagnosticMessage: Diagnostics.extends_clause_of_exported_class_0_has_or_is_using_private_name_1,
errorNode: extendsClause,
typeName: input.name,
});
const varDecl = factory.createVariableDeclaration(newId, /*exclamationToken*/ undefined, resolver.createTypeOfExpression(extendsClause.expression, input, declarationEmitNodeBuilderFlags, declarationEmitInternalNodeBuilderFlags, symbolTracker), /*initializer*/ undefined);
const statement = factory.createVariableStatement(needsDeclare ? [factory.createModifier(SyntaxKind.DeclareKeyword)] : [], factory.createVariableDeclarationList([varDecl], NodeFlags.Const));
const heritageClauses = factory.createNodeArray(map(input.heritageClauses, clause => {
if (clause.token === SyntaxKind.ExtendsKeyword) {
const oldDiag = getSymbolAccessibilityDiagnostic;
getSymbolAccessibilityDiagnostic = createGetSymbolAccessibilityDiagnosticForNode(clause.types[0]);
const newClause = factory.updateHeritageClause(clause, map(clause.types, t => factory.updateExpressionWithTypeArguments(t, newId, visitNodes(t.typeArguments, visitDeclarationSubtree, isTypeNode))));
getSymbolAccessibilityDiagnostic = oldDiag;
return newClause;
}
return factory.updateHeritageClause(clause, visitNodes(factory.createNodeArray(filter(clause.types, t => isEntityNameExpression(t.expression) || t.expression.kind === SyntaxKind.NullKeyword)), visitDeclarationSubtree, isExpressionWithTypeArguments));
}));
return [
statement,
cleanup(factory.updateClassDeclaration(
input,
modifiers,
input.name,
typeParameters,
heritageClauses,
members,
))!,
]; // TODO: GH#18217
}
else {
const heritageClauses = transformHeritageClauses(input.heritageClauses);
return cleanup(factory.updateClassDeclaration(
input,
modifiers,
input.name,
typeParameters,
heritageClauses,
members,
));
}

I suspect that entire if/else could be removed and use my suggested format in both cases, or just replace the

// We must add a temporary declaration for the extends clause expression
const oldId = input.name ? unescapeLeadingUnderscores(input.name.escapedText) : "default";
const newId = factory.createUniqueName(`${oldId}_base`, GeneratedIdentifierFlags.Optimistic);
getSymbolAccessibilityDiagnostic = () => ({
diagnosticMessage: Diagnostics.extends_clause_of_exported_class_0_has_or_is_using_private_name_1,
errorNode: extendsClause,
typeName: input.name,
});
const varDecl = factory.createVariableDeclaration(newId, /*exclamationToken*/ undefined, resolver.createTypeOfExpression(extendsClause.expression, input, declarationEmitNodeBuilderFlags, declarationEmitInternalNodeBuilderFlags, symbolTracker), /*initializer*/ undefined);
const statement = factory.createVariableStatement(needsDeclare ? [factory.createModifier(SyntaxKind.DeclareKeyword)] : [], factory.createVariableDeclarationList([varDecl], NodeFlags.Const));
const heritageClauses = factory.createNodeArray(map(input.heritageClauses, clause => {
if (clause.token === SyntaxKind.ExtendsKeyword) {
const oldDiag = getSymbolAccessibilityDiagnostic;
getSymbolAccessibilityDiagnostic = createGetSymbolAccessibilityDiagnosticForNode(clause.types[0]);
const newClause = factory.updateHeritageClause(clause, map(clause.types, t => factory.updateExpressionWithTypeArguments(t, newId, visitNodes(t.typeArguments, visitDeclarationSubtree, isTypeNode))));
getSymbolAccessibilityDiagnostic = oldDiag;
return newClause;
}
return factory.updateHeritageClause(clause, visitNodes(factory.createNodeArray(filter(clause.types, t => isEntityNameExpression(t.expression) || t.expression.kind === SyntaxKind.NullKeyword)), visitDeclarationSubtree, isExpressionWithTypeArguments));
}));
return [
statement,
cleanup(factory.updateClassDeclaration(
input,
modifiers,
input.name,
typeParameters,
heritageClauses,
members,
))!,
]; // TODO: GH#18217
segment with simplified logic using my suggested format.

@RyanCavanaugh RyanCavanaugh added Experimentation Needed Someone needs to try this out to see what happens and removed Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature labels Jan 30, 2025
@RyanCavanaugh RyanCavanaugh added this to the Backlog milestone Jan 30, 2025
@CraigMacomber CraigMacomber linked a pull request Jan 31, 2025 that will close this issue
@CraigMacomber
Copy link
Author

I have created #61087 with an implementation of this change.

@CraigMacomber
Copy link
Author

CraigMacomber commented Feb 7, 2025

The TypeScript team suggested I make a PR to implement my suggestion if I wanted progress to be made on this issue. That is the PR linked above. They then reviewed my proposal and communicated to me indirectly that my proposal exploits a parser bug since expressions are not permitted in ambient contexts (my entire suggestion was to cram an expression in an ambient context, so this makes sense).

This makes it difficult to find a solution since existing versions of TypeScript give an error "Expression expected.(1109)" if you provide something other than an expression in that location.

The requirement for existing TypeScript versions that you provide an expression for the extends clause (in declare statements and otherwise), and the desire that declare statements contain things that are not expressions means that the only valid approach is to provide something that parses validly as an expression, but when used in a declare class extends context could be parsed by future versions of TypeScript something other than an expression.

To me this seems like future versions of TypeScript would have to make a breaking change to the parser/grammar to redefine what is allowed in declare class extends clauses.

To make my change to be compatible with this future breaking change to TypeScript grammar, I'd like to know what would be accepted in this context by these theoretical future TypeScript versions?

My understanding is that in ambient contexts, we allow typeof something.
In declare class extends clauses the relevant type is the type form of typeof applied to the expression.

Thus am I correct in assuming that if future versions of TypeScript want to make a breaking change to what's allowed in extends clauses of declare class, then it would likely be to apply the same restrictions that typeof has?

Assuming the above is correct, it seems like the design of the declare class syntax for extends was a mistake, and probably should have taken in the type of the base class (explicitly using typeof in common cases) which would make it possible to use a type directly there when desired, which is the case here. Fixing that however is impractical as it would be a breaking change.

There is an interesting workaround for that problem however, which is also backwards compatible with existing versions of TypeScript: you allow syntaxes for declare class:

  1. declare class <Identifier> extends <thing valid to give to typeof> {...}
  2. declare class <Identifier> extends ({} as <TYPE> {...}

This makes it possible to give a types directly to the extends clause without using an expression (in compilers with the new grammer), while keeping most existing code still working (provides expressions compatible with typeof). This new syntax also just happens to work with existing compilers (they parse it as an expression), so generating it wouldn't cause compatibility issues.

Changing the parser to stop supporting expressions in this context is still a breaking change, but the above approach could allow my workaround to continue to function with or without that breaking change ensuring forwards and backwards compatibility for my suggestion here, even if other expressions are banned.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Experimentation Needed Someone needs to try this out to see what happens Suggestion An idea for TypeScript
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants