Skip to content

Commit

Permalink
Fix closure compiler type casts (#5947)
Browse files Browse the repository at this point in the history
* 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
  • Loading branch information
jridgewell authored and j-f1 committed Apr 26, 2019
1 parent ed15b6d commit c085aeb
Show file tree
Hide file tree
Showing 6 changed files with 175 additions and 123 deletions.
19 changes: 19 additions & 0 deletions CHANGELOG.unreleased.md
Expand Up @@ -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.

<!-- prettier-ignore -->
```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);
```
27 changes: 12 additions & 15 deletions src/language-js/needs-parens.js
Expand Up @@ -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;
}
Expand All @@ -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)) {
Expand Down Expand Up @@ -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;
}

Expand Down
108 changes: 0 additions & 108 deletions tests/comments/__snapshots__/jsfmt.spec.js.snap
Expand Up @@ -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.<string>} */ (['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.<string>} */ (["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"]
Expand Down
132 changes: 132 additions & 0 deletions 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.<string>} */ (['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.<string>} */ (["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
});
================================================================================
`;
Expand Up @@ -16,10 +16,21 @@ function returnValue() {
return /** @type {!Array.<string>} */ (['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,
Expand Down
1 change: 1 addition & 0 deletions tests/comments_closure_typecast/jsfmt.spec.js
@@ -0,0 +1 @@
run_spec(__dirname, ["babel"]);

0 comments on commit c085aeb

Please sign in to comment.