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

Emit needed parens for all kinds of Flow types #1127

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

gnprice
Copy link
Contributor

@gnprice gnprice commented Jun 11, 2022

When printing a Flow type expression, we were mostly neglecting to print parentheses where they're needed. A few specific cases were covered, but many others were not.

I ran into one of those cases in practice: A & (B | C) was getting printed as A & B | C, which means something different. After looking into it, I decided to go about just fixing this whole class of issues. Turns out not to need all that much code!

This includes a thorough set of test cases, which I wrote up before writing the fix and helped me figure out what the fix should look like. If we run this PR's test cases without the fixes it makes in lib/ (but with #1089, which enhances the test output and also fixes unrelated bugs that some of these tests would hit), the Mocha results summary is:

parenthesizes correctly
  1) handles: type Num = number & (empty | mixed);
  ✔ handles: type Num = number | empty & mixed;
  2) handles: type T = (number | string)[];
  ✔ handles: type T = number | string[];
  3) handles: type T = (number & mixed)[];
  ✔ handles: type T = number & mixed[];
  ✔ handles: type T = ?(A & B);
  ✔ handles: type T = ?(A | B);
  4) handles: type T = (?number)[];
  ✔ handles: type T = ?number[];
  5) handles: type T = (O & P)['x'];
  6) handles: type T = (O | P)['x'];
  7) handles: type T = (O & P)?.['x'];
  8) handles: type T = (O | P)?.['x'];
  9) handles: type T = (?O)['x'];
  ✔ handles: type T = ?O['x'];
  10) handles: type T = (?O)?.['x'];
  ✔ handles: type T = ?O?.['x'];
  ✔ handles: type T = A[B & C];
  ✔ handles: type T = A[B | C];
  ✔ handles: type T = A[?B];
  ✔ handles: type T = A[B[]];
  ✔ handles: type T = A[B[C]];
  11) handles: type T = A[B?.[C]];
  ✔ handles: type T = A?.[B & C];
  ✔ handles: type T = A?.[B | C];
  ✔ handles: type T = A?.[?B];
  ✔ handles: type T = A?.[B[]];
  ✔ handles: type T = A?.[B[C]];
  ✔ handles: type T = A?.[B?.[C]];
  12) handles: type T = (O?.['x']['y'])['z'];
  ✔ handles: type T = O?.['x']['y']['z'];
  13) handles: type T = (() => number) & O;
  14) handles: type T = (() => number) | void;
  ✔ handles: type T = () => number | void;
  15) handles: type T = (() => void)['x'];
  ✔ handles: type T = () => void['x'];
  16) handles: type T = (() => void)?.['x'];
  ✔ handles: type T = () => void?.['x'];
  17) handles: type T = (() => void)[];
  ✔ handles: type T = () => void[];
  ✔ handles equivalently `type T = ?() => void;` vs. `type T = ?(() => void);`
  ✔ handles equivalently `type T = A | () => void;` vs. `type T = A | (() => void);`
  ✔ handles equivalently `type T = A & () => void;` vs. `type T = A & (() => void);`
  18) handles: type T = ?(() => void);
  19) handles: type T = A | (() => void);
  20) handles: type T = A & (() => void);
  ✔ handles: type T = A[() => void];
  ✔ handles: type T = A?.[() => void];
  21) handles: type T = ?(() => void) | A;
  ✔ handles equivalently `type T = ?() => void | A;` vs. `type T = ?() => (void | A);`
  22) handles: type T = ?(() => void) & A;
  ✔ handles equivalently `type T = ?() => void & A;` vs. `type T = ?() => (void & A);`
  23) handles: type T = ?(() => void)[];
  24) handles: type T = ?(() => void)['x'];
  25) handles: type T = ?(() => void)?.['x'];
  26) handles: type T = A & (() => void) | B;
  27) handles: type T = ???(() => void) | B;
  28) handles: type T = A & ?(() => void) | B;

Each of those numbered lines (with no ✔) is a case where the code before this PR would drop a pair of parentheses that shouldn't be dropped. For all but three of them (18, 19, 20), the resulting output wouldn't parse the same way. So the other 25 test cases represent situations where the existing code gets a wrong result.

This just reorganizes a bit for organization, and fixes some comments.
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.
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 `?.`.
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 benjamn#1089, which fixes those other bugs.
gnprice added a commit to gnprice/recast that referenced this pull request Jun 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant