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

Fix missing parens in reprinted output #1068

Merged
merged 1 commit into from
Apr 27, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
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
14 changes: 7 additions & 7 deletions lib/fast-path.ts
Original file line number Diff line number Diff line change
Expand Up @@ -324,9 +324,6 @@ FPp.needsParens = function (assumeExpressionContext) {
}

const parent = this.getParentNode();
if (!parent) {
return false;
}
Comment on lines -327 to -329
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We delay this check so that we can reprint (function () {} ()), which otherwise will always have its outermost parens stripped since it has no parent.


const name = this.getName();

Expand All @@ -347,13 +344,16 @@ FPp.needsParens = function (assumeExpressionContext) {
return false;
}

if (
parent.type === "ParenthesizedExpression" ||
(node.extra && node.extra.parenthesized)
) {
if (parent && parent.type === "ParenthesizedExpression") {
return false;
}

if (node.extra && node.extra.parenthesized) {
return true;
}
Comment on lines +351 to +353
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem with short circuiting to false as we did previously is that a node may still really need parens (that is, non-decoratively). I opted to make this code responsible for decorative parens which as far as I can tell works fine, though it isn't apparent to me yet if there is some reason the code was written the way it was. The test suite seems to indicate that there is not, or at least is no longer.


if (!parent) return false;

switch (node.type) {
case "UnaryExpression":
case "SpreadElement":
Expand Down
4 changes: 2 additions & 2 deletions lib/printer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -196,14 +196,14 @@ function genericPrint(path: any, config: any, options: any, printPath: any) {
return linesWithoutParens;
}

let shouldAddParens = node.extra ? node.extra.parenthesized : false;
let shouldAddParens = false;
const decoratorsLines = printDecorators(path, printPath);

if (decoratorsLines.isEmpty()) {
// Nodes with decorators can't have parentheses, so we can avoid
// computing path.needsParens() except in this case.
if (!options.avoidRootParens) {
shouldAddParens = shouldAddParens || path.needsParens();
shouldAddParens = path.needsParens();
}
} else {
parts.push(decoratorsLines);
Expand Down
10 changes: 10 additions & 0 deletions parsers/babel-ts.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
import { parser } from "./babel";
import getBabelOptions, { Overrides } from "./_babel_options";

export { parser };

export function parse(source: string, options?: Overrides) {
const babelOptions = getBabelOptions(options);
babelOptions.plugins.push("jsx", "typescript");
return parser.parse(source, babelOptions);
}
27 changes: 21 additions & 6 deletions test/parens-babylon.ts → test/parens-extra.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
import assert from "assert";
// the babel parser denotes decorative parens with extra.parenthesized
import * as babylon from "@babel/parser";
import { parse as recastParse } from "../lib/parser";
import { Printer } from "../lib/printer";
import * as parser from "../parsers/babel-ts";
import * as types from "ast-types";

const printer = new Printer();
Expand All @@ -12,23 +14,20 @@ function parseExpression(expr: any) {
return n.ExpressionStatement.check(ast) ? ast.expression : ast;
}

const parse = (expr: string) =>
recastParse(expr, {
parser: babylon,
});
const parse = (expr: string) => recastParse(expr, { parser });

function check(expr: string) {
const ast = parse(expr);

const reprinted = printer.print(ast).code;
assert.strictEqual(reprinted, expr);
assert.strictEqual(expr, reprinted);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

expected is first


const expressionAst = parseExpression(expr);
const generic = printer.printGenerically(expressionAst).code;
types.astNodesAreEquivalent.assert(expressionAst, parseExpression(generic));
}

describe("babylon parens", function () {
describe("parens from node.extra.parenthesized", function () {
it("AwaitExpression", function () {
check("async () => ({...(await obj)})");
check("(async function* () { yield await foo })");
Expand All @@ -55,4 +54,20 @@ describe("babylon parens", function () {

assert.strictEqual(printer.print(ast).code, "(1).foo");
});

it("prints top level parens for an expression ast", function () {
check("(() => {})()");
check("(function () {} ())");
});

describe("reprinter", function () {
it("preserves necessary parens", function () {
const ast = parse("() => ({ prop: true })");
const expr = ast.program.body[0].expression;

expr.body.properties = [];

assert.strictEqual(printer.print(ast).code, "() => ({})");
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 haven't yet been able to verify that this test fails if the behavior regresses.

});
});
});
1 change: 1 addition & 0 deletions test/run.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import "./identity";
import "./jsx";
import "./lines";
import "./mapping";
import "./parens-extra";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

d'oh

import "./parens";
import "./parser";
import "./patcher";
Expand Down
32 changes: 0 additions & 32 deletions test/typescript.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,6 @@ import * as recast from "../main";
import * as types from "ast-types";
import { EOL as eol } from "os";
import * as parser from "../parsers/typescript";
import { Printer } from "../lib/printer";

const { namedTypes: n } = types;
const printer = new Printer();

// Babel 7 no longer supports Node 4 or 5.
const nodeMajorVersion = parseInt(process.versions.node, 10);
Expand Down Expand Up @@ -311,34 +307,6 @@ const nodeMajorVersion = parseInt(process.versions.node, 10);
"type Alpha = Color.a;",
]);
});

it("parens", function () {
function parseExpression(expr: any) {
let ast: any = parser.parse(expr).program;
n.Program.assert(ast);
ast = ast.body[0];
return n.ExpressionStatement.check(ast) ? ast.expression : ast;
}

const parse = (expr: string) => recast.parse(expr, { parser });

function check(expr: string) {
const ast = parse(expr);

const reprinted = recast.print(ast).code;
assert.strictEqual(reprinted, expr);

const expressionAst = parseExpression(expr);
const generic = printer.printGenerically(expressionAst).code;
types.astNodesAreEquivalent.assert(
expressionAst,
parseExpression(generic),
);
}

check("(() => {}) as void");
check("(function () {} as void)");
});
});

testReprinting(
Expand Down