Template tag allows specification of constraints #24600

sandersn · 2018-06-01T22:09:04Z

This change introduces new syntax to allow the @template tag to specify constraints on type parameters, which is used in JSDoc to declare type parameters.

Fixes #24283

Basic syntax

The constraint is a jsdoc type expression surrounded by curly braces. The type parameter name follows, not surrounded by curlies:

/**
 * @template {Constraint} T COMMENT this is the same as the following Typescript syntax.
 * @typedef {{ t: T }} Obj
 */
// type Obj<T extends Constraint> = { t: T }

Notable decisions

Which parameter is constrained when multiple parameters are specified with a single tag?

Only the first parameter is constrained.

/** @template {Constraint} T,U,V */
    // equivalent to
/** @template {Constraint} T
    @template U
    @template V */
   // not
/** @template {Constraint} T
    @template {Constraint} U
    @template {Constraint} V */

I chose this way because:

It's likely (though I did not try) that adding the same constraint node to multiple type parameter declarations would cause Bad Things to happen.
C did it first! char* c,d,b; and int i,j,k = 0;

What non-standard syntax should be allowed, and what are the semantics?

Closure does not allow the type parameter names to be enclosed with curly braces, but, currently, Typescript does because of the similarity because the type declaration reminds people a lot of a type reference. However, the thing between the curlies is still just an Identifier, not a TypeNode.

/** @template {T} OK, I guess? */
/** @template {[T,U]} Won't even parse */

This first line is now ambiguous, unfortunately. Before this PR, it was equivalent to the typescript <T>. Now it's equivalent to the typescript <OK extends T>, which is clear if you drop the rest of the comment:

/** @template {T} OK */

I am kind of stuck here because this is the only reasonable way to resolve the ambiguity; if you let the mistaken syntax remain intact, the only way to specify a constraint is to require a double-mistaken syntax:

/** @template {T} {OK} here is the actual comment */

(This works in the current PR, by the way, if you do type that.)

The other option is to disable the old mistaken syntax entirely and always take the curly braces to mean a constraint. I guess this would break too much, but I haven't checked with our user code tests yet.

Also

allows multiple template tags, to align better with Closure - Fixes Support multiple @template tags in JSDoc #23908
Edit: JSDoc template tag text includes "\n " #24130 is not fixed by this PR now (extra trailing whitespace on template tag quick info).

A bunch of tests hit the asserts I added though.

sandersn · 2018-06-01T22:48:43Z

After checking the user tests and other bunches of JS I have cloned from github, I think the right thing to do is to simplify the syntax to disallow curlies around the type parameter name. This is technically a breaking change, but Closure never allowed this so the number of actual breaks will be low: Of the 3400 @template tags I found (from over 800,000 js files), only 4 used curlies, and they were all from webpack. So I can just send a PR to webpack after this PR goes in.

This is a breaking change, but in my sample, nobody except webpack used the erroneous syntax. I need to improve the error message, so jsdocTemplateTag3 currently fails to remind me of that.

ghost · 2018-06-04T15:10:53Z

src/compiler/parser.ts

-                        if (!name) {
-                            parseErrorAtPosition(scanner.getStartPos(), 0, Diagnostics.Identifier_expected);
+                        if (!tokenIsIdentifierOrKeyword(token())) {
+                            parseErrorAtCurrentToken(Diagnostics.Unexpected_token_A_type_parameter_name_was_expected_without_curly_braces);
                            return undefined;


Was already this way, but it looks like parseTemplateTag is the only tag-parsing method out of 9 that can return undefined. Might be better to call parseJSDocIdentifierName with createIfMissing set, and a new parameter to allow a custom diagnostic. Than parseTag could return JSDocTag (without | undefined).

parseTag returns undefined near the beginning if it can't parseJSDocIdentifierName, too. As-is the change is not worth it in my opinion. It might be worthwhile to have parseJSDocIdentifierName always return a missing node. I tried that and it looks a bit better, but there's a good bit of churn. Would you like to review it as part of this PR or would you like to see it in a separate one?

Separate is good.

ghost · 2018-06-04T15:13:04Z

src/compiler/parser.ts

@@ -7044,6 +7042,11 @@ namespace ts {
                        }
                    }

+                    if (constraint) {
+                        Debug.assert(!!typeParameters.length);


first(typeParameters) will do this assertion for you.

Thanks, didn't think about using first(typeParameters).constraint as an lhs

ghost · 2018-06-04T15:14:40Z

src/compiler/utilities.ts

-        const tag = find(getJSDocTags(node), (tag): tag is JSDocTemplateTag =>
-            isJSDocTemplateTag(tag) && !(tag.parent.kind === SyntaxKind.JSDocComment && tag.parent.tags!.some(isJSDocTypeAlias)));
-        return (tag && tag.typeParameters) || emptyArray;
+        return flatMap(filter(getJSDocTags(node), isNonTypeAliasTemplate), tag => tag.typeParameters);


flatMap(getJSDocTags(node), tag => isNonTypeAliasTemplate(tag) ? tag.typeParameters : undefined);

ghost · 2018-06-04T15:17:00Z

tests/cases/fourslash/jsdocReturnsTag.ts

@@ -17,7 +17,6 @@ verify.signatureHelp({
    text: "find<T>(l: T[], x: T): T",
    docComment: "Find an item",
    tags: [
-        // TODO: GH#24130


Doesn't look like #24130 is fixed here -- text still contains "\n ".

Added the comment back in, with a pointer to this PR.

mhegazy · 2018-06-04T17:22:33Z

src/compiler/checker.ts

@@ -28417,8 +28417,8 @@ namespace ts {

        function checkGrammarConstructorTypeParameters(node: ConstructorDeclaration) {
            const typeParameters = getEffectiveTypeParameterDeclarations(node);
-            if (isNodeArray(typeParameters)) {
-                const { pos, end } = typeParameters;
+            if (isNodeArray(typeParameters) || typeParameters.length) {


when is it not a NodeArray?

getEffectiveTypeParameterDeclarations returns a ReadonlyArray now. Previously it returned templateTags[0].typeParameters, which was a NodeArray. Now it returns flatMap(templateTags, t => t.typeParameters), which is a ReadonlyArray.

I don't think it's worth the trouble to create a NodeArray because the resulting span would stretch across all 3 template tags below, which is very ugly:

/** @template T * @template U * @template V */ constructor() { }

in the past we stuck a pos and an end on the result to make it look like a NodeArray.. why can not we do the same here?

actually looking at the code, that is exactly what we do.. it looks like it will always be a NodeArray

In getEffectiveTypeParameterDeclarations, the code path that calls getJSDocTypeParameterDeclarations ends up calling flatMap, which is what returns the non-NodeArray:

/** * Gets the effective type parameters. If the node was parsed in a * JavaScript file, gets the type parameters from the `@template` tag from JSDoc. */ export function getEffectiveTypeParameterDeclarations(node: DeclarationWithTypeParameters): ReadonlyArray<TypeParameterDeclaration> { if (isJSDocSignature(node)) { return emptyArray; /*** Not a NodeArray ***/ } if (isJSDocTypeAlias(node)) { Debug.assert(node.parent.kind === SyntaxKind.JSDocComment); const templateTags = flatMap(filter(node.parent.tags, isJSDocTemplateTag), tag => tag.typeParameters) as ReadonlyArray<TypeParameterDeclaration>; const templateTagNodes = templateTags as NodeArray<TypeParameterDeclaration>; templateTagNodes.pos = templateTagNodes.length > 0 ? first(templateTagNodes).pos : node.pos; templateTagNodes.end = templateTagNodes.length > 0 ? last(templateTagNodes).end : node.end; templateTagNodes.hasTrailingComma = false; return templateTagNodes; } return node.typeParameters || (isInJavaScriptFile(node) ? getJSDocTypeParameterDeclarations(node) : emptyArray); } export function getJSDocTypeParameterDeclarations(node: DeclarationWithTypeParameters): ReadonlyArray<TypeParameterDeclaration> { /*** flatMap does not return a NodeArray ***/ return flatMap(getJSDocTags(node), tag => isNonTypeAliasTemplate(tag) ? tag.typeParameters : undefined); }

The jsdoc signature case returns emptyArray, which is also not a NodeArray.

Make checkGrammarConstructorTypeParameters do a little more work

sandersn added 7 commits May 31, 2018 16:24

Parse (and mostly support) template tag constraints

b208b4f

A bunch of tests hit the asserts I added though.

Messy version is finished. Need to add a few tests

e986333

Refactor to be smaller

e629fc9

Small refactor + Add one test

da9f937

Another test

24fdef6

Minor cleanup

efd8a4f

Fix error reporting on type parameters on ctors

d499fde

sandersn requested review from a user and mhegazy June 1, 2018 22:09

sandersn added 4 commits June 1, 2018 16:03

Simplify syntax of @template tag

781bc65

This is a breaking change, but in my sample, nobody except webpack used the erroneous syntax. I need to improve the error message, so jsdocTemplateTag3 currently fails to remind me of that.

Better error message for template tag

e5e9e43

Fix fourslash baselines

f6d56f3

Another fourslash update

0019f69

ghost reviewed Jun 4, 2018

View reviewed changes

sandersn mentioned this pull request Jun 4, 2018

Suggestion: Add support for jsdoc-based templates with constrains #24283

Closed

4 tasks

Address PR comments

3ce4b24

ghost approved these changes Jun 4, 2018

View reviewed changes

mhegazy reviewed Jun 4, 2018

View reviewed changes

mhegazy approved these changes Jun 4, 2018

View reviewed changes

Simplify getEffectiveTypeParameterDeclarations

e85d472

Make checkGrammarConstructorTypeParameters do a little more work

sandersn merged commit 34e68ef into master Jun 4, 2018

sandersn deleted the js/template-tag-constraints branch June 4, 2018 18:42

sandersn restored the js/template-tag-constraints branch June 4, 2018 18:42

sandersn mentioned this pull request Jun 4, 2018

Simplify parseJSDocIdentifierName #24660

Merged

ajafff mentioned this pull request Jun 4, 2018

API: expose getEffectiveTypeParameterDeclarations #24248

Closed

ghost deleted the js/template-tag-constraints branch June 4, 2018 19:52

brendankenny mentioned this pull request Jun 26, 2018

Support generic typedef #20831

Closed

microsoft locked and limited conversation to collaborators Aug 3, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Template tag allows specification of constraints #24600

Template tag allows specification of constraints #24600

sandersn commented Jun 1, 2018 •

edited

Loading

sandersn commented Jun 1, 2018

ghost Jun 4, 2018

sandersn Jun 4, 2018

ghost Jun 4, 2018

ghost Jun 4, 2018

sandersn Jun 4, 2018

ghost Jun 4, 2018

ghost Jun 4, 2018

sandersn Jun 4, 2018

mhegazy Jun 4, 2018

sandersn Jun 4, 2018

mhegazy Jun 4, 2018

mhegazy Jun 4, 2018

sandersn Jun 4, 2018

Template tag allows specification of constraints #24600

Template tag allows specification of constraints #24600

Conversation

sandersn commented Jun 1, 2018 • edited Loading

Basic syntax

Notable decisions

Which parameter is constrained when multiple parameters are specified with a single tag?

What non-standard syntax should be allowed, and what are the semantics?

Also

sandersn commented Jun 1, 2018

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

sandersn commented Jun 1, 2018 •

edited

Loading