-
Notifications
You must be signed in to change notification settings - Fork 78
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
base: master
Are you sure you want to change the base?
Conversation
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.
This should produce the exact same output.
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()`.
There was a problem hiding this 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!
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:
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, inprintDefaultObjectType
. The overall diffstat on non-test code is: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:
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 functioninterfaceType
, 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 helpertypeMembers
, and then the call sites ofinterfaceType
mostly turn into calls totypeMembers
followed by calls to one or another of theprint*ObjectType
functions. The work of disentanglinginterfaceType
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.