Skip to content

Commit

Permalink
fix: Use type nodes if converting a regular function
Browse files Browse the repository at this point in the history 
Fixes some instances of TypeDoc falling into the type interning trap
Closes #1454
  • Loading branch information
@Gerrit0
Gerrit0 committed Jan 6, 2021
1 parent 6027ba8 commit d528c69
Show file tree
Hide file tree
Showing 5 changed files with 187 additions and 13 deletions.
25 changes: 21 additions & 4 deletions src/lib/converter/factories/signature.ts
View file
Expand Up @@ -68,7 +68,9 @@ export function createSignature(
} else {
sigRef.type = context.converter.convertType(
context.withScope(sigRef),
signature.getReturnType()
(declaration?.kind === ts.SyntaxKind.FunctionDeclaration &&
declaration.type) ||
signature.getReturnType()
);
}

Expand Down Expand Up @@ -108,11 +110,26 @@ function convertParameters(
declaration
);

let type: ts.Type | ts.TypeNode;
if (
declaration &&
ts.isParameter(declaration) &&
ts.isFunctionDeclaration(declaration.parent) &&
declaration.type
) {
type = declaration.type;
} else if (declaration) {
type = context.checker.getTypeOfSymbolAtLocation(
param,
declaration
);
} else {
type = param.type;
}

paramRefl.type = context.converter.convertType(
context.withScope(paramRefl),
declaration
? context.checker.getTypeOfSymbolAtLocation(param, declaration)
: param.type
type
);

let isOptional = false;
Expand Down
11 changes: 11 additions & 0 deletions src/lib/converter/types.ts
View file
Expand Up @@ -579,6 +579,17 @@ const referenceConverter: TypeConverter<
> = {
kind: [ts.SyntaxKind.TypeReference],
convert(context, node) {
const isArray =
context.checker.typeToTypeNode(
context.checker.getTypeAtLocation(node.typeName),
void 0,
ts.NodeBuilderFlags.IgnoreErrors
)?.kind === ts.SyntaxKind.ArrayType;

if (isArray) {
return new ArrayType(convertType(context, node.typeArguments?.[0]));
}

const symbol = context.expectSymbolAtLocation(node.typeName);

const name = node.typeName.getText();
Expand Down
8 changes: 8 additions & 0 deletions src/test/converter/alias/alias.ts
View file
Expand Up @@ -65,3 +65,11 @@ export namespace GH1330 {
export namespace GH1408 {
export declare function foo<T extends unknown[]>(): T;
}

export namespace GH1454 {
export type Foo = string | number;
export type Bar = string | number;

export declare function bar(x: Bar): Bar;
export declare function foo(x: Foo): Foo;
}
140 changes: 139 additions & 1 deletion src/test/converter/alias/specs.json
View file
Expand Up @@ -238,6 +238,143 @@
}
]
},
{
"id": 43,
"name": "GH1454",
"kind": 2,
"kindString": "Namespace",
"flags": {},
"children": [
{
"id": 51,
"name": "Bar",
"kind": 4194304,
"kindString": "Type alias",
"flags": {},
"type": {
"type": "union",
"types": [
{
"type": "intrinsic",
"name": "string"
},
{
"type": "intrinsic",
"name": "number"
}
]
}
},
{
"id": 50,
"name": "Foo",
"kind": 4194304,
"kindString": "Type alias",
"flags": {},
"type": {
"type": "union",
"types": [
{
"type": "intrinsic",
"name": "string"
},
{
"type": "intrinsic",
"name": "number"
}
]
}
},
{
"id": 44,
"name": "bar",
"kind": 64,
"kindString": "Function",
"flags": {},
"signatures": [
{
"id": 45,
"name": "bar",
"kind": 4096,
"kindString": "Call signature",
"flags": {},
"parameters": [
{
"id": 46,
"name": "x",
"kind": 32768,
"kindString": "Parameter",
"flags": {},
"type": {
"type": "reference",
"id": 51,
"name": "Bar"
}
}
],
"type": {
"type": "reference",
"id": 51,
"name": "Bar"
}
}
]
},
{
"id": 47,
"name": "foo",
"kind": 64,
"kindString": "Function",
"flags": {},
"signatures": [
{
"id": 48,
"name": "foo",
"kind": 4096,
"kindString": "Call signature",
"flags": {},
"parameters": [
{
"id": 49,
"name": "x",
"kind": 32768,
"kindString": "Parameter",
"flags": {},
"type": {
"type": "reference",
"id": 50,
"name": "Foo"
}
}
],
"type": {
"type": "reference",
"id": 50,
"name": "Foo"
}
}
]
}
],
"groups": [
{
"title": "Type aliases",
"kind": 4194304,
"children": [
51,
50
]
},
{
"title": "Functions",
"kind": 64,
"children": [
44,
47
]
}
]
},
{
"id": 21,
"name": "HorribleRecursiveTypeThatShouldNotBeUsedByAnyone",
Expand Down Expand Up @@ -746,7 +883,8 @@
"kind": 2,
"children": [
28,
39
39,
43
]
},
{
Expand Down
16 changes: 8 additions & 8 deletions src/test/converter/function/specs.json
View file
Expand Up @@ -520,16 +520,16 @@
"type": "union",
"types": [
{
"type": "intrinsic",
"name": "undefined"
"type": "reference",
"name": "T"
},
{
"type": "literal",
"value": null
},
{
"type": "reference",
"name": "T"
"type": "intrinsic",
"name": "undefined"
}
]
}
Expand Down Expand Up @@ -1004,16 +1004,16 @@
"type": "union",
"types": [
{
"type": "intrinsic",
"name": "undefined"
"type": "reference",
"name": "T"
},
{
"type": "literal",
"value": null
},
{
"type": "reference",
"name": "T"
"type": "intrinsic",
"name": "undefined"
}
]
}
Expand Down

5 comments on commit d528c69

@jamesgpearce
Copy link

Choose a reason for hiding this comment

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

Any reason why this is limited only to functions and not also for methods in classes/interfaces? Thanks!

@Gerrit0
Copy link
Collaborator Author

@Gerrit0 Gerrit0 commented on d528c69 Mar 7, 2021

Choose a reason for hiding this comment

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

Because in classes/interfaces we have to deal with type parameters.

interface Base<T> { doIt(x: T): void }
// Should be documented with a method `doIt` accepting `number`
interface Child extends Base<number> {}

We could technically use type nodes for this as well, but then TypeDoc would have to duplicate the compiler's tracking of nodes, automatically resolve type parameters, and perform deduplication and normalization on the resolved types.
In most situations, TS 4.2 has made the difference not matter.

@jamesgpearce
Copy link

@jamesgpearce jamesgpearce commented on d528c69 Mar 7, 2021 •
edited

Choose a reason for hiding this comment

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

OK, I see. But this seems to mean that all the parameters with aliased types (except in plain functions) are now being resolved if they map to intrinsic types. eg:

type StringOrNull = string | null;
type StringOrBoolean = string | boolean;
type Numeric = number;

function myFunction(a: StringOrNull, b: StringOrBoolean): Numeric;

interface MyInterface {
  myMethod(a: StringOrNull, b: StringOrBoolean): Numeric;
}

Gets documented as :

function myFunction(a: StringOrNull, b: StringOrBoolean): Numeric; // great!

interface MyInterface {
  myMethod(a: string, b: StringOrBoolean): number; // not so great!
}

For me the use case is doing things like having a type alias called Json (which is literally just a string) but which self-documents its semantics. Previously TypeDoc preserved the aliases in callables' parameters and elsewhere.

Is this expected in TypeDoc 0.20.30, TypeScript 4.2.3? (If not, I'm happy to raise as an issue.)

PS thanks for the amazing work :)

@Gerrit0
Copy link
Collaborator Author

@Gerrit0 Gerrit0 commented on d528c69 Mar 8, 2021

Choose a reason for hiding this comment

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

Unfortunately, yes... it's expected. I'd label an issue about that with design limitation most likely.
You can work around it by modifying your aliases slightly so that TS doesn't intern their usage.

type StringOrNull = string | null // this is fine as is, so long as you have strictNullChecks on, which you don't
type Numeric = number & { __numeric?: never } // similar to branded types for nominal typing, but here just used to prevent interning

const x: Numeric = 123 // ok
function foo(x: Numeric) {}
let y = 123
foo(y) // ok

@jamesgpearce
Copy link

Choose a reason for hiding this comment

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

lol type Numeric = number | Number; etc is the least hacky solution I can find. Thanks for helping me think through this!

Please sign in to comment.