joarwilk / flowgen

Generate flowtype definition files from TypeScript
Other
658 stars 78 forks source link

Fix a class of bugs with exact vs. inexact object types #184

Open gnprice opened 2 years ago

gnprice commented 2 years ago

In Flow syntax, a type like {| x: number |} means an exact object type, while { x: number, ... } means an inexact object type.

The form { x: number }, with neither bars | nor an ellipsis, is ambiguous -- it can mean either an exact or inexact object type, depending on the Flow configuration. See docs: https://flow.org/en/docs/types/objects/#toc-explicit-inexact-object-types So we should avoid emitting that form, and always emit one of the explicit forms.

In several important places, we already make sure to always emit a type that's explicitly either exact or inexact. But there are still places where we emit the ambiguous form. I count 5 of them where we do so and assume it means an inexact type (as it did in early versions of Flow, and currently still does by default); and 2 of them where we assume the opposite, that it means an exact type (which is the modern recommended configuration, and is planned to become the default in the future.) That means that regardless of Flow configuration, the current output will be wrong in some places.

In this PR, I fix all 7 of those bugs. The first step is to introduce three small helpers:

/** Print as a Flow exact object type. */
export const printExactObjectType = (members: string[]) => // …

/** Print as a Flow inexact object type. */
export const printInexactObjectType = (members: string[]) => // …

/** Print as a Flow object type, applying the option `inexact`. */
export const printDefaultObjectType = (members: string[]) => // …

which always make sure to produce an explicit form. Then each of the separate implementations we have of printing a Flow object type, at a number of different spots in our code, get replaced with a call to one of those.

In addition to fixing those seven bugs, this generally simplifies the code at each of those sites; for example, instead of 5 different spots where we consult the inexact option, there's now one central such spot, in printDefaultObjectType. The overall diffstat on non-test code is:

$ git diff --shortstat upstream src/[a-z]*
 5 files changed, 119 insertions(+), 151 deletions(-)

The new helpers also keep the formatting more compact, using a single line if the object type has just a single member. Many snapshots get updated as a result. The total diffstat including tests (and including some newly-added tests) is:

$ git diff --shortstat upstream
 22 files changed, 253 insertions(+), 369 deletions(-)

At the end of the branch, we add a line implicit-inexact-object=error to the .flowconfig used in tests. This causes Flow to verify that we haven't produced any of the ambiguous form, each time that we ask it to check our test output for validity.


This is a PR that will definitely be easier to read one commit at a time than all as one diff. I'd also be happy to split it up into several PRs, if that'd be helpful.

The gnarliest part of developing this series of fixes was in src/printers/declarations.ts; particularly around the function interfaceType, which had complicated logic because it was really doing about four different jobs. The logic that's truly in common between those becomes a new smaller helper typeMembers, and then the call sites of interfaceType mostly turn into calls to typeMembers followed by calls to one or another of the print*ObjectType functions. The work of disentangling interfaceType that way happens mostly in a series of NFC commits (a term I appreciate learning from the LLVM community, and described here), followed by a small behavior-changing commit fixing each of its call sites.