Skip to content

VarArgs inference overly aggressive when checking JavaScript files. #1617

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

Closed
houghtonap opened this issue Jun 28, 2021 · 16 comments
Closed

VarArgs inference overly aggressive when checking JavaScript files. #1617

houghtonap opened this issue Jun 28, 2021 · 16 comments
Labels
no bug This is expected behavior

Comments

@houghtonap
Copy link

houghtonap commented Jun 28, 2021

Search terms

  1. VarArgs inference
  2. javascript files

Expected Behavior

  1. To use the @params to document the method and not infer an VarArgs.

Actual Behavior

  1. Actually, it's not what Typedoc failed to do, it's what it actually did.
    When using arguments in a JavaScript function, Typedoc infers VarArgs onto the end of the method even when an actual argument has not been used, e.g., using arguments.length as opposed to an actual argument arguments[1]. This appears to be a Typedoc issue since using the TypeScript playground indicates that the method has only one argument of type any as expected and correctly didn't apply VarArgs per this statement in the TypeScript documentation. What appears to be at issue with the documentation generated by Typedoc is that any reference to arguments will assume the method/function has , ...args: any[] rather than strictly only when an reference to an actual argument is made, e.g., arguments[1].

Steps to reproduce the bug

typedoc.json

{
  "entryPoints": ["test-class.js"],
  "out": "docs"
}

tsconfig.js

{
  "compilerOptions": {
    "target": "es2020",
    "module": "es2020",
    "allowJs": true,
    "checkJs": true,
    "outDir": "./dist",
    "strict": true,
    "moduleResolution": "node",
    "allowSyntheticDefaultImports": true,
    "esModuleInterop": true,
    "experimentalDecorators": true,
    "emitDecoratorMetadata": true,
    "skipLibCheck": true,
    "forceConsistentCasingInFileNames": true
  }
}

test-class.js

/**
 * This is a test class.
 */
export class test
{
  constructor ( )
  {
    if ( this.constructor === test )
      throw new TypeError( `class is not constructable` ) ;
    return this ;
  }
  /**
   * Determines whether a type instance is an instance of this class.
   * @remarks
   * Some remarks...
   * @param {any} type
   * The instance used.
   * @returns {boolean}
   * |What|Description|
   * |:---|:----------|
   * |`True` |When the instance is of type test class.|
   * |`False`|When the instance is not of type test class.|
   * |`TypeError`|When not enough or extraneous arguments were given.|
   */
    static [ Symbol.hasInstance ] ( type )
    {
      const meth = this[ Symbol.hasInstance ] ;
      const args = meth.length ;
      if ( arguments.length < args )
        throw new TypeError( `too few arguments specified` ) ;
      if ( arguments.length > args )
        throw new TypeError( `too many arguments specified` ) ;
      return type !== null && type !== undefined && type.constructor === test ;
    }
}

Environment

  • Typedoc version: 0.21.2
  • TypeScript version: 4.3.4
  • Node.js version: 12.22.1
  • OS: Windows 7 [which is why I'm limited to node 12.22.1]
@houghtonap houghtonap added the bug label Jun 28, 2021
@houghtonap
Copy link
Author

One additional oddity with the example I provided for the issue that I just noticed. When typedoc generates the documentation, it indicates the method name as hasInstance rather than [Symbol.hasInstance].

@Gerrit0
Copy link
Collaborator

Gerrit0 commented Jul 2, 2021

TypeDoc appears to be doing the right thing here. In the playground, if the language is set to TypeScript, then, yes meth will be (type: any) => boolean, however if you set the language to JS, I see output that matches TypeDoc's output (playground)

To set the language to JS, you have to go to the TS Config tab in the playground (weird spot, but...)

@Gerrit0 Gerrit0 added the no bug This is expected behavior label Jul 2, 2021
@houghtonap
Copy link
Author

OK, maybe I'm a bit blind, but clicking your link to the TypeScript playground, which has my example, and the TS Config options are set to:

  • Lang = JavaScript
  • Target = ES2020
  • JSX = none
  • Module = CommonJS

The sample compiles fine, but I don't see anywhere I can view the generated typedoc documentation.

I'm attaching my local project which has "allowJS": true, in my tsconfig.json, essentially the JavaScript option in the TypeScript Playground that allows JavaScript files to be processed by the TypeScript compiler. You can view the docs directory or the assets directory image and the VarArgs show up. This should not occur since I have not accessed a member of the arguments array, e.g., arguments[0], only arguments.length which is essentially providing what TypeScript would consider a type guard to insure the caller gives the correct number of arguments.

To run the project:

  1. create a directory
  2. unzip the files into the created directory
  3. npm run install
  4. npm run build

I also tried this project at home on my Windows 10 laptop with the latest version of nodejs with the same results. I'm limited at work to nodejs v12.22.1 because we are still running Windows 7 Enterprise.

typedoc-example.zip

@Gerrit0
Copy link
Collaborator

Gerrit0 commented Jul 2, 2021

Sorry, the playground doesn't show TypeDoc output, I was providing that to show that the method is variadic.

This should not occur since I have not accessed a member of the arguments array, e.g., arguments[0], only arguments.length which is essentially providing what TypeScript would consider a type guard to insure the caller gives the correct number of arguments.

This is not true - TypeScript infers this function as having a vararg, so TypeDoc does the same.

image

As the page you linked in the handbook says:

A function whose body has a reference to the arguments reference is implicitly considered to have a var-arg parameter (i.e. (...arg: any[]) => any).

This doesn't say anything about accessing individual indexes, even this function implicitly has varargs.

function foo(arg) {
  arguments;
}

@houghtonap
Copy link
Author

However, looking at the AST generated by the playground, goto settings to turn it on, it shows only one parameter for that method. TypeScript may infer ( type:any, ...args: any[]) so access to members of the arguments array doesn't cause further type issues while compiling, but the generated AST says what was intended. As you can see below, at the end in the parameters array, there is only one parameter listed for the method, not two.

members: [
Constructor
MethodDeclaration
pos: 203
end: 1066
flags: 163840
modifierFlagsCache: 536875040
transformFlags: 2302544
kind: 166 (SyntaxKind.MethodDeclaration)
decorators: undefined
modifiers: [
StaticKeyword
]
symbol: [object Object]
localSymbol: undefined
locals: [object Map]
nextContainer: undefined
name: ComputedPropertyName
typeParameters: undefined
parameters: [
Parameter
]

  1. it sounds like you are punting on this as an issue, but
  2. it doesn't resolve the second issue with the generated TypeDoc documentation incorrectly specifying the method as [hasInstance] rather than the correct [Symbol.hasInstance].

@Gerrit0
Copy link
Collaborator

Gerrit0 commented Jul 2, 2021

However, looking at the AST generated by the playground

The AST represents what you wrote - that function doesn't have a rest argument in the parameters, so the AST won't include it. The AST also won't include inferred types, so there will be no number type in

function doIt() {
  return 1
}

... even though doIt should be documented as returning number. Using only the AST for documentation has some advantages, and is what TypeDoc used to do, but has major disadvantages when dealing with more complicated types. Using types instead of the AST means that we can properly handle documenting cases like the following:

class Foo<T> {
  prop: T extends number ? string : boolean
}

class Bar extends Foo<number> {
  // bar.prop should be documented as type `string`, not `T extends number ? string : boolean`
}

The [hasInstance] behavior is also expected, this is a result of microsoft/TypeScript#42543, which was released in TS 4.2. The TS compiler no longer treats well known symbols specially, and so TypeDoc doesn't either. I'd be open to a PR which changes this, but it needs to properly handle user symbols as well.

const custom = globalThis.Symbol()
namespace Symbol {
  export const hasInstance = globalThis.Symbol()
}
class Sym {
  [custom]: 1
  [Symbol.hasInstance]: () => true
  [globalThis.Symbol.hasInstance]: () => false
}

@houghtonap
Copy link
Author

As far as symbols, would it be possible to allow @name to override the generated name, even though it's not part of tsdoc standard? Then I could throw in @name [Symbol.hasInstance] and the documentation would reflect the correct name.

@Gerrit0
Copy link
Collaborator

Gerrit0 commented Jul 2, 2021

You could do that pretty easily with a plugin:

const { Converter, ReflectionKind } = require("typedoc")

exports.load = function(app) {
    app.converter.on(Converter.EVENT_CREATE_SIGNATURE, (_, refl) => rename(refl));
    app.converter.on(Converter.EVENT_CREATE_DECLARATION, (_, refl) => rename(refl));
}

function rename(refl) {
    const name = refl.comment?.getTag("name")?.text.trim();
    refl.comment?.removeTags("name")

    if (name) {
        refl.name = name
        if (refl.kindOf(ReflectionKind.SomeSignature)) {
            refl.parent.name = refl.name;
        }
    }
}
typedoc --plugin ./plugins/name.js ./src/gh1617.ts

@houghtonap
Copy link
Author

Wow, I'll have to look into TypeDoc's plugin architecture!! Thanks for the starter code and pointer, much appreciated.

@Gerrit0
Copy link
Collaborator

Gerrit0 commented Jul 4, 2021

I realized, coming back to this, that you can also just use a plugin to remove the rest signature if you don't want TypeScript's inference behavior. This will remove any rest parameters of type any[], which likely means they came from using arguments You could also check the third argument to the callback, which will be either a ts.ParameterDeclaration or undefined, depending on if the node exists, but this is likely sufficient.

const { Converter, ReflectionKind } = require("typedoc")

exports.load = function(app) {
    app.converter.on(Converter.EVENT_CREATE_SIGNATURE, (_, refl) => {
        if (refl.parameters.length === 0) return;
        const param = refl.parameters[refl.parameters.length - 1];
        if (param.type.toString() === "any[]" && param.flags.isRest) {
            refl.project.removeReflection(param)
        }
    });
}

@houghtonap
Copy link
Author

You could do that pretty easily with a plugin:

const { Converter, ReflectionKind } = require("typedoc")

exports.load = function(app) {
    app.converter.on(Converter.EVENT_CREATE_SIGNATURE, (_, refl) => rename(refl));
    app.converter.on(Converter.EVENT_CREATE_DECLARATION, (_, refl) => rename(refl));
}

function rename(refl) {
    const name = refl.comment?.getTag("name")?.text.trim();
    refl.comment?.removeTags("name")

    if (name) {
        refl.name = name
        if (refl.kindOf(ReflectionKind.SomeSignature)) {
            refl.parent.name = refl.name;
        }
    }
}
typedoc --plugin ./plugins/name.js ./src/gh1617.ts

I tried to get this working, but typedoc says the plugin could not be loaded:

U:\Documents\Source>npx typedoc --theme typedoc/theme --plugin typedoc/plugins/name.js
Error: The plugin typedoc/plugins/name.js could not be loaded.
Error: Error: Cannot find module 'typedoc/plugins/name.js'
Require stack:
- U:\Documents\Source\node_modules\typedoc\dist\lib\utils\plugins.js
- U:\Documents\Source\node_modules\typedoc\dist\lib\utils\index.js
- U:\Documents\Source\node_modules\typedoc\dist\lib\models\reflections\project.js
- U:\Documents\Source\node_modules\typedoc\dist\lib\models\reflections\index.js
- U:\Documents\Source\node_modules\typedoc\dist\lib\models\index.js
- U:\Documents\Source\node_modules\typedoc\dist\lib\converter\context.js
- U:\Documents\Source\node_modules\typedoc\dist\lib\converter\index.js
- U:\Documents\Source\node_modules\typedoc\dist\lib\application.js
- U:\Documents\Source\node_modules\typedoc\dist\index.js
- U:\Documents\Source\node_modules\typedoc\bin\typedoc
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:815:15)
    at Function.Module._load (internal/modules/cjs/loader.js:667:27)
    at Module.require (internal/modules/cjs/loader.js:887:19)
    at require (internal/modules/cjs/helpers.js:74:18)
    at Object.loadPlugins (U:\Documents\Source\node_modules\typedoc\dist\lib\utils\plugins.js:13:30)
    at Application.bootstrap (U:\Documents\Source\node_modules\typedoc\dist\lib\application.js:156:17)
    at Object.<anonymous> (U:\Documents\Source\node_modules\typedoc\bin\typedoc:25:5)
    at Module._compile (internal/modules/cjs/loader.js:999:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1027:10)
    at Module.load (internal/modules/cjs/loader.js:863:32)

The file is really there in the directory specified: typedoc/plugins/name.js. I also tried adding a ./ in front of it and changed the forward slashes to backslashes since I'm on Windows. Nothing helped. Any suggestions?

@houghtonap
Copy link
Author

The [hasInstance] behavior is also expected, this is a result of microsoft/TypeScript#42543, which was released in TS 4.2. The TS compiler no longer treats well known symbols specially, and so TypeDoc doesn't either. I'd be open to a PR which changes this, but it needs to properly handle user symbols as well.

const custom = globalThis.Symbol()
namespace Symbol {
  export const hasInstance = globalThis.Symbol()
}
class Sym {
  [custom]: 1
  [Symbol.hasInstance]: () => true
  [globalThis.Symbol.hasInstance]: () => false
}

I had a further thought on this. Typedoc should know that it has a Symbol to document, so why not use the Symbol's description and if it's an empty string fall back to the existing strategy. For example, for all well known symbols in JavaScript, their description is the name of the well know symbol:

assert( Symbol.iterator.description === 'Symbol.iterator' ) ;
assert( Symbol.hasInstance.description === 'Symbol.hasInstance' ) ;

If you follow the existing convention in your own code, then:

const custom = globalThis.Symbol( 'Sym.custom' )
namespace Symbol {
  export const hasInstance = globalThis.Symbol( 'Symbol.hasInstance' )
}
class Sym {
  [custom]: 1
  [Symbol.hasInstance]: () => true
  [globalThis.Symbol.hasInstance]: () => false
}

Typedoc could generate: [Sym.custom] or [Symbol.hasInstance] rather than just [custom] or [hasInstance].

Thoughts?

@Gerrit0
Copy link
Collaborator

Gerrit0 commented Jul 10, 2021

I also tried adding a ./ in front of it

This should have worked, paths starting with . will be relative to where they are specified for plugins (cwd if on command line). Try adding --showConfig after the command, then TypeDoc will print out its config and reveal what path that is resolved to.

why not use the Symbol's description and if it's an empty string fall back to the existing strategy

The problem with this is that it conflates runtime values and type values. There is nothing on the type unique symbol type that well known symbols have which has the symbol's description, and going and trying to look it up is potentially expensive (and in some cases, impossible)

typeof Symbol.iterator["description"] // string, not "Symbol.iterator"

@houghtonap
Copy link
Author

Success!! OK to get the name plugin to work required a few adjustments, due to my environment, but there also appears to be a subtle bug in typedoc when determining the plugin directory.

  1. I'm using Node, but not Babel or TypeScript. Seems odd, why would you want to use typedoc. It's a long story, but the short story is reasonable looking documentation for a JavaScript only project.
  2. My package.json uses type: "module" and my tsconfig.json uses "target": "es2020", "module": "es2020", "allowJs": true and "checkJs": true. This has implications on how typedoc plugins are loaded. It appears that only commonjs modules are supported with this configuration, so I had to rename the plugin with a suffix of .cjs to get it working.
  3. Since I wasn't using Babel or TypeScript, I had to change the ? short circuits since they are not supported yet in Node or I have to add some flag to get the non-stage 4 proposal to work.
  4. Typedoc has a subtle bug when determine the plugin's directory. Executing npx typedoc --theme typedoc/theme --plugin typedoc/plugins/name.cjs, typedoc is unable to find the plugin however, dropping a ./ in front of the path to the plugin option works even though the typedoc directory is a sub-directory of where npx was executed. Yet the --theme command line option had no issue determining where my hacked project theme was?!

Attached is the updated source for the plugin. Works great 😄, thanks again for demonstrator to get my feet wet with typedoc plugins.

name.zip

@houghtonap
Copy link
Author

Spoke a little too soon... The @name plugin works OK, but I'm also using the @mermaid plugin and when the two of them are used together, the @mermaid plugin doesn't work. Not sure if it's because it's an older plugin. I get the message:

Warning: U:\Documents\Source\nodejs\decorators\node_modules\typedoc-plugin-mermaid uses a deprecated structure. Plugins should export a "load" function to be called with the Application
    at Application.bootstrap (U:\Documents\Source\nodejs\decorators\node_modules\typedoc\dist\lib\application.js:156:17)

Is there something else that a plugin needs to do to play nice with other plugins?

BTW, I'm using these two plugins in different comments. The @mermaid is being done in the class to provide a class diagram, the @name is being used in a comment before a class method.

@Gerrit0
Copy link
Collaborator

Gerrit0 commented Jul 14, 2021

not supported yet in Node

Supported since 14.5 or so, works on my machine ;)

Typedoc has a subtle bug when determine the plugin's directory. Executing npx typedoc --theme typedoc/theme --plugin typedoc/plugins/name.cjs, typedoc is unable to find the plugin however, dropping a ./ in front of the path to the plugin option works even though the typedoc directory is a sub-directory of where npx was executed. Yet the --theme command line option had no issue determining where my hacked project theme was?!

This is because these are different types of options. Plugins might be a npm package - specifying typedoc-plugin-mermaid should reference the npm package, not try to access a local directory. Themes on the other hand are just directories that are mined for some handlebars files, not a module that's executed, so they do not use Node's module resolution.

I took a quick look at the mermaid plugin, but don't see any obvious reason it would cause the name plugin to break. (Why does it inherit from the internal ConverterComponent?! There's absolutely no reason to do this...)

It might be that it isn't broken, but that it isn't being loaded. When you specify --plugin, you disable TypeDoc's auto plugin discovery for npm plugins, so if you want to load both your custom plugin and the mermaid one you need to specify them both.

@Gerrit0 Gerrit0 closed this as completed Jul 31, 2021
@Gerrit0 Gerrit0 removed the bug label Feb 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no bug This is expected behavior
Projects
None yet
Development

No branches or pull requests

2 participants