Skip to content
This repository has been archived by the owner on Mar 25, 2021. It is now read-only.

Stop using the type checker in completed-docs #3376

Closed
JoshuaKGoldberg opened this issue Oct 23, 2017 · 12 comments · Fixed by #3557
Closed

Stop using the type checker in completed-docs #3376

JoshuaKGoldberg opened this issue Oct 23, 2017 · 12 comments · Fixed by #3557

Comments

@JoshuaKGoldberg
Copy link
Contributor

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
Copy link
Contributor

ajafff commented Oct 23, 2017

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 microsoft/TypeScript#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
Copy link
Contributor Author

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

@ajafff
Copy link
Contributor

ajafff commented Nov 13, 2017

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
Copy link
Contributor

buu700 commented Nov 13, 2017

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
Copy link
Contributor

ajafff commented Nov 13, 2017

@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 typescript@2.7.0 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
Copy link
Contributor

buu700 commented Nov 13, 2017

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
Copy link
Contributor

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
Copy link
Contributor

ajafff commented Nov 14, 2017

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
Copy link
Contributor

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
Copy link
Contributor

ajafff commented Nov 16, 2017

@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
Copy link
Contributor

ajafff commented Nov 19, 2017

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
Copy link
Contributor

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants