From 38ae5d5210d9d81776672f2aea00d982160c53b8 Mon Sep 17 00:00:00 2001 From: Brian Kim Date: Tue, 1 Oct 2019 06:53:45 -0400 Subject: [PATCH] Javascript: Use function literals in arguments to detect function composition (#6033) --- CHANGELOG.unreleased.md | 55 +++++++- src/language-js/printer-estree.js | 119 ++++++++++-------- tests/arrows/__snapshots__/jsfmt.spec.js.snap | 10 +- .../__snapshots__/jsfmt.spec.js.snap | 5 +- .../__snapshots__/jsfmt.spec.js.snap | 5 +- .../__snapshots__/jsfmt.spec.js.snap | 77 ++++-------- tests/functional_composition/ramda_compose.js | 5 + .../__snapshots__/jsfmt.spec.js.snap | 7 +- 8 files changed, 172 insertions(+), 111 deletions(-) diff --git a/CHANGELOG.unreleased.md b/CHANGELOG.unreleased.md index 66319eb6..f7c89507 100644 --- a/CHANGELOG.unreleased.md +++ b/CHANGELOG.unreleased.md @@ -284,6 +284,57 @@ foo; ); ``` +### Javascript: Use function literals in arguments to detect function composition ([#6033] by [@brainkim]) + +Previously, we used a set of hard-coded names related to functional programming +(`compose`, `flow`, `pipe`, etc.) to detect function composition and chaining +patterns in code. This was done so that prettier would not put code like the +following call to `pipe` on the same line even if it fit within the allotted +column budget: + + +```js +source$ + .pipe( + filter(x => x % 2 === 0), + map(x => x + x), + scan((acc, x) => acc + x, 0), + ) + .subscribe(x => console.log(x)); +``` + +However, this heuristic caused people to complain because of false positives +where calls to functions or methods matching the hard-coded names would always +be split on multiple lines, even if the calls did not contain function +arguments (https://github.com/prettier/prettier/issues/5769, +https://github.com/prettier/prettier/issues/5969). For many, this blanket +decision to split functions based on name was both surprising and sub-optimal. + +We now use a refined heuristic which uses the presence of function literals to +detect function composition. This heuristic preserves the line-splitting +behavior above and eliminates many if not all of the false positives caused by +the older heuristic. + +We encourage prettier users to try out the new heuristic and provide feedback. + + +```js +// Input +eventStore.update(id, _.flow(updater, incrementVersion)); + +// Output (Prettier stable) +eventStore.update( + id, + _.flow( + updater, + incrementVersion + ) +); + +// Output (Prettier master) +eventStore.update(id, _.flow(updater, incrementVersion)); +``` + ### Handlebars: Improve comment formatting ([#6234] by [@gavinjoyce]) Previously, Prettier would incorrectly decode HTML entiites. @@ -696,6 +747,7 @@ Previously, the flag was not applied on html attributes. ```` [#5910]: https://github.com/prettier/prettier/pull/5910 +[#6033]: https://github.com/prettier/prettier/pull/6033 [#6186]: https://github.com/prettier/prettier/pull/6186 [#6206]: https://github.com/prettier/prettier/pull/6206 [#6209]: https://github.com/prettier/prettier/pull/6209 @@ -704,10 +756,10 @@ Previously, the flag was not applied on html attributes. [#6236]: https://github.com/prettier/prettier/pull/6236 [#6270]: https://github.com/prettier/prettier/pull/6270 [#6289]: https://github.com/prettier/prettier/pull/6289 -[#6332]: https://github.com/prettier/prettier/pull/6332 [#6284]: https://github.com/prettier/prettier/pull/6284 [#6301]: https://github.com/prettier/prettier/pull/6301 [#6307]: https://github.com/prettier/prettier/pull/6307 +[#6332]: https://github.com/prettier/prettier/pull/6332 [#6340]: https://github.com/prettier/prettier/pull/6340 [#6412]: https://github.com/prettier/prettier/pull/6412 [#6423]: https://github.com/prettier/prettier/pull/6423 @@ -720,6 +772,7 @@ Previously, the flag was not applied on html attributes. [#6514]: https://github.com/prettier/prettier/pull/6514 [#6467]: https://github.com/prettier/prettier/pull/6467 [#6377]: https://github.com/prettier/prettier/pull/6377 +[@brainkim]: https://github.com/brainkim [@duailibe]: https://github.com/duailibe [@gavinjoyce]: https://github.com/gavinjoyce [@sosukesuzuki]: https://github.com/sosukesuzuki diff --git a/src/language-js/printer-estree.js b/src/language-js/printer-estree.js index aa6f8af8..86ebc3d7 100644 --- a/src/language-js/printer-estree.js +++ b/src/language-js/printer-estree.js @@ -1210,7 +1210,7 @@ function printPathNoParens(path, options, print, args) { return printMemberChain(path, options, print); } - return concat([ + const contents = concat([ isNew ? "new " : "", path.call(print, "callee"), optional, @@ -1220,6 +1220,14 @@ function printPathNoParens(path, options, print, args) { printFunctionTypeParameters(path, options, print), printArgumentsList(path, options, print) ]); + + // We group here when the callee is itself a call expression. + // See `isLongCurriedCallExpression` for more info. + if (isCallOrOptionalCallExpression(n.callee)) { + return group(contents); + } + + return contents; } case "TSInterfaceDeclaration": if (isNodeStartingWithDeclare(n, options)) { @@ -4006,40 +4014,47 @@ function isSimpleTemplateLiteral(node) { }); } -const functionCompositionFunctionNames = new Set([ - "pipe", // RxJS, Ramda - "pipeP", // Ramda - "pipeK", // Ramda - "compose", // Ramda, Redux - "composeFlipped", // Not from any library, but common in Haskell, so supported - "composeP", // Ramda - "composeK", // Ramda - "flow", // Lodash - "flowRight", // Lodash - "connect", // Redux - "createSelector" // Reselect -]); -const ordinaryMethodNames = new Set([ - "connect" // GObject, MongoDB -]); - -function isFunctionCompositionFunction(node) { - switch (node.type) { - case "OptionalMemberExpression": - case "MemberExpression": { - return ( - isFunctionCompositionFunction(node.property) && - !ordinaryMethodNames.has(node.property.name) - ); - } - case "Identifier": { - return functionCompositionFunctionNames.has(node.name); - } - case "StringLiteral": - case "Literal": { - return functionCompositionFunctionNames.has(node.value); +// Logic to check for args with multiple anonymous functions. For instance, +// the following call should be split on multiple lines for readability: +// source.pipe(map((x) => x + x), filter((x) => x % 2 === 0)) +function isFunctionCompositionArgs(args) { + if (args.length <= 1) { + return false; + } + let count = 0; + for (const arg of args) { + if (isFunctionOrArrowExpression(arg)) { + count += 1; + if (count > 1) { + return true; + } + } else if (isCallOrOptionalCallExpression(arg)) { + for (const childArg of arg.arguments) { + if (isFunctionOrArrowExpression(childArg)) { + return true; + } + } } } + return false; +} + +// Logic to determine if a call is a “long curried function call”. +// See https://github.com/prettier/prettier/issues/1420. +// +// `connect(a, b, c)(d)` +// In the above call expression, the second call is the parent node and the +// first call is the current node. +function isLongCurriedCallExpression(path) { + const node = path.getValue(); + const parent = path.getParentNode(); + return ( + isCallOrOptionalCallExpression(node) && + isCallOrOptionalCallExpression(parent) && + parent.callee === node && + node.arguments.length > parent.arguments.length && + parent.arguments.length > 0 + ); } function printArgumentsList(path, options, print) { @@ -4143,14 +4158,7 @@ function printArgumentsList(path, options, print) { ); } - // We want to get - // pipe( - // x => x + 1, - // x => x - 1 - // ) - // here, but not - // process.stdout.pipe(socket) - if (isFunctionCompositionFunction(node.callee) && args.length > 1) { + if (isFunctionCompositionArgs(args)) { return allArgsBrokenOut(); } @@ -4217,16 +4225,22 @@ function printArgumentsList(path, options, print) { ]); } - return group( - concat([ - "(", - indent(concat([softline, concat(printedArguments)])), - ifBreak(maybeTrailingComma), - softline, - ")" - ]), - { shouldBreak: printedArguments.some(willBreak) || anyArgEmptyLine } - ); + const contents = concat([ + "(", + indent(concat([softline, concat(printedArguments)])), + ifBreak(maybeTrailingComma), + softline, + ")" + ]); + if (isLongCurriedCallExpression(path)) { + // By not wrapping the arguments in a group, the printer prioritizes + // breaking up these arguments rather than the args of the parent call. + return contents; + } + + return group(contents, { + shouldBreak: printedArguments.some(willBreak) || anyArgEmptyLine + }); } function printTypeAnnotation(path, options, print) { @@ -5199,6 +5213,9 @@ function printMemberChain(path, options, print) { // If we only have a single `.`, we shouldn't do anything fancy and just // render everything concatenated together. if (groups.length <= cutoff && !hasComment) { + if (isLongCurriedCallExpression(path)) { + return oneLine; + } return group(oneLine); } diff --git a/tests/arrows/__snapshots__/jsfmt.spec.js.snap b/tests/arrows/__snapshots__/jsfmt.spec.js.snap index 46cdbc7a..f9c58baa 100644 --- a/tests/arrows/__snapshots__/jsfmt.spec.js.snap +++ b/tests/arrows/__snapshots__/jsfmt.spec.js.snap @@ -1084,7 +1084,10 @@ foo(c, a => b, d) foo(a => b, d) =====================================output===================================== -promise.then(result => result, err => err); +promise.then( + result => result, + err => err +); promise.then( result => { @@ -1132,7 +1135,10 @@ foo(c, a => b, d) foo(a => b, d) =====================================output===================================== -promise.then((result) => result, (err) => err); +promise.then( + (result) => result, + (err) => err +); promise.then( (result) => { diff --git a/tests/decorators-ts/__snapshots__/jsfmt.spec.js.snap b/tests/decorators-ts/__snapshots__/jsfmt.spec.js.snap index a34eb397..898ab7e2 100644 --- a/tests/decorators-ts/__snapshots__/jsfmt.spec.js.snap +++ b/tests/decorators-ts/__snapshots__/jsfmt.spec.js.snap @@ -321,7 +321,10 @@ export class Board { @Column() description: string; - @OneToMany(type => Topic, topic => topic.board) + @OneToMany( + type => Topic, + topic => topic.board + ) topics: Topic[]; } diff --git a/tests/decorators/__snapshots__/jsfmt.spec.js.snap b/tests/decorators/__snapshots__/jsfmt.spec.js.snap index db8f10d3..10201131 100644 --- a/tests/decorators/__snapshots__/jsfmt.spec.js.snap +++ b/tests/decorators/__snapshots__/jsfmt.spec.js.snap @@ -344,10 +344,7 @@ export class MyApp extends React.Component {} export class Home extends React.Component {} =====================================output===================================== -@connect( - mapStateToProps, - mapDispatchToProps -) +@connect(mapStateToProps, mapDispatchToProps) export class MyApp extends React.Component {} @connect(state => ({ todos: state.todos })) diff --git a/tests/functional_composition/__snapshots__/jsfmt.spec.js.snap b/tests/functional_composition/__snapshots__/jsfmt.spec.js.snap index ca9068fa..8aeb875a 100644 --- a/tests/functional_composition/__snapshots__/jsfmt.spec.js.snap +++ b/tests/functional_composition/__snapshots__/jsfmt.spec.js.snap @@ -87,12 +87,7 @@ this.a.b.c.compose( sortBy(x => x), flatten ); -someObj.someMethod( - this.field.compose( - a, - b - ) -); +someObj.someMethod(this.field.compose(a, b)); class A extends B { compose() { @@ -105,10 +100,7 @@ class A extends B { this.subscriptions.add( this.componentUpdates - .pipe( - startWith(this.props), - distinctUntilChanged(isEqual) - ) + .pipe(startWith(this.props), distinctUntilChanged(isEqual)) .subscribe(props => {}) ); @@ -311,30 +303,25 @@ var followersForUser = R.composeP(lookupFollowers, lookupUser); followersForUser("JOE").then(followers => console.log("Followers:", followers)); // Followers: ["STEVE","SUZY"] +const mapStateToProps = state => ({ + users: R.compose( R.filter(R.propEq('status', 'active')), + R.values)(state.users) +}); + =====================================output===================================== var classyGreeting = (firstName, lastName) => "The name's " + lastName + ", " + firstName + " " + lastName; -var yellGreeting = R.compose( - R.toUpper, - classyGreeting -); +var yellGreeting = R.compose(R.toUpper, classyGreeting); yellGreeting("James", "Bond"); //=> "THE NAME'S BOND, JAMES BOND" -R.compose( - Math.abs, - R.add(1), - R.multiply(2) -)(-4); //=> 7 +R.compose(Math.abs, R.add(1), R.multiply(2))(-4); //=> 7 // get :: String -> Object -> Maybe * var get = R.curry((propName, obj) => Maybe(obj[propName])); // getStateCode :: Maybe String -> Maybe String var getStateCode = R.composeK( - R.compose( - Maybe.of, - R.toUpper - ), + R.compose(Maybe.of, R.toUpper), get("state"), get("address"), get("user") @@ -357,13 +344,17 @@ var lookupFollowers = user => Promise.resolve(user.followers); lookupUser("JOE").then(lookupFollowers); // followersForUser :: String -> Promise [UserId] -var followersForUser = R.composeP( - lookupFollowers, - lookupUser -); +var followersForUser = R.composeP(lookupFollowers, lookupUser); followersForUser("JOE").then(followers => console.log("Followers:", followers)); // Followers: ["STEVE","SUZY"] +const mapStateToProps = state => ({ + users: R.compose( + R.filter(R.propEq("status", "active")), + R.values + )(state.users) +}); + ================================================================================ `; @@ -398,11 +389,7 @@ getStateCode("[Invalid JSON]"); var followersForUser = R.pipeP(db.getUserById, db.getFollowers); =====================================output===================================== -var f = R.pipe( - Math.pow, - R.negate, - R.inc -); +var f = R.pipe(Math.pow, R.negate, R.inc); f(3, 4); // -(3^4) + 1 @@ -415,10 +402,7 @@ var getStateCode = R.pipeK( get("user"), get("address"), get("state"), - R.compose( - Maybe.of, - R.toUpper - ) + R.compose(Maybe.of, R.toUpper) ); getStateCode('{"user":{"address":{"state":"ny"}}}'); @@ -427,10 +411,7 @@ getStateCode("[Invalid JSON]"); //=> Nothing() // followersForUser :: String -> Promise [User] -var followersForUser = R.pipeP( - db.getUserById, - db.getFollowers -); +var followersForUser = R.pipeP(db.getUserById, db.getFollowers); ================================================================================ `; @@ -463,10 +444,7 @@ import reducer from "../reducers"; const store = createStore( reducer, - compose( - applyMiddleware(thunk), - DevTools.instrument() - ) + compose(applyMiddleware(thunk), DevTools.instrument()) ); ================================================================================ @@ -512,15 +490,12 @@ const resolve = createSelector( =====================================output===================================== import { createSelector } from "reselect"; -const resolve = createSelector( - getIds, - getObjects, - (ids, objects) => ids.map(id => objects[id]) +const resolve = createSelector(getIds, getObjects, (ids, objects) => + ids.map(id => objects[id]) ); -const resolve = createSelector( - [getIds, getObjects], - (ids, objects) => ids.map(id => objects[id]) +const resolve = createSelector([getIds, getObjects], (ids, objects) => + ids.map(id => objects[id]) ); ================================================================================ diff --git a/tests/functional_composition/ramda_compose.js b/tests/functional_composition/ramda_compose.js index 6ccfc238..767f6fbe 100644 --- a/tests/functional_composition/ramda_compose.js +++ b/tests/functional_composition/ramda_compose.js @@ -36,3 +36,8 @@ lookupUser("JOE").then(lookupFollowers); var followersForUser = R.composeP(lookupFollowers, lookupUser); followersForUser("JOE").then(followers => console.log("Followers:", followers)); // Followers: ["STEVE","SUZY"] + +const mapStateToProps = state => ({ + users: R.compose( R.filter(R.propEq('status', 'active')), + R.values)(state.users) +}); diff --git a/tests/typescript_arrow/__snapshots__/jsfmt.spec.js.snap b/tests/typescript_arrow/__snapshots__/jsfmt.spec.js.snap index ee60d5be..394c70a0 100644 --- a/tests/typescript_arrow/__snapshots__/jsfmt.spec.js.snap +++ b/tests/typescript_arrow/__snapshots__/jsfmt.spec.js.snap @@ -27,7 +27,12 @@ const bar = (...varargs: any[]) => { console.log(varargs); }; -const foo = (x: string): void => bar(x, () => {}, () => {}); +const foo = (x: string): void => + bar( + x, + () => {}, + () => {} + ); app.get("/", (req, res): void => { res.send("Hello world");