Skip to content

Commit

Permalink
Don't bind JSDoc type parameter in a TS file (#16413)
Browse files Browse the repository at this point in the history
* Don't bind JSDoc type parameter in a TS file

* Fix tests

* Remove unnecessary non-null assertions
  • Loading branch information
Andy committed Jun 9, 2017
1 parent a757e84 commit 13b7d17
Show file tree
Hide file tree
Showing 13 changed files with 104 additions and 11 deletions.
7 changes: 5 additions & 2 deletions src/compiler/binder.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1521,7 +1521,7 @@ namespace ts {
// All the children of these container types are never visible through another
// symbol (i.e. through another symbol's 'exports' or 'members'). Instead,
// they're only accessed 'lexically' (i.e. from code that exists underneath
// their container in the tree. To accomplish this, we simply add their declared
// their container in the tree). To accomplish this, we simply add their declared
// symbol to the 'locals' of the container. These symbols can then be found as
// the type checker walks up the containers, checking them for matching names.
return declareSymbol(container.locals, /*parent*/ undefined, node, symbolFlags, symbolExcludes);
Expand Down Expand Up @@ -2053,7 +2053,10 @@ namespace ts {
case SyntaxKind.TypePredicate:
return checkTypePredicate(node as TypePredicateNode);
case SyntaxKind.TypeParameter:
return declareSymbolAndAddToSymbolTable(<Declaration>node, SymbolFlags.TypeParameter, SymbolFlags.TypeParameterExcludes);
if (node.parent.kind !== ts.SyntaxKind.JSDocTemplateTag || isInJavaScriptFile(node)) {
return declareSymbolAndAddToSymbolTable(<Declaration>node, SymbolFlags.TypeParameter, SymbolFlags.TypeParameterExcludes);
}
return;
case SyntaxKind.Parameter:
return bindParameter(<ParameterDeclaration>node);
case SyntaxKind.VariableDeclaration:
Expand Down
9 changes: 7 additions & 2 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22554,10 +22554,16 @@ namespace ts {
}

if (entityName.parent!.kind === SyntaxKind.JSDocParameterTag) {
const parameter = ts.getParameterFromJSDoc(entityName.parent as JSDocParameterTag);
const parameter = getParameterFromJSDoc(entityName.parent as JSDocParameterTag);
return parameter && parameter.symbol;
}

if (entityName.parent.kind === SyntaxKind.TypeParameter && entityName.parent.parent.kind === SyntaxKind.JSDocTemplateTag) {
Debug.assert(!isInJavaScriptFile(entityName)); // Otherwise `isDeclarationName` would have been true.
const typeParameter = getTypeParameterFromJsDoc(entityName.parent as TypeParameterDeclaration & { parent: JSDocTemplateTag });
return typeParameter && typeParameter.symbol;
}

if (isPartOfExpression(entityName)) {
if (nodeIsMissing(entityName)) {
// Missing entity name.
Expand Down Expand Up @@ -24824,7 +24830,6 @@ namespace ts {
// falls through
default:
return isDeclarationName(name);

}
}
}
2 changes: 1 addition & 1 deletion src/compiler/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1797,7 +1797,7 @@ namespace ts {
block: Block;
}

export type DeclarationWithTypeParameters = SignatureDeclaration | ClassLikeDeclaration | InterfaceDeclaration | TypeAliasDeclaration;
export type DeclarationWithTypeParameters = SignatureDeclaration | ClassLikeDeclaration | InterfaceDeclaration | TypeAliasDeclaration | JSDocTemplateTag;

export interface ClassLikeDeclaration extends NamedDeclaration {
name?: Identifier;
Expand Down
10 changes: 10 additions & 0 deletions src/compiler/utilities.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1546,6 +1546,12 @@ namespace ts {
p.name.kind === SyntaxKind.Identifier && p.name.text === name);
}

export function getTypeParameterFromJsDoc(node: TypeParameterDeclaration & { parent: JSDocTemplateTag }): TypeParameterDeclaration | undefined {
const name = node.name.text;
const { typeParameters } = (node.parent.parent.parent as ts.SignatureDeclaration | ts.InterfaceDeclaration | ts.ClassDeclaration);
return find(typeParameters, p => p.name.text === name);
}

export function getJSDocType(node: Node): JSDocType {
let tag: JSDocTypeTag | JSDocParameterTag = getFirstJSDocTag(node, SyntaxKind.JSDocTypeTag) as JSDocTypeTag;
if (!tag && node.kind === SyntaxKind.Parameter) {
Expand Down Expand Up @@ -5274,6 +5280,10 @@ namespace ts {

/* @internal */
export function isDeclaration(node: Node): node is NamedDeclaration {
if (node.kind === SyntaxKind.TypeParameter) {
return node.parent.kind !== SyntaxKind.JSDocTemplateTag || isInJavaScriptFile(node);
}

return isDeclarationKind(node.kind);
}

Expand Down
4 changes: 2 additions & 2 deletions src/services/findAllReferences.ts
Original file line number Diff line number Diff line change
Expand Up @@ -784,8 +784,8 @@ namespace ts.FindAllReferences.Core {
return;
}

const fullStart = state.options.findInComments || container.jsDoc !== undefined || forEach(search.symbol.declarations, d => d.kind === ts.SyntaxKind.JSDocTypedefTag);
for (const position of getPossibleSymbolReferencePositions(sourceFile, search.text, container, fullStart)) {
// Need to search in the full start of the node in case there is a reference inside JSDoc.
for (const position of getPossibleSymbolReferencePositions(sourceFile, search.text, container, /*fullStart*/ true)) {
getReferencesAtLocation(sourceFile, position, search, state);
}
}
Expand Down
4 changes: 4 additions & 0 deletions src/services/utilities.ts
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,10 @@ namespace ts {
else if (isNamespaceReference(node)) {
return SemanticMeaning.Namespace;
}
else if (isTypeParameter(node.parent)) {
Debug.assert(isJSDocTemplateTag(node.parent.parent)); // Else would be handled by isDeclarationName
return SemanticMeaning.Type;
}
else {
return SemanticMeaning.Value;
}
Expand Down
11 changes: 10 additions & 1 deletion tests/baselines/reference/jsdocInTypeScript.errors.txt
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,16 @@ tests/cases/compiler/jsdocInTypeScript.ts(30,3): error TS2339: Property 'x' does
// @type has no effect either.
/** @type {{ x?: number }} */
const z = {};
z.x = 1;
z.x = 1; // Error
~
!!! error TS2339: Property 'x' does not exist on type '{}'.

// @template tag should not interfere with constraint or default.
/** @template T */
interface I<T extends number = 0> {}

/** @template T */
function tem<T extends number>(t: T): I<T> { return {}; }

let i: I; // Should succeed thanks to type parameter default

16 changes: 14 additions & 2 deletions tests/baselines/reference/jsdocInTypeScript.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,16 @@ f(1); f(true).length;
// @type has no effect either.
/** @type {{ x?: number }} */
const z = {};
z.x = 1;
z.x = 1; // Error

// @template tag should not interfere with constraint or default.
/** @template T */
interface I<T extends number = 0> {}

/** @template T */
function tem<T extends number>(t: T): I<T> { return {}; }

let i: I; // Should succeed thanks to type parameter default


//// [jsdocInTypeScript.js]
Expand All @@ -50,4 +59,7 @@ f(true).length;
// @type has no effect either.
/** @type {{ x?: number }} */
var z = {};
z.x = 1;
z.x = 1; // Error
/** @template T */
function tem(t) { return {}; }
var i; // Should succeed thanks to type parameter default
11 changes: 10 additions & 1 deletion tests/cases/compiler/jsdocInTypeScript.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,4 +27,13 @@ f(1); f(true).length;
// @type has no effect either.
/** @type {{ x?: number }} */
const z = {};
z.x = 1;
z.x = 1; // Error

// @template tag should not interfere with constraint or default.
/** @template T */
interface I<T extends number = 0> {}

/** @template T */
function tem<T extends number>(t: T): I<T> { return {}; }

let i: I; // Should succeed thanks to type parameter default
6 changes: 6 additions & 0 deletions tests/cases/fourslash/findAllRefsJsDocTemplateTag_class.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
/// <reference path='fourslash.ts'/>

/////** @template [|T|] */
////class C<[|{| "isWriteAccess": true, "isDefinition": true |}T|]> {}

verify.singleReferenceGroup("(type parameter) T in C<T>");
17 changes: 17 additions & 0 deletions tests/cases/fourslash/findAllRefsJsDocTemplateTag_class_js.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
/// <reference path='fourslash.ts'/>

// @allowJs: true
// @Filename: /a.js

// TODO: https://github.com/Microsoft/TypeScript/issues/16411
// Both uses of T should be referenced.

/////** @template [|{| "isWriteAccess": true, "isDefinition": true |}T|] */
////class C {
//// constructor() {
//// /** @type {T} */
//// this.x = null;
//// }
////}

verify.singleReferenceGroup("(type parameter) T in C");
6 changes: 6 additions & 0 deletions tests/cases/fourslash/findAllRefsJsDocTemplateTag_function.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
/// <reference path='fourslash.ts'/>

/////** @template [|{| "isWriteAccess": false, "isDefinition": false |}T|] */
////function f<[|{| "isWriteAccess": true, "isDefinition": true |}T|]>() {}

verify.singleReferenceGroup("(type parameter) T in f<T>(): void");
12 changes: 12 additions & 0 deletions tests/cases/fourslash/findAllRefsJsDocTemplateTag_function_js.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
/// <reference path='fourslash.ts'/>

// @allowJs: true
// @Filename: /a.js

/////**
//// * @template [|{| "isWriteAccess": true, "isDefinition": true |}T|]
//// * @return {[|T|]}
//// */
////function f() {}

verify.singleReferenceGroup("(type parameter) T"); // TODO:GH#??? should be "(type parameter) T in f<T>(): void"

0 comments on commit 13b7d17

Please sign in to comment.