Skip to content

Conversation

@tschaub
Copy link
Contributor

@tschaub tschaub commented Jul 30, 2018

Skip the newline, whitespace, asterisk combo while parsing param tags.

Fixes #26027

@sandersn
Copy link
Member

Which tests break without the first clause? I'd like to avoid lookahead whenever possible, but I don't know how essential it is here.

(You can run the tests with jake runtests-parallel and then diff the results in $ts/tests/baselines/local vs $ts/tests/baselines/reference.)

@tschaub
Copy link
Contributor Author

tschaub commented Jul 31, 2018

With the change in 4599356, here are the changes to the baselines...

Update - I pushed a new commit that passes all current tests.

@tschaub
Copy link
Contributor Author

tschaub commented Jul 31, 2018

@sandersn without a lookahead, this sort of thing would be allowed:

/** 
 * @param {string} * x
 * @param {number} y
 */
function f(x,y) {
}

Update - The most recent commit avoids a lookahead, but treats the above syntax as invalid. More below.

@tschaub tschaub force-pushed the fix-26027 branch 3 times, most recently from 72ca595 to 8e10dfc Compare July 31, 2018 02:41
@tschaub
Copy link
Contributor Author

tschaub commented Jul 31, 2018

My take right now is that when parsing JSDoc comments, you generally want to skip whitespace or newline, whitespace, asterisk together. As things are now, after calling skipWhitespace, if the current token is * it is not possible to distinguish between the decoration at the beginning of a line and a potentially significant * somewhere mid-comment.

The scanner.hasPrecedingLineBreak() check is nice. This would allow to distinguish between a * that comes after skipping whitespace except that the token flags aren't consistently maintained during scanJSDocToken.

The changes here parse this type of syntax:

/** 
 * @param
 * {string} x
 * @param {string}
 * y
 */
function f(x,y) {
}

And these are invalid:

/** 
 * @param {string} * x
 * @param * {string} y
 */
function f(x,y) {
}

If this looks like a good direction to go, I'll add tests.

The sometimes-reliable preceding linebreak check also feels fragile - it is not correct after a call to skipWhitespace - and I'm wondering if it makes sense to always skip whitespace or asterisk (when preceded by linebreak and whitespace).

@tschaub tschaub force-pushed the fix-26027 branch 2 times, most recently from 174879a to 7f5c439 Compare July 31, 2018 16:11
@tschaub
Copy link
Contributor Author

tschaub commented Jul 31, 2018

I added a conformance test.

Since skipping newline + asterisk would be nice elsewhere, I'm wondering if alternative approaches have been considered - like grouping it into a single trivia token during scanning. Perhaps nobody has asked for it yet, but it would be nice to be able to have linebreaks in typedefs for example:

/**
 * @typedef {{
 *    x: number,
 *    y: number,
 *    z: number
 * }} XYZ
 */

I think this would be more straightforward if the newline + asterisk were treated just like a single newline token while parsing the type expression.

Update: I see the linebreak in types issue is already ticked as #23667.

@tschaub tschaub changed the title WIP: Skip whitespace or asterisk in JSDoc param type and name Skip whitespace and asterisk in JSDoc param Jul 31, 2018
@sandersn
Copy link
Member

The typedef problem is unfortunately much harder because it uses the normal Typescript parser, so you'd have to make type parsing skip arbitrary asterisks. I don't think asterisk is a common character in types, so it might work.
Another approach would be to preprocess the jsdoc comment and rescan it for type (and other) parsing. But I think that would be quite expensive.

@sandersn sandersn merged commit 6fd725f into microsoft:master Aug 16, 2018
@sandersn
Copy link
Member

Thanks for the fix @tschaub!

@tschaub tschaub deleted the fix-26027 branch August 17, 2018 13:55
@microsoft microsoft locked as resolved and limited conversation to collaborators Oct 21, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants