Fixes to @augments handling #18775

ghost · 2017-09-26T20:42:34Z

We will now get a superclass from @augments even if there is no extends.

Allows us to parse /** @augments A */ without the curly braces, because that seems to be the style.

We will no longer parse arbitrary types, only a class (qualified) name followed by type arguments. So you can't declare to inherit from () => void without parse errors.

Adds errors for:

@augments tag not on a class
@augments A but extends B

ghost · 2017-09-26T20:44:20Z

src/compiler/checker.ts

+                return;
+            }
+
+            const name: Identifier = node.class.expression.kind === SyntaxKind.PropertyAccessExpression ? (node.class.expression as PropertyAccessExpression).name : (node.class.expression as Identifier);


Waiting on #18774 to get rid of these casts

sandersn · 2017-09-28T17:12:10Z

src/compiler/checker.ts

+            }
+        }
+
+        function getClassName(node: Expression): Identifier | undefined {


This feels like it already exists. Either way, getClassName is too vague+specific a name for something that treats an Expression like a QualifiedName and then gets the identifier at the bottom. getIdentifierFromExpressionAsQualifiedName? That's horrible in a different way. I'm not sure what the best name is.

sandersn · 2017-09-28T17:15:27Z

src/compiler/utilities.ts

@@ -1591,6 +1590,11 @@ namespace ts {
        return parameter && parameter.symbol;
    }

+    export function getNodeCommentedByJSDocTag(node: JSDocTag): Node {


we should really make up a name for "the node that this jsdoc is attached to". Commentee? Given that the parameter name and type could be tag: JSDocTag, I think getCommentee would be a fine name, given a good word for "Commentee".

I've been calling it the "host" node in my internal monologue.

sandersn · 2017-09-28T17:17:05Z

src/compiler/diagnosticMessages.json

+        "category": "Error",
+        "code": 8022
+    },
+    "JSDoc '@augments' tag declares to extend '{0}', but actual class augments '{1}'.": {


JSDoc '@Augments {0}' does not match the 'extends {1}' clause.

sandersn · 2017-09-28T17:17:33Z

src/compiler/diagnosticMessages.json

@@ -3511,6 +3511,14 @@
        "category": "Error",
        "code": 8021
    },
+    "JSDoc '@augments' tag will be ignored if not attached to a class declaration.": {


JSDoc '@Augments' tag is not attached to a class declaration.

sandersn · 2017-09-28T17:18:43Z

src/compiler/checker.ts

+                return;
+            }
+
+            const name = node.class.expression.kind === SyntaxKind.PropertyAccessExpression ? node.class.expression.name : node.class.expression;


why isn't this a call to getClassName too?

sandersn · 2017-09-28T17:24:44Z

src/compiler/scanner.ts

@@ -1856,6 +1856,12 @@ namespace ts {
                case CharacterCodes.closeBracket:
                    pos++;
                    return token = SyntaxKind.CloseBracketToken;
+                case CharacterCodes.lessThan:


can you make sure that we have some tests that use < and > inside jsdoc comments in various places? it should be allowed everywhere in comment text, especially in misaligned cases like

/** * @param x hi < > still part of the previous comment */

And of course it should cause error in places like the tag name, etc.

Added a test.

sandersn · 2017-09-28T17:26:40Z

src/compiler/types.ts

@@ -2161,7 +2161,7 @@ namespace ts {

    export interface JSDocAugmentsTag extends JSDocTag {
        kind: SyntaxKind.JSDocAugmentsTag;
-        typeExpression: JSDocTypeExpression;
+        class: ExpressionWithTypeArguments & { expression: Identifier | PropertyAccessEntityNameExpression };


don't you want a subclass of ExpressionWithTypeArguments that has the override expression: Identifier | PropertyAccessEntityNameExpression ? Seems like intersection would be too permissive.

How is intersection permissive where extends isn't? Could you give an example?

I forgot that Identifier is assignable to Expression & Identifier but (for example) BinaryExpression isn't. Never mind.

sandersn · 2017-09-28T17:28:31Z

src/compiler/parser.ts

                    return finishNode(result);
                }

+                function parseValidExpressionWithTypeArguments(): ExpressionWithTypeArguments & { expression: Identifier | PropertyAccessEntityNameExpression } {


this doesn't parse all valid expressions, only PropertyAccessEntityNameExpressions, right? This name is confusing.

weswigham · 2017-09-28T18:30:58Z

src/compiler/utilities.ts

@@ -1591,6 +1590,11 @@ namespace ts {
        return parameter && parameter.symbol;
    }

+    export function getJSDocHost(node: JSDocTag): Node {


This can have a return type of HasJSDoc instead of Node. 😉

minestarks · 2017-09-28T20:13:01Z

@Andy-MS <> aren't legal characters on Windows file systems. I can't git pull after this commit:

E:\src\TypeScript>git pull upstream master
From https://github.com/Microsoft/TypeScript
 * branch                  master     -> FETCH_HEAD
error: unable to create file tests/baselines/reference/JSDocParsing/DocComments.parsesCorrectly.<> characters.json: Invalid argument

aozgaa · 2017-10-06T22:23:05Z

src/compiler/checker.ts

+        function checkJSDocAugmentsTag(node: JSDocAugmentsTag): void {
+            const cls = getJSDocHost(node);
+            if (!isClassDeclaration(cls) && !isClassExpression(cls)) {
+                error(cls, Diagnostics.JSDoc_augments_is_not_attached_to_a_class_declaration);


Should this message mention class expressions?

ghost requested a review from sandersn September 26, 2017 20:42

ghost force-pushed the jsdoc_augments branch from f0bedf7 to e056e83 Compare September 26, 2017 20:43

ghost commented Sep 26, 2017

View reviewed changes

Fixes to @Augments handling

41e239d

ghost force-pushed the jsdoc_augments branch from e056e83 to 41e239d Compare September 26, 2017 21:01

This was referenced Sep 26, 2017

support @extends in jsdoc #18706

Merged

Make it a noImplicitAny error to fail to provide type arguments to a superclass via @augments #18778

Merged

sandersn requested changes Sep 28, 2017

View reviewed changes

Renames and diagnostic changes

171c054

ghost force-pushed the jsdoc_augments branch from ca1bfda to 171c054 Compare September 28, 2017 18:25

Add test for < > characters

cbdc4a7

weswigham reviewed Sep 28, 2017

View reviewed changes

Use more specific return type

d2e4fed

sandersn approved these changes Sep 28, 2017

View reviewed changes

ghost merged commit 1a2de72 into master Sep 28, 2017

ghost deleted the jsdoc_augments branch September 28, 2017 19:34

This was referenced Sep 28, 2017

Rename test baseline to be compatibile with windows #18827

Merged

Fix bug: Report errors on extends expression in JS even if an @augments tag is present #18854

Merged

aozgaa reviewed Oct 6, 2017

View reviewed changes

ghost mentioned this pull request Oct 7, 2017

Improve JSDoc @augments diagnostics #19011

Merged

microsoft locked and limited conversation to collaborators Jun 14, 2018

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes to @augments handling #18775

Fixes to @augments handling #18775

ghost commented Sep 26, 2017

ghost Sep 26, 2017

sandersn Sep 28, 2017

sandersn Sep 28, 2017

weswigham Sep 28, 2017

sandersn Sep 28, 2017

sandersn Sep 28, 2017

sandersn Sep 28, 2017

sandersn Sep 28, 2017

ghost Sep 28, 2017

sandersn Sep 28, 2017

ghost Sep 28, 2017

sandersn Sep 28, 2017

sandersn Sep 28, 2017

weswigham Sep 28, 2017

minestarks commented Sep 28, 2017

aozgaa Oct 6, 2017

Fixes to @augments handling #18775

Fixes to @augments handling #18775

Conversation

ghost commented Sep 26, 2017

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

Choose a reason for hiding this comment

minestarks commented Sep 28, 2017

Choose a reason for hiding this comment