Parse jsdoc with normal TS type parser by sandersn · Pull Request #17176 · microsoft/TypeScript

sandersn · 2017-07-13T18:38:03Z

This means that JSDoc types can include the full range of Typescript types now. It also means that Typescript annotations can include JSDoc types. This is disallowed with a new error, however. But Typescript can still give the correct types to JSDoc that shows up in .ts files by mistake. This can easily happen, for example with types like:

var x: number? = null;
var y: ?string = null;
var z: function(string,string): string = (s,t) => s + t;

// less likely to show up, but still understood.
var ka: ? = 1;

This change saves around 300 lines of code and some of the confusion from having exact duplicate parsing and types for JSDoc vs normal types. Now only JSDoc types and syntax that differ from Typescript's need to be handled separately.

In the future, I will add a quick fix to convert these into the correct types.

Fixes #16550

This means that JSDoc types can include the full range of Typescript types now. It also means that Typescript annotations can include JSDoc types. This is disallowed with a new error, however. But Typescript can still give the correct types to JSDoc that shows up in .ts files by mistake. This can easily happen, for example with types like ```ts var x: number? = null; var y: ?string = null; var z: function(string,string): string = (s,t) => s + t; // less likely to show up, but still understood. var ka: ? = 1; ``` In the future, I will add a quick fix to convert these into the correct types. Fixes #16550

sandersn · 2017-07-13T20:22:47Z

@Andy-MS asked for fourslash tests for function(this: T, string): string and function(new: T, string) for the following areas:

parameter help for these functions
completions in the scope of these functions
find all refs on the new parameter should not crash (true of JSDoc function parameters)
find all refs on the this parameter should find all uses of this in the function.

If any of these fails I will probably file issues instead of fixing in this PR because (1) It's not clear how much of this worked before because the binder didn't handle these parameters, and (2) I'm not sure anybody even uses this syntax.

weswigham · 2017-07-13T19:41:02Z

                parsesCorrectly("tupleType3", "{[number,string,boolean]}");
+                parsesCorrectly("tupleTypeWithTrailingComma", "{[number,]}");
+                parsesCorrectly("typeOfType", "{typeof M}");
+                parsesCorrectly("tsConstructoType", "{new () => string}");


Typo: constructo rather than constructor.

ghost · 2017-07-13T21:09:12Z

                        paramSymbol = resolvedSymbol;
                    }
-                    if (i === 0 && paramSymbol.name === "this") {
+                    if (i === 0 && paramSymbol.name === "this" || (param.type && param.type.kind === SyntaxKind.JSDocThisType)) {


I'm not a fan of JSDocThisType. It looks like you would have a parameter with no name, and a type of kind JSDocThisType, where the type itself has a .type property storing the real type.
I think this: T should be parsed to the same AST whether we're in TypeScript or in JSDoc, so you don't need a condition for both here.

I removed JSDocThisType and -ConstructorType like you suggested.

ghost · 2017-07-13T21:21:28Z

        return false;
    }

+    export function isJSDocTypeReference(node: TypeReferenceType): node is TypeReferenceNode {


This is going to be super slow if we're not in jsdoc -- for any type reference not in jsdoc we will climb the whole tree looking for a JSDocTypeExpression that never comes. Can you use isInJavaScriptFile?

That's correct. Any ideas for a better implementation?

See edited comment.

isInJavaScriptFile may not be sufficient because JSDoc types can appear anywhere in TS or JS files, inside or outside JSDoc. For example:

/** @type {...number} */ var x; var y: ...number; // annotations are errors in JS file, but still fully checked and treated as number[]

and

/** @type {...number} */ // actually, this isn't checked. var x; var y: ...number; // ... is an error in TS file, but still fully checked and treated as number[]

So if you have let o: ...object; in a JS file, you will see object and climb up the tree to determine whether it's in a JSDoc comment? Since that's a grammar error anyway, I don't think it's a big deal if we give the object there JSDoc semantics instead of TS semantics. Sure it appears in a TypeScript type position, but it's in a JS file, so we might as well treat it the same as any other JSDoc type (and issue a grammar error).

There's a JSDoc Parser context flag now, right? Can it not just be persisted into all nodes as a node flag when they are created within a jsdoc context? So you just check if your node has the flag, and if so, then its inside a jsdoc and has jsdoc semantics?

Yep, I realised that last night. In fact, finishNode in the parser persists the context flag, so it was there all along.

ghost · 2017-07-13T21:37:47Z

                case SyntaxKind.ObjectKeyword:
-                    return nonPrimitiveType;
+                    if (node.flags & NodeFlags.JavaScriptFile) {
+                        return anyType;


There seems to be duplicate code in getPrimitiveTypeFromJSDocTypeReference, are these both needed?

getPrimitiveTypeFromJSDocTypeReference operates on TypeReferenceNode not on a TypeNode with SyntaxKind.Object. Actually, the "object" case there no longer applies since it @param {object} x is no longer parsed as a type reference.

ghost · 2017-07-13T21:39:50Z


        function checkTypeReferenceNode(node: TypeReferenceNode | ExpressionWithTypeArguments) {
            checkGrammarTypeArguments(node, node.typeArguments);
+            if (node.kind === SyntaxKind.TypeReference && node.typeName.jsdocDot && !isInJavaScriptFile(node) && !findAncestor(node, n => n.kind === SyntaxKind.JSDocTypeExpression)) {


Another findAncestor call here. We shouldn't be checking a type reference in a jsdoc comment if we're not in a JS file anyway, right?

ghost · 2017-07-13T21:41:35Z

        function checkTypeReferenceNode(node: TypeReferenceNode | ExpressionWithTypeArguments) {
            checkGrammarTypeArguments(node, node.typeArguments);
+            if (node.kind === SyntaxKind.TypeReference && node.typeName.jsdocDot && !isInJavaScriptFile(node) && !findAncestor(node, n => n.kind === SyntaxKind.JSDocTypeExpression)) {
+                grammarErrorOnNode(node, Diagnostics.JSDoc_types_can_only_be_used_inside_documentation_comments);


So if I write: let x: N.M.O.<P, Q>; I'll get a grammar error on the whole node... it would be hard to spot the extra .. Could we try to calculate the range between O and < for the error, or use a more specific message?

I'll just save the dots as we parse them and retain it for this error in the jsdoc case.

Actually, that creates additional garbage from the dots we don't use, which is all dots except the last one in a JSDoc-syntax generic. These dots are relatively very rare, so I will save the start and end instead and manually specify the error span.

ghost · 2017-07-13T21:42:11Z

            }
        }

+        function checkJsDoc(node: FunctionDeclaration | MethodDeclaration) {


This could be inlined. forEach(node.jsDoc, checkSourceElement).

I'd like to keep the function around for readability and because it may someday be used from more locations.

ghost · 2017-07-13T21:46:04Z

+                case SyntaxKind.JSDocNullableType:
+                case SyntaxKind.JSDocAllType:
+                case SyntaxKind.JSDocUnknownType:
+                    if (!isInJavaScriptFile(node) && !findAncestor(node, n => n.kind === SyntaxKind.JSDocTypeExpression)) {


Another findAncestor here

ghost · 2017-07-13T21:59:56Z

+        }
+
        function checkFunctionOrMethodDeclaration(node: FunctionDeclaration | MethodDeclaration): void {
+            checkJsDoc(node);


Seems silly to call this in a TS file if the checkers for every possible jsdoc element will test if we're in a TS file and then do nothing?

Good point. I moved the check into checkJSDoc

ghost · 2017-07-14T15:01:14Z

        // The allowReservedWords parameter controls whether reserved words are permitted after the first dot
        function parseEntityName(allowReservedWords: boolean, diagnosticMessage?: DiagnosticMessage): EntityName {
-            let entity: EntityName = parseIdentifier(diagnosticMessage);
+            let entity: EntityName = allowReservedWords ? parseIdentifierName() : parseIdentifier(diagnosticMessage);


Why this change? So we can write const.const?
Comment needs an update if so.

Yes, you can say @param {const} number in JSDoc. I'll update the comment.

Actually, without the "right hand side" qualification, it's a garbage comment ("allowReservedWords allows reserved words!"). I deleted it.

ghost · 2017-07-14T15:06:55Z

+            return finishNode(parameter);
+        }
+
+        function parseJSDocNodeWithType(kind: SyntaxKind): TypeNode {


Could provide a more specific type to kind.

ghost · 2017-07-14T15:07:40Z


        function isStartOfParameter(): boolean {
-            return token() === SyntaxKind.DotDotDotToken || isIdentifierOrPattern() || isModifierKind(token()) || token() === SyntaxKind.AtToken || token() === SyntaxKind.ThisKeyword;
+            return token() === SyntaxKind.DotDotDotToken || isIdentifierOrPattern() || isModifierKind(token()) || token() === SyntaxKind.AtToken || token() === SyntaxKind.ThisKeyword || token() === SyntaxKind.NewKeyword;


If JSDoc parameters can include any type, doesn't this leave out a lot of things?

isIdentifierOrPattern and isModifierKind cover a lot of things like number, const, etc. But they happen not to cover this or new.

Can we parse this?

/** @type {function({ x: number }, 1 | 2): number} */ const f = (x, y) => x.x + y;

not 1 | 2. I will try adding isStartOfType to the end of isStartOfParameter but I'm not sure if it will be successful.

At least one test entered an infinite loop with isStartOfType inside isStartOfParameter. Instead I just added StringLiteral and NumericLiteral to the end.

ghost · 2017-07-14T15:09:30Z

        }

-        function parseFunctionOrConstructorType(kind: SyntaxKind): FunctionOrConstructorTypeNode {
+        function parseFunctionOrConstructorType(kind: SyntaxKind): FunctionOrConstructorTypeNode | JSDocConstructorType {


Could provide a more specific type to kind. I only see this being called with kind of SyntaxKind.FunctionType of SyntaxKind.ConstructorType, never SyntaxKind.JSDocConstructorType.

ghost · 2017-07-14T15:10:43Z

            if (kind === SyntaxKind.ConstructorType) {
                parseExpected(SyntaxKind.NewKeyword);
+                if (token() === SyntaxKind.ColonToken) {
+                    // JSDoc -- `new:T` as in `function(new:T, string, string)`; an infix constructor-return-type


What happens if we see new:T and it's not inside of function()?

ghost · 2017-07-14T15:22:06Z

            return token() === SyntaxKind.CloseParenToken || isStartOfParameter() || isStartOfType();
        }

+        function parseJSDocPostfixTypeOrHigher(): TypeNode {


Move this next to (or into) parseTypeOperatorOrHigher.
Can we parse foo?[] or foo![]? Could merge with parseArrayTypeOrHigher (also only called in one place).
I took a stab at making this neater:

function parseJSDocPostfixTypeOrHigher(): TypeNode { const type = parseArrayTypeOrHigher(); const kind = getKind(token()); if (!kind) return type; const postfix = createNode(kind, type.pos) as JSDocOptionalType | JSDocNonNullableType | JSDocNullableType; postfix.type = type; return finishNode(postfix); function getKind(tokenKind: SyntaxKind): SyntaxKind | undefined { switch (tokenKind) { case SyntaxKind.EqualsToken: // only parse postfix = inside jsdoc, because it's ambiguous elsewhere return contextFlags & NodeFlags.JSDoc ? SyntaxKind.JSDocOptionalType : undefined; case SyntaxKind.ExclamationToken: return SyntaxKind.JSDocNonNullableType; case SyntaxKind.QuestionToken: return SyntaxKind.JSDocNullableType; } } }

No, foo?[] would need to parse inside parseArrayTypeOrHigher. That does seem like it should parse as Array<foo | null>.

I don't think we should merge functions that would map to individual rules in a grammar.

Thanks for the refactor.

Seems like solving 1 would require ditching 2. If we want to be able to parse foo?[]?[]!, we would need to handle postfix types in a uniform way.

So now I can type syntactically valid jsdoc types in place of swearing.

sandersn · 2017-07-14T15:55:54Z

If you're checking out this branch on windows, note that I inadvertently created a new test jsdocInTypescript.ts that differs only in case from an existing test jsdocInTypeScript.ts. I'll fix this in the next commit, but in the meantime, Wesley found that the tests are broken.

ghost · 2017-07-14T15:40:02Z


                    return finishNode(typedefTag);

+                    function isObjectTypeReference(node: TypeNode) {


node.kind === SyntaxKind.ObjectKeyword || isTypeReferenceNode(node) && ts.isIdentifier(node.typeName) && node.typeName.text === "Object";

Much better.

ghost · 2017-07-14T15:46:18Z

 tests/cases/conformance/classes/propertyMemberDeclarations/initializerReferencingConstructorLocals.ts(6,14): error TS2339: Property 'z' does not exist on type 'C'.
-tests/cases/conformance/classes/propertyMemberDeclarations/initializerReferencingConstructorLocals.ts(7,15): error TS1003: Identifier expected.
-tests/cases/conformance/classes/propertyMemberDeclarations/initializerReferencingConstructorLocals.ts(7,20): error TS2339: Property 'z' does not exist on type 'C'.
+tests/cases/conformance/classes/propertyMemberDeclarations/initializerReferencingConstructorLocals.ts(7,15): error TS2304: Cannot find name 'this'.


This tests a TS file -- it looks like we are now parsing this as an identifier.

That's because parseEntityName now allows reserved words anywhere, not just on the right of a dot. That affects even single identifiers, but in this case it now allows this in this.x to be parsed as an identifier.

Other callers that pass allowReservedWords: true are parseTypeQuery and parseIsolatedEntityName. Plus parseTypeReference in the case of JSDoc.

I think the change is fine in parseTypeQuery since this error is at least as easy to read as the old one. parseIsolatedEntityName just seems to be used for parsing --jsxFactory, so that change should be fine too (although I'm not sure why it allows reserved words at all).

ghost · 2017-07-14T15:58:39Z

+    var weird1: new:string = {};
+        ~~~~~~
+!!! error TS2322: Type '{}' is not assignable to type 'string'.
+                   ~~~~~~~


This range seems off -- shouldn't it include new?

ghost · 2017-07-14T16:01:17Z

+// @module: commonjs
+// @filename: node.d.ts
+// @noImplicitAny: true
+declare function require(id: string): any;


This doesn't seem relevant to the test?
Also, the test name seems to imply that jsdoc types will appear inside of type annotations.

Actually, I don't think this test is testing anything unique. I'll just remove it.

sandersn · 2017-07-14T16:36:03Z

From in-person discussion with @Andy-MS: I should maybe try to check JSDocFunctionType with checkFunctionLikeDeclaration in order to further reduce duplication and improve checking thoroughness.

1. Remove checkJSDocFunctionType in favour of checkSignature. 2. Check that 'new', in addition to 'this', must be the first parameter. 3. Remove prematurely added JSDoc-quickfix test.

Also some cleanup from PR comments

sandersn · 2017-07-14T22:03:20Z

I think I've addressed all the comments up to this afternoon.

ghost · 2017-07-14T22:55:35Z

 >z : Symbol(z, Decl(functions.js, 26, 3))
 >length : Symbol(length, Decl(functions.js, 12, 27))

+/** @type {function ("a" | "b"): 1 | 2} */


This doesn't actually test the ability to parse a number literal as a parameter type, that would be function("a" | "b", 1 | 2): whatever.

sandersn added 3 commits July 13, 2017 11:27

Test JSDoc parsing using TS parser

91633cd

Update baselines

48d9012

msftclas added the cla-not-required label Jul 13, 2017

sandersn requested review from a user and rbuckton July 13, 2017 19:21

Add missing word in error message

ac478a9

weswigham reviewed Jul 13, 2017

View reviewed changes

ghost reviewed Jul 13, 2017

View reviewed changes

Add fourslash tests for function(new/this:T) syntax

d24b3a3

ghost reviewed Jul 13, 2017

View reviewed changes

ghost reviewed Jul 14, 2017

View reviewed changes

Update baselines

da5285e

sandersn added 6 commits July 14, 2017 09:36

Remove JSDocConstructor/ThisType and just use named parameters

6e861fd

Improve JSDoc function checking

f1145c3

1. Remove checkJSDocFunctionType in favour of checkSignature. 2. Check that 'new', in addition to 'this', must be the first parameter. 3. Remove prematurely added JSDoc-quickfix test.

Better JSDoc generic errors and faster isInJSDoc

40ae422

Update baselines

b2e892f

Address more PR comments

bdc3f1f

Parse more types in JSDoc function() syntax

172db13

Also some cleanup from PR comments

weswigham mentioned this pull request Jul 14, 2017

Allow JSDoc casts of parenthesized expressions in JS #17211

Closed

Merge branch 'master' into parse-jsdoc-with-ts-type-parser

61c9f98

ghost reviewed Jul 14, 2017

View reviewed changes

ghost approved these changes Jul 14, 2017

View reviewed changes

Improve test of jsdoc literal type parsing

3f60364

sandersn merged commit 240f1f1 into master Jul 17, 2017

sandersn mentioned this pull request Jul 17, 2017

Dotted type in jsdoc is not parsed correctly #16268

Closed

ghost mentioned this pull request Aug 10, 2017

Use NodeFlags to detect nodes inside with statements instead of climbing ancestors #17721

Merged

mhegazy deleted the parse-jsdoc-with-ts-type-parser branch November 2, 2017 21:04

andrewiggins mentioned this pull request May 21, 2018

Improve JSDoc comments preactjs/preact#1115

Merged

microsoft locked and limited conversation to collaborators Jun 14, 2018


		return finishNode(typedefTag);

		function isObjectTypeReference(node: TypeNode) {

Conversation

sandersn commented Jul 13, 2017

Uh oh!

sandersn commented Jul 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost Jul 13, 2017 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost Jul 14, 2017 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sandersn commented Jul 13, 2017 •

edited

Loading

ghost Jul 13, 2017 •

edited by ghost

Loading

ghost Jul 14, 2017 •

edited by ghost

Loading

ghost Jul 14, 2017 •

edited by ghost

Loading

ghost Jul 14, 2017 •

edited by ghost

Loading