Skip to content

Improve declaration emit type safety. #124

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 5 commits into from
Jan 8, 2024
Merged

Improve declaration emit type safety. #124

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
211b056
Improve declaration emit type safety.
dragomirtitian Nov 29, 2023
97161e0
Added kind to transformation context.
dragomirtitian Dec 13, 2023
c45ffaf
Addressed code review.
dragomirtitian Jan 2, 2024
a3a5049
Added back runtime members to nullTransformationContext
dragomirtitian Jan 3, 2024
202d522
Changed null context to have all methods from the regular transformat…
dragomirtitian Jan 4, 2024
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
1 change: 0 additions & 1 deletion src/compiler/_namespaces/ts.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,6 @@ export * from "../transformers/declarations/emitBinder";
export * from "../transformers/declarations/emitResolver";
export * from "../transformers/declarations/transpileDeclaration";
export * from "../transformers/declarations/localInferenceResolver";
export * from "../transformers/declarations/types";
export * from "../transformers/declarations";
export * from "../transformer";
export * from "../emitter";
Expand Down
82 changes: 53 additions & 29 deletions src/compiler/transformer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,13 @@ import {
Bundle,
chainBundle,
CompilerOptions,
CoreEmitResolver,
createEmitHelperFactory,
CustomTransformer,
CustomTransformerFactory,
CustomTransformers,
Debug,
Diagnostic,
DiagnosticWithLocation,
disposeEmitNodes,
EmitFlags,
Expand All @@ -30,6 +32,7 @@ import {
getUseDefineForClassFields,
Identifier,
isBundle,
IsolatedTransformationContext,
isSourceFile,
LexicalEnvironmentFlags,
map,
Expand All @@ -40,6 +43,7 @@ import {
NodeFlags,
noop,
notImplemented,
NullTransformationContext,
returnUndefined,
ScriptTarget,
setEmitFlags,
Expand All @@ -49,6 +53,7 @@ import {
SyntaxKind,
tracing,
TransformationContext,
TransformationContextKind,
TransformationResult,
transformClassFields,
transformDeclarations,
Expand Down Expand Up @@ -267,6 +272,7 @@ export function transformNodes<T extends Node>(resolver: EmitResolver | undefine
// The transformation context is provided to each transformer as part of transformer
// initialization.
const context: TransformationContext = {
kind: TransformationContextKind.FullContext,
factory,
getCompilerOptions: () => options,
getEmitResolver: () => resolver!, // TODO: GH#18217
Expand Down Expand Up @@ -662,33 +668,51 @@ export function transformNodes<T extends Node>(resolver: EmitResolver | undefine
}
}
}

/** @internal */
export const nullTransformationContext: TransformationContext = {
factory: factory, // eslint-disable-line object-shorthand
getCompilerOptions: () => ({}),
getEmitResolver: notImplemented,
Copy link

Choose a reason for hiding this comment

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

Removing these results in a different shape than a normal transformation context, which is observable to the runtime during normal compilation since the null context is used by the command line compiler. I'd prefer we keep these in for the null context.

Copy link
Author

Choose a reason for hiding this comment

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

Since some of the removed members throw an exception at runtime, I would not expose them on the NullTransformationContext type. If someone somewhere is actually using the nullTransformationContext as a full transformation context I kept the nullTransformationContext to have the same runtime members but hid it behind the more restrictive NullTransformationContext

Or did you have in mind that the NullTransformationContext type is more similar to a full TransformationContext. I don't particularly like that idea since some of them throw runtime exceptions so ideally the type system should not let you access those members.

Copy link

Choose a reason for hiding this comment

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

NullTransformationContext has always just been a normal TransformationContext with the features that depended on a type checker or program disabled. Having the shape remain stable avoids V8 recompiling large chunks of tsc.js because the checker sees one shape when it creates synthetic nodes when producing diagnostics via typeToTypeNode and another shape when it sees a full checker when doing emit.

Copy link

Choose a reason for hiding this comment

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

I don't particularly like that idea since some of them throw runtime exceptions so ideally the type system should not let you access those members.

If we want the type to follow the shape but limit access, we could always define a NullTransformationContext type that makes the return types of those methods never.

Copy link
Author

Choose a reason for hiding this comment

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

@rbuckton I changed NullTransformationContext to have the same shape as TransformationContext (just the not implemented methods return never)

getEmitHost: notImplemented,
getEmitHelperFactory: notImplemented,
startLexicalEnvironment: noop,
resumeLexicalEnvironment: noop,
suspendLexicalEnvironment: noop,
endLexicalEnvironment: returnUndefined,
setLexicalEnvironmentFlags: noop,
getLexicalEnvironmentFlags: () => 0,
hoistVariableDeclaration: noop,
hoistFunctionDeclaration: noop,
addInitializationStatement: noop,
startBlockScope: noop,
endBlockScope: returnUndefined,
addBlockScopedVariable: noop,
requestEmitHelper: noop,
readEmitHelpers: notImplemented,
enableSubstitution: noop,
enableEmitNotification: noop,
isSubstitutionEnabled: notImplemented,
isEmitNotificationEnabled: notImplemented,
onSubstituteNode: noEmitSubstitution,
onEmitNode: noEmitNotification,
addDiagnostic: noop,
};
export function createTransformationContext(kind: TransformationContextKind.NullContext): NullTransformationContext;
Copy link

Choose a reason for hiding this comment

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

The overload seems a bit overcomplicated since we'll only ever create one nullTransformationContext. You could just leave nullTransformationContext as is with the exception of giving it the type NullTransformationContext and adding the kind, and then just use return { ...nullTransformationContext, kind: TransformationContext.IsolatedContext, <whatever overrides you want> }. I realize that's pretty much what you had before, but I think kind is actually a bit clearer than the boolean field you were using previously.

/** @internal */
export function createTransformationContext(
kind: TransformationContextKind.IsolatedContext,
options: CompilerOptions,
diagnostics: Diagnostic[],
resolver: CoreEmitResolver,
): IsolatedTransformationContext;
export function createTransformationContext(
kind: TransformationContextKind.IsolatedContext | TransformationContextKind.NullContext,
options: CompilerOptions = {},
diagnostics?: Diagnostic[],
resolver?: EmitResolver | CoreEmitResolver,
host?: EmitHost,
): NullTransformationContext | IsolatedTransformationContext | TransformationContext {
return {
kind,
factory: factory, // eslint-disable-line object-shorthand
getCompilerOptions: () => options,
getEmitResolver: !resolver ? notImplemented : () => resolver,
getEmitHost: !host ? notImplemented : () => host,
getEmitHelperFactory: notImplemented,
startLexicalEnvironment: noop,
resumeLexicalEnvironment: noop,
suspendLexicalEnvironment: noop,
endLexicalEnvironment: returnUndefined,
setLexicalEnvironmentFlags: noop,
getLexicalEnvironmentFlags: () => 0,
hoistVariableDeclaration: noop,
hoistFunctionDeclaration: noop,
addInitializationStatement: noop,
startBlockScope: noop,
endBlockScope: returnUndefined,
addBlockScopedVariable: noop,
requestEmitHelper: noop,
readEmitHelpers: notImplemented,
enableSubstitution: noop,
enableEmitNotification: noop,
isSubstitutionEnabled: notImplemented,
isEmitNotificationEnabled: notImplemented,
onSubstituteNode: noEmitSubstitution,
onEmitNode: noEmitNotification,
addDiagnostic: !diagnostics ? noop : (diag: Diagnostic) => diagnostics.push(diag),
};
}
/** @internal */
export const nullTransformationContext: NullTransformationContext = createTransformationContext(TransformationContextKind.NullContext);
119 changes: 46 additions & 73 deletions src/compiler/transformers/declarations.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import {
ConstructorTypeNode,
ConstructSignatureDeclaration,
contains,
CoreEmitResolver,
createDiagnosticForNode,
createDiagnosticForRange,
createEmptyExports,
Expand All @@ -46,7 +47,6 @@ import {
EnumDeclaration,
ExportAssignment,
ExportDeclaration,
Expression,
ExpressionWithTypeArguments,
factory,
FileReference,
Expand Down Expand Up @@ -139,7 +139,7 @@ import {
isMethodSignature,
isModifier,
isModuleDeclaration,
IsolatedEmitResolver,
IsolatedTransformationContext,
isOmittedExpression,
isPrivateIdentifier,
isPropertySignature,
Expand Down Expand Up @@ -167,7 +167,6 @@ import {
LateBoundDeclaration,
LateVisibilityPaintedStatement,
length,
LocalInferenceResolver,
map,
mapDefined,
MethodDeclaration,
Expand Down Expand Up @@ -221,6 +220,7 @@ import {
SyntaxKind,
toFileNameLowerCase,
TransformationContext,
TransformationContextKind,
transformNodes,
tryCast,
tryGetModuleSpecifierFromDeclaration,
Expand Down Expand Up @@ -293,7 +293,7 @@ const declarationEmitNodeBuilderFlags = NodeBuilderFlags.MultilineObjectLiterals
*
* @internal
*/
export function transformDeclarations(context: TransformationContext, _useTscEmit = true) {
export function transformDeclarations(context: TransformationContext | IsolatedTransformationContext) {
const throwDiagnostic = () => Debug.fail("Diagnostic emitted without context");
let getSymbolAccessibilityDiagnostic: GetSymbolAccessibilityDiagnostic = throwDiagnostic;
let needsDeclare = true;
Expand All @@ -318,108 +318,80 @@ export function transformDeclarations(context: TransformationContext, _useTscEmi
let refs: Map<NodeId, SourceFile>;
let libs: Map<string, boolean>;
let emittedImports: readonly AnyImportSyntax[] | undefined; // must be declared in container so it can be `undefined` while transformer's first pass
const { localInferenceResolver, isolatedDeclarations, host, resolver, symbolTracker, ensureNoInitializer, useTscEmit } = createTransformerServices();
const { localInferenceResolver, isolatedDeclarations, host, resolver, symbolTracker, ensureNoInitializer, useLocalInferenceTypePrint } = createTransformerServices();
const options = context.getCompilerOptions();
const { noResolve, stripInternal } = options;
return transformRoot;

function createTransformerServices(): {
isolatedDeclarations: true;
useTscEmit: false;
resolver: IsolatedEmitResolver;
localInferenceResolver: LocalInferenceResolver;
host: undefined;
symbolTracker: undefined;
ensureNoInitializer: (node: CanHaveLiteralInitializer) => Expression | undefined;
} | {
isolatedDeclarations: true;
useTscEmit: true;
resolver: EmitResolver;
localInferenceResolver: LocalInferenceResolver;
host: EmitHost;
symbolTracker: SymbolTracker;
ensureNoInitializer: (node: CanHaveLiteralInitializer) => Expression | undefined;
} | {
isolatedDeclarations: false;
useTscEmit: false;
resolver: EmitResolver;
localInferenceResolver: undefined;
host: EmitHost;
symbolTracker: SymbolTracker;
ensureNoInitializer: (node: CanHaveLiteralInitializer) => Expression | undefined;
} {
const { isolatedDeclarations, resolver: localInferenceResolver } = createLocalInferenceResolver({
ensureParameter,
context,
visitDeclarationSubtree,
setEnclosingDeclarations(node) {
const oldNode = enclosingDeclaration;
enclosingDeclaration = node;
return oldNode;
},
checkEntityNameVisibility(name, container) {
return checkEntityNameVisibility(name, container ?? enclosingDeclaration);
},
});
function createTransformerServices() {
const isolatedDeclarations = context.getCompilerOptions().isolatedDeclarations;

if (isolatedDeclarations) {
if (!_useTscEmit) {
const resolver: IsolatedEmitResolver = context.getEmitResolver();
const localInferenceResolver = createLocalInferenceResolver({
ensureParameter,
context,
visitDeclarationSubtree,
setEnclosingDeclarations(node) {
const oldNode = enclosingDeclaration;
enclosingDeclaration = node;
return oldNode;
},
checkEntityNameVisibility(name, container) {
return checkEntityNameVisibility(name, container ?? enclosingDeclaration);
},
});
if (context.kind === TransformationContextKind.IsolatedContext) {
const resolver: CoreEmitResolver = context.getEmitResolver();
// Ideally nothing should require the symbol tracker in isolated declarations mode.
// createLiteralConstValue is the one exception
const emptySymbolTracker = {};
return {
isolatedDeclarations,
useTscEmit: false,
useLocalInferenceTypePrint: true,
resolver,
localInferenceResolver,
symbolTracker: undefined,
host: undefined,
ensureNoInitializer: (node: CanHaveLiteralInitializer) => {
if (shouldPrintWithInitializer(node)) {
return resolver.createLiteralConstValue(getParseTreeNode(node) as CanHaveLiteralInitializer, emptySymbolTracker); // TODO: Make safe
}
return undefined;
},
};
ensureNoInitializer: createEnsureNoInitializer(emptySymbolTracker),
} as const;
}
else {
const host = context.getEmitHost();
const resolver: EmitResolver = context.getEmitResolver();
const symbolTracker = createSymbolTracker(resolver, host);
return {
isolatedDeclarations,
useTscEmit: true,
useLocalInferenceTypePrint: false,
resolver,
localInferenceResolver,
symbolTracker,
host,
ensureNoInitializer: (node: CanHaveLiteralInitializer) => {
if (shouldPrintWithInitializer(node)) {
return resolver.createLiteralConstValue(getParseTreeNode(node) as CanHaveLiteralInitializer, symbolTracker); // TODO: Make safe
}
return undefined;
},
};
ensureNoInitializer: createEnsureNoInitializer(symbolTracker),
} as const;
}
}
else {
Debug.assert(context.kind === TransformationContextKind.FullContext);
const host = context.getEmitHost();
const resolver = context.getEmitResolver();
const symbolTracker: SymbolTracker = createSymbolTracker(resolver, host);
return {
isolatedDeclarations,
useTscEmit: false,
localInferenceResolver,
isolatedDeclarations: false,
useLocalInferenceTypePrint: false,
localInferenceResolver: undefined,
resolver,
symbolTracker,
host,
ensureNoInitializer: (node: CanHaveLiteralInitializer) => {
if (shouldPrintWithInitializer(node)) {
return resolver.createLiteralConstValue(getParseTreeNode(node) as CanHaveLiteralInitializer, symbolTracker); // TODO: Make safe
}
return undefined;
},
ensureNoInitializer: createEnsureNoInitializer(symbolTracker),
} as const;
}

function createEnsureNoInitializer(symbolTracker: SymbolTracker) {
return function ensureNoInitializer(node: CanHaveLiteralInitializer) {
if (shouldPrintWithInitializer(node)) {
return resolver.createLiteralConstValue(getParseTreeNode(node) as CanHaveLiteralInitializer, symbolTracker); // TODO: Make safe
}
return undefined;
};
}
}
Expand Down Expand Up @@ -659,6 +631,7 @@ export function transformDeclarations(context: TransformationContext, _useTscEmi
libs = new Map();
existingTypeReferencesSources = node.sourceFiles;
let hasNoDefaultLib = false;
Debug.assert(!isolatedDeclarations, "Bundles are not supported in isolated declarations");
const bundle = factory.createBundle(
map(node.sourceFiles, sourceFile => {
if (sourceFile.isDeclarationFile) return undefined!; // Omit declaration files from bundle results, too // TODO: GH#18217
Expand All @@ -681,7 +654,7 @@ export function transformDeclarations(context: TransformationContext, _useTscEmi
sourceFile,
[factory.createModuleDeclaration(
[factory.createModifier(SyntaxKind.DeclareKeyword)],
factory.createStringLiteral(getResolvedExternalModuleName(context.getEmitHost(), sourceFile)),
factory.createStringLiteral(getResolvedExternalModuleName(host, sourceFile)),
factory.createModuleBlock(setTextRange(factory.createNodeArray(transformAndReplaceLatePaintedStatements(statements)), sourceFile.statements)),
)],
/*isDeclarationFile*/ true,
Expand Down Expand Up @@ -952,7 +925,7 @@ export function transformDeclarations(context: TransformationContext, _useTscEmi
}
if (isolatedDeclarations) {
const { typeNode, isInvalid } = localInferenceResolver.fromInitializer(node, type, currentSourceFile);
if (!useTscEmit || isInvalid) {
if (useLocalInferenceTypePrint || isInvalid) {
return typeNode;
}
}
Expand Down Expand Up @@ -1121,7 +1094,7 @@ export function transformDeclarations(context: TransformationContext, _useTscEmi
if (isBundledEmit) {
// Bundle emit not supported for isolatedDeclarations
if (!isolatedDeclarations) {
const newName = getExternalModuleNameFromDeclaration(context.getEmitHost(), resolver, parent);
const newName = getExternalModuleNameFromDeclaration(host, resolver, parent);
if (newName) {
return factory.createStringLiteral(newName);
}
Expand Down
6 changes: 5 additions & 1 deletion src/compiler/transformers/declarations/emitBinder.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,6 @@ import {
isVarConst,
isVariableDeclaration,
MappedTypeNode,
MemberKey,
MethodDeclaration,
MethodSignature,
ModifierFlags,
Expand Down Expand Up @@ -816,6 +815,11 @@ export function bindSourceFileForDeclarationEmit(file: SourceFile) {
}
}

/** @internal */
export type MemberKey = string & {
__memberKey: void;
};

/**
* Gets the symbolic name for a member from its type.
* @internal
Expand Down
Loading