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 handling of colons on Flow function types #1089

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

Conversation

gnprice
Copy link
Contributor

@gnprice gnprice commented Apr 26, 2022

This way is simpler to reason about: it's fewer conditionals, and
less peeking up and down the tree to try to anticipate what other
nodes will do. And it works the same on all existing tests.

It also works correctly on a number of other test cases where the
existing logic breaks. Add a bunch of those, and a few others.


The commit at the tip of this branch is the one that makes the actual changes. It's small. Reading the commits individually will make them easier to read than reading the whole PR's diff at once.

That tip commit doesn't actually require any of the other changes: it works just fine on its own. I'd be happy to see it merged and drop the others. But they came naturally along the way and they seem like useful improvements for the next person working in these parts of the code, so I figured I'd share them.

This file is very nicely structured as data-driven tests!  Make
that structure visible to Mocha too.  That way when something
breaks a particular handful of tests, it's easy to see which ones.
When something's wrong, this produces a nice clear diff,
which the comparison of ASTs doesn't.
This way is simpler to reason about: it's fewer conditionals, and
less peeking up and down the tree to try to anticipate what other
nodes will do.  And it works the same on all existing tests.

It also works correctly on a number of other test cases where the
existing logic breaks.  Add a bunch of those, and a few others.
gnprice added a commit to gnprice/tsflower that referenced this pull request Apr 26, 2022
I've sent the fix as a PR:
  benjamn/recast#1089
which hopefully will get merged in due course.  In the meantime,
start using it.

The library needs a build step.  Oddly it gives a couple of TS
warnings, which it doesn't when I build the same version in its own
worktree.  Perhaps `tsc` is letting this project's tsconfig leak in,
because it's at an ancestor directory?  Anyway, just ignore for now,
with `|| :`.
@gnprice gnprice changed the title Fix handling of Flow function types Fix handling of colons on Flow function types May 9, 2022
gnprice added a commit to gnprice/recast that referenced this pull request Jun 11, 2022
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.
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