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

Mixed refined to an object produces a read-only object, which breaks a lot of existing code. #7685

Closed
lxe opened this issue Apr 30, 2019 · 5 comments
Labels

Comments

@lxe
Copy link

lxe commented Apr 30, 2019

Flow version: 0.98

https://flow.org/try/#0GYVwdgxgLglg9mABMOcAUAjAhgJwFyIC2MAHgKYAmAlIgN4BQiiMwiaUAngA5lyvY5EAQgC8IxAHI4GAFZloExAB8lwgTQZMmOMlBA4kYEABtjAbkaIAvvUss2AgHRQAFjDABzRGPESAznCEum6eEhqWTE6u7l6+OFiuZDgSFkw2VkA

function foo(bar: mixed) {
  if (typeof bar !== 'object' || !bar) {
    return null;
  }

  if (bar.thing === 'something') {
    bar.thing = 'rather';
  }
}

Expected behavior

No Errors

Actual behavior

7:     bar.thing = 'rather';
           ^ Cannot assign `'rather'` to `bar.thing` because property `thing` is not writable.

Also #7684

@goodmind goodmind added regression Seems to have worked before, but is broken now (issue has mention of last working version) and removed needs triage labels May 1, 2019
@goodmind
Copy link
Contributor

goodmind commented May 1, 2019

0983bd0

@goodmind goodmind removed the regression Seems to have worked before, but is broken now (issue has mention of last working version) label May 1, 2019
@goodmind
Copy link
Contributor

goodmind commented May 1, 2019

@dsainati1

@goodmind
Copy link
Contributor

goodmind commented May 1, 2019

It is unsafe even if you refine type, because original type may have been string literal

const m: {a: 'foo'} = {a: 'foo'}
function unsafeWrite(obj: mixed) {
   if (typeof obj === 'object' && obj !== null) {
      if (typeof obj.a === 'string') {
        obj.a = 'bar' // unsafe
      }
   }
}
unsafeWrite(m)
m.a // bar

@dsainati1
Copy link
Contributor

dsainati1 commented May 1, 2019

Yes, this is an intentional change. Consider the example in the linked commit:

function bad (x : mixed) {
  if (typeof x === "object" && x !== null) {
    x.a = 3;
  }
}

let obj : {a : string} = {a : "oops"};
bad(obj); // yikes

It is rarely necessary to use mixed, your example could easily be rewritten as:

function foo(bar: ?{thing : string}) {
  if (typeof bar !== 'object' || !bar) {
    return null;
  }

  if (bar.thing === 'something') {
    bar.thing = 'rather';
  }
}

Alternatively you could use generics or bounded generics if your function needs to be able to accept a variety of inputs.

@lxe
Copy link
Author

lxe commented May 6, 2019

It is rarely necessary to use mixed, your example could easily be rewritten as:

We're adding flow to a large code base, and mixed is a useful and practical tool to enforce refinements when it's woefully difficult to hunt down the type of function arguments.

Sure, I can simply replace mixed with an explicit type based on already written refinements, but that's something that the type system should be doing for me automatically, which is exactly what mixed used to do.

Why not have a warning instead when attempting to write properties of objects of mixed types?

lydell added a commit to lydell/tiny-decoders that referenced this issue Jun 7, 2019
See these Flow issues for explanation:

- facebook/flow#7684
- facebook/flow#7685

This is a breaking change.

Fixes #3.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants