Skip to content

Commit

Permalink
Fix handling of defaulted type arguments
Browse files Browse the repository at this point in the history
Resolves #2823
  • Loading branch information
Gerrit0 committed Jan 12, 2025
1 parent 3060d36 commit fa3077c
Show file tree
Hide file tree
Showing 10 changed files with 171 additions and 77 deletions.
8 changes: 8 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,14 @@ title: Changelog

## Unreleased

### Features

- The `@inline` tag now works in more places for generic types.

### Bug Fixes

- Fixed an issue where TypeDoc would incorrectly ignore type arguments in references, #2823.

## v0.27.6 (2024-12-26)

### Features
Expand Down
7 changes: 5 additions & 2 deletions src/lib/converter/converter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -376,11 +376,14 @@ export class Converter extends AbstractComponent<Application, ConverterEvents> {
* @returns The TypeDoc type reflection representing the given node and type.
* @internal
*/
convertType(context: Context, node: ts.TypeNode | undefined): SomeType;
convertType(context: Context, type: ts.Type, node?: ts.TypeNode): SomeType;
convertType(
context: Context,
node: ts.TypeNode | ts.Type | undefined,
typeOrNode: ts.TypeNode | ts.Type | undefined,
maybeNode?: ts.TypeNode,
): SomeType {
return convertType(context, node);
return convertType(context, typeOrNode, maybeNode);
}

/**
Expand Down
20 changes: 17 additions & 3 deletions src/lib/converter/factories/signature.ts
Original file line number Diff line number Diff line change
Expand Up @@ -100,11 +100,15 @@ export function createSignature(
} else if (declaration?.type?.kind === ts.SyntaxKind.ThisType) {
sigRef.type = new IntrinsicType("this");
} else {
let typeNode = declaration?.type;
if (typeNode && ts.isJSDocReturnTag(typeNode)) {
typeNode = typeNode.typeExpression?.type;
}

sigRef.type = context.converter.convertType(
sigRefCtx,
(declaration?.kind === ts.SyntaxKind.FunctionDeclaration &&
declaration.type) ||
signature.getReturnType(),
signature.getReturnType(),
typeNode,
);
}

Expand Down Expand Up @@ -224,11 +228,18 @@ function convertParameters(
);

let type: ts.Type | ts.TypeNode | undefined;
let typeNode: ts.TypeNode | undefined;
if (declaration) {
type = context.checker.getTypeOfSymbolAtLocation(
param,
declaration,
);

if (ts.isParameter(declaration)) {
typeNode = declaration.type;
} else {
typeNode = declaration.typeExpression?.type;
}
} else {
type = param.type;
}
Expand All @@ -239,10 +250,13 @@ function convertParameters(
declaration.type?.kind === ts.SyntaxKind.ThisType
) {
paramRefl.type = new IntrinsicType("this");
} else if (!type) {
paramRefl.type = new IntrinsicType("any");
} else {
paramRefl.type = context.converter.convertType(
context.withScope(paramRefl),
type,
typeNode,
);
}

Expand Down
35 changes: 25 additions & 10 deletions src/lib/converter/symbols.ts
Original file line number Diff line number Diff line change
Expand Up @@ -759,7 +759,7 @@ function convertProperty(
);

const declaration = symbol.getDeclarations()?.[0];
let parameterType: ts.TypeNode | undefined;
let parameterTypeNode: ts.TypeNode | undefined;

if (
declaration &&
Expand All @@ -773,19 +773,26 @@ function convertProperty(
!ts.isPropertyAccessExpression(declaration) &&
!ts.isPropertyAssignment(declaration)
) {
parameterType = declaration.type;
parameterTypeNode = declaration.type;
}
setModifiers(symbol, declaration, reflection);
} else {
setSymbolModifiers(symbol, reflection);
}
reflection.defaultValue = declaration && convertDefaultValue(declaration);

reflection.type = context.converter.convertType(
context.withScope(reflection),
(context.convertingTypeNode ? parameterType : void 0) ??
if (context.convertingTypeNode && parameterTypeNode) {
reflection.type = context.converter.convertType(
context.withScope(reflection),
parameterTypeNode,
);
} else {
reflection.type = context.converter.convertType(
context.withScope(reflection),
context.checker.getTypeOfSymbol(symbol),
);
parameterTypeNode,
);
}

if (reflection.flags.isOptional) {
reflection.type = removeUndefined(reflection.type);
Expand Down Expand Up @@ -984,10 +991,18 @@ function convertVariable(
typeNode = declaration.type;
}

reflection.type = context.converter.convertType(
context.withScope(reflection),
typeNode ?? type,
);
if (typeNode) {
reflection.type = context.converter.convertType(
context.withScope(reflection),
typeNode,
);
} else {
reflection.type = context.converter.convertType(
context.withScope(reflection),
type,
typeNode,
);
}

setModifiers(symbol, declaration, reflection);

Expand Down
68 changes: 48 additions & 20 deletions src/lib/converter/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,14 @@ export interface TypeConverter<
convert(context: Context, node: TNode): SomeType;
// We use typeToTypeNode to figure out what method to call in the first place,
// so we have a non-type-checkable node here, necessary for some converters.
convertType(context: Context, type: TType, node: TNode): SomeType;
// We *may* also have an original node which is the node the type was originally
// retrieved from.
convertType(
context: Context,
type: TType,
serializedNode: TNode,
originalNode: ts.TypeNode | undefined,
): SomeType;
}

const converters = new Map<ts.SyntaxKind, TypeConverter>();
Expand Down Expand Up @@ -86,6 +93,7 @@ export function loadConverters() {
tupleConverter,
typeOperatorConverter,
unionConverter,
jSDocTypeExpressionConverter,
// Only used if skipLibCheck: true
jsDocNullableTypeConverter,
jsDocNonNullableTypeConverter,
Expand Down Expand Up @@ -120,6 +128,7 @@ let typeConversionDepth = 0;
export function convertType(
context: Context,
typeOrNode: ts.Type | ts.TypeNode | undefined,
maybeNode?: ts.TypeNode,
): SomeType {
if (!typeOrNode) {
return new IntrinsicType("any");
Expand Down Expand Up @@ -178,7 +187,12 @@ export function convertType(

seenTypes.add(typeOrNode.id);
++typeConversionDepth;
const result = converter.convertType(context, typeOrNode, node);
const result = converter.convertType(
context,
typeOrNode,
node,
maybeNode,
);
--typeConversionDepth;
seenTypes.delete(typeOrNode.id);
return result;
Expand Down Expand Up @@ -515,8 +529,10 @@ const jsDocVariadicTypeConverter: TypeConverter<ts.JSDocVariadicType> = {
convert(context, node) {
return new ArrayType(convertType(context, node.type));
},
// Should just be an ArrayType
convertType: requestBugReport,
convertType(context, type, _node, origNode) {
assert(isTypeReference(type));
return arrayConverter.convertType(context, type, null!, origNode);
},
};

const keywordNames = {
Expand Down Expand Up @@ -757,7 +773,12 @@ const referenceConverter: TypeConverter<
// This might not actually be safe, it appears that it is in the relatively small
// amount of testing I've done with it, but I wouldn't be surprised if someone manages
// to find a crash.
return typeLiteralConverter.convertType(context, type, null!);
return typeLiteralConverter.convertType(
context,
type,
null!,
undefined,
);
}

const name = node.typeName.getText();
Expand All @@ -772,7 +793,7 @@ const referenceConverter: TypeConverter<
);
return ref;
},
convertType(context, type, node) {
convertType(context, type, node, originalNode) {
// typeName.symbol handles the case where this is a union which happens to refer
// to an enumeration. TS doesn't put the symbol on the type for some reason, but
// does add it to the constructed type node.
Expand All @@ -798,10 +819,15 @@ const referenceConverter: TypeConverter<
// This might not actually be safe, it appears that it is in the relatively small
// amount of testing I've done with it, but I wouldn't be surprised if someone manages
// to find a crash.
return typeLiteralConverter.convertType(context, type, null!);
return typeLiteralConverter.convertType(
context,
type,
null!,
undefined,
);
}

let name;
let name: string;
if (ts.isIdentifier(node.typeName)) {
name = node.typeName.text;
} else {
Expand All @@ -824,23 +850,17 @@ const referenceConverter: TypeConverter<
convertType(context, (type as ts.StringMappingType).type),
];
} else {
// Default type arguments are filled with a reference to the default
// type. As TS doesn't support specifying earlier defaults, we know
// that this will only filter out type arguments which aren't specified
// by the user.
let ignoredArgs: ts.Type[] | undefined;
if (isTypeReference(type)) {
ignoredArgs = type.target.typeParameters
?.map((p) => p.getDefault())
.filter((x) => !!x);
}

const args = type.aliasSymbol
? type.aliasTypeArguments
: (type as ts.TypeReference).typeArguments;

const maxArgLength =
originalNode && ts.isTypeReferenceNode(originalNode)
? (originalNode.typeArguments?.length ?? 0)
: args?.length;

ref.typeArguments = args
?.filter((ref) => !ignoredArgs?.includes(ref))
?.slice(0, maxArgLength)
.map((ref) => convertType(context, ref));
}
return ref;
Expand Down Expand Up @@ -1144,6 +1164,14 @@ const unionConverter: TypeConverter<ts.UnionTypeNode, ts.UnionType> = {
},
};

const jSDocTypeExpressionConverter: TypeConverter<ts.JSDocTypeExpression> = {
kind: [ts.SyntaxKind.JSDocTypeExpression],
convert(context, node) {
return convertType(context, node.type);
},
convertType: requestBugReport,
};

const jsDocNullableTypeConverter: TypeConverter<ts.JSDocNullableType> = {
kind: [ts.SyntaxKind.JSDocNullableType],
convert(context, node) {
Expand Down
7 changes: 4 additions & 3 deletions src/test/behavior.c2.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1212,9 +1212,10 @@ describe("Behavior Tests", () => {
foo.parameters?.map((p) => p.type?.toString()),
["{ inlined: true }"],
);
// Future: Should we just use types everywhere to get rid of this?
// It still wouldn't get rid of it when converting type aliases...
equal(foo.type?.toString(), "Complex<number>");
equal(foo.type?.toString(), "{ imag: number; real: number }");

const genericInline = querySig(project, "genericInline");
equal(genericInline.type?.toString(), "{ imag: T; real: T }");

const bar = querySig(project, "bar");
equal(
Expand Down
38 changes: 0 additions & 38 deletions src/test/converter/mixin/specs.json
Original file line number Diff line number Diff line change
Expand Up @@ -629,17 +629,6 @@
"type": {
"type": "reference",
"target": 23,
"typeArguments": [
{
"type": "query",
"queryType": {
"type": "reference",
"target": 13,
"name": "Base",
"package": "typedoc"
}
}
],
"name": "Mixin1Class",
"package": "typedoc"
}
Expand Down Expand Up @@ -1734,15 +1723,6 @@
"type": {
"type": "reference",
"target": 23,
"typeArguments": [
{
"type": "reference",
"target": 21,
"name": "T",
"package": "typedoc",
"refersToTypeParameter": true
}
],
"name": "Mixin1Class",
"package": "typedoc"
}
Expand Down Expand Up @@ -1933,15 +1913,6 @@
"type": {
"type": "reference",
"target": 39,
"typeArguments": [
{
"type": "reference",
"target": 37,
"name": "T",
"package": "typedoc",
"refersToTypeParameter": true
}
],
"name": "Mixin2",
"package": "typedoc"
}
Expand Down Expand Up @@ -2125,15 +2096,6 @@
"type": {
"type": "reference",
"target": 59,
"typeArguments": [
{
"type": "reference",
"target": 57,
"name": "T",
"package": "typedoc",
"refersToTypeParameter": true
}
],
"name": "Mixin3",
"package": "typedoc"
}
Expand Down
5 changes: 4 additions & 1 deletion src/test/converter2/behavior/inlineTag.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,14 @@ type Foo = { inlined: true };
/** @inline */
type Complex<T> = { real: T; imag: T };

// TypeNode * 2
export function foo(param: Foo): Complex<number> {
return { real: 1.0, imag: 2.0 };
}

export function genericInline<T>(): Complex<T> {
throw new Error();
}

// TypeNode, nested
export function bar(param: Record<string, Foo>) {}

Expand Down
Loading

0 comments on commit fa3077c

Please sign in to comment.