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 a class of bugs with exact vs. inexact object types #184

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

Conversation

gnprice
Copy link
Contributor

@gnprice gnprice commented Apr 20, 2022

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.

These will let us replace a number of separate implementations at
different spots in our code of printing a Flow object type.

In particular, these will always emit a form that's explicitly either
exact or inexact, eliminating places where we currently emit an
ambiguous form like `{ x: number }`.  See Flow docs:
  https://flow.org/en/docs/types/objects/#toc-explicit-inexact-object-types

By my count, we currently have 5 places where we emit this form and
intend it to mean an inexact type, and 2 where we intend the opposite.

These will also let our own code be explicit about whether it
specifically intends an exact object type, or an inexact object type,
or wants the exactness to be controlled by the `inexact` option.
At least one of those bugs (the one affecting an `interface` with an
`extends` clause) appears to be due to confusion where one bit of code
is trying to specifically produce an inexact object type, but another
bit that it's calling is trying to let the exactness be controlled by
the `inexact` option.  I suspect that sort of bug will be easier to
avoid when the choice is explicit.
When inexact object types are requested, we were producing the
ambiguous form of a Flow object type.  Depending on Flow settings
(and in the future probably by default), this ends up getting
interpreted as exact:
  https://flow.org/en/docs/types/objects/#toc-explicit-inexact-object-types
This needs to be an exact object type in order to correctly emulate
TS's `Omit`.  We were emitting the ambiguous form, which by default
Flow interprets as an inexact type instead.
Otherwise we accidentally get an inexact type after all, in Flow's
default configuration.

Also add a test.
This second argument to `$Rest` needs to be the inexact empty object
type (`{ ... }`), not the exact empty object type, in order for this
to do its job of implementing the equivalent of TS's `Partial`.
… types

These types should always be inexact object types.  We were producing
the ambiguous form, which can end up meaning an exact object type:
  https://flow.org/en/docs/types/objects/#toc-explicit-inexact-object-types
This produces exactly the same type, but using our common printer
function for it.  Some snapshots change just because of formatting.
The only cases this `filter` can filter out (or is meant to) are
those from this `map` up at the top.  So put it directly there.
This has just one call site, and it doesn't use this flag.
This function has three call sites.  One of them has needs that are
fairly divergent from the needs of the other two.  So, split the
function in two, and make both sides simpler.
The code that gets copied here is quite short, and we can immediately
make both copies simpler and easier to read than when there was one
copy trying to do both things.
This makes it possible to cleanly describe what each of these
functions should do.

Now that one of them is always printing a Flow object type,
we'll shortly be able to replace it with calls to the common
printers for object types.
Those `.map` calls will mostly go away as we convert these call sites
to use `printers.common.print*Object` to combine the members.
This will help make it clearer how to replace the two calls to
this `objectType` method with calls to the common `print*ObjectType`.
This produces exactly the same types: the existing code was already
successfully producing an explicit inexact object type when the
`inexact` option is true, and an explicit exact object type when
that option is false.

The formatting in the case of a type with a single member is nicely
more compact, though, and some snapshots change as a result.
…act` false

These object types must be inexact in order for the intersection to
have the intended effect.

When the `inexact` option was false, this logic would end up adjusting
part of the syntax as if we wanted an exact object type.  The net
result was an object type that was ambiguously exact or inexact, with
no `{|` and `|}` but no `...` either.

A bonus is that when the interface has just one member of its own, the
formatting is nicely more compact.  This causes a number of snapshots
to update.

Also add a test, with `inexact: false`.
Now that this function uses the common printDefaultObjectType, it's
so short that we can simplify the code by just inlining it.  That
also lets us further simplify, by treating the "heritage" part as
just more "members".

If it were possible for `node.heritageClauses` to have two or more
elements, this change would fix a bug: we weren't putting a comma
between the types from different heritage clauses, so we'd write
something like `{ ...$Exact<I>...$Exact<J>, x: number }`, a syntax
error.  But that's only a latent bug: this code handles only interface
declarations, not classes, and interfaces can have only an `extends`
clause and no other heritage clause.  (Classes can have `extends` and
also `implements`.)

Given that `node.heritageClauses` has at most one element, this new
code produces exactly the same type as the old.
This would produce output like:

    class C {
    x: number;static y: string
    ;static z: boolean
    ;
    }

Instead, produce:

    class C {
    x: number;
    static y: string;
    static z: boolean;
    }

No snapshots change, because we use Prettier there to normalize
such things.
These were separated by a couple hundred lines of code.  Easier to
see everything that's going on when they're together.

Leave behind a small helper `formatMembers`, which we'll use for
interfaces too.
…htly

This could be an `implements` clause as well as an `extends`,
or both.
This function is separated from its one call site by about a hundred
lines; bringing them together will make the logic simpler to follow.
The function is very short now, so just inline it.
Originally the implementation of enum members was rather indirect, and
these comments provided useful information about the original value.
But since a2cd5f0 which simplified the implementation, the value
shown earlier on the same line has had exactly the same content as the
comment.  So we can now drop the comment without losing anything.
This produces exactly the same type.  A snapshot changes because
the formatting is now more compact when there's only one member.
I believe we've now fixed all the places that we were producing
ambiguous object-type syntax!

This causes Flow to start verifying that, each time we ask it
to check our output in a test case for validity with
`expect(…).toBeValidFlowTypeDeclarations()`.
Copy link
Collaborator

@orta orta left a comment

Choose a reason for hiding this comment

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

This looks good to me (judging mostly by how all the tests work out) - I'm struggling a bit to figure out if this should be considered a semver major. Are we making the type definitions more strict here? It doesn't feel like it (for example the danger.d.ts didn't seem to change) but I want to be certain

I've shipped 1.20 with your prior PRs and merged all the trivial PRs inbetween which caused a merge conflict or two. If you fix those up and let me know about major/minor then I'll ship this. Thanks!

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.

None yet

2 participants