RFC: Handling of ambiguous comment blocks

[octogonz]

Consider the following TypeScript source file:

/** [1]
 * @file Copyright (c) Example Corporation
 */

/** [2]
 * The Widget class
 */
// [3] TODO: This class needs to be refactored
export class Widget {
  /** [4]
   * The width of the Widget class
   */
  // [5] private width: string;

  /* [6] Renders the class */
  public render(): void {
  }

  /**
   * [7] The height of the widget
   */
  public get height(): number {
  }

  // [8] This is a setter for the above getter
  /**
   * [9] Sets the height
   */
  public set height(value: number) {
  }
}

Which comments are associated with which API items?

We might expect a documentation tool to associate the comments as follows:

• Widget: Uses [2] as its doc comment
• Widget.render: No doc comment, but warn that [6] looks like it was intended to be JSDoc, and warn that [4] doesn't seem to be attached to anything
• Widget.height: Normally setters/getters are documented as one API item, so use [7] and report a warning for [9]

Does this make sense? Is there an unambiguous algorithm for selecting the right comment?

[aciccarello]

Here is how TypeDoc parses the example. (note: this was edited to reflect what TypeDoc actually does)

1. Is considered a comment on the file (since there is no declaration immediately after it).
2. Would not be used since there is another comment before the next declaration (Widget wouldn't have any top level documentation) Is used on the class because there are no doc comments between
3. Is ignored because it is a single line comment
4. Is ignored because there is no declaration immediately following it Is used because the two following comments are not doc comments
5. Is ignored because it is a single line comment
6. Is used on the render method? Is not used because it does not start with two asterisks
7. Is used on the height accessor
8. Is ignored because it is a single line comment
9. Is used on the height setter (separate from accessor)

The getter/setter is rendered as one api item in TypeDoc but you can toggle between the items to see their documentation

I would expect [2] to not be used. I think [6] also should not be used. Only having one API item and using [7] for getter/setters makes sense to me. Concatenating [7] and [9] is another option.

In general, I would recommend limiting the number of warnings. I wouldn't want to get complaints that I commented out [5]. In this case [6] does seem like a mistake but how do you tell the intention? Using /* seems like a simple way to opt out of documentation.

export class Config {}
/* The following classes need to be reviewed for reason x */
export class WidgetA {}
export class WidgetB {}

Another parsing consideration is extra lines.

/** I think TypeDoc will use this on `Widget` even though there is a blank line */

export class Widget { }

[aciccarello]

After testing out my expectations, you can see that TypeDoc only pays attention to comments that begin with /**. This causes a potential problem when commenting out lines like [5] where the following symbol does not have a doc comment. Are there any situations where you'd want there to be content between a doc comment and the symbol or should a single line comment prevent the doc comment from being applied to the following symbol (like in [3])?

[MartynasZilinskas]

@aciccarello the only usage of a single comment between symbol and JSDoc is tslint disable next line
// tslint:disable-next-line.

[aciccarello]

Yes, the tslint use case seem significant. I don't think TSDoc should do anything too tslint specific though. How else could you avoid messing with documentation by commenting out a class property?

/**
 * Some documentation
 */
export class MyClass {
  /** this documentation should be applied */
  // tslint:disable-next-line
  someNameTslintDoesntLike: string;

  /** documentation */
  // prop = 'initializer';
  thisShouldntHaveDocumentation: string;
}

[MartynasZilinskas]

I think there are two possible solutions.

1. Strict. JSDoc comment block must be in immediate previous line.

class Foo {
    /**
     * Some documentation for `bar`.
     */
    public bar: string;

    /**
     * This JSDoc comment block is not part of `bar2`.
     * WARN developer for floating JSDoc comment block.
     */

    public bar2: string;

    /**
     * Same goes here like in `bar2` example.
     * This JSDoc comment block is not part of `bar3`.
     * WARN developer for floating JSDoc comment block.
     */
    // tslint:disable-next-line
    public bar3: string;    
}

2. Sticky. Empty lines separate comment blocks.

class Foo {
    /**
     * Some documentation for `bar`.
     */
    public bar: string;

    /**
     * This JSDoc comment block is not part of `bar2`.
     * WARN developer for a floating JSDoc comment block.
     */

    public bar2: string;

    /**
     * JSDoc for `bar3`.
     */
    // tslint:disable-next-line
    public bar3: string;

    /**
     * JSDoc comment will be assigned for `bar5`. But it was intended for commented property `bar4`.
     */
    //public bar4: string;
    public bar5: string;

    /**
     * WARN developer for a floating JSDoc comment block.
     */
    /**
     * Some documentation for `bar6`.
     */
    public bar6: string;
}

[DovydasNavickas]

I think this situation, where commented property documentation gets applied the next property as they are not separated by an empty line, shouldn't be much of a concern for us:

class Foo {
    /**
     * JSDoc comment will be assigned for `bar5`. But it was intended for commented property `bar4`.
     */
    //public bar4: string;
    public bar5: string;
}

This could be easily solved with a TSLint rule ensuring that declarations have at least one line separating them. Would that be too intrusive for others? I'm sure I could live with it.

[aciccarello]

I think I'm leaning towards requiring whitespace aftwards to prevent documentation shifting to the next variable. While I often write my properties in a tight group, those groupings already get harder to read if there are doc comments. A TSLint rule would seem appropriate for preventing these types of errors. You could even have a rule which warns on a /** doc comment before a // single line comment without whitespace.

class Foo {
  /** This comment is clearly about `bar1` */
  bar1: string;

  bar2: string;
  /** This comment is not great for readability */
  bar3: string;
  bar4: string;

  /**
   * Documentation that is not applied correctly
   */
  // bar5: string; // ts-lint throws "TSDoc comments falling through"
  bar6: string;
}

[octogonz]

BTW the api-demo/src/advancedDemo.ts demo illustrates an approach for collecting comments. The compiler itself already discards some of the weirder places where /** */ comments can appear (e.g. inserted after function parameter types).

[aciccarello]

@octogonz I'm curious if you have thought any more about file level doc comments? In TypeStrong/typedoc#603 users have noted that they expect the first comment of the file to be for the entire file, however there is plenty of ambiguity as to what is acceptable without having a standard format.

[octogonz]

We don't use files-as-namespacs in any of the projects I work on, although I've had external requests for this feature that seem reasonable.

API Extractor does rely on file-level comments from the entry point of a package to document the package itself. However, I would be super paranoid about blindly publishing the "first" comment we encounter. What if it's a copyright banner or other junk that wasn't intended for customers to see? Or what if we counted wrong, because of the weird way that trivia gets parsed by the compiler (and often omitted from the .d.ts file)? For these reasons we insist on the file comment containing a TSDoc tag @packageDocumentation and do some checks to make sure it's not missing, nor are there more than one of them.

It's not great code, but you can see the implementation here:
https://github.com/Microsoft/web-build-tools/blob/25d2894edaebb8fc30fe21f13cc691a5e8d78878/apps/api-extractor/src/aedoc/PackageDocComment.ts

I was thinking about revisiting this topic when we add support for namespaces introduced using import * as n1 from ___.