Skip to content

[ID-Prep] PR3 - Preserve parameter types for optional parameters /fields with undefined in type and for required params with default value #57484

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
12 changes: 10 additions & 2 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -48219,6 +48219,15 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
return false;
}

function declaredParameterTypeContainsUndefined(parameter: ParameterDeclaration) {
if (!parameter.type) return false;
const type = getTypeFromTypeNode(parameter.type);
return containsUndefinedType(type);
}
function requiresAddingImplicitUndefined(parameter: ParameterDeclaration) {
return (isRequiredInitializedParameter(parameter) || isOptionalUninitializedParameterProperty(parameter)) && !declaredParameterTypeContainsUndefined(parameter);
}

function isRequiredInitializedParameter(parameter: ParameterDeclaration | JSDocParameterTag): boolean {
return !!strictNullChecks &&
!isOptionalParameter(parameter) &&
Expand Down Expand Up @@ -48612,8 +48621,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
isTopLevelValueImportEqualsWithEntityName,
isDeclarationVisible,
isImplementationOfOverload,
isRequiredInitializedParameter,
isOptionalUninitializedParameterProperty,
requiresAddingImplicitUndefined,
isExpandoFunctionDeclaration,
getPropertiesOfContainerFunction,
createTypeOfDeclaration,
Expand Down
3 changes: 1 addition & 2 deletions src/compiler/emitter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1161,8 +1161,7 @@ export const notImplementedResolver: EmitResolver = {
isLateBound: (_node): _node is LateBoundDeclaration => false,
collectLinkedAliases: notImplemented,
isImplementationOfOverload: notImplemented,
isRequiredInitializedParameter: notImplemented,
isOptionalUninitializedParameterProperty: notImplemented,
requiresAddingImplicitUndefined: notImplemented,
isExpandoFunctionDeclaration: notImplemented,
getPropertiesOfContainerFunction: notImplemented,
createTypeOfDeclaration: notImplemented,
Expand Down
57 changes: 26 additions & 31 deletions src/compiler/transformers/declarations.ts
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,6 @@ import {
isModuleDeclaration,
isOmittedExpression,
isPrivateIdentifier,
isPropertySignature,
isSemicolonClassElement,
isSetAccessorDeclaration,
isSourceFile,
Expand Down Expand Up @@ -722,7 +721,6 @@ export function transformDeclarations(context: TransformationContext) {
| FunctionDeclaration
| MethodDeclaration
| GetAccessorDeclaration
| SetAccessorDeclaration
| BindingElement
| ConstructSignatureDeclaration
| VariableDeclaration
Expand All @@ -741,46 +739,43 @@ export function transformDeclarations(context: TransformationContext) {
// Literal const declarations will have an initializer ensured rather than a type
return;
}
const shouldUseResolverType = node.kind === SyntaxKind.Parameter &&
(resolver.isRequiredInitializedParameter(node) ||
resolver.isOptionalUninitializedParameterProperty(node));
if (type && !shouldUseResolverType) {
const shouldAddImplicitUndefined = node.kind === SyntaxKind.Parameter && resolver.requiresAddingImplicitUndefined(node);
if (type && !shouldAddImplicitUndefined) {
return visitNode(type, visitDeclarationSubtree, isTypeNode);
}
if (!getParseTreeNode(node)) {
return type ? visitNode(type, visitDeclarationSubtree, isTypeNode) : factory.createKeywordTypeNode(SyntaxKind.AnyKeyword);
}
if (node.kind === SyntaxKind.SetAccessor) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ensureType is never called with a SetAccessorDeclaration. The set accessor type does not have a return type and the parameter type is inferred independently. Removing this had 0 impact on tests.

// Set accessors with no associated type node (from it's param or get accessor return) are `any` since they are never contextually typed right now
// (The inferred type here will be void, but the old declaration emitter printed `any`, so this replicates that)
return factory.createKeywordTypeNode(SyntaxKind.AnyKeyword);
}

errorNameNode = node.name;
let oldDiag: typeof getSymbolAccessibilityDiagnostic;
if (!suppressNewDiagnosticContexts) {
oldDiag = getSymbolAccessibilityDiagnostic;
getSymbolAccessibilityDiagnostic = createGetSymbolAccessibilityDiagnosticForNode(node);
}
if (node.kind === SyntaxKind.VariableDeclaration || node.kind === SyntaxKind.BindingElement) {
return cleanup(resolver.createTypeOfDeclaration(node, enclosingDeclaration, declarationEmitNodeBuilderFlags, symbolTracker));
}
if (
node.kind === SyntaxKind.Parameter
|| node.kind === SyntaxKind.PropertyDeclaration
|| node.kind === SyntaxKind.PropertySignature
) {
if (isPropertySignature(node) || !node.initializer) return cleanup(resolver.createTypeOfDeclaration(node, enclosingDeclaration, declarationEmitNodeBuilderFlags, symbolTracker, shouldUseResolverType));
return cleanup(resolver.createTypeOfDeclaration(node, enclosingDeclaration, declarationEmitNodeBuilderFlags, symbolTracker, shouldUseResolverType) || resolver.createTypeOfExpression(node.initializer, enclosingDeclaration, declarationEmitNodeBuilderFlags, symbolTracker));
Comment on lines -772 to -773
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The logic here is overly complicated. Both calls to createTypeOfDeclaration boil down to the same arguments. Also I have not found a test that benefits from the use of createTypeOfExpression on the initializer and I can't think of a case when this would help. (There might be some error case, but it's not tested for and returning any for that seems just as appropriate.

let typeNode;
switch (node.kind) {
case SyntaxKind.Parameter:
case SyntaxKind.PropertySignature:
case SyntaxKind.PropertyDeclaration:
case SyntaxKind.BindingElement:
case SyntaxKind.VariableDeclaration:
typeNode = resolver.createTypeOfDeclaration(node, enclosingDeclaration, declarationEmitNodeBuilderFlags, symbolTracker, shouldAddImplicitUndefined);
break;
case SyntaxKind.FunctionDeclaration:
case SyntaxKind.ConstructSignature:
case SyntaxKind.MethodSignature:
case SyntaxKind.MethodDeclaration:
case SyntaxKind.GetAccessor:
case SyntaxKind.CallSignature:
typeNode = resolver.createReturnTypeOfSignatureDeclaration(node, enclosingDeclaration, declarationEmitNodeBuilderFlags, symbolTracker);
break;
default:
Debug.assertNever(node);
}
return cleanup(resolver.createReturnTypeOfSignatureDeclaration(node, enclosingDeclaration, declarationEmitNodeBuilderFlags, symbolTracker));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the cleanup closure. Since the logic is simpler now it's easier to just assign a variable and do the same cleanup after the switch.


function cleanup(returnValue: TypeNode | undefined) {
errorNameNode = undefined;
if (!suppressNewDiagnosticContexts) {
getSymbolAccessibilityDiagnostic = oldDiag;
}
return returnValue || factory.createKeywordTypeNode(SyntaxKind.AnyKeyword);
errorNameNode = undefined;
if (!suppressNewDiagnosticContexts) {
getSymbolAccessibilityDiagnostic = oldDiag!;
}
return typeNode ?? factory.createKeywordTypeNode(SyntaxKind.AnyKeyword);
}

function isDeclarationAndNotVisible(node: NamedDeclaration) {
Expand Down
3 changes: 1 addition & 2 deletions src/compiler/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5699,8 +5699,7 @@ export interface EmitResolver {
isLateBound(node: Declaration): node is LateBoundDeclaration;
collectLinkedAliases(node: Identifier, setVisibility?: boolean): Node[] | undefined;
isImplementationOfOverload(node: SignatureDeclaration): boolean | undefined;
isRequiredInitializedParameter(node: ParameterDeclaration): boolean;
isOptionalUninitializedParameterProperty(node: ParameterDeclaration): boolean;
requiresAddingImplicitUndefined(node: ParameterDeclaration): boolean;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

isRequiredInitializedParameter and isOptionalUninitializedParameterProperty were used only once in the declaration transform in (resolver.isRequiredInitializedParameter(node) || resolver.isOptionalUninitializedParameterProperty(node)). I think it's worth simplifying the API and only expose a function taht tells us is the parameter requires adding an implcit undefined to it's type or not.

requiresAddingImplicitUndefined also takes into account if the type already contains undefined and thus avoids re-printing types unless it's absolutely necessary. (Fixing #57483)

isExpandoFunctionDeclaration(node: FunctionDeclaration): boolean;
getPropertiesOfContainerFunction(node: Declaration): Symbol[];
createTypeOfDeclaration(declaration: AccessorDeclaration | VariableLikeDeclaration | PropertyAccessExpression | ElementAccessExpression | BinaryExpression, enclosingDeclaration: Node, flags: NodeBuilderFlags, tracker: SymbolTracker, addUndefined?: boolean): TypeNode | undefined;
Expand Down
74 changes: 74 additions & 0 deletions tests/baselines/reference/verbatim-declarations-parameters.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
//// [tests/cases/compiler/verbatim-declarations-parameters.ts] ////

//// [verbatim-declarations-parameters.ts]
type Map = {} & { [P in string]: any }
type MapOrUndefined = Map | undefined | "dummy"
export class Foo {
constructor(
// Type node is accurate, preserve
public reuseTypeNode?: Map | undefined,
public reuseTypeNode2?: Exclude<MapOrUndefined, "dummy">,
// Resolve type node, requires adding | undefined
public resolveType?: Map,
) { }
}

export function foo1(
// Type node is accurate, preserve
reuseTypeNode: Map | undefined = {},
reuseTypeNode2: Exclude<MapOrUndefined, "dummy"> = {},
// Resolve type node, requires adding | undefined
resolveType: Map = {},
requiredParam: number) {

}


//// [verbatim-declarations-parameters.js]
"use strict";
Object.defineProperty(exports, "__esModule", { value: true });
exports.foo1 = exports.Foo = void 0;
var Foo = /** @class */ (function () {
function Foo(
// Type node is accurate, preserve
reuseTypeNode, reuseTypeNode2,
// Resolve type node, requires adding | undefined
resolveType) {
this.reuseTypeNode = reuseTypeNode;
this.reuseTypeNode2 = reuseTypeNode2;
this.resolveType = resolveType;
}
return Foo;
}());
exports.Foo = Foo;
function foo1(
// Type node is accurate, preserve
reuseTypeNode, reuseTypeNode2,
// Resolve type node, requires adding | undefined
resolveType, requiredParam) {
if (reuseTypeNode === void 0) { reuseTypeNode = {}; }
if (reuseTypeNode2 === void 0) { reuseTypeNode2 = {}; }
if (resolveType === void 0) { resolveType = {}; }
}
exports.foo1 = foo1;


//// [verbatim-declarations-parameters.d.ts]
type Map = {} & {
[P in string]: any;
};
type MapOrUndefined = Map | undefined | "dummy";
export declare class Foo {
reuseTypeNode?: Map | undefined;
reuseTypeNode2?: Exclude<MapOrUndefined, "dummy">;
resolveType?: {
[x: string]: any;
} | undefined;
constructor(reuseTypeNode?: Map | undefined, reuseTypeNode2?: Exclude<MapOrUndefined, "dummy">, resolveType?: {
[x: string]: any;
} | undefined);
}
export declare function foo1(reuseTypeNode: Map | undefined, reuseTypeNode2: Exclude<MapOrUndefined, "dummy">, resolveType: {
[x: string]: any;
} | undefined, requiredParam: number): void;
export {};
56 changes: 56 additions & 0 deletions tests/baselines/reference/verbatim-declarations-parameters.symbols
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
//// [tests/cases/compiler/verbatim-declarations-parameters.ts] ////

=== verbatim-declarations-parameters.ts ===
type Map = {} & { [P in string]: any }
>Map : Symbol(Map, Decl(verbatim-declarations-parameters.ts, 0, 0))
>P : Symbol(P, Decl(verbatim-declarations-parameters.ts, 0, 19))

type MapOrUndefined = Map | undefined | "dummy"
>MapOrUndefined : Symbol(MapOrUndefined, Decl(verbatim-declarations-parameters.ts, 0, 38))
>Map : Symbol(Map, Decl(verbatim-declarations-parameters.ts, 0, 0))

export class Foo {
>Foo : Symbol(Foo, Decl(verbatim-declarations-parameters.ts, 1, 47))

constructor(
// Type node is accurate, preserve
public reuseTypeNode?: Map | undefined,
>reuseTypeNode : Symbol(Foo.reuseTypeNode, Decl(verbatim-declarations-parameters.ts, 3, 14))
>Map : Symbol(Map, Decl(verbatim-declarations-parameters.ts, 0, 0))

public reuseTypeNode2?: Exclude<MapOrUndefined, "dummy">,
>reuseTypeNode2 : Symbol(Foo.reuseTypeNode2, Decl(verbatim-declarations-parameters.ts, 5, 43))
>Exclude : Symbol(Exclude, Decl(lib.es5.d.ts, --, --))
>MapOrUndefined : Symbol(MapOrUndefined, Decl(verbatim-declarations-parameters.ts, 0, 38))

// Resolve type node, requires adding | undefined
public resolveType?: Map,
>resolveType : Symbol(Foo.resolveType, Decl(verbatim-declarations-parameters.ts, 6, 61))
>Map : Symbol(Map, Decl(verbatim-declarations-parameters.ts, 0, 0))

) { }
}

export function foo1(
>foo1 : Symbol(foo1, Decl(verbatim-declarations-parameters.ts, 10, 1))

// Type node is accurate, preserve
reuseTypeNode: Map | undefined = {},
>reuseTypeNode : Symbol(reuseTypeNode, Decl(verbatim-declarations-parameters.ts, 12, 21))
>Map : Symbol(Map, Decl(verbatim-declarations-parameters.ts, 0, 0))

reuseTypeNode2: Exclude<MapOrUndefined, "dummy"> = {},
>reuseTypeNode2 : Symbol(reuseTypeNode2, Decl(verbatim-declarations-parameters.ts, 14, 40))
>Exclude : Symbol(Exclude, Decl(lib.es5.d.ts, --, --))
>MapOrUndefined : Symbol(MapOrUndefined, Decl(verbatim-declarations-parameters.ts, 0, 38))

// Resolve type node, requires adding | undefined
resolveType: Map = {},
>resolveType : Symbol(resolveType, Decl(verbatim-declarations-parameters.ts, 15, 59))
>Map : Symbol(Map, Decl(verbatim-declarations-parameters.ts, 0, 0))

requiredParam: number) {
>requiredParam : Symbol(requiredParam, Decl(verbatim-declarations-parameters.ts, 17, 26))

}

49 changes: 49 additions & 0 deletions tests/baselines/reference/verbatim-declarations-parameters.types
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
//// [tests/cases/compiler/verbatim-declarations-parameters.ts] ////

=== verbatim-declarations-parameters.ts ===
type Map = {} & { [P in string]: any }
>Map : { [x: string]: any; }

type MapOrUndefined = Map | undefined | "dummy"
>MapOrUndefined : { [x: string]: any; } | "dummy" | undefined

export class Foo {
>Foo : Foo

constructor(
// Type node is accurate, preserve
public reuseTypeNode?: Map | undefined,
>reuseTypeNode : { [x: string]: any; } | undefined

public reuseTypeNode2?: Exclude<MapOrUndefined, "dummy">,
>reuseTypeNode2 : { [x: string]: any; } | undefined

// Resolve type node, requires adding | undefined
public resolveType?: Map,
>resolveType : { [x: string]: any; } | undefined

) { }
}

export function foo1(
>foo1 : (reuseTypeNode: Map | undefined, reuseTypeNode2: Exclude<MapOrUndefined, "dummy">, resolveType: { [x: string]: any; } | undefined, requiredParam: number) => void

// Type node is accurate, preserve
reuseTypeNode: Map | undefined = {},
>reuseTypeNode : { [x: string]: any; } | undefined
>{} : {}

reuseTypeNode2: Exclude<MapOrUndefined, "dummy"> = {},
>reuseTypeNode2 : { [x: string]: any; } | undefined
>{} : {}

// Resolve type node, requires adding | undefined
resolveType: Map = {},
>resolveType : { [x: string]: any; }
>{} : {}

requiredParam: number) {
>requiredParam : number

}

24 changes: 24 additions & 0 deletions tests/cases/compiler/verbatim-declarations-parameters.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
// @strictNullChecks: true
// @declaration: true

type Map = {} & { [P in string]: any }
type MapOrUndefined = Map | undefined | "dummy"
export class Foo {
constructor(
// Type node is accurate, preserve
public reuseTypeNode?: Map | undefined,
public reuseTypeNode2?: Exclude<MapOrUndefined, "dummy">,
// Resolve type node, requires adding | undefined
public resolveType?: Map,
) { }
}

export function foo1(
// Type node is accurate, preserve
reuseTypeNode: Map | undefined = {},
reuseTypeNode2: Exclude<MapOrUndefined, "dummy"> = {},
// Resolve type node, requires adding | undefined
resolveType: Map = {},
requiredParam: number) {

}