Tslint: Stop using the type checker in completed-docs

[JoshuaKGoldberg]

As per @ajafff's feedback in #2415:

If you want to get the jsdoc comment, you can simply do this:

const [firstChild] = documentationNode.getChildren();
if (firstChild.kind === ts.SyntaxKind.JSDocComment) {
// firstChild is ts.JSDoc
// do stuff with the comment
}
For more info on JSDoc, see https://github.com/Microsoft/TypeScript/blob/master/src/compiler/types.ts#L2065
You also don't need to parse it manually with the regex magic below. Typescript's parser already did that for you

@JoshuaKGoldberg SyntaxWalker#visitNode will never visit any JSDoc nodes, that's why I pointed you to node.getChildren() which does some magic to include jsdoc as the first entries if available.
However, I don't consider this a blocker for the PR. I'm fine with merging this without that change and tweaking it afterwards.

This should also fix #3365.

[ajafff]

A little update on my comment: you can now use getJsDoc from tsutils. It returns all JSDoc comments for a given node.

BUT you most likely don't want to switch because:

The type checker merges documentation of merged declarations:

/** Doc */
interface Foo {}
class Foo {} // also has 'Doc' as JSDoc comment

// it even merges documentation for declarations that don't merge
/** MyType */
interface T {}
var T; // has 'MyType' as JSDoc comment

If https://github.com/Microsoft/TypeScript/pull/18804 lands, the type checker will even include the JSDoc comments of base types.
I don't think it's a good thing to try to duplicate that logic.

It only makes sense if you only want to check the JSDoc of the current AST node. But that might break users for the reasons above.

[JoshuaKGoldberg]

So, to clarify, would you recommend completed-docs still use the type checker?

[ajafff]

So, to clarify, would you recommend completed-docs still use the type checker?

That depends on how it is supposed to work. Making this change now could break some users.

If you don't want JSDoc to merge (or being inherited), you change the rule to not use the type checker.
But I don't know your requirements.

I'd also like to hear what other users of the rule think about this. /cc @buu700 @reduckted

[buu700]

What are the implications of dropping the type checker? It sounds like you're saying that a case like this currently doesn't flag a rule violation, but would start to do after the type checker is dropped:

/** Foo */
class Foo {
    /** Balls */
    public balls: string;
}

/** Bar */
class Bar extends Foo {
    public balls: string = 'balls';
}

In my case, we've been consistently using /** @inheritDoc */ for situations like that, so it wouldn't make a difference to me. More generally, I personally wouldn't expect that code as written to necessarily not be flagged.

[ajafff]

@buu700 your example contains a rule violation either way because Bar.balls has no documentation. There's also no inheritance involved for properties AFAICT.

Currently the only difference is for merged declarations as I described above:

/** Foo */
interface Foo {}
class Foo {} // currently no error; without the type checker this would be an error

As of [email protected] typescript starts to inherit the documentation when @inheritdoc is present:

/** Foo */
class Foo {
}

/** @inheritdoc */
class Bar extends Foo {
}

That means class Bar will have a lint error (depending on your configuration of the rule) without the type checker, while there will never be an error in typescript@>=2.7.0 with the type checker.
That one is probably not relevant for you, because you want to exclude declarations with the @inheritdoc tag anyway.

[buu700]

Got it, thanks for the clarification. I didn't know jsdocs could be used that way, so when you said "merge" I thought you meant that Bar.balls would automatically inherit the doc comment from Foo.balls.

In that case, I can see how it would be an issue for users already using jsdocs that way (if that is indeed meant to be supported by the jsdoc spec), but for me it wouldn't be an issue at all.

[reduckted]

What's the motivation for removing the type checker?

It sounds like removing the type checker would be OK. I doubt it would affect me personally, and if the only thing that really changes is the merging of declarations, then I can't see it breaking anything that isn't already questionable (declaring a class and interface with the same name seems like a strange thing to do).

Having said that, if the type checker is what's providing JSDoc comments, then maybe we should continue to use that instead of, effectively, creating our own implementation.

@JoshuaKGoldberg If you wanted to create a fork without the type checker, I can run it across a bunch of repos from work and see if it changes the results.

[ajafff]

What's the motivation for removing the type checker?

The TypeChecker is not available in most IDE plugins. That means you will never know about missing documentation until you run tslint from command line. #2767

if the type checker is what's providing JSDoc comments, then maybe we should continue to use that instead of, effectively, creating our own implementation.

The TypeChecker only makes it a bit more convenient to use. We should definitely not try to duplicate the logic regarding merging and inheritance. The rest, i.e. getting JSDoc directly from an AST node, is not a big deal.

[reduckted]

So just to make sure I understand, if I have Foo.ts:

/**
 * This is foo.
 */
export class Foo { }

And I have Bar.ts:

import { Foo } from './bar';

export class Bar extends Foo { }

Then if we use the type checker, Bar will be considered to have JSDoc comments (when Microsoft/TypeScript#18804 lands), whereas _not_ using the type checker, it won't be considered to have JSDoc comments?

Assuming that's correct, then personally, I'm OK with not using the type checker. I prefer that _everything_ publicly exposed have JSDoc comments, even if that's just an @inheritdoc comment (rather than having it implicitly inherited).

[ajafff]

@reduckted I looked at the TypeScript PR again. IIUC It does not work for class or interface declarations. It seems to only work for properties and methods (I answered the exact opposite to @buu700 above).
If there is no JSDoc comment or one containing the @inheritdoc tag, it merges the JSDoc of the property on the supertype.

So @buu700's example from above would actually be valid:

/** Foo */
class Foo {
    /** Balls */
    public balls: string;
}

/** Bar */
class Bar extends Foo {
    public balls: string = 'balls';
}

Bar.balls inherits the documentation from Foo.balls. If Bar had no JSDoc, it would not inherit the /** Foo */ comment of Foo.

I agree that using @inheritdoc makes the intent clear to everyone looking at the code. The JSDoc spec on the other hand uses inheritance if there is no documentation comment.

[ajafff]

Another difference is the handling of overload signatures:

function foo(): void;
function foo(bar: string): string;
function foo(bar?: string) {
  return bar;
}

The TypeChecker merges the documentation of all overloads and the implementation. If any of the declarations has a documentation comment, all other declarations are automatically valid. The behavior is the same for functions and methods.
That may not be the desired behavior. You cannot enforce every overload signature to have a documentation comment.
Without the TypeChecker we could avoid merging documentation.

[reduckted]

Just thought I'd mention something about property accessors now that #3497 has been merged.

Because we're using the type checker, the documentation comments from the getter and setter are merged. This means you can have comments on the getter, but not on the setter (or vice versa), and that is considered valid. If we get rid of the type checker, then we would need some sort of special handling _if_ we wanted to keep that behaviour.