Set startPos at EOF in jsdoc token scanner so node end positions for nodes terminated at EoF are right #24184

weswigham · 2018-05-16T23:40:37Z

Fixes an incremental parsing crash @amcasey reported.

Note that EoF for jsdoc comments also includes end of comment - our jsdoc parsing tests just don't include incremental validations.

…nodes terminated at EoF are right

mhegazy · 2018-05-16T23:50:00Z

@sandersn can you take a look

mhegazy · 2018-05-17T15:51:29Z

the node6 jest issue is still happening..

sandersn

One change and one question:

sandersn · 2018-05-17T15:47:14Z

src/compiler/parser.ts

+                        return true;
+                    }
+                    if (token() === SyntaxKind.WhitespaceTrivia || token() === SyntaxKind.NewLineTrivia) {
+                        return lookAhead(isNextNonwhitespaceTokenEndOfFile); // We must use infinte lookahead, as there could be any number of newlines :(


typo:infinite

why do we need lookahead inside of lookahead? Is there a way to get around that?

Yeah, I think you should be able to return isNextNonwhitespaceTokenEndOfFile() instead

Even just recursion for each line/whitespace seems suboptimal, but preferable, and it not like you'll have thousands of blank lines in a source file (hopefully!)

Yeah. It's just a little unfortunate that jsdoc parsing needs infinite lookahead - previously only arrow functions needed that.

Actually, yes, without nested lookaheads, this should just be a loop.

sandersn · 2018-05-17T15:49:51Z

src/services/classifier.ts

                            break;
                    }
-
-                    pos = tag.end;


what prompts this change?

There's a test expecting a single classification for the trailing whitespace and */ on a comment. With the parse change, the trailing whitespace isn't part of the tag name/type, but is part of the overall tag itself (here, tag) - so using the end from the tag itself was causing us to skip the whitespace in the output. By the way, can I add that whitespace parsing in jsdoc is really inconsistent? Where whitespace is consumed vs isn't is very... Arbitrary? I feel like only newlines should really be significant in jsdoc - do you know why were scanning whitespace into tokens at all?

/** * @param x Comment starts here * - indented * - indented */

Needs to produce a comment text that looks like

Comment starts here - indented - indented

That is, the second lines need to be indented two spaces, not 0 and not 17 and not 16.

But... why are we doing that indentation analysis with token scanning and not with string manipulation on the actual comment text? We already manipulate it to strip leading and trailing newlines.

sandersn · 2018-05-17T15:53:03Z

tests/cases/fourslash/incrementalJsDocAdjustsLengthsRight.ts

+/// <reference path="fourslash.ts" />
+
+// @noLib: true
+////function camelcase(flag) {


lol irony in naming

billti · 2018-05-17T17:10:12Z

tests/baselines/reference/JSDocParsing/DocComments.parsesCorrectly.noReturnType.json

@@ -6,7 +6,7 @@
        "0": {
            "kind": "JSDocReturnTag",
            "pos": 8,
-            "end": 16,
+            "end": 15,


Odd to see all these end position shift sometimes +2, sometime +1, and sometimes -1. I assume you've done a spot check and the new positions are (hopefully more) correct in each case?

Yeah, the end just no longer includes the trailing whitespace at the end of the comment.

billti · 2018-05-17T17:14:55Z

tests/cases/fourslash/incrementalJsDocAdjustsLengthsRight.ts

+//// * @param {String} str
+//// * @param {Number} wid/*1*/
+goTo.marker('1');
+edit.insert("th\n@");


Is there anything worth verifying here beyond "it doesn't crash"? I don't see it checking any lengths have been adjusted right (per the name).

Just the not crashing. The position being wrong causes crashing.

I get that was the issue being addressed. I'm just suggesting that if you're going to add a fourslash test (which aren't cheap to run), might as well make it more useful and actually verify the JsDoc tags/spans also. It's a pretty narrow/specific test as-is that adds little value once this specific bug is fixed.

billti

Minor comments, else looks good.

billti · 2018-05-17T17:24:34Z

tests/baselines/reference/JSDocParsing/DocComments.parsesCorrectly.paramTagBracketedName2.json

@@ -6,7 +6,7 @@
        "0": {
            "kind": "JSDocParameterTag",
            "pos": 8,
-            "end": 62,
+            "end": 64,


Per your other comment - why does this one get longer then?

Description scanning is including trailing whitespace now that the comment end token's start pos is fixed - nonfinal descriptions already had positions that included trailing whitespace like this, so.... 🤷‍♂️

TBH, I'd prefer if whitespace was nonsignificant in jsdoc parsing and only handled newlines - that's require some changes to how we handle descriptions, though (we'd need to scan them more like jsx text).

sandersn · 2018-05-17T18:24:26Z

src/compiler/parser.ts

-                    }
-                    if (token() === SyntaxKind.WhitespaceTrivia || token() === SyntaxKind.NewLineTrivia) {
-                        return lookAhead(isNextNonwhitespaceTokenEndOfFile); // We must use infinte lookahead, as there could be any number of newlines :(
+                    // We must use infinte lookahead, as there could be any number of newlines :(


still has typo 😅

sandersn · 2018-05-17T18:24:55Z

src/compiler/parser.ts

+                            return true;
+                        }
+                        if (!(token() === SyntaxKind.WhitespaceTrivia || token() === SyntaxKind.NewLineTrivia)) {
+                            break;


isn't this just return false? If so, I think that's easier to read.

weswigham · 2018-05-17T20:11:10Z

@sandersn You may wanna double check the most recent change - I had to do some ugly stuff to keep typdef end positions in the right place because parseTagComments consumes tokens even if it doesn't capture anything. I suppose I could also just wrap parseTagComments in a tryParse to reset the scanner if it fails, but that's not great, either.

sandersn · 2018-05-17T20:34:50Z

src/compiler/parser.ts

@@ -6848,10 +6849,12 @@ namespace ts {
                            typedefTag.typeExpression = childTypeTag && childTypeTag.typeExpression && !isObjectOrObjectArrayTypeReference(childTypeTag.typeExpression.type) ?
                                childTypeTag.typeExpression :
                                finishNode(jsdocTypeLiteral);
+                                end = jsdocTypeLiteral.end;


is the indent on this statement correct? It looks weird to me.

sandersn · 2018-05-17T20:35:11Z

src/compiler/parser.ts

@@ -6820,6 +6820,7 @@ namespace ts {
                    typedefTag.comment = parseTagComments(indent);

                    typedefTag.typeExpression = typeExpression;
+                    let end: number;


does this quirk apply to callback tags too?

From our discussion: yes, and nested @param types too, but the problem isn't bad enough to fix right now. Instead we should fix it by fixing the jsdoc scanner to ignore whitespace (and change the way we generate tag descriptions).

@typedef

* Optimize intersections of unions of unit types * Add regression test * Accept new baselines * LEGO: check in for master to temporary branch. * Add Unicode ThirdPartyNotice * Properly handle edge cases * Sort the whole diagnostic, plus giving up on isolating tests (#24186) * Sort the whole diagnostic * Also strip references to our repos node_modules, since removing it is hard * LEGO: check in for master to temporary branch. * add quick fix for import type missing typeof * Add callback tag, with type parameters (#23947) * Add initial tests * Add types * Half of parsing (builds but does not pass tests) * Parsing done; types are uglier; doesn't crash but doesn't pass * Bind callback tag Builds but tests still don't pass * Only bind param tags inside callback tags * Fix binding switch to only handle param tags once * Checking is 1/3 done or so. Now I'm going to go rename some members to be more uniform. I hate unnnecessary conditionals. * Rename typeExpression to type (for some jsdoc) (maybe I'll rename more later) * Rename the rest of typeExpressions Turns out there is a constraint in services such that they all need to be named the same. * Few more checker changes * Revert "Rename the rest of typeExpressions" This reverts commit f41a96b. * Revert "Rename typeExpression to type (for some jsdoc)" This reverts commit 7d2233a. * Finish undoing typeExpression rename * Rename and improve getTypeParametersForAliasSymbol Plus some other small fixes * Core checking works, but is flabbergastingly messy I'm serious. * Callback return types work now * Fix crash in services * Make github diff smaller * Try to make github diff even smaller * Fix rename for callback tag * Fix nav bar for callback tag Also clean up some now-redundant code there to find the name of typedefs. * Handle ooorder callback tags Also get rid of redundant typedef name code *in the binder*. It's everywhere! * Add ooorder callback tag test * Parse comments for typedef/callback+display param comments * Always export callbacks This requires almost no new code since it is basically the same as typedefs * Update baselines * Fix support for nested namespaced callbacks And add test * Callbacks support type parameters 1. Haven't run it with all tests 2. Haven't tested typedef tags yet 3. Still allows shared symbols when on function or class declarations. * Template tags are now bound correctly * Test oorder template tags It works. * Parser cleanup * Cleanup types and utilities As much as possible, and not as much as I would like. * Handle callback more often in services * Cleanup of binder and checker * More checker cleanup * Remove TODOs and one more cleanup * Support parameter-less callback tags * Remove extra bind call on template type parameters * Bind template tag containers Doesn't quite work with typedefs, but that's because it's now stricter, without the typedef fixes. I'm going to merge with jsdoc/callback and see how it goes. * Fix fourslash failures * Stop pre-binding js type aliases Next up, stop pre-binding js type parameters * Further cleanup of delayed js type alias binding * Stop prebinding template tags too This gets rid of prebinding entirely * Remove TODO * Fix lint * Finish merge with use-jsdoc-aliases * Update callback tag baselines * Rename getTypeParametersForAliasSymbol The real fix is *probably* to rename Type.aliasTypeArguments to aliasTypeParameters, but I want to make sure and then put it in a separate PR. * moveToNewFile: Don't move imports (#24177) * Reduce map lookups (#24203) * Fix jsdoc type resolution [merge to master] (#24204) * Fix JSDoc type resolution Breaks type parameter resolution that is looked up through prototype methods, though. I need to fix that still. * Check for prototype method assignments first * Undo dedupe changes to getJSDocTags * JS Type aliases can't refer to host type params Previously, js type aliases (@typedef and @callback) could refer to type paremeters defined in @template tags in a *different* jsdoc tag, as long as both tags were hosted on the same signature. * Reduce dedupe changes+update baseline The only reason I had undone them was to merge successfully with an older state of master. * Add undefined guard * moveToNewFile: Fix bug for VariableDeclaration missing initializer (#24214) * Use import types to refer to declarations in declaration emit (#24071) * Stand up a simple implementation using import types for exports of modules which are otherwise inaccessible * Ensure references exist to link to modules containing utilized ambient modules * Accept baselines with new import type usage * Fix lint * Accept changed baseline (#24222) * fixUnusedIdentifier: Don't delete node whose ancestor was already deleted (#24207) * More robust circularity detection in node builder (#24225) * Improve ChangeTracker#deleteNodeInList (#24221) * moveToNewFile: Fix bug for missing importClause (#24224) * Set startPos at EOF in jsdoc token scanner so node end positions for nodes terminated at EoF are right (#24184) * Set startPos at EOF in jsdoc token scanner to node end positions for nodes terminated at EoF are right * More complete nonwhitespace token check, fix syntactica jsdoc classifier * Use loop and no nested lookahead * Do thigns unrelated to the bug in the test * Fix typo move return * Patch up typedef end pos * Fix indentation, make end pos target more obvious

Set startPos at EOF in jsdoc token scanner to node end positions for …

02f2081

…nodes terminated at EoF are right

weswigham requested review from a user, sandersn, billti, mhegazy and amcasey May 16, 2018 23:41

mhegazy approved these changes May 16, 2018

View reviewed changes

More complete nonwhitespace token check, fix syntactica jsdoc classifier

525d091

sandersn changed the title ~~Set startPos at EOF in jsdoc token scanner to node end positions for nodes terminated at EoF are right~~ Set startPos at EOF in jsdoc token scanner so node end positions for nodes terminated at EoF are right May 17, 2018

sandersn requested changes May 17, 2018

View reviewed changes

sandersn reviewed May 17, 2018

View reviewed changes

billti reviewed May 17, 2018

View reviewed changes

billti approved these changes May 17, 2018

View reviewed changes

billti reviewed May 17, 2018

View reviewed changes

weswigham added 2 commits May 17, 2018 11:09

Use loop and no nested lookahead

de173ee

Do thigns unrelated to the bug in the test

238cb59

sandersn reviewed May 17, 2018

View reviewed changes

weswigham added 2 commits May 17, 2018 11:26

Fix typo move return

f3afefe

Merge branch 'master' into fourslash-incremental-incrunundrum

880ab4a

sandersn approved these changes May 17, 2018

View reviewed changes

Patch up typedef end pos

8eabd89

sandersn approved these changes May 17, 2018

View reviewed changes

weswigham added 2 commits May 17, 2018 14:01

Fix indentation, make end pos target more obvious

f37086d

Merge branch 'master' into fourslash-incremental-incrunundrum

9318641

weswigham mentioned this pull request May 17, 2018

Revise whitespace handling in JSDoc #24217

Open

weswigham merged commit d82d35c into microsoft:master May 17, 2018

weswigham deleted the fourslash-incremental-incrunundrum branch May 17, 2018 22:16

microsoft locked and limited conversation to collaborators Jul 31, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set startPos at EOF in jsdoc token scanner so node end positions for nodes terminated at EoF are right #24184

Set startPos at EOF in jsdoc token scanner so node end positions for nodes terminated at EoF are right #24184

weswigham commented May 16, 2018

mhegazy commented May 16, 2018

mhegazy commented May 17, 2018

sandersn left a comment

sandersn May 17, 2018

sandersn May 17, 2018

sandersn May 17, 2018

billti May 17, 2018

weswigham May 17, 2018

sandersn May 17, 2018

sandersn May 17, 2018

weswigham May 17, 2018

sandersn May 17, 2018 •

edited

Loading

weswigham May 17, 2018 •

edited

Loading

sandersn May 17, 2018

billti May 17, 2018

weswigham May 17, 2018

billti May 17, 2018

weswigham May 17, 2018

billti May 17, 2018

billti left a comment

billti May 17, 2018

weswigham May 17, 2018

sandersn May 17, 2018

sandersn May 17, 2018

weswigham commented May 17, 2018 •

edited

Loading

sandersn May 17, 2018

sandersn May 17, 2018

sandersn May 17, 2018

Set startPos at EOF in jsdoc token scanner so node end positions for nodes terminated at EoF are right #24184

Set startPos at EOF in jsdoc token scanner so node end positions for nodes terminated at EoF are right #24184

Conversation

weswigham commented May 16, 2018

mhegazy commented May 16, 2018

mhegazy commented May 17, 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

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 May 17, 2018 • edited Loading

Choose a reason for hiding this comment

weswigham May 17, 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

billti left a 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

weswigham commented May 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sandersn May 17, 2018 •

edited

Loading

weswigham May 17, 2018 •

edited

Loading

weswigham commented May 17, 2018 •

edited

Loading