Please revert the JSON.stringify definition patch(es)

[lll000111]

EDIT See summary comment below: #7565 (comment) This issue here just illustrates an aspect of the larger point made there.

---

Flow version: 0.95,1

Flow Try link

It's "correct" because according to the new definition the return value could always be undefined, regardless of input parameter(s). This is what the change to the definition tried to get rid of — but it never was a problem until the change....

Code

declare function getData(): mixed;

const data: mixed = getData();

if (data !== undefined) {
  throw new TypeError('Expected undefined, got ' + JSON.stringify(data));
}

Result

6:   throw new TypeError('Expected undefined, got ' + JSON.stringify(data));
                                                      ^ Cannot add `'Expected u...'` and `JSON.stringify(...)` because undefined [1] is incompatible with string [2].
References:
[LIB] ..//static/v0.95.1/flowlib/core.js:489:     ): string | void;
                                                              ^ [1]
6:   throw new TypeError('Expected undefined, got ' + JSON.stringify(data));
                         ^ [2]

[lll000111]

Unlike what the announcement says, the new definitions do not disallow functions.

Flow Try link

Code

function getData() {};
JSON.stringify(getData);

Result

No errors!

[villesau]

Some other weird examples:

https://flow.org/try/#0PTAEAEDMBsHsHcBQBjWA7AzgF1AEwKaQCGArtFgLJFbIAWAklvgLagC8oAPIywCoCeAB3wA+ABQBLJswBcoHswHCANKAz4iAJzoAFak01o52TRLQBzAJTsRiUKABSAZQDyAOQB0Js+YmR+ktKWHmYEAB4ukGLqWrr6+IbWAIRsHAC0AIwA3EA

and especially this:

https://flow.org/try/#0PTAEAEDMBsHsHcBQBjWA7AzgF1AEwKaQCGArtFgLJFbIAWAklvgLagC8oAPIywCoCeAB3wA+ABQBLJswBcoHswHCANKAz4iAJzoAFak01o52TRLQBzAJTsRiUKABSAZQDyAOQB0Js+YmR+ktKgAD7BAAUA5BGWHmYEAB4ukGLqWrr6+

Maybe I don't understand something, but especially in the last example there shouldn't be possibility to pass any nullish values in?

[lll000111]

@villesau Your first example gives no definition of ItemType, so it could actually include undefined. I think Flow is correct to complain. Different version here, restricted to types that don't include undefined. If you add void to the union Flow complains, and rightly so. Your second example does not have any stringify?

I just had some issues with Flow-Try links showing different code depending on where I open them though so I'm not sure I see what you see. Weird, the incognito window shows different code then the regular one, for the exact same link. EDIT: After clearing all site data for Flow-Try it seems to work. I see as your 2nd example this:

/* @flow */

function foo(x: ?number): string {
  if (x) {
    return x;
  }
  return "default string";
}

which Flow rightly complains about and which does not have any JSON.stringify...

[villesau]

Looks like the URL of the second example is malformed and shows your previous flow example. Dunno why. This is what it was supposed to be: https://flow.org/try/#0PTAEAEDMBsHsHcBQBjWA7AzgF1AEwKaQCGArtFgLJFbIAWAklvgLagC8oAPIywCoCeAB3wA+ABQBLJswBcoHswHCANKAz4iAJzoAFak01o52TRLQBzAJTsRiUKABSAZQDyAOQB0Js+YmR+ktKgAD7BAAUA5BGWHmYEAB4ukGLqWrr6+IbWAIRsHAC0AIwA3EA

E: This is what it's supposed to be:

// @flow
const defaultMatchItem = <ItemType>(item: ItemType, searchPattern: string) =>
  JSON.stringify(item || '').indexOf(searchPattern) !== -1;

[lll000111]

@villesau That's the first example, but now with an additional || '' fallback value which makes all the difference. The second example is what I copied into the comment just above.

[villesau]

Yeah. And I believe that || '' should enforce non-unfined type so the refinement is doing something weird in this case.

[lll000111]

Please revert the stringify definition patch

The old (0.94 and before) definition does not include void in the return type, which as stated in the 0.95 release note is a goal. But the attempt to forbid values that lead to undefined failed.

Goal of the PR #7447:

To disallow undefined and functions from being passed in.
JSON.stringify will return undefined if undefined or a function is passed in.

This was not achieved (0.95.1). Now it always returns undefined.

So now we are worse off than before:

• No restrictions on the input (reverted by 0.95.1). Current two definitions: https://github.com/facebook/flow/blob/v0.95.1/lib/core.js#L480 I see no gain over the previous simple any type (you get rid of "any" but the behavior is no better, actually, worse).
• Two definitions instead of one (in core.js lib file)
• Return type now includes the rarely or never used undefined and now we all have to add type refinements for it which clutter the code, and, the stated intent was the opposite. From 0.95 release notes:

Rather than make it always return string | void, or use overloads to return void on those inputs, we instead disallow those inputs since they are rarely the intended behavior.

Well, the previous definition didn't include void as return type so it was fine to begin with.

The original PR even says, emphasis added:

if we allow undefined and functions to be passed in, we would need to change the return value to be void | string to be correct. Since that would cause way more trouble for users of JSON.stringify, disallowing the problematic inputs seems like the correct approach here.

Yet this is exactly what we got now!

[jbrown215]

cc @mroch and @nmote

[nmote]

The original definition (pre-0.95) was unsound, because Flow would assign JSON.stringify(undefined) the type string. #7447 was a little bit extreme in that it disallowed undefined as an input, but we mitigated that with the change in v0.95.1. Now, we do allow any type to be used as an input to stringify, but we model the possibility that the output might be undefined if the input doesn't match one of the types specified in #7447.

This new libdef is a step towards soundness, and while I recognize that it can be inconvenient at times for the type system to be conservative like this, I believe it is the right direction for us to move in. I think it's also worth noting that the original PR got several people interested enough to comment, the original thread on Twitter got several other people interested, and it looks like TypeScript intends to merge an equivalent PR: microsoft/TypeScript#29744.

There may be room for improvement in the libdef, and I'd be happy to review PRs which improve usability without sacrificing soundness. However, I don't currently believe that reverting is the right decision.

As you noted, it still allows functions to be passed in, which isn't desirable. I also noted this in the tests. This is unfortunate but at this point I believe there is little we can do about it.

[lll000111]

@nmote I know it was unsound, that is reflected in the comments I quoted. Did you read them? If you did you ignored them. The current state is against the very principles laid out right there!

If it's not representable by Flow — as is a lot of other stuff! — than one has to compromise. As happens all the time, right here. This is a strange hill that you chose to make your stand on, a lonely hill (the circumstances that cause a void return are exceedingly rare, as those including the patch themselves wrote, I quoted it).

Of what use is your "soundness" if you force people to overwrite that definition? Or do you think people will add lots and lots of utterly useless checks for undefined for every stringify?

[mroch]

@lll000111 you seem to be upset that Flow is "always" returning undefined:

Now it always returns undefined.

but that's not true:

(JSON.stringify(123): string); // no errors

Even your getData example above shows we don't always return undefined.

Flow returns string | void when the input may be undefined, such as with mixed. refinements like if (x !== undefined) are not sufficient because mixed includes functions, and functions make it return undefined.

There are also cases where it may actually return undefined at runtime, but we still unsoundly say it'll be a string, like when you pass something we know is a function (your getData example). This is because we don't track a function's properties and it might implement toJSON(). we'll tighten this up in the future.