Skip to main content
Josh Goldberg
Book Blog Projects Speaking

TypeScript Contribution Diary: Type Parameters in the "Add Missing Function" Codefix

Aug 9, 202225 minute read

Corrects the missing function codefix to add type parameters when the function needs to be generic.

I recently had the pleasure of joining Dan Jutan’s Breaking Down the Web series of Twitch streams to talk about TypeScript. You can find the recording on YouTube (main content start at 4:36). At 1:29:37, Dan used TypeScript’s Add Missing Function TypeScript codefix to add a missing runEvilSideEffect in the following code…

function returnSelf<T>(self: T) {
	if (typeof self === "number") {
		runEvilSideEffect(self);
		// ^ Codefix: Add missing function declaration 'runEvilSideEffect'
	}
}

… and the function TypeScript added came with a type error! 😱

function runEvilSideEffect(self: T & number) {
	//                           ~ Cannot find name 'T'.
	throw new Error("Function not implemented.");
}

That T came from the returnSelf<T> type parameter, but the newly declared runEvilSideEffect function didn’t declare its own T.

Dan was kind enough to file issue #49693: Add Missing Function Declaration: Does Not Correctly Declare Generic Function on TypeScript to report the bug.

I sent in PR #49727: Account for type parameters in missing function codefix a couple days later to fix the issue. Let’s dig into how that PR works!

Finding the Codefix

TypeScript stores the various codefixes that can be applied in a src/services/codefixes/ directory. But, quick filename searches for terms like “add function” or “missing” in that directory didn’t show anything that seemed related. Which codefix was being applied in the first place?

TypeScript stores all messages that get shown to users -include codefix labels- in its src/compiler/diagnosticMessages.json file. They get transformed from text like ”Add missing function declaration '...'” to runtime values like Diagnostics.add_missing_function_declaration_0. I ran a search for that runtime value and saw a result in src/services/codefixes/fixAddMissingMember.ts.

if (info.kind === InfoKind.Function) {
	const changes = textChanges.ChangeTracker.with(context, (t) =>
		addFunctionDeclaration(t, context, info),
	);
	return [
		createCodeFixAction(
			fixMissingFunctionDeclaration,
			changes,
			[Diagnostics.Add_missing_function_declaration_0, info.token.text],
			fixMissingFunctionDeclaration,
			Diagnostics.Add_all_missing_function_declarations,
		),
	];
}

Found it!

Aside: Debugging TypeScript

You can refer to Andrew Branch’s excellent “Debugging the TypeScript Codebase” post as a reference for getting the TypeScript repo cloned and in a debuggable state locally.

Aside: Terminology Recap

Before we dive into the function, I should explain the terminology I’m going to use:

Within those two terms:

For example, in the following code snippet…

function withGenericValue<Value>(value: Value) {}

withGenericValue<string>("");

The distinction is important when discussing logic around function calls and declarations.

Exploring the Codefix

Out of the two functions called in the earlier TypeScript source snippet:

addFunctionDeclaration was almost certainly the right place to go. It was calling to a createSignatureDeclarationFromCallExpression function, declared in src/services/codefixes/helpers.ts.

That function is a little hefty -lots of calls to other functions- so it took me a little while to squint through it. I put a debugger breakpoint in it and looked at the values at runtime. The main points that seemed relevant were:

  1. types: calling the typeToAutoImportableTypeNode function to create an array of TypeNodes to be inserted in the new function as parameters

    • Looking through typeToAutoImportableTypeNode, it included logic to handle import("...")... nodes for types declared in other files

  2. typeParameters: for each explicit type argument in the call, creating a type parameter node to be inserted in the new function

    • Example input: the T in myFunction<T>()
    • Example output: the <T> in function myFunction<T>() {}

  3. parameters: for each argument in the call (e.g. the value in myFunction(value), creating a parameter node to be inserted in the new function

    • Example input: the value in myFunction(value)
    • Example output: the value: string in function myFunction(value: string) {}

At this point I still didn’t fully understand how createSignatureDeclarationFromCallExpression worked, but could generally see how the parts I cared about came together:

  1. types was generated by retrieving the type of each argument
  2. typeParameters was generated by copying each explicit type argument
  3. parameters was generated by creating a parameter node for each argument node and its type in types
  4. The function combined all that information to print out a new function

That explicit type argument mention in typeParameters was key to the bug we’d seen. TypeScript was generating type parameters only for explicit type arguments. If the type of an argument was generic, TypeScript wouldn’t know to create a new type parameter for it.

Fixing the Codefix

Equipped with a barely-workable understanding of the original code, my goal was to make the code:

  1. Understand when the type of an argument included an existing generic parameter type
  2. Add any of those type arguments to the list of created type parameters

Detecting Existing Generics

The existing logic for types called checker.getTypeAtLocation on each argument to get its type, and immediately passed that type to typeToAutoImportableTypeNode. I needed the code to additionally keep track of which of those types referenced a type argument.

I split out the first bit of retrieving types into a new instanceTypes variable:

const instanceTypes = isJs
	? []
	: map(args, (arg) => checker.getTypeAtLocation(arg));

…then created a new getArgumentTypesAndTypeParameters function to return two values:

const { argumentTypeNodes, argumentTypeParameters } =
	getArgumentTypesAndTypeParameters(
		checker,
		importAdder,
		instanceTypes,
		// (pass-through parameters omitted for brevity)
	);

I used a Set<string> internally for the argument type parameters, to de-duplicate their names in case two arguments referred to the same type parameter name.

export function getArgumentTypesAndTypeParameters(
	checker: TypeChecker,
	importAdder: ImportAdder,
	instanceTypes: Type[],
	// (pass-through parameters omitted for brevity)
) {
	const argumentTypeNodes: TypeNode[] = [];
	const argumentTypeParameters = new Set<string>();

	for (const instanceType of instanceTypes) {
		const argumentTypeNode = typeToAutoImportableTypeNode(
			checker,
			importAdder,
			instanceType,
			// (pass-through arguments omitted for brevity)
		);
		if (!argumentTypeNode) {
			continue;
		}

		argumentTypeNodes.push(argumentTypeNode);
		const argumentTypeParameter = getFirstTypeParameterName(instanceType);

		if (argumentTypeParameter) {
			argumentTypeParameters.add(argumentTypeParameter);
		}
	}

	return {
		argumentTypeNodes,
		argumentTypeParameters: arrayFrom(argumentTypeParameters.values()),
	};
}

Getting Type Parameter Names

I also wrote a getFirstTypeParameterName function meant to determine if a type involved a type parameter, and return the name of that type parameter if so. Given a type: Type, it set up three cases:

You can think of a type’s symbol as TypeScript’s deeper understanding of a type. A symbol is TypeScript’s storage for what names are declared by various things. For example, if a type indicates a value is an instance of a class, the backing symbol would contain class information such as lists of class members and constructor signatures.

function getFirstTypeParameterName(type: Type): string | undefined {
	if (type.flags & TypeFlags.Intersection) {
		for (const subType of (type as IntersectionType).types) {
			const subTypeName = getFirstTypeParameterName(subType);
			if (subTypeName) {
				return subTypeName;
			}
		}
	}

	return type.flags & TypeFlags.TypeParameter
		? type.getSymbol()?.getName()
		: undefined;
}

At this point, I had instanceTypes, argumentTypeNodes, and argumentTypeParameters: all the information needed to correct the printing of the new function! 🚀

Printing Those Generics

The previous logic to create new type arguments directly mapped typeArguments to type parameter declarations. The names of type parameters started alphabetically at T, U, etc. until the end of the alphabet, at which point they switched to T1, T2, etc.

const typeParameters =
	isJs || typeArguments === undefined
		? undefined
		: map(typeArguments, (_, i) =>
				factory.createTypeParameterDeclaration(
					/*modifiers*/ undefined,
					CharacterCodes.T + typeArguments.length - 1 <= CharacterCodes.Z
						? String.fromCharCode(CharacterCodes.T + i)
						: `T${i}`,
				),
			);

My new logic would need to account for the new argumentTypeParameters I’d created as well as the existing typeArguments. The logic for that ended up being a little tricky:

I created a usedNames Set<string> to keep track of previously taken names. When typeArguments was copied over in a loop, I also created a targetSize loop bound for how large usedNames would have to get too.

Here’s what the function looked like at commit time:

function createTypeParametersForArguments(
	argumentTypeParameters: string[],
	typeArguments: NodeArray<TypeNode> | undefined,
) {
	const usedNames = new Set(argumentTypeParameters);

	if (typeArguments) {
		for (
			let i = 0, targetSize = usedNames.size + typeArguments.length;
			usedNames.size < targetSize;
			i += 1
		) {
			usedNames.add(
				CharacterCodes.T + i <= CharacterCodes.Z
					? String.fromCharCode(CharacterCodes.T + i)
					: `T${i}`,
			);
		}
	}

	return map(arrayFrom(usedNames.values()), (usedName) =>
		factory.createTypeParameterDeclaration(/*modifiers*/ undefined, usedName),
	);
}

The Set trick is a nice way to guarantee string uniqueness. I was pleased with myself for finding an excuse a need to use it twice in this PR.

Initial Tests

At this point, my code seemed to be working when I tried it out locally. It was time to write some tests!

TypeScript stores many tests for codefixes and similar language service behavior in a tests/cases/fourslash/ directory. The tests set up a virtual code file in code indented by four slashes (////, hence the name), then run actions and assertions on that virtual file with the TypeScript language service.

For example, my first test:

Here’s what that test looked like:

/// <reference path='fourslash.ts' />

// @noImplicitAny: true
////function existing<T>(value: T) {
////  const keyofTypeof = Object.keys(value)[0] as keyof T;
////  added/*1*/(value);
////}

goTo.marker("1");
verify.codeFix({
	description: "Add missing function declaration 'added'",
	index: 0,
	newFileContent: `function existing<T>(value: T) {
  const keyofTypeof = Object.keys(value)[0] as keyof T;
  added(value);
}
function added<T>(value: T) {
    throw new Error("Function not implemented.");
}
`,
});

I added eight tests in total, exercising a few variations of explicit and/or implicit type arguments. The code looked about ready to send in as a pull request!

PR Review: Take One

A pull request review came in a couple weeks after the PR. Most of the comments were pointing out small changes. A few did point out real gaps in the added or changed logic. Summarizing those gaps here:

Handling Unions in getFirstTypeParameterName

This one wasn’t too bad to adjust for. I added TypeFlags.Union | and UnionType | to the bitwise check and type, respectively:

-if (type.flags & TypeFlags.Intersection) {
+if (type.flags & (TypeFlags.Union | TypeFlags.Intersection)) {
-    for (const subType of (type as IntersectionType).types) {
+    for (const subType of (type as UnionType | IntersectionType).types) {

Supporting type parameter constraints

Type parameter constraints are the extends clause on a type parameter that limit it to only types assignable to the constraint. For example, in the following function declaration, the Input type parameter has a string constraint:

function onlyStrings<Input extends string>(input: Input) {}

Constraints support was a bit tricky. createTypeParametersForArguments would need to provide each parameter’s constraints to factory.createTypeParameterDeclaration. That meant I’d need to store not just parameter names, but also their constraint.

Most of the changes went inside getArgumentTypesAndTypeParameters in the PR, along with a horde of comments I needed to add so I could keep track of what all the new variables did. After much tinkering with constraints and types, the main changes were:

Over in createTypeParametersForArguments, its argumentTypeParameters: string[] parameter needed to also attach that ArgumentTypeParameterAndConstraint for each name. A [string, ArgumentTypeParameterAndConstraint | undefined][] parameter type was the quickest way to have each element in the array contain both a name and the optional extra info.

function createTypeParametersForArguments(
+    checker: TypeChecker,
-    argumentTypeParameters: string[],
+    argumentTypeParameters: [
+        string,
+        ArgumentTypeParameterAndConstraint | undefined
+    ][],
    typeArguments: NodeArray<TypeNode> | undefined
) {

Adding the type checker parameter was also necessary in order to use those types. When computing how many new type names to parameter add (usedNames), the code was able to only count typeArguments that aren’t the same computed argument type as any of the argumentTypeParameters [Line 361].

Doing so prevents the codefix from creating unused type parameters that act as duplicates of previous type parameters. For example, on code like added/*1*/<T>(value, value); (incompleteFunctionCallCodefixTypeParameterArgumentSame.ts):

const typeArgumentsWithNewTypes = typeArguments.filter(
	(typeArgument) =>
		!argumentTypeParameters.some(
			(pair) =>
				checker.getTypeAtLocation(typeArgument) === pair[1]?.argumentType,
		),
);
const targetSize = usedNames.size + typeArgumentsWithNewTypes.length;
for (let i = 0; usedNames.size < targetSize; i += 1) {
	usedNames.add(createTypeParameterName(i));
}

With these edge cases seemingly working, I pushed it all up, merged upstream changes from the main branch, and re-requested review…

PR Review: Take Two

…and it was accepted! 🎉

This codefix improvement is now available in the TypeScript nightlies. It will be available in stable versions of TypeScript >=4.8.

TypeScript will now be able to add missing functions without introducing type errors, even in the presence of type parameters:

function returnSelf<T>(self: T) {
	if (typeof self === "number") {
		runEvilSideEffect(self);
		// ^ Codefix: Add missing function declaration 'runEvilSideEffect'
	}
}

function runEvilSideEffect<T>(self: T) {
	throw new Error("Function not implemented.");
}

Wonderful! 🥳

P.S. Declined Refactors

I also spotted and attempted a couple of refactors as a result of this PR. Neither of them ended up making their way into TypeScript, but I still think they’re interesting and relevant enough to bring up.

One Teeny Refactor

I noticed a potential for refactor while preparing the PR: typeToAutoImportableTypeNode was also called in a different codefix in extractSymbol.ts. Both locations had code like:

// Widen the type so we don't emit nonsense annotations like "function fn(x: 3) {"
type = checker.getBaseTypeOfLiteralType(type);

I figured a small cleanup to move that duplicated code inside typeToAutoImportableTypeNode would help reduce lines of code.

export function typeToAutoImportableTypeNode(
    checker: TypeChecker,
    importAdder: ImportAdder,
-    type: Type,
+    instanceType: Type,
    /* (more parameters omitted for brevity) */
): TypeNode | undefined {
+    // Widen the type so we don't emit nonsense annotations like "function fn(x: 3) {"
+    const type = checker.getBaseTypeOfLiteralType(instanceType);

That change made it into the first few commits in the PR. Unfortunately, I had to revert it in 977328 after b1d35a3’s merge from main. A separate PR, #50004: fix(49964): Incorrect code generated by “add extends constraint” fix for multiple usages of the same type parameter, happened to add another call to typeToAutoImportableTypeNode that did not widen types with checker.getBaseTypeOfLiteralType.

Ah well. 🤷

Many Teeny, Repeated Refactors

These two lines of code from getFirstTypeParameterName irked me:

if (type.flags & TypeFlags.Intersection) {
    for (const subType of (type as IntersectionType).types) {

Type assertions in TypeScript are often a sign that the type system isn’t being told everything it needs to understand code. In this case, it didn’t understand that type.flags & TypeFlags.Intersection meant the type is an IntersectionType.

I’d seen similar patterns elsewhere in the TypeScript codebase. Refactoring all those cases would have been far too big a change for my little PR.

I instead…

  1. Filed a comment in the PR asking about type assertions
  2. Filed issue #500005: Code cleanup: isIntersectionType & similar internally after Nathan confirmed it seemed like a good idea
  3. Sent PR #50010: Added some Type type predicates internally with the proposed changes applied to a few hundred locations
  4. Closed the PR after performance testing indicated it caused a ~3% performance loss

Ah well. Sometimes you refactor the code, and sometimes limitations in V8 runtime optimization mean the existing code plays better with startup execution and/or JIT optimization. 🤷

Wrapping Up

This PR involved a lot of tricky logic around comparing and counting arguments and parameters. But with help from the TypeScript team and a lot of time spent tweaking the details, TypeScript’s missing function codefix is now better equipped to handle generic functions. Hooray!

Thanks again to…

We did it! 🙌

Liked this post? Thanks! Let the world know: