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.

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

Skip asterisks after newline when parsing JSDoc types #26528

Merged
merged 3 commits into from
Sep 4, 2018
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
2 changes: 2 additions & 0 deletions src/compiler/parser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2377,8 +2377,10 @@ namespace ts {
}

function parseJSDocType(): TypeNode {
scanner.setInJSDocType(true);
const dotdotdot = parseOptionalToken(SyntaxKind.DotDotDotToken);
let type = parseTypeOrTypePredicate();
scanner.setInJSDocType(false);
if (dotdotdot) {
const variadic = createNode(SyntaxKind.JSDocVariadicType, dotdotdot.pos) as JSDocVariadicType;
variadic.type = type;
Expand Down
15 changes: 15 additions & 0 deletions src/compiler/scanner.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,8 @@ namespace ts {
setScriptTarget(scriptTarget: ScriptTarget): void;
setLanguageVariant(variant: LanguageVariant): void;
setTextPos(textPos: number): void;
/* @internal */
setInJSDocType(inType: boolean): void;
Copy link
Member

Choose a reason for hiding this comment

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

I think this is where you need to add /* @internal */

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I'll try that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added in 64bbf5e.

// Invokes the provided callback then unconditionally restores the scanner to the state it
// was in immediately prior to invoking the callback. The result of invoking the callback
// is returned from this function.
Expand Down Expand Up @@ -824,6 +826,8 @@ namespace ts {
let tokenValue!: string;
let tokenFlags: TokenFlags;

let inJSDocType = 0;

setText(text, start, length);

return {
Expand Down Expand Up @@ -854,6 +858,7 @@ namespace ts {
setLanguageVariant,
setOnError,
setTextPos,
setInJSDocType,
tryScan,
lookAhead,
scanRange,
Expand Down Expand Up @@ -1350,6 +1355,7 @@ namespace ts {
function scan(): SyntaxKind {
startPos = pos;
tokenFlags = 0;
let asteriskSeen = false;
while (true) {
tokenPos = pos;
if (pos >= end) {
Expand Down Expand Up @@ -1447,6 +1453,11 @@ namespace ts {
return pos += 2, token = SyntaxKind.AsteriskAsteriskToken;
}
pos++;
if (inJSDocType && !asteriskSeen && (tokenFlags & TokenFlags.PrecedingLineBreak)) {
// decoration at the start of a JSDoc comment line
asteriskSeen = true;
continue;
}
return token = SyntaxKind.AsteriskToken;
case CharacterCodes.plus:
if (text.charCodeAt(pos + 1) === CharacterCodes.plus) {
Expand Down Expand Up @@ -2078,5 +2089,9 @@ namespace ts {
tokenValue = undefined!;
tokenFlags = 0;
}

function setInJSDocType(inType: boolean) {
inJSDocType += inType ? 1 : -1;
}
Copy link
Member

Choose a reason for hiding this comment

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

Are there nested calls to setInJDSocType? Or is there another reason that inJSDocType = inType isn’t enough?

Copy link
Member

Choose a reason for hiding this comment

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

I looked for nested calls and there don't seem to be any.

Copy link
Contributor Author

@tschaub tschaub Sep 4, 2018

Choose a reason for hiding this comment

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

I was seeing nesting while parsing a @typedef with a function property. I think the tests should fail if inJSDocType is a boolean. I'll confirm.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add another test, but there are nested calls to parseJSDocType here:

/**
 * @typedef {{foo: function(string)}} SomeType
 */

And more for a function that accepts a function etc.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks. It's too bad -- I thought our JSDoc type parsing was simpler than that.

}
}
19 changes: 18 additions & 1 deletion src/compiler/utilities.ts
Original file line number Diff line number Diff line change
Expand Up @@ -490,12 +490,29 @@ namespace ts {
return getTextOfNodeFromSourceText(sourceFile.text, node, includeTrivia);
}

function isJSDocTypeExpressionOrChild(node: Node): boolean {
if (node.kind === SyntaxKind.JSDocTypeExpression) {
Copy link
Member

Choose a reason for hiding this comment

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

This function would be easier to read (for me) as a single boolean expression.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean like this?

function isJSDocTypeExpressionOrChild(node: Node): boolean {
    return node.kind === SyntaxKind.JSDocTypeExpression || (node.parent && isJSDocTypeExpressionOrChild(node.parent)) || false;
}

Copy link
Member

Choose a reason for hiding this comment

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

Yes, except you don't need the trailing || false.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That will return undefined if node.parent is undefined. Will TypeScript accept that as boolean?

Copy link
Member

@sandersn sandersn Sep 4, 2018

Choose a reason for hiding this comment

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

Since we have strictNullChecks on now, yes. Put a !! before (node.parent && ...

Edit: I mean, no, it will not accept it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like there are no complaints from the compiler. I pushed 3b24fba to change this to a single boolean expression.

return true;
}
if (node.parent) {
return isJSDocTypeExpressionOrChild(node.parent);
}
return false;
}

export function getTextOfNodeFromSourceText(sourceText: string, node: Node, includeTrivia = false): string {
if (nodeIsMissing(node)) {
return "";
}

return sourceText.substring(includeTrivia ? node.pos : skipTrivia(sourceText, node.pos), node.end);
let text = sourceText.substring(includeTrivia ? node.pos : skipTrivia(sourceText, node.pos), node.end);

if (isJSDocTypeExpressionOrChild(node)) {
// strip space + asterisk at line start
Copy link
Member

Choose a reason for hiding this comment

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

Seems like this will do the wrong thing for the non-leading-asterisk use of the * type.

Copy link
Member

Choose a reason for hiding this comment

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

We discussed this and agreed that it's acceptable to get this wrong as long as there is a test showing how it gets it wrong.

Copy link
Contributor Author

@tschaub tschaub Sep 4, 2018

Choose a reason for hiding this comment

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

In 64bbf5e I added a test that shows how getTextOfNodeFromSourceText works on this (ugly) code:

/**
 * @param {{
 *   stringProp: string,
 *   numProp: number,
 *   boolProp: boolean,
 *   anyProp: *,
 *   anotherAnyProp:
 *   *,
 *   functionProp:
 *   function(string,
 *   *):
 *   *
 * }} o
 */
function f1(o) {
    o;
}

That results in this being (pretty) printed:

(parameter) o: {
    stringProp: string;
    numProp: number;
    boolProp: boolean;
    anyProp: any;
    anotherAnyProp: any;
    functionProp: (arg0: string, arg1: any) => any;
}

Not sure if this covers what you are talking about.

text = text.replace(/(^|\r?\n|\r)\s*\*\s*/g, "$1");
}

return text;
Copy link
Member

Choose a reason for hiding this comment

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

Does this change fix only fourslash tests or does it affect the type baselines as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This does not affect type baselines, only verify.quickInfoIs() in tests/cases/fourslash/jsDocFunctionSignatures12.ts

}

export function getTextOfNode(node: Node, includeTrivia = false): string {
Expand Down
136 changes: 136 additions & 0 deletions tests/baselines/reference/typedefTagWrapping.errors.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,136 @@
tests/cases/conformance/jsdoc/mod7.js(5,7): error TS1110: Type expected.
tests/cases/conformance/jsdoc/mod7.js(8,4): error TS1110: Type expected.


==== tests/cases/conformance/jsdoc/mod1.js (0 errors) ====
/**
* @typedef {function(string): boolean}
* Type1
*/

/**
* Tries to use a type whose name is on a different
* line than the typedef tag.
* @param {Type1} func The function to call.
* @param {string} arg The argument to call it with.
* @returns {boolean} The return.
*/
function callIt(func, arg) {
return func(arg);
}

==== tests/cases/conformance/jsdoc/mod2.js (0 errors) ====
/**
* @typedef {{
* num: number,
* str: string,
* boo: boolean
* }} Type2
*/

/**
* Makes use of a type with a multiline type expression.
* @param {Type2} obj The object.
* @returns {string|number} The return.
*/
function check(obj) {
return obj.boo ? obj.num : obj.str;
}

==== tests/cases/conformance/jsdoc/mod3.js (0 errors) ====
/**
* A function whose signature is very long.
*
* @typedef {function(boolean, string, number):
* (string|number)} StringOrNumber1
*/

/**
* Makes use of a function type with a long signature.
* @param {StringOrNumber1} func The function.
* @param {boolean} bool The condition.
* @param {string} str The string.
* @param {number} num The number.
* @returns {string|number} The return.
*/
function use1(func, bool, str, num) {
return func(bool, str, num)
}

==== tests/cases/conformance/jsdoc/mod4.js (0 errors) ====
/**
* A function whose signature is very long.
*
* @typedef {function(boolean, string,
* number):
* (string|number)} StringOrNumber2
*/

/**
* Makes use of a function type with a long signature.
* @param {StringOrNumber2} func The function.
* @param {boolean} bool The condition.
* @param {string} str The string.
* @param {number} num The number.
* @returns {string|number} The return.
*/
function use2(func, bool, str, num) {
return func(bool, str, num)
}

==== tests/cases/conformance/jsdoc/mod5.js (0 errors) ====
/**
* @typedef {{
* num:
* number,
* str:
* string,
* boo:
* boolean
* }} Type5
*/

/**
* Makes use of a type with a multiline type expression.
* @param {Type5} obj The object.
* @returns {string|number} The return.
*/
function check5(obj) {
return obj.boo ? obj.num : obj.str;
}

==== tests/cases/conformance/jsdoc/mod6.js (0 errors) ====
/**
* @typedef {{
* foo:
* *,
* bar:
* *
* }} Type6
*/

/**
* Makes use of a type with a multiline type expression.
* @param {Type6} obj The object.
* @returns {*} The return.
*/
function check6(obj) {
return obj.foo;
}


==== tests/cases/conformance/jsdoc/mod7.js (2 errors) ====
/**
Multiline type expressions in comments without leading * are not supported.
@typedef {{
foo:
*,
~
!!! error TS1110: Type expected.
bar:
*
}} Type7
~
!!! error TS1110: Type expected.
*/

Loading