Skip asterisks after newline when parsing JSDoc types #26528

tschaub · 2018-08-18T04:24:45Z

This makes it possible to split types across lines in JSDoc comments.

For example:

/** 
 * @returns {{someReallyReallyReallyLongType: number,
 *    withLotsOfProperties: boolean}} The return.
 */

If the approach is acceptable, I'll add tests.

The one failing test is jsDocFunctionSignatures12.ts

  1 failing

  1) fourslash tests
       tests/cases/fourslash/jsDocFunctionSignatures12.ts
         fourslash test jsDocFunctionSignatures12.ts runs correctly:

      AssertionError: At line 6, col 5: quick info text: expected '(parameter) o: {\n    stringProp: string;\n    *           numProp: number;\n}' to equal '(parameter) o: any'
      + expected - actual

      -(parameter) o: {
      -    stringProp: string;
      -    *           numProp: number;
      -}
      +(parameter) o: any
      
      at TestState.verifyQuickInfoString (src/harness/fourslash.ts:1361:20)
      at Verify.quickInfoIs (src/harness/fourslash.ts:4167:24)
      at eval (eval at runCode (src/harness/fourslash.ts:3425:23), <anonymous>:13:8)
      at runCode (src/harness/fourslash.ts:3426:13)
      at runFourSlashTestContent (src/harness/fourslash.ts:3408:9)
      at Object.runFourSlashTest (src/harness/fourslash.ts:3393:9)
      at Context.<anonymous> (src/testRunner/fourslashRunner.ts:54:39)

I haven't looked at quick info yet, but it looks like that will need to ignore * after newline as well.

Fixes #23667.
Fixes #26846.

tschaub · 2018-08-18T05:36:40Z

Fixing up the printing of the type is not entirely straightforward. It comes down to getTextOfNodeFromSourceText(). Here includeTrivia is false, but skipTrivia doesn't skip * (which is sort of the whole problem). Not sure what to do about that.

tschaub · 2018-08-30T18:25:40Z

@sandersn or @andy-ms - any opinions on the problem above (skipping newline+asterisk while skipping trivia in getTextOfNodeFromSourceText())? Or thoughts on an alternative solution to skipping newline+asterisk while parsing JSDoc type expressions?

Adding an inJSDoc (or similar) arg to skipTrivia seems like it would make sense (trivia in JSDoc includes * if preceded by newline & whitespace). I'm uncertain if the right context is available at all the call sites or if this type of change would be accepted.

tschaub · 2018-09-02T16:05:47Z

Ok, turns out it isn't skipTrivia's responsibility, since this just skips trivia at the start of a string.

A minor change in getTextOfNodeFromSourceText can make things work. By checking if the node is a JSDoc type expression node there, asterisks at the start of lines can be stripped. I'll push a new commit that implements this.

tschaub · 2018-09-02T20:19:06Z

I've updated the commits so this addresses issues with multiline function signatures as well (see #26846).

sandersn

I think this approach might work but I want to double check with the team to decide whether a short-term fix is the right thing. Also I want to run the user tests and see whether leading * types are a problem in real code. I’ll push a commit with the baseline changes if they’re an improvement.

sandersn · 2018-09-04T15:28:52Z

src/compiler/scanner.ts

+
+        function setInJSDocType(inType: boolean) {
+            inJSDocType += inType ? 1 : -1;
+        }


Are there nested calls to setInJDSocType? Or is there another reason that inJSDocType = inType isn’t enough?

I looked for nested calls and there don't seem to be any.

I was seeing nesting while parsing a @typedef with a function property. I think the tests should fail if inJSDocType is a boolean. I'll confirm.

I'll add another test, but there are nested calls to parseJSDocType here:

/** * @typedef {{foo: function(string)}} SomeType */

And more for a function that accepts a function etc.

Thanks. It's too bad -- I thought our JSDoc type parsing was simpler than that.

sandersn · 2018-09-04T15:30:23Z

tests/baselines/reference/api/tsserverlibrary.d.ts

@@ -3087,6 +3087,7 @@ declare namespace ts {
        setScriptTarget(scriptTarget: ScriptTarget): void;
        setLanguageVariant(variant: LanguageVariant): void;
        setTextPos(textPos: number): void;
+        setInJSDocType(inType: boolean): void;


Can’t remember if the internal comment works in scanner.ts, but if so, please mark setinjsdoctype as internal to revert this API change.

Done in 64bbf5e.

sandersn · 2018-09-04T15:32:33Z

tests/cases/conformance/jsdoc/typedefTagWrapping.ts

+ *
+ * @typedef {function(boolean, string,
+ *    number):
+ *    (string|number)} StringOrNumber2


Can you add a test that uses closure’s * type in interesting ways? Eg (1) at the beginning of line (2) following an asterisk at beginning of line.

In 64bbf5e I added tests that demonstrate that this @typedef is correctly parsed:

/** * @typedef {{ * foo: * *, * bar: * * * }} Works */

And this is not supported:

/** Multiline type expressions in comments without leading * are not supported. @typedef {{ foo: *, bar: * }} DoesNotWork */

sandersn · 2018-09-04T15:33:19Z

src/compiler/utilities.ts

+            text = text.replace(/(^|\r?\n|\r)\s*\*\s*/g, "$1");
+        }
+
+        return text;


Does this change fix only fourslash tests or does it affect the type baselines as well?

This does not affect type baselines, only verify.quickInfoIs() in tests/cases/fourslash/jsDocFunctionSignatures12.ts

sandersn · 2018-09-04T15:34:09Z

src/compiler/utilities.ts

@@ -490,12 +490,29 @@ namespace ts {
        return getTextOfNodeFromSourceText(sourceFile.text, node, includeTrivia);
    }

+    function isJSDocTypeExpressionOrChild(node: Node): boolean {
+        if (node.kind === SyntaxKind.JSDocTypeExpression) {


This function would be easier to read (for me) as a single boolean expression.

Do you mean like this?

function isJSDocTypeExpressionOrChild(node: Node): boolean { return node.kind === SyntaxKind.JSDocTypeExpression || (node.parent && isJSDocTypeExpressionOrChild(node.parent)) || false; }

Yes, except you don't need the trailing || false.

That will return undefined if node.parent is undefined. Will TypeScript accept that as boolean?

Since we have strictNullChecks on now, yes. Put a !! before (node.parent && ...

Edit: I mean, no, it will not accept it.

Looks like there are no complaints from the compiler. I pushed 3b24fba to change this to a single boolean expression.

sandersn · 2018-09-04T15:35:49Z

src/compiler/utilities.ts

+        let text = sourceText.substring(includeTrivia ? node.pos : skipTrivia(sourceText, node.pos), node.end);
+
+        if (isJSDocTypeExpressionOrChild(node)) {
+            // strip space + asterisk at line start


Seems like this will do the wrong thing for the non-leading-asterisk use of the * type.

We discussed this and agreed that it's acceptable to get this wrong as long as there is a test showing how it gets it wrong.

In 64bbf5e I added a test that shows how getTextOfNodeFromSourceText works on this (ugly) code:

/** * @param {{ * stringProp: string, * numProp: number, * boolProp: boolean, * anyProp: *, * anotherAnyProp: * *, * functionProp: * function(string, * *): * * * }} o */ function f1(o) { o; }

That results in this being (pretty) printed:

(parameter) o: { stringProp: string; numProp: number; boolProp: boolean; anyProp: any; anotherAnyProp: any; functionProp: (arg0: string, arg1: any) => any; }

Not sure if this covers what you are talking about.

sandersn · 2018-09-04T16:43:34Z

src/compiler/scanner.ts

@@ -42,6 +42,7 @@ namespace ts {
        setScriptTarget(scriptTarget: ScriptTarget): void;
        setLanguageVariant(variant: LanguageVariant): void;
        setTextPos(textPos: number): void;
+        setInJSDocType(inType: boolean): void;


I think this is where you need to add /* @internal */

Thanks. I'll try that.

Added in 64bbf5e.

sandersn · 2018-09-04T18:04:11Z

All right, we talked about this change and will take it. We could wait until we have time to make the Right Fix so that the scanner (1) correctly skips initial asterisk at a certain indent level (2) correctly handles comment text, including multiline text with consistent indentation. However, I think that change would probably start similarly to this one, and would require ten times more work. I'm not sure when we'll be able to justify it given that the current code doesn't have too many bugs open on it and the refactoring would not produce new functionality.

Please address the comments, except for fixing the ambiguity of leading *, which I think is fine to skip in all cases. But do add tests showing that we always skip it, even when it produces the wrong results.

sandersn · 2018-09-04T20:30:34Z

I ran the user tests on this branch but didn't see as many changes as I expected. Turns out chrome-devtools-frontend only has a few places with leading asterisks inside a type literal.

tschaub · 2018-09-04T21:55:41Z

Ok, I've updated things and included more tests to demonstrate what works and what doesn't. The tl;dr is this:

/**
 * @typedef {{
 *   stringProp: string,
 *   numProp: number,
 *   boolProp: boolean,
 *   anyProp: *,
 *   anotherAnyProp:
 *   *,
 *   functionProp:
 *   function(string,
 *   *):
 *   *
 * }} ThisAllWorks
 */

/**
   @typedef {{
     stringProp: string,
     numProp: number,
     boolProp: boolean,
     anyProp: *
   }} ThisAlsoWorks
 */

/**
   @typedef {{
     anotherAnyProp:
     *,
     functionProp:
     function(string,
     *):
     *
   }} ThisDoesNotWork
 */

Or, put another way, if you do not use * at the start of multiline comments, you cannot start a line with * to represent the any type.

sandersn · 2018-09-04T22:41:06Z

@tschaub Yep, I didn't see any breaks in chrome-devtools-frontend from this ambiguity, although it's a big codebase so I might have missed something. I think it's a wart that we can live with given the amount of people who (1) always use * to begin their JSDoc lines (2) never use * as a type.

Thanks for contributing this fix. I think it'll help unlock a lot of Javascript code.

tschaub · 2018-09-04T23:59:23Z

Thanks for helping get this in @sandersn!

SamB · 2019-08-01T17:24:07Z

Hmm. Would be great to get some documentation about when various multiline constructs actually started to work...

tschaub mentioned this pull request Aug 18, 2018

Multiline JSDoc types don't skip asterisks #23667

Closed

This was referenced Aug 30, 2018

TypeScript (1/n) openlayers/openlayers#8345

Merged

Type checking and API docs with TypeScript JSDoc annotations openlayers/openlayers#8448

Closed

tschaub force-pushed the asteriskless branch 3 times, most recently from f92be3f to f46389c Compare September 2, 2018 20:02

tschaub mentioned this pull request Sep 2, 2018

Multiline JSDoc function signatures aren't properly parsed #26846

Closed

sandersn reviewed Sep 4, 2018

View reviewed changes

sandersn mentioned this pull request Sep 4, 2018

🤖 User test baselines have changed #26861

Merged

tschaub mentioned this pull request Sep 4, 2018

@types/ol out of date (has no exported member) openlayers/openlayers#8388

Closed

Skip asterisks after newline when parsing JSDoc types

64bbf5e

tschaub force-pushed the asteriskless branch from f46389c to 64bbf5e Compare September 4, 2018 21:02

tschaub added 2 commits September 4, 2018 15:30

Single boolean expression

3b24fba

Test for parsing and printing multiline function signatures with *

aca9919

sandersn merged commit 262ea5b into microsoft:master Sep 4, 2018

tschaub deleted the asteriskless branch September 4, 2018 23:45

Raynos mentioned this pull request May 1, 2020

Leading JSDoc * in type expression incorrectly parsed #38290

Open

tschaub mentioned this pull request Feb 1, 2021

use eslint-plugin-jsdoc openlayers/eslint-config-openlayers#25

Merged

simonseyock mentioned this pull request Feb 2, 2021

Multi-line name/description parsing syavorsky/comment-parser#121

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Skip asterisks after newline when parsing JSDoc types #26528

Skip asterisks after newline when parsing JSDoc types #26528

tschaub commented Aug 18, 2018 •

edited

Loading

tschaub commented Aug 18, 2018 •

edited

Loading

tschaub commented Aug 30, 2018 •

edited

Loading

tschaub commented Sep 2, 2018

tschaub commented Sep 2, 2018

sandersn left a comment

sandersn Sep 4, 2018

sandersn Sep 4, 2018

tschaub Sep 4, 2018 •

edited

Loading

tschaub Sep 4, 2018

sandersn Sep 4, 2018

sandersn Sep 4, 2018

tschaub Sep 4, 2018

sandersn Sep 4, 2018

tschaub Sep 4, 2018

sandersn Sep 4, 2018

tschaub Sep 4, 2018

sandersn Sep 4, 2018

tschaub Sep 4, 2018

sandersn Sep 4, 2018

tschaub Sep 4, 2018

sandersn Sep 4, 2018 •

edited

Loading

tschaub Sep 4, 2018

sandersn Sep 4, 2018

sandersn Sep 4, 2018

tschaub Sep 4, 2018 •

edited

Loading

sandersn Sep 4, 2018

tschaub Sep 4, 2018

tschaub Sep 4, 2018

sandersn commented Sep 4, 2018

sandersn commented Sep 4, 2018

tschaub commented Sep 4, 2018 •

edited

Loading

sandersn commented Sep 4, 2018

tschaub commented Sep 4, 2018

SamB commented Aug 1, 2019

Skip asterisks after newline when parsing JSDoc types #26528

Skip asterisks after newline when parsing JSDoc types #26528

Conversation

tschaub commented Aug 18, 2018 • edited Loading

tschaub commented Aug 18, 2018 • edited Loading

tschaub commented Aug 30, 2018 • edited Loading

tschaub commented Sep 2, 2018

tschaub commented Sep 2, 2018

sandersn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tschaub Sep 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sandersn Sep 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tschaub Sep 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sandersn commented Sep 4, 2018

sandersn commented Sep 4, 2018

tschaub commented Sep 4, 2018 • edited Loading

sandersn commented Sep 4, 2018

tschaub commented Sep 4, 2018

SamB commented Aug 1, 2019

tschaub commented Aug 18, 2018 •

edited

Loading

tschaub commented Aug 18, 2018 •

edited

Loading

tschaub commented Aug 30, 2018 •

edited

Loading

tschaub Sep 4, 2018 •

edited

Loading

sandersn Sep 4, 2018 •

edited

Loading

tschaub Sep 4, 2018 •

edited

Loading

tschaub commented Sep 4, 2018 •

edited

Loading