Skip to content

Harden node builder APIs to no longer return undefined for a node when NodeBuilderFlags.IgnoreErrors is provided #38273

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 1 commit into from
May 4, 2020
Merged

Harden node builder APIs to no longer return undefined for a node when NodeBuilderFlags.IgnoreErrors is provided #38273

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
10 changes: 7 additions & 3 deletions src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4211,8 +4211,12 @@ namespace ts {
context.flags &= ~NodeBuilderFlags.InTypeAlias;

if (!type) {
context.encounteredError = true;
return undefined!; // TODO: GH#18217
if (!(context.flags & NodeBuilderFlags.AllowEmptyUnionOrIntersection)) {
Copy link
Member

Choose a reason for hiding this comment

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

As we discussed offline, I think this (and below) would be clearer as IgnoreErrors, but I can live with this version if it's more idiomatic.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, I struggled to enumerate it earlier, but if we check for NodeBuilderFlags.IgnoreErrors, then if any of the AllowX flags are set, the error would be suppressed (as that's how binary & works) - it's much neater to tie it to a specific flag (I'd have been tempted to add new ones, but we're actually entirely our of flag space in this enum).

context.encounteredError = true;
return undefined!; // TODO: GH#18217
}
context.approximateLength += 3;
return createKeywordTypeNode(SyntaxKind.AnyKeyword);
}

if (!(context.flags & NodeBuilderFlags.NoTypeReduction)) {
Expand Down Expand Up @@ -5925,7 +5929,7 @@ namespace ts {
function serializeSymbolWorker(symbol: Symbol, isPrivate: boolean, propertyAsAlias: boolean) {
const symbolName = unescapeLeadingUnderscores(symbol.escapedName);
const isDefault = symbol.escapedName === InternalSymbolName.Default;
if (isStringANonContextualKeyword(symbolName) && !isDefault) {
if (!(context.flags & NodeBuilderFlags.AllowAnonymousIdentifier) && isStringANonContextualKeyword(symbolName) && !isDefault) {
// Oh no. We cannot use this symbol's name as it's name... It's likely some jsdoc had an invalid name like `export` or `default` :(
context.encounteredError = true;
// TODO: Issue error via symbol tracker?
Expand Down
22 changes: 11 additions & 11 deletions src/compiler/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3435,24 +3435,24 @@ namespace ts {

// TODO: GH#18217 `xToDeclaration` calls are frequently asserted as defined.
/** Note that the resulting nodes cannot be checked. */
typeToTypeNode(type: Type, enclosingDeclaration?: Node, flags?: NodeBuilderFlags): TypeNode | undefined;
/* @internal */ typeToTypeNode(type: Type, enclosingDeclaration?: Node, flags?: NodeBuilderFlags, tracker?: SymbolTracker): TypeNode | undefined; // eslint-disable-line @typescript-eslint/unified-signatures
typeToTypeNode(type: Type, enclosingDeclaration: Node | undefined, flags: NodeBuilderFlags | undefined): TypeNode | undefined;
/* @internal */ typeToTypeNode(type: Type, enclosingDeclaration: Node | undefined, flags: NodeBuilderFlags | undefined, tracker?: SymbolTracker): TypeNode | undefined; // eslint-disable-line @typescript-eslint/unified-signatures
/** Note that the resulting nodes cannot be checked. */
signatureToSignatureDeclaration(signature: Signature, kind: SyntaxKind, enclosingDeclaration?: Node, flags?: NodeBuilderFlags): SignatureDeclaration & {typeArguments?: NodeArray<TypeNode>} | undefined;
/* @internal */ signatureToSignatureDeclaration(signature: Signature, kind: SyntaxKind, enclosingDeclaration?: Node, flags?: NodeBuilderFlags, tracker?: SymbolTracker): SignatureDeclaration & {typeArguments?: NodeArray<TypeNode>} | undefined; // eslint-disable-line @typescript-eslint/unified-signatures
signatureToSignatureDeclaration(signature: Signature, kind: SyntaxKind, enclosingDeclaration: Node | undefined, flags: NodeBuilderFlags | undefined): SignatureDeclaration & {typeArguments?: NodeArray<TypeNode>} | undefined;
/* @internal */ signatureToSignatureDeclaration(signature: Signature, kind: SyntaxKind, enclosingDeclaration: Node | undefined, flags: NodeBuilderFlags | undefined, tracker?: SymbolTracker): SignatureDeclaration & {typeArguments?: NodeArray<TypeNode>} | undefined; // eslint-disable-line @typescript-eslint/unified-signatures
/** Note that the resulting nodes cannot be checked. */
indexInfoToIndexSignatureDeclaration(indexInfo: IndexInfo, kind: IndexKind, enclosingDeclaration?: Node, flags?: NodeBuilderFlags): IndexSignatureDeclaration | undefined;
/* @internal */ indexInfoToIndexSignatureDeclaration(indexInfo: IndexInfo, kind: IndexKind, enclosingDeclaration?: Node, flags?: NodeBuilderFlags, tracker?: SymbolTracker): IndexSignatureDeclaration | undefined; // eslint-disable-line @typescript-eslint/unified-signatures
indexInfoToIndexSignatureDeclaration(indexInfo: IndexInfo, kind: IndexKind, enclosingDeclaration: Node | undefined, flags: NodeBuilderFlags | undefined): IndexSignatureDeclaration | undefined;
/* @internal */ indexInfoToIndexSignatureDeclaration(indexInfo: IndexInfo, kind: IndexKind, enclosingDeclaration: Node | undefined, flags: NodeBuilderFlags | undefined, tracker?: SymbolTracker): IndexSignatureDeclaration | undefined; // eslint-disable-line @typescript-eslint/unified-signatures
/** Note that the resulting nodes cannot be checked. */
symbolToEntityName(symbol: Symbol, meaning: SymbolFlags, enclosingDeclaration?: Node, flags?: NodeBuilderFlags): EntityName | undefined;
symbolToEntityName(symbol: Symbol, meaning: SymbolFlags, enclosingDeclaration: Node | undefined, flags: NodeBuilderFlags | undefined): EntityName | undefined;
/** Note that the resulting nodes cannot be checked. */
symbolToExpression(symbol: Symbol, meaning: SymbolFlags, enclosingDeclaration?: Node, flags?: NodeBuilderFlags): Expression | undefined;
symbolToExpression(symbol: Symbol, meaning: SymbolFlags, enclosingDeclaration: Node | undefined, flags: NodeBuilderFlags | undefined): Expression | undefined;
/** Note that the resulting nodes cannot be checked. */
symbolToTypeParameterDeclarations(symbol: Symbol, enclosingDeclaration?: Node, flags?: NodeBuilderFlags): NodeArray<TypeParameterDeclaration> | undefined;
symbolToTypeParameterDeclarations(symbol: Symbol, enclosingDeclaration: Node | undefined, flags: NodeBuilderFlags | undefined): NodeArray<TypeParameterDeclaration> | undefined;
/** Note that the resulting nodes cannot be checked. */
symbolToParameterDeclaration(symbol: Symbol, enclosingDeclaration?: Node, flags?: NodeBuilderFlags): ParameterDeclaration | undefined;
symbolToParameterDeclaration(symbol: Symbol, enclosingDeclaration: Node | undefined, flags: NodeBuilderFlags | undefined): ParameterDeclaration | undefined;
/** Note that the resulting nodes cannot be checked. */
typeParameterToDeclaration(parameter: TypeParameter, enclosingDeclaration?: Node, flags?: NodeBuilderFlags): TypeParameterDeclaration | undefined;
typeParameterToDeclaration(parameter: TypeParameter, enclosingDeclaration: Node | undefined, flags: NodeBuilderFlags | undefined): TypeParameterDeclaration | undefined;

getSymbolsInScope(location: Node, meaning: SymbolFlags): Symbol[];
getSymbolAtLocation(node: Node): Symbol | undefined;
Expand Down
2 changes: 1 addition & 1 deletion src/services/codefixes/convertToAsyncFunction.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -282,7 +282,7 @@ namespace ts.codefix {
varDeclIdentifier = getSynthesizedDeepClone(possibleNameForVarDecl.identifier);
const typeArray: Type[] = possibleNameForVarDecl.types;
const unionType = transformer.checker.getUnionType(typeArray, UnionReduction.Subtype);
const unionTypeNode = transformer.isInJSFile ? undefined : transformer.checker.typeToTypeNode(unionType);
const unionTypeNode = transformer.isInJSFile ? undefined : transformer.checker.typeToTypeNode(unionType, /*enclosingDeclaration*/ undefined, /*flags*/ undefined);
const varDecl = [createVariableDeclaration(varDeclIdentifier, unionTypeNode)];
varDeclList = createVariableStatement(/*modifiers*/ undefined, createVariableDeclarationList(varDecl, NodeFlags.Let));
}
Expand Down
4 changes: 2 additions & 2 deletions src/services/codefixes/fixAddMissingMember.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,11 +230,11 @@ namespace ts.codefix {
const binaryExpression = token.parent.parent as BinaryExpression;
const otherExpression = token.parent === binaryExpression.left ? binaryExpression.right : binaryExpression.left;
const widenedType = checker.getWidenedType(checker.getBaseTypeOfLiteralType(checker.getTypeAtLocation(otherExpression)));
typeNode = checker.typeToTypeNode(widenedType, classDeclaration);
typeNode = checker.typeToTypeNode(widenedType, classDeclaration, /*flags*/ undefined);
}
else {
const contextualType = checker.getContextualType(token.parent as Expression);
typeNode = contextualType ? checker.typeToTypeNode(contextualType) : undefined;
typeNode = contextualType ? checker.typeToTypeNode(contextualType, /*enclosingDeclaration*/ undefined, /*flags*/ undefined) : undefined;
}
return typeNode || createKeywordTypeNode(SyntaxKind.AnyKeyword);
}
Expand Down
2 changes: 1 addition & 1 deletion src/services/codefixes/fixJSDocTypes.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ namespace ts.codefix {
});

function doChange(changes: textChanges.ChangeTracker, sourceFile: SourceFile, oldTypeNode: TypeNode, newType: Type, checker: TypeChecker): void {
changes.replaceNode(sourceFile, oldTypeNode, checker.typeToTypeNode(newType, /*enclosingDeclaration*/ oldTypeNode)!); // TODO: GH#18217
changes.replaceNode(sourceFile, oldTypeNode, checker.typeToTypeNode(newType, /*enclosingDeclaration*/ oldTypeNode, /*flags*/ undefined)!); // TODO: GH#18217
}

function getInfo(sourceFile: SourceFile, pos: number, checker: TypeChecker): { readonly typeNode: TypeNode, readonly type: Type } | undefined {
Expand Down
4 changes: 2 additions & 2 deletions src/services/signatureHelp.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -580,7 +580,7 @@ namespace ts.SignatureHelp {
const printer = createPrinter({ removeComments: true });
const typeParameterParts = mapToDisplayParts(writer => {
if (candidateSignature.typeParameters && candidateSignature.typeParameters.length) {
const args = createNodeArray(candidateSignature.typeParameters.map(p => checker.typeParameterToDeclaration(p, enclosingDeclaration)!));
const args = createNodeArray(candidateSignature.typeParameters.map(p => checker.typeParameterToDeclaration(p, enclosingDeclaration, signatureHelpNodeBuilderFlags)!));
printer.writeList(ListFormat.TypeParameters, args, sourceFile, writer);
}
});
Expand All @@ -599,7 +599,7 @@ namespace ts.SignatureHelp {

function createSignatureHelpParameterForTypeParameter(typeParameter: TypeParameter, checker: TypeChecker, enclosingDeclaration: Node, sourceFile: SourceFile, printer: Printer): SignatureHelpParameter {
const displayParts = mapToDisplayParts(writer => {
const param = checker.typeParameterToDeclaration(typeParameter, enclosingDeclaration)!;
const param = checker.typeParameterToDeclaration(typeParameter, enclosingDeclaration, signatureHelpNodeBuilderFlags)!;
printer.writeNode(EmitHint.Unspecified, param, sourceFile, writer);
});
return { name: typeParameter.symbol.name, documentation: typeParameter.symbol.getDocumentationComment(checker), displayParts, isOptional: false };
Expand Down
6 changes: 4 additions & 2 deletions src/services/symbolDisplay.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
/* @internal */
namespace ts.SymbolDisplay {
const symbolDisplayNodeBuilderFlags = NodeBuilderFlags.OmitParameterModifiers | NodeBuilderFlags.IgnoreErrors | NodeBuilderFlags.UseAliasDefinedOutsideCurrentScope;

// TODO(drosen): use contextual SemanticMeaning.
export function getSymbolKind(typeChecker: TypeChecker, symbol: Symbol, location: Node): ScriptElementKind {
const result = getSymbolKindOfConstructorPropertyMethodAccessorFunctionOrVar(typeChecker, symbol, location);
Expand Down Expand Up @@ -471,7 +473,7 @@ namespace ts.SymbolDisplay {
// If the type is type parameter, format it specially
if (type.symbol && type.symbol.flags & SymbolFlags.TypeParameter) {
const typeParameterParts = mapToDisplayParts(writer => {
const param = typeChecker.typeParameterToDeclaration(type as TypeParameter, enclosingDeclaration)!;
const param = typeChecker.typeParameterToDeclaration(type as TypeParameter, enclosingDeclaration, symbolDisplayNodeBuilderFlags)!;
getPrinter().writeNode(EmitHint.Unspecified, param, getSourceFileOfNode(getParseTreeNode(enclosingDeclaration)), writer);
});
addRange(displayParts, typeParameterParts);
Expand Down Expand Up @@ -631,7 +633,7 @@ namespace ts.SymbolDisplay {

function writeTypeParametersOfSymbol(symbol: Symbol, enclosingDeclaration: Node | undefined) {
const typeParameterParts = mapToDisplayParts(writer => {
const params = typeChecker.symbolToTypeParameterDeclarations(symbol, enclosingDeclaration);
const params = typeChecker.symbolToTypeParameterDeclarations(symbol, enclosingDeclaration, symbolDisplayNodeBuilderFlags);
getPrinter().writeList(ListFormat.TypeParameters, params, getSourceFileOfNode(getParseTreeNode(enclosingDeclaration)), writer);
});
addRange(displayParts, typeParameterParts);
Expand Down
16 changes: 8 additions & 8 deletions tests/baselines/reference/api/tsserverlibrary.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2040,23 +2040,23 @@ declare namespace ts {
getNonNullableType(type: Type): Type;
getTypeArguments(type: TypeReference): readonly Type[];
/** Note that the resulting nodes cannot be checked. */
typeToTypeNode(type: Type, enclosingDeclaration?: Node, flags?: NodeBuilderFlags): TypeNode | undefined;
typeToTypeNode(type: Type, enclosingDeclaration: Node | undefined, flags: NodeBuilderFlags | undefined): TypeNode | undefined;
Copy link
Member

Choose a reason for hiding this comment

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

This would be a break if we want it for 3.9. Any way we can avoid it?

Copy link
Member Author

@weswigham weswigham Apr 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Easily - now that I've used it to audit our internal usages, swapping it back is pretty simple; changing it is just for safety at callsites (so we may still want it for the 4.0 branch, where we may eventually add more).

/** Note that the resulting nodes cannot be checked. */
signatureToSignatureDeclaration(signature: Signature, kind: SyntaxKind, enclosingDeclaration?: Node, flags?: NodeBuilderFlags): (SignatureDeclaration & {
signatureToSignatureDeclaration(signature: Signature, kind: SyntaxKind, enclosingDeclaration: Node | undefined, flags: NodeBuilderFlags | undefined): (SignatureDeclaration & {
typeArguments?: NodeArray<TypeNode>;
}) | undefined;
/** Note that the resulting nodes cannot be checked. */
indexInfoToIndexSignatureDeclaration(indexInfo: IndexInfo, kind: IndexKind, enclosingDeclaration?: Node, flags?: NodeBuilderFlags): IndexSignatureDeclaration | undefined;
indexInfoToIndexSignatureDeclaration(indexInfo: IndexInfo, kind: IndexKind, enclosingDeclaration: Node | undefined, flags: NodeBuilderFlags | undefined): IndexSignatureDeclaration | undefined;
/** Note that the resulting nodes cannot be checked. */
symbolToEntityName(symbol: Symbol, meaning: SymbolFlags, enclosingDeclaration?: Node, flags?: NodeBuilderFlags): EntityName | undefined;
symbolToEntityName(symbol: Symbol, meaning: SymbolFlags, enclosingDeclaration: Node | undefined, flags: NodeBuilderFlags | undefined): EntityName | undefined;
/** Note that the resulting nodes cannot be checked. */
symbolToExpression(symbol: Symbol, meaning: SymbolFlags, enclosingDeclaration?: Node, flags?: NodeBuilderFlags): Expression | undefined;
symbolToExpression(symbol: Symbol, meaning: SymbolFlags, enclosingDeclaration: Node | undefined, flags: NodeBuilderFlags | undefined): Expression | undefined;
/** Note that the resulting nodes cannot be checked. */
symbolToTypeParameterDeclarations(symbol: Symbol, enclosingDeclaration?: Node, flags?: NodeBuilderFlags): NodeArray<TypeParameterDeclaration> | undefined;
symbolToTypeParameterDeclarations(symbol: Symbol, enclosingDeclaration: Node | undefined, flags: NodeBuilderFlags | undefined): NodeArray<TypeParameterDeclaration> | undefined;
/** Note that the resulting nodes cannot be checked. */
symbolToParameterDeclaration(symbol: Symbol, enclosingDeclaration?: Node, flags?: NodeBuilderFlags): ParameterDeclaration | undefined;
symbolToParameterDeclaration(symbol: Symbol, enclosingDeclaration: Node | undefined, flags: NodeBuilderFlags | undefined): ParameterDeclaration | undefined;
/** Note that the resulting nodes cannot be checked. */
typeParameterToDeclaration(parameter: TypeParameter, enclosingDeclaration?: Node, flags?: NodeBuilderFlags): TypeParameterDeclaration | undefined;
typeParameterToDeclaration(parameter: TypeParameter, enclosingDeclaration: Node | undefined, flags: NodeBuilderFlags | undefined): TypeParameterDeclaration | undefined;
getSymbolsInScope(location: Node, meaning: SymbolFlags): Symbol[];
getSymbolAtLocation(node: Node): Symbol | undefined;
getSymbolsOfParameterPropertyDeclaration(parameter: ParameterDeclaration, parameterName: string): Symbol[];
Expand Down
16 changes: 8 additions & 8 deletions tests/baselines/reference/api/typescript.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2040,23 +2040,23 @@ declare namespace ts {
getNonNullableType(type: Type): Type;
getTypeArguments(type: TypeReference): readonly Type[];
/** Note that the resulting nodes cannot be checked. */
typeToTypeNode(type: Type, enclosingDeclaration?: Node, flags?: NodeBuilderFlags): TypeNode | undefined;
typeToTypeNode(type: Type, enclosingDeclaration: Node | undefined, flags: NodeBuilderFlags | undefined): TypeNode | undefined;
/** Note that the resulting nodes cannot be checked. */
signatureToSignatureDeclaration(signature: Signature, kind: SyntaxKind, enclosingDeclaration?: Node, flags?: NodeBuilderFlags): (SignatureDeclaration & {
signatureToSignatureDeclaration(signature: Signature, kind: SyntaxKind, enclosingDeclaration: Node | undefined, flags: NodeBuilderFlags | undefined): (SignatureDeclaration & {
typeArguments?: NodeArray<TypeNode>;
}) | undefined;
/** Note that the resulting nodes cannot be checked. */
indexInfoToIndexSignatureDeclaration(indexInfo: IndexInfo, kind: IndexKind, enclosingDeclaration?: Node, flags?: NodeBuilderFlags): IndexSignatureDeclaration | undefined;
indexInfoToIndexSignatureDeclaration(indexInfo: IndexInfo, kind: IndexKind, enclosingDeclaration: Node | undefined, flags: NodeBuilderFlags | undefined): IndexSignatureDeclaration | undefined;
/** Note that the resulting nodes cannot be checked. */
symbolToEntityName(symbol: Symbol, meaning: SymbolFlags, enclosingDeclaration?: Node, flags?: NodeBuilderFlags): EntityName | undefined;
symbolToEntityName(symbol: Symbol, meaning: SymbolFlags, enclosingDeclaration: Node | undefined, flags: NodeBuilderFlags | undefined): EntityName | undefined;
/** Note that the resulting nodes cannot be checked. */
symbolToExpression(symbol: Symbol, meaning: SymbolFlags, enclosingDeclaration?: Node, flags?: NodeBuilderFlags): Expression | undefined;
symbolToExpression(symbol: Symbol, meaning: SymbolFlags, enclosingDeclaration: Node | undefined, flags: NodeBuilderFlags | undefined): Expression | undefined;
/** Note that the resulting nodes cannot be checked. */
symbolToTypeParameterDeclarations(symbol: Symbol, enclosingDeclaration?: Node, flags?: NodeBuilderFlags): NodeArray<TypeParameterDeclaration> | undefined;
symbolToTypeParameterDeclarations(symbol: Symbol, enclosingDeclaration: Node | undefined, flags: NodeBuilderFlags | undefined): NodeArray<TypeParameterDeclaration> | undefined;
/** Note that the resulting nodes cannot be checked. */
symbolToParameterDeclaration(symbol: Symbol, enclosingDeclaration?: Node, flags?: NodeBuilderFlags): ParameterDeclaration | undefined;
symbolToParameterDeclaration(symbol: Symbol, enclosingDeclaration: Node | undefined, flags: NodeBuilderFlags | undefined): ParameterDeclaration | undefined;
/** Note that the resulting nodes cannot be checked. */
typeParameterToDeclaration(parameter: TypeParameter, enclosingDeclaration?: Node, flags?: NodeBuilderFlags): TypeParameterDeclaration | undefined;
typeParameterToDeclaration(parameter: TypeParameter, enclosingDeclaration: Node | undefined, flags: NodeBuilderFlags | undefined): TypeParameterDeclaration | undefined;
getSymbolsInScope(location: Node, meaning: SymbolFlags): Symbol[];
getSymbolAtLocation(node: Node): Symbol | undefined;
getSymbolsOfParameterPropertyDeclaration(parameter: ParameterDeclaration, parameterName: string): Symbol[];
Expand Down
Loading