From 50c488f4829790bb1cbc97d1e15d5f16031f8469 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 10 Jun 2022 21:52:10 -0700 Subject: [PATCH 1/4] parens [nfc]: Clean up slightly in preparation to handle Flow types This just reorganizes a bit for organization, and fixes some comments. --- lib/fast-path.ts | 26 ++++++++++++++++---------- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/lib/fast-path.ts b/lib/fast-path.ts index 925cbaac..9e80bbcd 100644 --- a/lib/fast-path.ts +++ b/lib/fast-path.ts @@ -329,13 +329,13 @@ FPp.needsParens = function (assumeExpressionContext) { const name = this.getName(); // If the value of this path is some child of a Node and not a Node - // itself, then it doesn't need parentheses. Only Node objects (in fact, - // only Expression nodes) need parentheses. + // itself, then it doesn't need parentheses. Only Node objects + // need parentheses. if (this.getValue() !== node) { return false; } - // Only statements don't need parentheses. + // Statements don't need parentheses. if (n.Statement.check(node)) { return false; } @@ -424,13 +424,6 @@ FPp.needsParens = function (assumeExpressionContext) { return true; } - case "OptionalIndexedAccessType": - return node.optional && parent.type === "IndexedAccessType"; - - case "IntersectionTypeAnnotation": - case "UnionTypeAnnotation": - return parent.type === "NullableTypeAnnotation"; - case "Literal": return ( parent.type === "MemberExpression" && @@ -531,6 +524,19 @@ FPp.needsParens = function (assumeExpressionContext) { ) { return true; } + break; + + // Flow type nodes. + // + // (TS type nodes don't need any logic here, because they represent + // parentheses explicitly in the AST, with TSParenthesizedType.) + + case "OptionalIndexedAccessType": + return node.optional && parent.type === "IndexedAccessType"; + + case "IntersectionTypeAnnotation": + case "UnionTypeAnnotation": + return parent.type === "NullableTypeAnnotation"; } if ( From 1a48ca26505c43ddc6bb2c63ac2058d893d3b8c5 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Sat, 7 May 2022 22:48:15 -0700 Subject: [PATCH 2/4] test: Use Flow parser throughout Flow tests At this point there are a number of Flow features that the Esprima parser doesn't support. Rather than switch to the Flow parser ad hoc for those test cases, simplify by just using it systematically. --- test/flow.ts | 74 +++++++++++++++++++++------------------------------- 1 file changed, 30 insertions(+), 44 deletions(-) diff --git a/test/flow.ts b/test/flow.ts index 335d9570..e455d4e4 100644 --- a/test/flow.ts +++ b/test/flow.ts @@ -10,15 +10,12 @@ describe("type syntax", function () { quote: "single", flowObjectCommas: false, }); - const esprimaParserParseOptions = { - parser: require("esprima-fb"), - }; const flowParserParseOptions = { parser: require("flow-parser"), }; function check(source: string, parseOptions?: any) { - parseOptions = parseOptions || esprimaParserParseOptions; + parseOptions = parseOptions || flowParserParseOptions; const ast1 = parse(source, parseOptions); const code = printer.printGenerically(ast1).code; const ast2 = parse(code, parseOptions); @@ -30,7 +27,7 @@ describe("type syntax", function () { // Import type annotations check("import type foo from 'foo';"); check("import typeof foo from 'foo';"); - check("import { type foo } from 'foo';", flowParserParseOptions); + check("import { type foo } from 'foo';"); // Export type annotations check("export type { foo };"); @@ -69,9 +66,8 @@ describe("type syntax", function () { " optionalNumber?: number;" + eol + "};", - flowParserParseOptions, ); - check("type A = {| optionalNumber?: number |};", flowParserParseOptions); + check("type A = {| optionalNumber?: number |};"); check( "type A = {|" + eol + @@ -80,18 +76,14 @@ describe("type syntax", function () { " optionalNumber?: number;" + eol + "|};", - flowParserParseOptions, ); // Opaque types - check("opaque type A = B;", flowParserParseOptions); - check("opaque type A = B.C;", flowParserParseOptions); - check( - "opaque type A = { optionalNumber?: number };", - flowParserParseOptions, - ); - check("opaque type A: X = B;", flowParserParseOptions); - check("opaque type A: X.Y = B.C;", flowParserParseOptions); + check("opaque type A = B;"); + check("opaque type A = B.C;"); + check("opaque type A = { optionalNumber?: number };"); + check("opaque type A: X = B;"); + check("opaque type A: X.Y = B.C;"); check( "opaque type A: { stringProperty: string } = {" + eol + @@ -100,10 +92,9 @@ describe("type syntax", function () { " optionalNumber?: number;" + eol + "};", - flowParserParseOptions, ); - check("opaque type A: X = B;", flowParserParseOptions); - check("opaque type A: X.Y = B.C;", flowParserParseOptions); + check("opaque type A: X = B;"); + check("opaque type A: X.Y = B.C;"); check( "opaque type A: { optional?: T } = {" + eol + @@ -112,7 +103,6 @@ describe("type syntax", function () { " optional?: T;" + eol + "};", - flowParserParseOptions, ); // Generic @@ -163,16 +153,13 @@ describe("type syntax", function () { "}", ); - check("declare opaque type A;", flowParserParseOptions); - check("declare opaque type A: X;", flowParserParseOptions); - check("declare opaque type A: X.Y;", flowParserParseOptions); - check( - "declare opaque type A: { stringProperty: string };", - flowParserParseOptions, - ); - check("declare opaque type A: X;", flowParserParseOptions); - check("declare opaque type A: X.Y;", flowParserParseOptions); - check("declare opaque type A: { property: T };", flowParserParseOptions); + check("declare opaque type A;"); + check("declare opaque type A: X;"); + check("declare opaque type A: X.Y;"); + check("declare opaque type A: { stringProperty: string };"); + check("declare opaque type A: X;"); + check("declare opaque type A: X.Y;"); + check("declare opaque type A: { property: T };"); // Classes check("class A {" + eol + " a: number;" + eol + "}"); @@ -194,7 +181,7 @@ describe("type syntax", function () { check("class A {}"); // Inexact object types - check("type InexactFoo = { foo: number; ... };", flowParserParseOptions); + check("type InexactFoo = { foo: number; ... };"); check( [ "type MultiLineInexact = {", @@ -203,26 +190,25 @@ describe("type syntax", function () { " ...", "};", ].join(eol), - flowParserParseOptions, ); // typeArguments - check("new A();", flowParserParseOptions); - check("createPlugin();", flowParserParseOptions); + check("new A();"); + check("createPlugin();"); - check("function myFunction([param1]: Params) {}", flowParserParseOptions); + check("function myFunction([param1]: Params) {}"); }); it("can pretty-print [Optional]IndexedAccessType AST nodes", () => { - check("type A = Obj?.['a'];", flowParserParseOptions); - check("type B = Array?.[number];", flowParserParseOptions); - check("type C = Obj?.['bar']['baz'];", flowParserParseOptions); - check("type D = (Obj?.['bar'])['baz'];", flowParserParseOptions); - check("type E = Obj?.['bar'][];", flowParserParseOptions); - check("type F = Obj?.['bar'][boolean][];", flowParserParseOptions); - check("type G = Obj['bar']?.[boolean][];", flowParserParseOptions); - check("type H = (Obj?.['bar'])[string][];", flowParserParseOptions); - check("type I = Obj?.['bar']?.[string][];", flowParserParseOptions); + check("type A = Obj?.['a'];"); + check("type B = Array?.[number];"); + check("type C = Obj?.['bar']['baz'];"); + check("type D = (Obj?.['bar'])['baz'];"); + check("type E = Obj?.['bar'][];"); + check("type F = Obj?.['bar'][boolean][];"); + check("type G = Obj['bar']?.[boolean][];"); + check("type H = (Obj?.['bar'])[string][];"); + check("type I = Obj?.['bar']?.[string][];"); function checkEquiv(a: string, b: string) { const aAst = parse(a, flowParserParseOptions); From 8a33061cd990cf086f8917bce18ab912312d995a Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 10 Jun 2022 21:53:29 -0700 Subject: [PATCH 3/4] parens: Fix OptionalIndexedAccessType inside plain IndexedAccessType As the added tests show, we need these parens regardless of whether `node.optional` is true. That is, we need them even for a type like `(Obj?.['foo']['bar'])['baz']`, which is * an IndexedAccessType, whose objectType is * an OptionalIndexedAccessType with `optional` false, whose objectType is * an OptionalIndexedAccessType with `optional` true. The parens stop the OptionalIndexedAccessType nature from propagating outward in the absence of `?.`. --- lib/fast-path.ts | 2 +- test/flow.ts | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/fast-path.ts b/lib/fast-path.ts index 9e80bbcd..7f2f2a82 100644 --- a/lib/fast-path.ts +++ b/lib/fast-path.ts @@ -532,7 +532,7 @@ FPp.needsParens = function (assumeExpressionContext) { // parentheses explicitly in the AST, with TSParenthesizedType.) case "OptionalIndexedAccessType": - return node.optional && parent.type === "IndexedAccessType"; + return parent.type === "IndexedAccessType"; case "IntersectionTypeAnnotation": case "UnionTypeAnnotation": diff --git a/test/flow.ts b/test/flow.ts index e455d4e4..30d6082f 100644 --- a/test/flow.ts +++ b/test/flow.ts @@ -204,6 +204,8 @@ describe("type syntax", function () { check("type B = Array?.[number];"); check("type C = Obj?.['bar']['baz'];"); check("type D = (Obj?.['bar'])['baz'];"); + check("type C3 = Obj?.['foo']['bar']['baz'];"); + check("type D3 = (Obj?.['foo']['bar'])['baz'];"); check("type E = Obj?.['bar'][];"); check("type F = Obj?.['bar'][boolean][];"); check("type G = Obj['bar']?.[boolean][];"); From 08b0ed8bbdf1c3e4c190996f4405a88bc47650cf Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 10 Jun 2022 22:40:11 -0700 Subject: [PATCH 4/4] parens: Emit parens where needed throughout Flow types; add tests There were a couple of particular cases that were already covered, but many others that were not. I ran into one of them in practice (`A & (B | C)` was getting printed as `A & B | C`, which means something different) and looked into it, and decided to go about just fixing this whole class of issues. I started by scanning through all the different kinds of FlowType node, looking for any that could need parens around them; then I wrote up a comprehensive list of test cases. Once that was done, the whole thing turned out to be doable with a pretty reasonable amount of code! And given the tests, I'm hopeful that this logic is pretty much complete. Some of the added tests are skipped for now, because they involve function types and those currently have other bugs that cause those test cases to break. They start passing when this branch is merged with #1089, which fixes those other bugs. --- lib/fast-path.ts | 74 +++++++++++++++++++++++++++++- test/flow.ts | 116 ++++++++++++++++++++++++++++++++++++++++++++--- 2 files changed, 182 insertions(+), 8 deletions(-) diff --git a/lib/fast-path.ts b/lib/fast-path.ts index 7f2f2a82..bf3b7200 100644 --- a/lib/fast-path.ts +++ b/lib/fast-path.ts @@ -532,11 +532,81 @@ FPp.needsParens = function (assumeExpressionContext) { // parentheses explicitly in the AST, with TSParenthesizedType.) case "OptionalIndexedAccessType": - return parent.type === "IndexedAccessType"; + switch (parent.type) { + case "IndexedAccessType": + // `(O?.['x'])['y']` is distinct from `O?.['x']['y']`. + return name === "objectType" && parent.objectType === node; + default: + return false; + } + + case "IndexedAccessType": + case "ArrayTypeAnnotation": + return false; + + case "NullableTypeAnnotation": + switch (parent.type) { + case "OptionalIndexedAccessType": + case "IndexedAccessType": + return name === "objectType" && parent.objectType === node; + case "ArrayTypeAnnotation": + return true; + default: + return false; + } case "IntersectionTypeAnnotation": + switch (parent.type) { + case "OptionalIndexedAccessType": + case "IndexedAccessType": + return name === "objectType" && parent.objectType === node; + case "ArrayTypeAnnotation": + case "NullableTypeAnnotation": + return true; + default: + return false; + } + case "UnionTypeAnnotation": - return parent.type === "NullableTypeAnnotation"; + switch (parent.type) { + case "OptionalIndexedAccessType": + case "IndexedAccessType": + return name === "objectType" && parent.objectType === node; + case "ArrayTypeAnnotation": + case "NullableTypeAnnotation": + case "IntersectionTypeAnnotation": + return true; + default: + return false; + } + + case "FunctionTypeAnnotation": + switch (parent.type) { + case "OptionalIndexedAccessType": + case "IndexedAccessType": + return name === "objectType" && parent.objectType === node; + + case "ArrayTypeAnnotation": + // We need parens. + + // fallthrough + case "NullableTypeAnnotation": + // We don't *need* any parens here… unless some ancestor + // means we do, by putting a `&` or `|` on the right. + // Just use parens; probably more readable that way anyway. + // (FWIW, this agrees with Prettier's behavior.) + + // fallthrough + case "IntersectionTypeAnnotation": + case "UnionTypeAnnotation": + // We need parens if there's another `&` or `|` after this node. + // For consistency, just always use parens. + // (FWIW, this agrees with Prettier's behavior.) + return true; + + default: + return false; + } } if ( diff --git a/test/flow.ts b/test/flow.ts index 30d6082f..a1f4a131 100644 --- a/test/flow.ts +++ b/test/flow.ts @@ -14,6 +14,12 @@ describe("type syntax", function () { parser: require("flow-parser"), }; + function checkEquiv(a: string, b: string) { + const aAst = parse(a, flowParserParseOptions); + const bAst = parse(b, flowParserParseOptions); + types.astNodesAreEquivalent.assert(aAst, bAst); + } + function check(source: string, parseOptions?: any) { parseOptions = parseOptions || flowParserParseOptions; const ast1 = parse(source, parseOptions); @@ -212,12 +218,6 @@ describe("type syntax", function () { check("type H = (Obj?.['bar'])[string][];"); check("type I = Obj?.['bar']?.[string][];"); - function checkEquiv(a: string, b: string) { - const aAst = parse(a, flowParserParseOptions); - const bAst = parse(b, flowParserParseOptions); - types.astNodesAreEquivalent.assert(aAst, bAst); - } - // Since FastPath#needsParens does not currently add any parentheses to // these expressions, make sure they do not matter for parsing the AST. @@ -231,4 +231,108 @@ describe("type syntax", function () { "type F = Obj['bar']?.[string][];", ); }); + + it("parenthesizes correctly", () => { + // The basic binary operators `&` and `|`. + // `&` binds tighter than `|` + check("type Num = number & (empty | mixed);"); // parens needed + check("type Num = number | empty & mixed;"); // equivalent to `…|(…&…)` + + // Unary suffix `[]`, with the above. + // `[]` binds tighter than `&` or `|` + check("type T = (number | string)[];"); + check("type T = number | string[];"); // a union + check("type T = (number & mixed)[];"); + check("type T = number & mixed[];"); // an intersection + + // Unary prefix `?`, with the above. + // `?` binds tighter than `&` or `|` + check("type T = ?(A & B);"); + check("type T = ?(A | B);"); + // `?` binds less tightly than `[]` + check("type T = (?number)[];"); // array of nullable + check("type T = ?number[];"); // nullable of array + + // (Optional) indexed-access types, with the above. + // `[…]` and `?.[…]` bind (their left) tighter than either `&` or `|` + check("type T = (O & P)['x'];"); + check("type T = (O | P)['x'];"); + check("type T = (O & P)?.['x'];"); + check("type T = (O | P)?.['x'];"); + // `[…]` and `?.[…]` bind (their left) tighter than `?` + check("type T = (?O)['x'];"); // indexed-access of nullable + check("type T = ?O['x'];"); // nullable of indexed-access + check("type T = (?O)?.['x'];"); // optional-indexed-access of nullable + check("type T = ?O?.['x'];"); // nullable of optional-indexed-access + // `[…]` and `?.[…]` provide brackets on their right, so skip parens: + check("type T = A[B & C];"); + check("type T = A[B | C];"); + check("type T = A[?B];"); + check("type T = A[B[]];"); + check("type T = A[B[C]];"); + check("type T = A[B?.[C]];"); + check("type T = A?.[B & C];"); + check("type T = A?.[B | C];"); + check("type T = A?.[?B];"); + check("type T = A?.[B[]];"); + check("type T = A?.[B[C]];"); + check("type T = A?.[B?.[C]];"); + // `[…]` and `?.[…]` interact in a nonobvious way: + // OptionalIndexedAccessType inside IndexedAccessType. + check("type T = (O?.['x']['y'])['z'];"); // indexed of optional-indexed + check("type T = O?.['x']['y']['z'];"); // optional-indexed throughout + + return; + // Skip test cases involving function types, because those are currently + // broken in other ways. Those will be fixed by: + // https://github.com/benjamn/recast/pull/1089 + + // Function types. + // Function binds less tightly than binary operators at right: + check("type T = (() => number) & O;"); // an intersection + check("type T = (() => number) | void;"); // a union + check("type T = () => number | void;"); // a function + check("type T = (() => void)['x'];"); + check("type T = () => void['x'];"); // a function + check("type T = (() => void)?.['x'];"); + check("type T = () => void?.['x'];"); // a function + // … and less tightly than suffix operator: + check("type T = (() => void)[];"); // an array + check("type T = () => void[];"); // a function + + // Function does bind tighter than prefix operator (how could it not?) + checkEquiv("type T = ?() => void;", "type T = ?(() => void);"); + // … and tighter than `&` or `|` at left (ditto): + checkEquiv("type T = A | () => void;", "type T = A | (() => void);"); + checkEquiv("type T = A & () => void;", "type T = A & (() => void);"); + // … but we choose to insert parens anyway: + check("type T = ?(() => void);"); + check("type T = A | (() => void);"); + check("type T = A & (() => void);"); + // We don't insert parens for the *right* operand of indexed access, + // though, that'd be silly (sillier than writing such a type at all?): + check("type T = A[() => void];"); + check("type T = A?.[() => void];"); + + // Here's one reason we insert those parens we don't strictly have to: + // Even when the parent is something at left so that function binds + // tighter than it, *its* parent (or further ancestor) might be + // something at right that binds tighter than function. + // E.g., union of nullable of function: + check("type T = ?(() => void) | A;"); + checkEquiv("type T = ?() => void | A;", "type T = ?() => (void | A);"); + // … or intersection of nullable of function: + check("type T = ?(() => void) & A;"); + checkEquiv("type T = ?() => void & A;", "type T = ?() => (void & A);"); + // … or array or (optional-)indexed-access of nullable of function: + check("type T = ?(() => void)[];"); + check("type T = ?(() => void)['x'];"); + check("type T = ?(() => void)?.['x'];"); + // … or union of intersection: + check("type T = A & (() => void) | B;"); + // Or for an example beyond the grandparent: union of cubic nullable: + check("type T = ???(() => void) | B;"); + // … or union of intersection of nullable: + check("type T = A & ?(() => void) | B;"); + }); });