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 13 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
16 changes: 5 additions & 11 deletions src/compiler/binder.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1395,14 +1395,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 +1420,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 +1501,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 +2093,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 +2156,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 +2177,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
101 changes: 64 additions & 37 deletions src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
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,20 @@ 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 isJSDocTypeReference(node: TypeReferenceType): node is TypeReferenceNode {
return node.flags & NodeFlags.JSDoc && node.kind === SyntaxKind.TypeReference;
}

function getPrimitiveTypeFromJSDocTypeReference(node: TypeReferenceNode): Type {
if (isIdentifier(node.typeName)) {
switch (node.typeName.text) {
case "String":
return stringType;
case "Number":
Expand All @@ -6883,7 +6886,6 @@ namespace ts {
case "Null":
return nullType;
case "Object":
case "object":
return anyType;
case "Function":
case "function":
Expand All @@ -6909,7 +6911,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 +7801,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 +7845,13 @@ namespace ts {
case SyntaxKind.NeverKeyword:
return neverType;
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.

case SyntaxKind.ObjectKeyword:
return nonPrimitiveType;
return node.flags & NodeFlags.JavaScriptFile ? anyType : 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,25 +7860,19 @@ 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);
case SyntaxKind.JSDocNullableType:
return getTypeFromJSDocNullableTypeNode(<JSDocNullableType>node);
case SyntaxKind.ParenthesizedType:
case SyntaxKind.JSDocNonNullableType:
case SyntaxKind.JSDocConstructorType:
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 +7891,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 @@ -12381,7 +12363,9 @@ namespace ts {
const jsdocType = getJSDocType(node);
if (jsdocType && jsdocType.kind === SyntaxKind.JSDocFunctionType) {
const jsDocFunctionType = <JSDocFunctionType>jsdocType;
if (jsDocFunctionType.parameters.length > 0 && jsDocFunctionType.parameters[0].type.kind === SyntaxKind.JSDocThisType) {
if (jsDocFunctionType.parameters.length > 0 &&
jsDocFunctionType.parameters[0].name &&
(jsDocFunctionType.parameters[0].name as Identifier).text === "this") {
return getTypeFromTypeNode(jsDocFunctionType.parameters[0].type);
}
}
Expand Down Expand Up @@ -17883,9 +17867,9 @@ namespace ts {
if (node.questionToken && isBindingPattern(node.name) && (func as FunctionLikeDeclaration).body) {
error(node, Diagnostics.A_binding_pattern_parameter_cannot_be_optional_in_an_implementation_signature);
}
if ((<Identifier>node.name).text === "this") {
if (node.name && ((node.name as Identifier).text === "this" || (node.name as Identifier).text === "new")) {
if (indexOf(func.parameters, node) !== 0) {
error(node, Diagnostics.A_this_parameter_must_be_the_first_parameter);
error(node, Diagnostics.A_0_parameter_must_be_the_first_parameter, (node.name as Identifier).text as string);
}
if (func.kind === SyntaxKind.Constructor || func.kind === SyntaxKind.ConstructSignature || func.kind === SyntaxKind.ConstructorType) {
error(node, Diagnostics.A_constructor_cannot_have_a_this_parameter);
Expand Down Expand Up @@ -18474,6 +18458,10 @@ namespace ts {

function checkTypeReferenceNode(node: TypeReferenceNode | ExpressionWithTypeArguments) {
checkGrammarTypeArguments(node, node.typeArguments);
if (node.kind === SyntaxKind.TypeReference && node.typeName.jsdocDotPos !== undefined && !isInJavaScriptFile(node) && !isInJSDoc(node)) {
grammarErrorAtPos(getSourceFileOfNode(node), node.typeName.jsdocDotPos, 1, Diagnostics.JSDoc_types_can_only_be_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.

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?

}
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 +19360,23 @@ namespace ts {
}
}

function checkJSDoc(node: FunctionDeclaration | MethodDeclaration) {
if (!isInJavaScriptFile(node)) {
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.

return;
}
forEach(node.jsDoc, checkSourceElement);
}

function checkJSDocComment(node: JSDoc) {
if ((node as JSDoc).tags) {
for (const tag of (node as JSDoc).tags) {
checkSourceElement(tag);
}
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

}
}

function checkFunctionOrMethodDeclaration(node: FunctionDeclaration | MethodDeclaration): void {
checkJSDoc(node);
checkDecorators(node);
checkSignatureDeclaration(node);
const functionFlags = getFunctionFlags(node);
Expand Down Expand Up @@ -19916,6 +19920,11 @@ namespace ts {
function checkVariableLikeDeclaration(node: VariableLikeDeclaration) {
checkDecorators(node);
checkSourceElement(node.type);

// JSDoc `function(string, string): string` syntax results in parameters with no name
if (!node.name) {
return;
}
// For a computed property, just check the initializer and exit
// Do not use hasDynamicName here, because that returns false for well known symbols.
// We want to perform checkComputedPropertyName for all computed properties, including
Expand Down Expand Up @@ -22030,6 +22039,24 @@ 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:
checkSignatureDeclaration(node as JSDocFunctionType);
// falls through
case SyntaxKind.JSDocVariadicType:
case SyntaxKind.JSDocNonNullableType:
case SyntaxKind.JSDocNullableType:
case SyntaxKind.JSDocAllType:
case SyntaxKind.JSDocUnknownType:
if (!isInJavaScriptFile(node) && !isInJSDoc(node)) {
grammarErrorOnNode(node, Diagnostics.JSDoc_types_can_only_be_used_inside_documentation_comments);
}
return;
case SyntaxKind.JSDocTypeExpression:
return checkSourceElement((node as JSDocTypeExpression).type);
case SyntaxKind.IndexedAccessType:
return checkIndexedAccessType(<IndexedAccessTypeNode>node);
case SyntaxKind.MappedType:
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

Expand Down Expand Up @@ -22386,7 +22413,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 +22567,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
31 changes: 17 additions & 14 deletions src/compiler/diagnosticMessages.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2064,7 +2064,7 @@
"category": "Error",
"code": 2679
},
"A 'this' parameter must be the first parameter.": {
"A '{0}' parameter must be the first parameter.": {
"category": "Error",
"code": 2680
},
Expand Down 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 be 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