Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Parse jsdoc with normal TS type parser #17176

Merged
merged 14 commits into from
Jul 17, 2017
Merged

Parse jsdoc with normal TS type parser #17176

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
bc69c7e
Parse JSDoc types using the normal TS parser
sandersn Jul 13, 2017
91633cd
Test JSDoc parsing using TS parser
sandersn Jul 13, 2017
48d9012
Update baselines
sandersn Jul 13, 2017
ac478a9
Add missing word in error message
sandersn Jul 13, 2017
d24b3a3
Add fourslash tests for function(new/this:T) syntax
sandersn Jul 13, 2017
da5285e
Update baselines
sandersn Jul 14, 2017
6e861fd
Remove JSDocConstructor/ThisType and just use named parameters
sandersn Jul 14, 2017
f1145c3
Improve JSDoc function checking
sandersn Jul 14, 2017
40ae422
Better JSDoc generic errors and faster isInJSDoc
sandersn Jul 14, 2017
b2e892f
Update baselines
sandersn Jul 14, 2017
bdc3f1f
Address more PR comments
sandersn Jul 14, 2017
172db13
Parse more types in JSDoc function() syntax
sandersn Jul 14, 2017
61c9f98
Merge branch 'master' into parse-jsdoc-with-ts-type-parser
sandersn Jul 14, 2017
3f60364
Improve test of jsdoc literal type parsing
sandersn Jul 17, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 13 additions & 12 deletions src/compiler/binder.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -280,7 +280,14 @@ namespace ts {
Debug.assert(node.parent.kind === SyntaxKind.JSDocFunctionType);
const functionType = <JSDocFunctionType>node.parent;
const index = indexOf(functionType.parameters, node);
return "arg" + index as __String;
switch ((node as ParameterDeclaration).type.kind) {
case SyntaxKind.JSDocThisType:
return "this" as __String;
case SyntaxKind.JSDocConstructorType:
return "new" as __String;
default:
return "arg" + index as __String;
}
case SyntaxKind.JSDocTypedefTag:
const parentNode = node.parent && node.parent.parent;
let nameFromParentNode: __String;
Expand Down Expand Up @@ -1395,14 +1402,12 @@ namespace ts {
case SyntaxKind.ObjectLiteralExpression:
case SyntaxKind.TypeLiteral:
case SyntaxKind.JSDocTypeLiteral:
case SyntaxKind.JSDocRecordType:
case SyntaxKind.JsxAttributes:
return ContainerFlags.IsContainer;

case SyntaxKind.InterfaceDeclaration:
return ContainerFlags.IsContainer | ContainerFlags.IsInterface;

case SyntaxKind.JSDocFunctionType:
case SyntaxKind.ModuleDeclaration:
case SyntaxKind.TypeAliasDeclaration:
case SyntaxKind.MappedType:
Expand All @@ -1422,9 +1427,10 @@ namespace ts {
case SyntaxKind.GetAccessor:
case SyntaxKind.SetAccessor:
case SyntaxKind.CallSignature:
case SyntaxKind.JSDocFunctionType:
case SyntaxKind.FunctionType:
case SyntaxKind.ConstructSignature:
case SyntaxKind.IndexSignature:
case SyntaxKind.FunctionType:
case SyntaxKind.ConstructorType:
return ContainerFlags.IsContainer | ContainerFlags.IsControlFlowContainer | ContainerFlags.HasLocals | ContainerFlags.IsFunctionLike;

Expand Down Expand Up @@ -1502,7 +1508,6 @@ namespace ts {
case SyntaxKind.TypeLiteral:
case SyntaxKind.ObjectLiteralExpression:
case SyntaxKind.InterfaceDeclaration:
case SyntaxKind.JSDocRecordType:
case SyntaxKind.JSDocTypeLiteral:
case SyntaxKind.JsxAttributes:
// Interface/Object-types always have their children added to the 'members' of
Expand Down Expand Up @@ -2095,6 +2100,7 @@ namespace ts {
case SyntaxKind.SetAccessor:
return bindPropertyOrMethodOrAccessor(<Declaration>node, SymbolFlags.SetAccessor, SymbolFlags.SetAccessorExcludes);
case SyntaxKind.FunctionType:
case SyntaxKind.JSDocFunctionType:
case SyntaxKind.ConstructorType:
return bindFunctionOrConstructorType(<SignatureDeclaration>node);
case SyntaxKind.TypeLiteral:
Expand Down Expand Up @@ -2157,18 +2163,13 @@ namespace ts {
case SyntaxKind.ModuleBlock:
return updateStrictModeStatementList((<Block | ModuleBlock>node).statements);

case SyntaxKind.JSDocRecordMember:
return bindPropertyWorker(node as JSDocRecordMember);
case SyntaxKind.JSDocPropertyTag:
return declareSymbolAndAddToSymbolTable(node as JSDocPropertyTag,
(node as JSDocPropertyTag).isBracketed || ((node as JSDocPropertyTag).typeExpression && (node as JSDocPropertyTag).typeExpression.type.kind === SyntaxKind.JSDocOptionalType) ?
SymbolFlags.Property | SymbolFlags.Optional : SymbolFlags.Property,
SymbolFlags.PropertyExcludes);
case SyntaxKind.JSDocFunctionType:
return bindFunctionOrConstructorType(<SignatureDeclaration>node);
case SyntaxKind.JSDocTypeLiteral:
case SyntaxKind.JSDocRecordType:
return bindAnonymousTypeWorker(node as JSDocTypeLiteral | JSDocRecordType);
return bindAnonymousTypeWorker(node as JSDocTypeLiteral);
case SyntaxKind.JSDocTypedefTag: {
const { fullName } = node as JSDocTypedefTag;
if (!fullName || fullName.kind === SyntaxKind.Identifier) {
Expand All @@ -2183,7 +2184,7 @@ namespace ts {
return bindPropertyOrMethodOrAccessor(node, SymbolFlags.Property | (node.questionToken ? SymbolFlags.Optional : SymbolFlags.None), SymbolFlags.PropertyExcludes);
}

function bindAnonymousTypeWorker(node: TypeLiteralNode | MappedTypeNode | JSDocTypeLiteral | JSDocRecordType) {
function bindAnonymousTypeWorker(node: TypeLiteralNode | MappedTypeNode | JSDocTypeLiteral) {
return bindAnonymousDeclaration(<Declaration>node, SymbolFlags.TypeLiteral, InternalSymbolName.Type);
}

Expand Down
99 changes: 67 additions & 32 deletions src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6339,7 +6339,7 @@ namespace ts {
const resolvedSymbol = resolveName(param, paramSymbol.name, SymbolFlags.Value, undefined, undefined);
paramSymbol = resolvedSymbol;
}
if (i === 0 && paramSymbol.name === "this") {
if (i === 0 && paramSymbol.name === "this" || (param.type && param.type.kind === SyntaxKind.JSDocThisType)) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed JSDocThisType and -ConstructorType like you suggested.

hasThisParameter = true;
thisParameter = param.symbol;
}
Expand Down Expand Up @@ -6798,16 +6798,13 @@ namespace ts {
switch (node.kind) {
case SyntaxKind.TypeReference:
return (<TypeReferenceNode>node).typeName;
case SyntaxKind.JSDocTypeReference:
return (<JSDocTypeReference>node).name;
case SyntaxKind.ExpressionWithTypeArguments:
// We only support expressions that are simple qualified names. For other
// expressions this produces undefined.
const expr = (<ExpressionWithTypeArguments>node).expression;
if (isEntityNameExpression(expr)) {
return expr;
}

// fall through;
}

Expand All @@ -6833,8 +6830,8 @@ namespace ts {
return type;
}

if (symbol.flags & SymbolFlags.Value && node.kind === SyntaxKind.JSDocTypeReference) {
// A JSDocTypeReference may have resolved to a value (as opposed to a type). If
if (symbol.flags & SymbolFlags.Value && isJSDocTypeReference(node)) {
// A jsdoc TypeReference may have resolved to a value (as opposed to a type). If
// the symbol is a constructor function, return the inferred class type; otherwise,
// the type of this reference is just the type of the value we resolved to.
const valueType = getTypeOfSymbol(symbol);
Expand Down Expand Up @@ -6862,14 +6859,16 @@ namespace ts {
return getTypeFromTypeAliasReference(node, symbol, typeArguments);
}

if (symbol.flags & SymbolFlags.Function && node.kind === SyntaxKind.JSDocTypeReference && (symbol.members || getJSDocClassTag(symbol.valueDeclaration))) {
if (symbol.flags & SymbolFlags.Function &&
isJSDocTypeReference(node) &&
(symbol.members || getJSDocClassTag(symbol.valueDeclaration))) {
return getInferredClassType(symbol);
}
}

function getPrimitiveTypeFromJSDocTypeReference(node: JSDocTypeReference): Type {
if (isIdentifier(node.name)) {
switch (node.name.text) {
function getPrimitiveTypeFromJSDocTypeReference(node: TypeReferenceNode): Type {
if (isIdentifier(node.typeName)) {
switch (node.typeName.text) {
case "String":
return stringType;
case "Number":
Expand Down Expand Up @@ -6909,7 +6908,7 @@ namespace ts {
let symbol: Symbol;
let type: Type;
let meaning = SymbolFlags.Type;
if (node.kind === SyntaxKind.JSDocTypeReference) {
if (isJSDocTypeReference(node)) {
type = getPrimitiveTypeFromJSDocTypeReference(node);
meaning |= SymbolFlags.Value;
}
Expand Down Expand Up @@ -7799,15 +7798,6 @@ namespace ts {
return links.resolvedType;
}

function getTypeFromJSDocTupleType(node: JSDocTupleType): Type {
const links = getNodeLinks(node);
if (!links.resolvedType) {
const types = map(node.types, getTypeFromTypeNode);
links.resolvedType = createTupleType(types);
}
return links.resolvedType;
}

function getThisType(node: Node): Type {
const container = getThisContainer(node, /*includeArrowFunctions*/ false);
const parent = container && container.parent;
Expand Down Expand Up @@ -7852,16 +7842,18 @@ namespace ts {
case SyntaxKind.NeverKeyword:
return neverType;
case SyntaxKind.ObjectKeyword:
return nonPrimitiveType;
if (node.flags & NodeFlags.JavaScriptFile) {
return anyType;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
else {
return nonPrimitiveType;
}
case SyntaxKind.ThisType:
case SyntaxKind.ThisKeyword:
return getTypeFromThisTypeNode(node);
case SyntaxKind.LiteralType:
return getTypeFromLiteralTypeNode(<LiteralTypeNode>node);
case SyntaxKind.JSDocLiteralType:
return getTypeFromLiteralTypeNode((<JSDocLiteralType>node).literal);
case SyntaxKind.TypeReference:
case SyntaxKind.JSDocTypeReference:
return getTypeFromTypeReference(<TypeReferenceNode>node);
case SyntaxKind.TypePredicate:
return booleanType;
Expand All @@ -7870,12 +7862,10 @@ namespace ts {
case SyntaxKind.TypeQuery:
return getTypeFromTypeQueryNode(<TypeQueryNode>node);
case SyntaxKind.ArrayType:
case SyntaxKind.JSDocArrayType:
return getTypeFromArrayTypeNode(<ArrayTypeNode>node);
case SyntaxKind.TupleType:
return getTypeFromTupleTypeNode(<TupleTypeNode>node);
case SyntaxKind.UnionType:
case SyntaxKind.JSDocUnionType:
return getTypeFromUnionTypeNode(<UnionTypeNode>node);
case SyntaxKind.IntersectionType:
return getTypeFromIntersectionTypeNode(<IntersectionTypeNode>node);
Expand All @@ -7887,8 +7877,6 @@ namespace ts {
case SyntaxKind.JSDocThisType:
case SyntaxKind.JSDocOptionalType:
return getTypeFromTypeNode((<ParenthesizedTypeNode | JSDocTypeReferencingNode>node).type);
case SyntaxKind.JSDocRecordType:
return getTypeFromTypeNode((node as JSDocRecordType).literal);
case SyntaxKind.FunctionType:
case SyntaxKind.ConstructorType:
case SyntaxKind.TypeLiteral:
Expand All @@ -7907,8 +7895,6 @@ namespace ts {
case SyntaxKind.QualifiedName:
const symbol = getSymbolAtLocation(node);
return symbol && getDeclaredTypeOfSymbol(symbol);
case SyntaxKind.JSDocTupleType:
return getTypeFromJSDocTupleType(<JSDocTupleType>node);
case SyntaxKind.JSDocVariadicType:
return getTypeFromJSDocVariadicType(<JSDocVariadicType>node);
default:
Expand Down Expand Up @@ -18474,6 +18460,9 @@ namespace ts {

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)) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

grammarErrorOnNode(node, Diagnostics.JSDoc_types_can_only_used_inside_documentation_comments);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
const type = getTypeFromTypeReference(node);
if (type !== unknownType) {
if (node.typeArguments) {
Expand Down Expand Up @@ -19372,7 +19361,17 @@ namespace ts {
}
}

function checkJsDoc(node: FunctionDeclaration | MethodDeclaration) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (!node.jsDoc) {
return;
}
for (const doc of node.jsDoc) {
checkSourceElement(doc);
}
}

function checkFunctionOrMethodDeclaration(node: FunctionDeclaration | MethodDeclaration): void {
checkJsDoc(node);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I moved the check into checkJSDoc

checkDecorators(node);
checkSignatureDeclaration(node);
const functionFlags = getFunctionFlags(node);
Expand Down Expand Up @@ -21971,6 +21970,22 @@ namespace ts {
}
}

function checkJSDocComment(node: JSDoc) {
if (isInJavaScriptFile(node) && (node as JSDoc).tags) {
for (const tag of (node as JSDoc).tags) {
checkSourceElement(tag);
}
}
}

function checkJSDocFunctionType(node: JSDocFunctionType) {
for (const p of node.parameters) {
// don't bother with normal parameter checking since jsdoc function parameters only consist of a type
checkSourceElement(p.type);
}
checkSourceElement(node.type);
}

function checkSourceElement(node: Node): void {
if (!node) {
return;
Expand Down Expand Up @@ -22030,6 +22045,26 @@ namespace ts {
case SyntaxKind.ParenthesizedType:
case SyntaxKind.TypeOperator:
return checkSourceElement((<ParenthesizedTypeNode | TypeOperatorNode>node).type);
case SyntaxKind.JSDocComment:
return checkJSDocComment(node as JSDoc);
case SyntaxKind.JSDocParameterTag:
return checkSourceElement((node as JSDocParameterTag).typeExpression);
case SyntaxKind.JSDocFunctionType:
checkJSDocFunctionType(node as JSDocFunctionType);
// falls through
case SyntaxKind.JSDocConstructorType:
case SyntaxKind.JSDocThisType:
case SyntaxKind.JSDocVariadicType:
case SyntaxKind.JSDocNonNullableType:
case SyntaxKind.JSDocNullableType:
case SyntaxKind.JSDocAllType:
case SyntaxKind.JSDocUnknownType:
if (!isInJavaScriptFile(node) && !findAncestor(node, n => n.kind === SyntaxKind.JSDocTypeExpression)) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another findAncestor here

grammarErrorOnNode(node, Diagnostics.JSDoc_types_can_only_used_inside_documentation_comments);
}
return;
case SyntaxKind.JSDocTypeExpression:
return checkSourceElement((node as JSDocTypeExpression).type);
case SyntaxKind.IndexedAccessType:
return checkIndexedAccessType(<IndexedAccessTypeNode>node);
case SyntaxKind.MappedType:
Expand Down Expand Up @@ -22386,7 +22421,7 @@ namespace ts {
node = node.parent;
}

return node.parent && (node.parent.kind === SyntaxKind.TypeReference || node.parent.kind === SyntaxKind.JSDocTypeReference) ;
return node.parent && node.parent.kind === SyntaxKind.TypeReference ;
}

function isHeritageClauseElementIdentifier(entityName: Node): boolean {
Expand Down Expand Up @@ -22540,7 +22575,7 @@ namespace ts {
}
}
else if (isTypeReferenceIdentifier(<EntityName>entityName)) {
const meaning = (entityName.parent.kind === SyntaxKind.TypeReference || entityName.parent.kind === SyntaxKind.JSDocTypeReference) ? SymbolFlags.Type : SymbolFlags.Namespace;
const meaning = entityName.parent.kind === SyntaxKind.TypeReference ? SymbolFlags.Type : SymbolFlags.Namespace;
return resolveEntityName(<EntityName>entityName, meaning, /*ignoreErrors*/ false, /*dontResolveAlias*/ true);
}
else if (entityName.parent.kind === SyntaxKind.JsxAttribute) {
Expand Down
29 changes: 16 additions & 13 deletions src/compiler/diagnosticMessages.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3463,6 +3463,22 @@
"category": "Error",
"code": 8016
},
"Octal literal types must use ES2015 syntax. Use the syntax '{0}'.": {
"category": "Error",
"code": 8017
},
"Octal literals are not allowed in enums members initializer. Use the syntax '{0}'.": {
"category": "Error",
"code": 8018
},
"Report errors in .js files.": {
"category": "Message",
"code": 8019
},
"JSDoc types can only used inside documentation comments.": {
"category": "Error",
"code": 8020
},
"Only identifiers/qualified-names with optional type arguments are currently supported in a class 'extends' clause.": {
"category": "Error",
"code": 9002
Expand Down Expand Up @@ -3645,18 +3661,5 @@
"Convert function '{0}' to class": {
"category": "Message",
"code": 95002
},

"Octal literal types must use ES2015 syntax. Use the syntax '{0}'.": {
"category": "Error",
"code": 8017
},
"Octal literals are not allowed in enums members initializer. Use the syntax '{0}'.": {
"category": "Error",
"code": 8018
},
"Report errors in .js files.": {
"category": "Message",
"code": 8019
}
}
Loading