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

JSON.decode is not very handy in strong mode #31876

Open
mraleph opened this Issue Jan 12, 2018 · 10 comments

Comments

Projects
None yet
5 participants
@mraleph
Contributor

mraleph commented Jan 12, 2018

This is an umbrella issue for unhandiness of using objects returned by JSON.decode in strong mode.

JSON.decode returns List<dynamic> and Map<String, dynamic>, while in strong mode people expect it to return the most precise type.

For example in Flutter code like this:

final Map<String, List<String>> parsedManifest = JSON.decode(json)

needs to be replaced by this

final Map<String, dynamic> parsedJson = JSON.decode(json);
final Iterable<String> keys = parsedJson.keys;
final Map<String, List<String>> parsedManifest =
      new Map<String, List<String>>.fromIterables(keys,
         keys.map((key) => new List<String>.from(parsedJson[key])));

to make it correctly typed.

We might be doing work post-Dart 2.0 on improving the usability, but right now the code simply needs to be fixed. /cc @leafpetersen @lrhn

mraleph added a commit to mraleph/flutter that referenced this issue Jan 12, 2018

Fix a couple of strong mode issues.
* JSON.decode produces Map<String, dynamic> and List<dynamic>
objects. If a more tight type is required then object needs to
be converted explicitly (see dart-lang/sdk#31876);
* Completer<dynamic> produces Future<dynamic>. In Dart 2 it is
runtime error to assign Future<dynamic> to variable of type Future<T>;
@leafpetersen

This comment has been minimized.

Show comment
Hide comment
@leafpetersen

leafpetersen Jan 12, 2018

Member

There's been internal discussion of this: quoting myself:

One possible interface to this that would require no compiler magic at all might end up looking something like this:

const decoder = const json.decodeMap<String, int>(json.decodeString, json.decodeInt);

String preference = read(preferenceKey) ?? '{}';
return decoder.decode(preference); // No cast required

@rakudrama and @munificent played around with this a bit, I'd love for us to follow up on it when we have more time.

Member

leafpetersen commented Jan 12, 2018

There's been internal discussion of this: quoting myself:

One possible interface to this that would require no compiler magic at all might end up looking something like this:

const decoder = const json.decodeMap<String, int>(json.decodeString, json.decodeInt);

String preference = read(preferenceKey) ?? '{}';
return decoder.decode(preference); // No cast required

@rakudrama and @munificent played around with this a bit, I'd love for us to follow up on it when we have more time.

@leafpetersen

This comment has been minimized.

Show comment
Hide comment
@leafpetersen

leafpetersen Jan 12, 2018

Member

Note that once the .cast methods being added to the core library have landed, you could use those to avoid re-allocating the lists at the expense of checking each access.

Member

leafpetersen commented Jan 12, 2018

Note that once the .cast methods being added to the core library have landed, you could use those to avoid re-allocating the lists at the expense of checking each access.

mraleph added a commit to flutter/flutter that referenced this issue Jan 13, 2018

Fix a couple of strong mode issues. (#14070)
* JSON.decode produces Map<String, dynamic> and List<dynamic>
objects. If a more tight type is required then object needs to
be converted explicitly (see dart-lang/sdk#31876);
* Completer<dynamic> produces Future<dynamic>. In Dart 2 it is
runtime error to assign Future<dynamic> to variable of type Future<T>;
@matanlurey

This comment has been minimized.

Show comment
Hide comment
@matanlurey

matanlurey Jan 23, 2018

Contributor

There is still the issue of "using" JSON as structured data. This is quite popular, for example:

Imagine the following JSON blob:

{
  "names": [
    "Leaf Peterson",
    "Vyacheslav Egorov",
  ]
}
Future<void> getAndPrintNames() async {
  Map<String, dynamic> json = await _getJson();
  // Error: List<dynamic> is not a List<String>
  List<String> names = json['names'];
  names.forEach(print);
}

Once we have cast we could do it, but, not easily for our users:

var names = (json['names'] as List).cast<String>().toList();

Let's break that down:

var /* dynamic */ namesDynamic = json['names'];
var /* List<dynamic> */ namesListOfDynamic = namesDynamic as List;
var /* Iterable<String> */ namesIterableOfString = namesListOfDynamic.cast<String>();
var /* List<String> */ namesListOfString = namesIterableOfString.toList();

That's a lot of work, and easy to get wrong, especially with implicit casting:

  1. You could forget to do "as List", and do dynamic calls instead.
  2. You could forget to use "cast", and fail at runtime with the original error.
  3. You could forget to use "toList", and fail at runtime with Iterable<String> is not List<String>.
Contributor

matanlurey commented Jan 23, 2018

There is still the issue of "using" JSON as structured data. This is quite popular, for example:

Imagine the following JSON blob:

{
  "names": [
    "Leaf Peterson",
    "Vyacheslav Egorov",
  ]
}
Future<void> getAndPrintNames() async {
  Map<String, dynamic> json = await _getJson();
  // Error: List<dynamic> is not a List<String>
  List<String> names = json['names'];
  names.forEach(print);
}

Once we have cast we could do it, but, not easily for our users:

var names = (json['names'] as List).cast<String>().toList();

Let's break that down:

var /* dynamic */ namesDynamic = json['names'];
var /* List<dynamic> */ namesListOfDynamic = namesDynamic as List;
var /* Iterable<String> */ namesIterableOfString = namesListOfDynamic.cast<String>();
var /* List<String> */ namesListOfString = namesIterableOfString.toList();

That's a lot of work, and easy to get wrong, especially with implicit casting:

  1. You could forget to do "as List", and do dynamic calls instead.
  2. You could forget to use "cast", and fail at runtime with the original error.
  3. You could forget to use "toList", and fail at runtime with Iterable<String> is not List<String>.
@lrhn

This comment has been minimized.

Show comment
Hide comment
@lrhn

lrhn Jan 24, 2018

Member

We have made this harder for ourselves by returning mutable collections.
If they were immutable, we could let the decoder pick the most specific type that actually match its contents. (Well, it would probably require extra copying or the ability to change the type of a list/map, but internally we should be able to do that).

When the returned collections are mutable, a user might expect a List<Object>, and try to add a number to it. We can't return a List<String> just because it happens to only contains strings.

Specifying the type of the decoded data is doable too, probably better using composable decoders rather than generics. Maybe something like:

  JsonMap(JsonList(JsonString)).decode(text);
  JsonList(JsonStruct({"name": JsonString, "address": JsonList(JsonString)})).decode(people)

(say, where JsonMap defines a uniform map and JsonStruct has a separate decoder per named entry in the map).
We do run the risk of inventing another schema language :)

Member

lrhn commented Jan 24, 2018

We have made this harder for ourselves by returning mutable collections.
If they were immutable, we could let the decoder pick the most specific type that actually match its contents. (Well, it would probably require extra copying or the ability to change the type of a list/map, but internally we should be able to do that).

When the returned collections are mutable, a user might expect a List<Object>, and try to add a number to it. We can't return a List<String> just because it happens to only contains strings.

Specifying the type of the decoded data is doable too, probably better using composable decoders rather than generics. Maybe something like:

  JsonMap(JsonList(JsonString)).decode(text);
  JsonList(JsonStruct({"name": JsonString, "address": JsonList(JsonString)})).decode(people)

(say, where JsonMap defines a uniform map and JsonStruct has a separate decoder per named entry in the map).
We do run the risk of inventing another schema language :)

@munificent

This comment has been minimized.

Show comment
Hide comment
@munificent

munificent Jan 29, 2018

Member

Yeah, I would definitely love to sink my teeth into this one. One approach is to lazily deserialize the JSON. In most cases, the user does know what types they expect a given piece of the JSON to have. The problem is that by the time they tell us, we've already deserialized the JSON and built the collection.

If we could parse it and build the collections on demand as the user traverses into the JSON, we could ensure the sub-parts are built with the right reified types.

I toyed with a prototype of this a while back, and it seemed promising.

Member

munificent commented Jan 29, 2018

Yeah, I would definitely love to sink my teeth into this one. One approach is to lazily deserialize the JSON. In most cases, the user does know what types they expect a given piece of the JSON to have. The problem is that by the time they tell us, we've already deserialized the JSON and built the collection.

If we could parse it and build the collections on demand as the user traverses into the JSON, we could ensure the sub-parts are built with the right reified types.

I toyed with a prototype of this a while back, and it seemed promising.

@matanlurey

This comment has been minimized.

Show comment
Hide comment
@matanlurey

matanlurey Jan 29, 2018

Contributor

How would that work in environments such as JS where we don't control the parse impl?

Contributor

matanlurey commented Jan 29, 2018

How would that work in environments such as JS where we don't control the parse impl?

@munificent

This comment has been minimized.

Show comment
Hide comment
@munificent

munificent Jan 29, 2018

Member

In JS, there is (as far as I know, I only poked at it for a couple of hours or so) still a two-stage "parsing" process:

  1. Parse the JSON string to a JS JSON object.
  2. Go through and convert the JS collections to Dart ones.

Step 2 is done in Dart and would be where we could inject the types. Though, now that I look at DDC's implementation of dart:convert, it looks like step 2 only really happens for maps. For lists, we use the JS array as-is. :-/

So there would be overhead for what I propose, to go through and copy the JSArray to a new typed list. Though I imagine we might be able to do something more clever and actually jam the type argument into the existing JSArray, since we know it hasn't escaped anyway.

Member

munificent commented Jan 29, 2018

In JS, there is (as far as I know, I only poked at it for a couple of hours or so) still a two-stage "parsing" process:

  1. Parse the JSON string to a JS JSON object.
  2. Go through and convert the JS collections to Dart ones.

Step 2 is done in Dart and would be where we could inject the types. Though, now that I look at DDC's implementation of dart:convert, it looks like step 2 only really happens for maps. For lists, we use the JS array as-is. :-/

So there would be overhead for what I propose, to go through and copy the JSArray to a new typed list. Though I imagine we might be able to do something more clever and actually jam the type argument into the existing JSArray, since we know it hasn't escaped anyway.

@matanlurey

This comment has been minimized.

Show comment
Hide comment
@matanlurey

matanlurey Jan 29, 2018

Contributor

Note that JSON performance in Dart2JS is already 3x worse than using plain JavaScript:
https://github.com/matanlurey/dart-js-improvements#json

... so I'd be really hesitant to make this even worse if possible.

Contributor

matanlurey commented Jan 29, 2018

Note that JSON performance in Dart2JS is already 3x worse than using plain JavaScript:
https://github.com/matanlurey/dart-js-improvements#json

... so I'd be really hesitant to make this even worse if possible.

@munificent

This comment has been minimized.

Show comment
Hide comment
@munificent

munificent Jan 29, 2018

Member

Yeah, understood. Performance was one of the things I was looking at when I was prototyping. I wouldn't consider any solution that had bad perf to be acceptable.

Member

munificent commented Jan 29, 2018

Yeah, understood. Performance was one of the things I was looking at when I was prototyping. I wouldn't consider any solution that had bad perf to be acceptable.

@leafpetersen

This comment has been minimized.

Show comment
Hide comment
@leafpetersen
Member

leafpetersen commented Jan 29, 2018

DaveShuckerow pushed a commit to DaveShuckerow/flutter that referenced this issue May 14, 2018

Fix a couple of strong mode issues. (flutter#14070)
* JSON.decode produces Map<String, dynamic> and List<dynamic>
objects. If a more tight type is required then object needs to
be converted explicitly (see dart-lang/sdk#31876);
* Completer<dynamic> produces Future<dynamic>. In Dart 2 it is
runtime error to assign Future<dynamic> to variable of type Future<T>;

@leafpetersen leafpetersen self-assigned this Jun 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment