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

Incomplete support for disjoint union copy and refinement #7457

Open
skeggse opened this issue Feb 8, 2019 · 0 comments
Open

Incomplete support for disjoint union copy and refinement #7457

skeggse opened this issue Feb 8, 2019 · 0 comments
Labels
incompleteness Something is missing

Comments

@skeggse
Copy link
Contributor

skeggse commented Feb 8, 2019

Flow version: v0.92.1

I'm reporting two problems, but they seem related because they produce the same error, and a solution to one might be a solution to the other. Happy to report them separately if that's flat-out wrong. Maybe related: #7422.

Broken disjoint union identity

Expected behavior

Flow should understand that copying a disjoint union of objects is valid:

type EitherOptional =
  {| allow?: string[], deny?: null |} |
  {| allow?: null, deny?: string[] |};

// Yes, this code can incur a stack overflow; this is just to illustrate the types involved.
function withOptional1(options: EitherOptional) {
  // Works, as expected.
  withOptional1(options);
}

function withOptional2(options: EitherOptional) {
  // Does not work, even though we're not doing anything material beyond copying the object.
  withOptional2({
    allow: options.allow,
    deny: options.deny,
  });
}

Actual behavior

15:   withOptional2({
                    ^ Could not decide which case to select. Since case 1 [1] may work but if it doesn't case 2 [2] looks promising too. To fix add a type annotation to property `allow` [3] or to property `deny` [4].
References:
4:   {| allow?: string[], deny?: null |} |
     ^ [1]
5:   {| allow?: null, deny?: string[] |};
     ^ [2]
16:     allow: options.allow,
               ^ [3]
17:     deny: options.deny,
              ^ [4]

Broken disjoint union refinement propagation

Expected behavior

Flow should understand that groups of object properties that are shared across a disjoint union and have distinct types can be re-used in other structures with the same properties that have the same or similar types.

That is, if we refine the fields in EitherOptional to ensure that a specific field is truthy, or that either of the two fields are truthy, then that refinement should persist to a new object of the same type, or a new object of the similar type EitherRequired:

function withOptional3(options: EitherOptional) {
  if (options.allow || options.deny) {
    // Does not work, as in withOptional2. This example is mostly to demonstrate the simpler case of the following example.
    withOptional3({
      allow: options.allow,
      deny: options.deny,
    });
  }
}

// A new variant of the previous disjoint union.
type EitherRequired =
  {| allow: string[], deny?: null |} |
  {| allow?: null, deny: string[] |};

function withRequired(options: EitherRequired) { /* irrelevant */ }

function withOptional4(options: EitherOptional) {
  if (options.allow || options.deny) {
    // A perfect type analysis system would be able to infer from the existing type
    // refinement information that a new object containing either a truthy allow
    // field from case [1] or a truthy deny field from case [2] of EitherOptional.
    withRequired({
      allow: options.allow,
      deny: options.deny,
    });
  }
}

Actual behavior

Trivial copy case

24:     withOptional3({
                      ^ Could not decide which case to select. Since case 1 [1] may work but if it doesn't case 2 [2] looks promising too. To fix add a type annotation to property `allow` [3] or to property `deny` [4].
References:
4:   {| allow?: string[], deny?: null |} |
     ^ [1]
5:   {| allow?: null, deny?: string[] |};
     ^ [2]
25:       allow: options.allow,
                 ^ [3]
26:       deny: options.deny,
                ^ [4]

Refinement propagation case

43:     withRequired({
                     ^ Could not decide which case to select. Since case 1 [1] may work but if it doesn't case 2 [2] looks promising too. To fix add a type annotation to property `allow` [3] or to property `deny` [4].
References:
33:   {| allow: string[], deny?: null |} |
      ^ [1]
34:   {| allow?: null, deny: string[] |};
      ^ [2]
44:       allow: options.allow,
                 ^ [3]
45:       deny: options.deny,
                ^ [4]
  • Link to Try-Flow or Github repo.
@skeggse skeggse added the bug label Feb 8, 2019
@jbrown215 jbrown215 added incompleteness Something is missing and removed bug labels Feb 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
incompleteness Something is missing
Projects
None yet
Development

No branches or pull requests

2 participants