From c085aeb152a7d881e4b85ebfc8a071f3aa1eb36e Mon Sep 17 00:00:00 2001 From: Justin Ridgewell Date: Fri, 26 Apr 2019 13:05:57 -0400 Subject: [PATCH] Fix closure compiler type casts (#5947) * Fix closure compiler type casts This fixes casts when they are followed by a closing parenthesis, eg: ```js foo( /** @type {!Array} */(arrOrString).length ); ``` The old code would see the `CallExpresion`'s closing `)` and assume the typecast belonged to the `MemberExpression`, not the `arrOrString` `Identifier`. This would be easier to accomplish if every AST would tell us if the expression were parenthesized. If they did, we could check that the node were parenthesized and either it or an ancestor has a typecast, stopping when we find an ancestor is itself parenthesized. * More tests, and changelog * Fix while loop * Update changelog * Update CHANGELOG.unreleased.md * Use babel's parenthesized information * Cleanup call --- CHANGELOG.unreleased.md | 19 +++ src/language-js/needs-parens.js | 27 ++-- .../comments/__snapshots__/jsfmt.spec.js.snap | 108 -------------- .../__snapshots__/jsfmt.spec.js.snap | 132 ++++++++++++++++++ .../closure-compiler-type-cast.js | 11 ++ tests/comments_closure_typecast/jsfmt.spec.js | 1 + 6 files changed, 175 insertions(+), 123 deletions(-) create mode 100644 tests/comments_closure_typecast/__snapshots__/jsfmt.spec.js.snap rename tests/{comments => comments_closure_typecast}/closure-compiler-type-cast.js (68%) create mode 100644 tests/comments_closure_typecast/jsfmt.spec.js diff --git a/CHANGELOG.unreleased.md b/CHANGELOG.unreleased.md index 7b5a8f5be608..a89a80824180 100644 --- a/CHANGELOG.unreleased.md +++ b/CHANGELOG.unreleased.md @@ -41,3 +41,22 @@ Examples: ``` --> + +- JavaScript: Fix closure compiler typecasts ([#5947] by [@jridgewell]) + + If a closing parenthesis follows after a typecast in an inner expression, the typecast would wrap everything to the that following parenthesis. + + + ```js + // Input + test(/** @type {!Array} */(arrOrString).length); + test(/** @type {!Array} */((arrOrString)).length + 1); + + // Output (Prettier stable) + test(/** @type {!Array} */ (arrOrString.length)); + test(/** @type {!Array} */ (arrOrString.length + 1)); + + // Output (Prettier master) + test(/** @type {!Array} */ (arrOrString).length); + test(/** @type {!Array} */ (arrOrString).length + 1); + ``` diff --git a/src/language-js/needs-parens.js b/src/language-js/needs-parens.js index 4ad67228afa9..51135fcae3e5 100644 --- a/src/language-js/needs-parens.js +++ b/src/language-js/needs-parens.js @@ -6,23 +6,21 @@ const util = require("../common/util"); const comments = require("./comments"); const { hasFlowShorthandAnnotationComment } = require("./utils"); -function hasClosureCompilerTypeCastComment(text, path, locStart, locEnd) { +function hasClosureCompilerTypeCastComment(text, path) { // https://github.com/google/closure-compiler/wiki/Annotating-Types#type-casts // Syntax example: var x = /** @type {string} */ (fruit); const n = path.getValue(); return ( - util.getNextNonSpaceNonCommentCharacter(text, n, locEnd) === ")" && + isParenthesized(n) && (hasTypeCastComment(n) || hasAncestorTypeCastComment(0)) ); // for sub-item: /** @type {array} */ (numberOrString).map(x => x); function hasAncestorTypeCastComment(index) { const ancestor = path.getParentNode(index); - return ancestor && - util.getNextNonSpaceNonCommentCharacter(text, ancestor, locEnd) !== ")" && - /^[\s(]*$/.test(text.slice(locStart(ancestor), locStart(n))) + return ancestor && !isParenthesized(ancestor) ? hasTypeCastComment(ancestor) || hasAncestorTypeCastComment(index + 1) : false; } @@ -34,12 +32,18 @@ function hasClosureCompilerTypeCastComment(text, path, locStart, locEnd) { comment => comment.leading && comments.isBlockComment(comment) && - isTypeCastComment(comment.value) && - util.getNextNonSpaceNonCommentCharacter(text, comment, locEnd) === "(" + isTypeCastComment(comment.value) ) ); } + function isParenthesized(node) { + // Closure typecast comments only really make sense when _not_ using + // typescript or flow parsers, so we take advantage of the babel parser's + // parenthesized expressions. + return node.extra && node.extra.parenthesized; + } + function isTypeCastComment(comment) { const trimmed = comment.trim(); if (!/^\*\s*@type\s*\{[^]+\}$/.test(trimmed)) { @@ -100,14 +104,7 @@ function needsParens(path, options) { // Closure compiler requires that type casted expressions to be surrounded by // parentheses. - if ( - hasClosureCompilerTypeCastComment( - options.originalText, - path, - options.locStart, - options.locEnd - ) - ) { + if (hasClosureCompilerTypeCastComment(options.originalText, path)) { return true; } diff --git a/tests/comments/__snapshots__/jsfmt.spec.js.snap b/tests/comments/__snapshots__/jsfmt.spec.js.snap index 9198e1dc3efc..1cc4bce165fe 100644 --- a/tests/comments/__snapshots__/jsfmt.spec.js.snap +++ b/tests/comments/__snapshots__/jsfmt.spec.js.snap @@ -323,114 +323,6 @@ React.render( ================================================================================ `; -exports[`closure-compiler-type-cast.js 1`] = ` -====================================options===================================== -parsers: ["flow", "babel"] -printWidth: 80 - | printWidth -=====================================input====================================== -// test to make sure comments are attached correctly -let inlineComment = /* some comment */ ( - someReallyLongFunctionCall(withLots, ofArguments)); - -let object = { - key: /* some comment */ (someReallyLongFunctionCall(withLots, ofArguments)) -}; - -// preserve parens only for type casts -let assignment = /** @type {string} */ (getValue()); -let value = /** @type {string} */ (this.members[0]).functionCall(); - -functionCall(1 + /** @type {string} */ (value), /** @type {!Foo} */ ({})); - -function returnValue() { - return /** @type {!Array.} */ (['hello', 'you']); -} - -var newArray = /** @type {array} */ (numberOrString).map(x => x); -var newArray = /** @type {array} */ ((numberOrString)).map(x => x); -var newArray = /** @type {array} */ ((numberOrString).map(x => x)); - - -const data = functionCall( - arg1, - arg2, - /** @type {{height: number, width: number}} */ (arg3)); - -// Invalid type casts -const v = /** @type {} */ (value); -const v = /** @type {}} */ (value); -const v = /** @type } */ (value); -const v = /** @type { */ (value); -const v = /** @type {{} */ (value); - -const style = /** @type {{ - width: number, - height: number, - marginTop: number, - marginLeft: number, - marginRight: number, - marginBottom: number, -}} */ ({ - width, - height, - ...margins, -}); - -=====================================output===================================== -// test to make sure comments are attached correctly -let inlineComment = /* some comment */ someReallyLongFunctionCall( - withLots, - ofArguments -); - -let object = { - key: /* some comment */ someReallyLongFunctionCall(withLots, ofArguments) -}; - -// preserve parens only for type casts -let assignment = /** @type {string} */ (getValue()); -let value = /** @type {string} */ (this.members[0]).functionCall(); - -functionCall(1 + /** @type {string} */ (value), /** @type {!Foo} */ ({})); - -function returnValue() { - return /** @type {!Array.} */ (["hello", "you"]); -} - -var newArray = /** @type {array} */ (numberOrString).map(x => x); -var newArray = /** @type {array} */ (numberOrString).map(x => x); -var newArray = /** @type {array} */ (numberOrString.map(x => x)); - -const data = functionCall( - arg1, - arg2, - /** @type {{height: number, width: number}} */ (arg3) -); - -// Invalid type casts -const v = /** @type {} */ value; -const v = /** @type {}} */ value; -const v = /** @type } */ value; -const v = /** @type { */ value; -const v = /** @type {{} */ value; - -const style = /** @type {{ - width: number, - height: number, - marginTop: number, - marginLeft: number, - marginRight: number, - marginBottom: number, -}} */ ({ - width, - height, - ...margins -}); - -================================================================================ -`; - exports[`dangling.js 1`] = ` ====================================options===================================== parsers: ["flow", "babel"] diff --git a/tests/comments_closure_typecast/__snapshots__/jsfmt.spec.js.snap b/tests/comments_closure_typecast/__snapshots__/jsfmt.spec.js.snap new file mode 100644 index 000000000000..a2549b7b2306 --- /dev/null +++ b/tests/comments_closure_typecast/__snapshots__/jsfmt.spec.js.snap @@ -0,0 +1,132 @@ +// Jest Snapshot v1, https://goo.gl/fbAQLP + +exports[`closure-compiler-type-cast.js 1`] = ` +====================================options===================================== +parsers: ["babel"] +printWidth: 80 + | printWidth +=====================================input====================================== +// test to make sure comments are attached correctly +let inlineComment = /* some comment */ ( + someReallyLongFunctionCall(withLots, ofArguments)); + +let object = { + key: /* some comment */ (someReallyLongFunctionCall(withLots, ofArguments)) +}; + +// preserve parens only for type casts +let assignment = /** @type {string} */ (getValue()); +let value = /** @type {string} */ (this.members[0]).functionCall(); + +functionCall(1 + /** @type {string} */ (value), /** @type {!Foo} */ ({})); + +function returnValue() { + return /** @type {!Array.} */ (['hello', 'you']); +} + +// Only numberOrString is typecast +var newArray = /** @type {array} */ (numberOrString).map(x => x); +var newArray = /** @type {array} */ ((numberOrString)).map(x => x); +var newArray = test(/** @type {array} */ (numberOrString).map(x => x)); +var newArray = test(/** @type {array} */ ((numberOrString)).map(x => x)); + +// The numberOrString.map CallExpression is typecast +var newArray = /** @type {array} */ (numberOrString.map(x => x)); +var newArray = /** @type {array} */ ((numberOrString).map(x => x)); +var newArray = test(/** @type {array} */ (numberOrString.map(x => x))); +var newArray = test(/** @type {array} */ ((numberOrString).map(x => x))); + +test(/** @type {number} */(num) + 1); +test(/** @type {!Array} */(arrOrString).length + 1); +test(/** @type {!Array} */((arrOrString)).length + 1); + +const data = functionCall( + arg1, + arg2, + /** @type {{height: number, width: number}} */ (arg3)); + +// Invalid type casts +const v = /** @type {} */ (value); +const v = /** @type {}} */ (value); +const v = /** @type } */ (value); +const v = /** @type { */ (value); +const v = /** @type {{} */ (value); + +const style = /** @type {{ + width: number, + height: number, + marginTop: number, + marginLeft: number, + marginRight: number, + marginBottom: number, +}} */ ({ + width, + height, + ...margins, +}); + +=====================================output===================================== +// test to make sure comments are attached correctly +let inlineComment = /* some comment */ someReallyLongFunctionCall( + withLots, + ofArguments +); + +let object = { + key: /* some comment */ someReallyLongFunctionCall(withLots, ofArguments) +}; + +// preserve parens only for type casts +let assignment = /** @type {string} */ (getValue()); +let value = /** @type {string} */ (this.members[0]).functionCall(); + +functionCall(1 + /** @type {string} */ (value), /** @type {!Foo} */ ({})); + +function returnValue() { + return /** @type {!Array.} */ (["hello", "you"]); +} + +// Only numberOrString is typecast +var newArray = /** @type {array} */ (numberOrString).map(x => x); +var newArray = /** @type {array} */ (numberOrString).map(x => x); +var newArray = test(/** @type {array} */ (numberOrString).map(x => x)); +var newArray = test(/** @type {array} */ (numberOrString).map(x => x)); + +// The numberOrString.map CallExpression is typecast +var newArray = /** @type {array} */ (numberOrString.map(x => x)); +var newArray = /** @type {array} */ (numberOrString.map(x => x)); +var newArray = test(/** @type {array} */ (numberOrString.map(x => x))); +var newArray = test(/** @type {array} */ (numberOrString.map(x => x))); + +test(/** @type {number} */ (num) + 1); +test(/** @type {!Array} */ (arrOrString).length + 1); +test(/** @type {!Array} */ (arrOrString).length + 1); + +const data = functionCall( + arg1, + arg2, + /** @type {{height: number, width: number}} */ (arg3) +); + +// Invalid type casts +const v = /** @type {} */ value; +const v = /** @type {}} */ value; +const v = /** @type } */ value; +const v = /** @type { */ value; +const v = /** @type {{} */ value; + +const style = /** @type {{ + width: number, + height: number, + marginTop: number, + marginLeft: number, + marginRight: number, + marginBottom: number, +}} */ ({ + width, + height, + ...margins +}); + +================================================================================ +`; diff --git a/tests/comments/closure-compiler-type-cast.js b/tests/comments_closure_typecast/closure-compiler-type-cast.js similarity index 68% rename from tests/comments/closure-compiler-type-cast.js rename to tests/comments_closure_typecast/closure-compiler-type-cast.js index 8fce7d6f21c7..52ed42dd0f0d 100644 --- a/tests/comments/closure-compiler-type-cast.js +++ b/tests/comments_closure_typecast/closure-compiler-type-cast.js @@ -16,10 +16,21 @@ function returnValue() { return /** @type {!Array.} */ (['hello', 'you']); } +// Only numberOrString is typecast var newArray = /** @type {array} */ (numberOrString).map(x => x); var newArray = /** @type {array} */ ((numberOrString)).map(x => x); +var newArray = test(/** @type {array} */ (numberOrString).map(x => x)); +var newArray = test(/** @type {array} */ ((numberOrString)).map(x => x)); + +// The numberOrString.map CallExpression is typecast +var newArray = /** @type {array} */ (numberOrString.map(x => x)); var newArray = /** @type {array} */ ((numberOrString).map(x => x)); +var newArray = test(/** @type {array} */ (numberOrString.map(x => x))); +var newArray = test(/** @type {array} */ ((numberOrString).map(x => x))); +test(/** @type {number} */(num) + 1); +test(/** @type {!Array} */(arrOrString).length + 1); +test(/** @type {!Array} */((arrOrString)).length + 1); const data = functionCall( arg1, diff --git a/tests/comments_closure_typecast/jsfmt.spec.js b/tests/comments_closure_typecast/jsfmt.spec.js new file mode 100644 index 000000000000..8382eddeb1db --- /dev/null +++ b/tests/comments_closure_typecast/jsfmt.spec.js @@ -0,0 +1 @@ +run_spec(__dirname, ["babel"]);