Skip to content

Commit

Permalink
Refactor, optimize and fix call args expansion (prettier#13341)
Browse files Browse the repository at this point in the history
  • Loading branch information
@thorn0 @medikoo
thorn0 authored and medikoo committed Jan 31, 2024
1 parent c510621 commit 40f0ae7
Show file tree
Hide file tree
Showing 15 changed files with 616 additions and 82 deletions.
68 changes: 68 additions & 0 deletions changelog_unreleased/javascript/13341.md
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
#### Fix edge cases of the first call argument expansion (#13341 by @thorn0)

<!-- prettier-ignore -->
```jsx
// Input
export default whatever(function (a: {
aaaaaaaaa: string;
bbbbbbbbb: string;
ccccccccc: string;
}) {
return null;
}, "xyz");

call(
function() {
return 1;
},
$var ?? $var ?? $var ?? $var ?? $var ?? $var ?? $var ?? $var ?? $var ?? 'test'
);

// Prettier stable
export default whatever(function (a: {
aaaaaaaaa: string;
bbbbbbbbb: string;
ccccccccc: string;
}) {
return null;
},
"xyz");

call(function () {
return 1;
}, $var ??
$var ??
$var ??
$var ??
$var ??
$var ??
$var ??
$var ??
$var ??
"test");

// Prettier main
export default whatever(function (a: {
aaaaaaaaa: string,
bbbbbbbbb: string,
ccccccccc: string,
}) {
return null;
}, "xyz");

call(
function () {
return 1;
},
$var ??
$var ??
$var ??
$var ??
$var ??
$var ??
$var ??
$var ??
$var ??
"test",
);
```
187 changes: 125 additions & 62 deletions src/language-js/print/call-arguments.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,12 @@ import {
isStringLiteral,
isObjectProperty,
isTSTypeExpression,
getCallArgumentSelector,
isSimpleCallArgument,
isBinaryish,
isRegExpLiteral,
isSimpleType,
isCallLikeExpression,
} from "../utils/index.js";

import {
Expand All @@ -36,7 +42,6 @@ import { isConciselyPrintedArray } from "./array.js";

function printCallArguments(path, options, print) {
const node = path.getValue();
const isDynamicImport = node.type === "ImportExpression";

const args = getCallArguments(node);
if (args.length === 0) {
Expand All @@ -53,7 +58,6 @@ function printCallArguments(path, options, print) {
}

let anyArgEmptyLine = false;
let hasEmptyLineFollowingFirstArg = false;
const lastArgIndex = args.length - 1;
const separators = [];
const printedArguments = [];
Expand All @@ -64,10 +68,6 @@ function printCallArguments(path, options, print) {
if (index === lastArgIndex) {
// do nothing
} else if (isNextLineEmpty(arg, options)) {
if (index === 0) {
hasEmptyLineFollowingFirstArg = true;
}

anyArgEmptyLine = true;
separators.push([",", hardline, hardline]);
} else {
Expand All @@ -77,12 +77,11 @@ function printCallArguments(path, options, print) {
printedArguments.push(parts);
});

// Dynamic imports cannot have trailing commas
const isDynamicImport =
node.type === "ImportExpression" || node.callee.type === "Import";
const maybeTrailingComma =
// Dynamic imports cannot have trailing commas
!(isDynamicImport || (node.callee && node.callee.type === "Import")) &&
shouldPrintComma(options, "all")
? ","
: "";
!isDynamicImport && shouldPrintComma(options, "all") ? "," : "";

const joinPrinted = (target, index = 0, end = 0) => {
const res = [];
Expand Down Expand Up @@ -115,33 +114,58 @@ function printCallArguments(path, options, print) {
);
}

const shouldGroupFirst = shouldGroupFirstArg(args);
const shouldGroupLast = shouldGroupLastArg(args, options);
if (shouldGroupFirst || shouldGroupLast) {
if (
shouldGroupFirst
? printedArguments.slice(1).some(willBreak)
: printedArguments.slice(0, -1).some(willBreak)
) {
if (
anyArgEmptyLine ||
(path.getParentNode().type !== "Decorator" &&
isFunctionCompositionArgs(args))
) {
return allArgsBrokenOut();
}

if (shouldExpandFirstArg(args)) {
const tailArgs = joinPrinted(printedArguments, 1);
if (tailArgs.some(willBreak)) {
return allArgsBrokenOut();
}
let firstArg;
try {
firstArg = print(getCallArgumentSelector(node, 0), {
expandFirstArg: true,
});
} catch (caught) {
if (caught instanceof ArgExpansionBailout) {
return allArgsBrokenOut();
}
/* istanbul ignore next */
throw caught;
}

if (willBreak(firstArg)) {
return [
breakParent,
conditionalGroup([
["(", group(firstArg, { shouldBreak: true }), ", ", ...tailArgs, ")"],
allArgsBrokenOut(),
]),
];
}

return conditionalGroup([
["(", firstArg, ", ", ...tailArgs, ")"],
["(", group(firstArg, { shouldBreak: true }), ", ", ...tailArgs, ")"],
allArgsBrokenOut(),
]);
}

// We want to print the last argument with a special flag
let printedExpanded = [];
if (shouldExpandLastArg(args, options)) {
const headArgs = joinPrinted(printedArguments, 0, -1);
if (headArgs.some(willBreak)) {
return allArgsBrokenOut();
}
let lastArg;
try {
iterateCallArgumentsPath(path, (argPath, i) => {
if (shouldGroupFirst && i === 0) {
printedExpanded = [
print([], { expandFirstArg: true }),
...printedArguments.slice(1),
];
}
if (shouldGroupLast && i === lastArgIndex) {
printedExpanded = [
...printedArguments.slice(0, -1),
print([], { expandLastArg: true }),
];
}
lastArg = print(getCallArgumentSelector(node, -1), {
expandLastArg: true,
});
} catch (caught) {
if (caught instanceof ArgExpansionBailout) {
Expand All @@ -151,26 +175,21 @@ function printCallArguments(path, options, print) {
throw caught;
}

return [
printedArguments.some(willBreak) ? breakParent : "",
conditionalGroup([
["(", ...joinPrinted(printedExpanded), ")"],
shouldGroupFirst
? [
"(",
group(printedExpanded[0], { shouldBreak: true }),
...joinPrinted(printedExpanded, 1),
")",
]
: [
"(",
...joinPrinted(printedArguments, 0, -1),
group(getLast(printedExpanded), { shouldBreak: true }),
")",
],
allArgsBrokenOut(),
]),
];
if (willBreak(lastArg)) {
return [
breakParent,
conditionalGroup([
["(", ...headArgs, group(lastArg, { shouldBreak: true }), ")"],
allArgsBrokenOut(),
]),
];
}

return conditionalGroup([
["(", ...headArgs, lastArg, ")"],
["(", ...headArgs, group(lastArg, { shouldBreak: true }), ")"],
allArgsBrokenOut(),
]);
}

const printedArgumentsConcat = {
Expand Down Expand Up @@ -201,14 +220,14 @@ function printCallArguments(path, options, print) {
});
}

function couldGroupArg(arg, arrowChainRecursion = false) {
function couldExpandArg(arg, arrowChainRecursion = false) {
return (
(arg.type === "ObjectExpression" &&
(arg.properties.length > 0 || hasComment(arg))) ||
(arg.type === "ArrayExpression" &&
(arg.elements.length > 0 || hasComment(arg))) ||
(arg.type === "TSTypeAssertion" && couldGroupArg(arg.expression)) ||
(isTSTypeExpression(arg) && couldGroupArg(arg.expression)) ||
(arg.type === "TSTypeAssertion" && couldExpandArg(arg.expression)) ||
(arg.type === "TSAsExpression" && couldExpandArg(arg.expression)) ||
arg.type === "FunctionExpression" ||
(arg.type === "ArrowFunctionExpression" &&
// we want to avoid breaking inside composite return types but not simple keywords
Expand All @@ -229,7 +248,7 @@ function couldGroupArg(arg, arrowChainRecursion = false) {
isNonEmptyBlockStatement(arg.body)) &&
(arg.body.type === "BlockStatement" ||
(arg.body.type === "ArrowFunctionExpression" &&
couldGroupArg(arg.body, true)) ||
couldExpandArg(arg.body, true)) ||
arg.body.type === "ObjectExpression" ||
arg.body.type === "ArrayExpression" ||
(!arrowChainRecursion &&
Expand All @@ -241,13 +260,13 @@ function couldGroupArg(arg, arrowChainRecursion = false) {
);
}

function shouldGroupLastArg(args, options) {
function shouldExpandLastArg(args, options) {
const lastArg = getLast(args);
const penultimateArg = getPenultimate(args);
return (
!hasComment(lastArg, CommentCheckFlags.Leading) &&
!hasComment(lastArg, CommentCheckFlags.Trailing) &&
couldGroupArg(lastArg) &&
couldExpandArg(lastArg) &&
// If the last two arguments are of the same type,
// disable last element expansion.
(!penultimateArg || penultimateArg.type !== lastArg.type) &&
Expand All @@ -263,7 +282,7 @@ function shouldGroupLastArg(args, options) {
);
}

function shouldGroupFirstArg(args) {
function shouldExpandFirstArg(args) {
if (args.length !== 2) {
return false;
}
Expand All @@ -285,10 +304,54 @@ function shouldGroupFirstArg(args) {
secondArg.type !== "FunctionExpression" &&
secondArg.type !== "ArrowFunctionExpression" &&
secondArg.type !== "ConditionalExpression" &&
!couldGroupArg(secondArg)
isHopefullyShortCallArgument(secondArg) &&
!couldExpandArg(secondArg)
);
}

// A hack to fix most manifestations of
// https://github.com/prettier/prettier/issues/2456
// https://github.com/prettier/prettier/issues/5172
// https://github.com/prettier/prettier/issues/12892
// A proper (printWidth-aware) fix for those would require a complex change in the doc printer.
function isHopefullyShortCallArgument(node) {
if (node.type === "ParenthesizedExpression") {
return isHopefullyShortCallArgument(node.expression);
}

if (node.type === "TSAsExpression") {
let { typeAnnotation } = node;
if (typeAnnotation.type === "TSArrayType") {
typeAnnotation = typeAnnotation.elementType;
if (typeAnnotation.type === "TSArrayType") {
typeAnnotation = typeAnnotation.elementType;
}
}
if (
(typeAnnotation.type === "GenericTypeAnnotation" ||
typeAnnotation.type === "TSTypeReference") &&
typeAnnotation.typeParameters?.params.length === 1
) {
typeAnnotation = typeAnnotation.typeParameters.params[0];
}
return (
isSimpleType(typeAnnotation) && isSimpleCallArgument(node.expression, 1)
);
}

if (isCallLikeExpression(node) && getCallArguments(node).length > 1) {
return false;
}

if (isBinaryish(node)) {
return (
isSimpleCallArgument(node.left, 1) && isSimpleCallArgument(node.right, 1)
);
}

return isRegExpLiteral(node) || isSimpleCallArgument(node);
}

function isReactHookCallWithDepsArray(args) {
return (
args.length === 2 &&
Expand Down
12 changes: 4 additions & 8 deletions src/language-js/print/function.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,7 @@ function printFunction(path, print, options, args) {
if (
(node.type === "FunctionDeclaration" ||
node.type === "FunctionExpression") &&
args &&
args.expandLastArg
args?.expandLastArg
) {
const parent = path.getParentNode();
if (isCallExpression(parent) && getCallArguments(parent).length > 1) {
Expand Down Expand Up @@ -306,10 +305,7 @@ function printArrowFunction(path, options, print, args) {
node.typeParameters ||
getFunctionParameters(node).some((param) => param.type !== "Identifier");

if (
node.body.type !== "ArrowFunctionExpression" ||
(args && args.expandLastArg)
) {
if (node.body.type !== "ArrowFunctionExpression" || args?.expandLastArg) {
body.unshift(print("body", args));
} else {
node = node.body;
Expand Down Expand Up @@ -360,12 +356,12 @@ function printArrowFunction(path, options, print, args) {
// with the opening (, or if it's inside a JSXExpression (e.g. an attribute)
// we should align the expression's closing } with the line with the opening {.
const shouldAddSoftLine =
((args && args.expandLastArg) ||
(args?.expandLastArg ||
path.getParentNode().type === "JSXExpressionContainer") &&
!hasComment(node);

const printTrailingComma =
args && args.expandLastArg && shouldPrintComma(options, "all");
args?.expandLastArg && shouldPrintComma(options, "all");

// In order to avoid confusion between
// a => a ? a : a
Expand Down

0 comments on commit 40f0ae7

Please sign in to comment.