Skip to content
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

Multiline JSDoc types don't skip asterisks #23667

Closed
sandersn opened this issue Apr 24, 2018 · 7 comments · Fixed by #26528
Closed

Multiline JSDoc types don't skip asterisks #23667

sandersn opened this issue Apr 24, 2018 · 7 comments · Fixed by #26528
Assignees
Labels
Bug A bug in TypeScript Domain: JSDoc Relates to JSDoc parsing and type generation
Milestone

Comments

@sandersn
Copy link
Member

In a JS file, this typedef doesn't work

/**
 * @typedef {{ 
 *     krazy: 'chad', 
 *     and: 'his',
 *     tag: 'team'
 * }} 
 * Show */

Expected behavior:
No parse errors, and the type is the same as the single-line version

Actual behavior:
Lots of parse errors.

The problem is that JSDoc type parsing uses the Typescript parser, which does not expect asterisks.

@sandersn
Copy link
Member Author

@bterlson, who found this bug, suggests just skipping asterisks everywhere in the type parser which might work if we really never think that types will have asterisks in them.

@mhegazy
Copy link
Contributor

mhegazy commented Apr 25, 2018

looks like a duplicate of #23517

@mhegazy mhegazy added the Duplicate An existing issue was already created label Apr 25, 2018
@sandersn
Copy link
Member Author

The symptom may be the same, but the cause is different #23517 is failure to skip asterisks inside (unrecognised) tag text, whereas this bug is a failure to skip asterisks inside a type. The fix will be different since (1) whitespace is still important in the text case (2) the jsdoc lexer and parser are used for the text case. Neither is true for the type parser.

@sandersn sandersn removed the Duplicate An existing issue was already created label Apr 26, 2018
@mhegazy mhegazy added the Bug A bug in TypeScript label Apr 26, 2018
@mhegazy mhegazy added the Domain: JSDoc Relates to JSDoc parsing and type generation label Apr 26, 2018
@mhegazy mhegazy added this to the Future milestone Apr 26, 2018
@tschaub
Copy link
Contributor

tschaub commented Aug 6, 2018

See also #26067. As mentioned there, it seems like it would be easier to group the asterisk together with the newline/whitespace trivia while scanning (and set a scanner flag about the linebreak).

@tschaub
Copy link
Contributor

tschaub commented Aug 18, 2018

It feels like this could be implemented with a relatively minor change - if it were possible to differentiate between scanning a comment and scanning code:

diff --git a/src/compiler/scanner.ts b/src/compiler/scanner.ts
index 61ce8330bf..71541f356c 100644
--- a/src/compiler/scanner.ts
+++ b/src/compiler/scanner.ts
@@ -1447,6 +1447,10 @@ namespace ts {
                             return pos += 2, token = SyntaxKind.AsteriskAsteriskToken;
                         }
                         pos++;
+                        if (skipTrivia && (tokenFlags & TokenFlags.PrecedingLineBreak)) {
+                            // decoration at the start of a JSDoc comment line
+                            continue;
+                        }
                         return token = SyntaxKind.AsteriskToken;
                     case CharacterCodes.plus:
                         if (text.charCodeAt(pos + 1) === CharacterCodes.plus) {

This change fixes comment parsing but breaks code parsing (generator methods and any other multiline expression where the line starts with a *). It may also break the indent handling when parsing comments.

So far I don't see an easy way to skip * after newline only while parsing JSDoc types - it feels like there are too many branches of the type parsing to cover.

@sandersn - any thoughts on how to provide the scanner with more context (similar to skipTrivia)? Or any other ideas?

@tschaub
Copy link
Contributor

tschaub commented Aug 18, 2018

In case the "provide the scanner with more context" idea sounds like a good one, I've pushed up a branch that makes things work with multi-line JSDoc types in #26528.

@tschaub
Copy link
Contributor

tschaub commented Sep 2, 2018

All tests are now passing on #26528. I've also added cases to demonstrate that #26846 is fixed by the same.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript Domain: JSDoc Relates to JSDoc parsing and type generation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants