Skip to content

Conversation

@devoncarew
Copy link
Contributor

Fix several issues when running flutter_tools in --preview-dart-2:

  • fix declared return types to address strong mode at runtime errors
  • cast() data structures parsed from json so that the types at runtime match what we expect
  • add generic types to some methods to allow the types to flow through a method call

We'll need to change the flutter_tools tests over to running in --preview-dart-2 in order to keep the code dart 2 safe, but we're not quite ready to do that (our test suite is reparsed for each individual test - the test suite takes 40-50 minutes to run).

@devoncarew
Copy link
Contributor Author

Follow up to #18960 and #18998.

@devoncarew
Copy link
Contributor Author

We'll also want to update to the (just recently published) latest version of json_schema.

Copy link
Contributor

@mravn-google mravn-google left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

List<Map<String, dynamic>> _getMaterialFonts(String fontSet) {
return _materialFontsManifest[fontSet];
final List<dynamic> fontsList = _materialFontsManifest[fontSet];
return fontsList?.map(castStringKeyedMap)?.toList();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

List.map should be given a type parameter per Flutter style guide:

return fontsList?.map<Map<String, dynamic>>(castStringKeyedMap)?.toList();

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

return _handlers[command](args);
throw 'command not understood: $name.$command';
}).then<Null>((dynamic result) {
}).then<dynamic>((dynamic result) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

then<void>?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we want dynamic here, to match what we get from _handlers[command](args).

});

await app._runInZone(this, () async {
await app._runInZone<Null>(this, () async {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_runInZone<void>?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, at runtime, this seemed to want a Null.

final List<AnalysisError> errors = issueInfo['errors']
final List<dynamic> errorsList = issueInfo['errors'];
final List<AnalysisError> errors = errorsList
.map(castStringKeyedMap)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The two map calls should have type parameters Map<String, dynamic> and AnalysisError, respectively.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added

pubspec._flutterDescriptor = flutterMap.cast<String, dynamic>();
} else {
pubspec._flutterDescriptor = <String, dynamic>{};
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe extract this pattern to a facility like castStringKeyedMap. We could let that method return an empty map on null input (but then we might also want to rename it toStringKeyedMap).

final List<dynamic> fontList = _flutterDescriptor['fonts'];
return fontList == null
? const <Map<String, dynamic>>[]
: fontList.map(castStringKeyedMap).toList();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Type parameter on map...

This gets annoying. Perhaps we should have

Map<String, dynamic> toStringKeyedMap(dynamic json) {
  if (json == null) {
    return <String, dynamic>{};
  } else {
    final Map<dynamic, dynamic> map = json;
    return map.cast<String, dynamic>();
  }
}

List<Map<String, dynamic>> toStringKeyedMaps(dynamic json) {
  if (json == null) {
    return <Map<String, dynamic>>[];
  } else {
    final List<dynamic> list = json;
    return list.map<Map<String, dynamic>>(toStringKeyedMap).toList();
  }
}

Alternative naming: toJsonObject/toJsonArray.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The empty map and list could be const, but then the semantics gets complicated: toStringKeyedMap(null) is immutable, whereas toStringKeyedMap({}) is not. If we do use const we should return immutable objects in all cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me look into creating a toStringKeyedMaps() method.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mravn-google, I may save that for a follow-up PR (toStringKeyedMaps).

if (assets == null) {
return const <Uri>[];
}
return assets.cast<String>().map(Uri.encodeFull)?.map(Uri.parse)?.toList();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Type parameter on map, twice.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

for (String deviceCategory in devicesSection.keys) {
final List<dynamic> devicesData = devicesSection[deviceCategory];
for (Map<String, dynamic> data in devicesData.map(_castStringKeyedMap)) {
for (Map<String, dynamic> data in devicesData.map(castStringKeyedMap)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Type parameter on map.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

) {
return Future.any(<Future<Map<String, dynamic>>>[
_peer.sendRequest(method, params),
_peer.sendRequest(method, params).then(castStringKeyedMap),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Type parameter on then.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added

bool get isClosed => _peer.isClosed;
Future<Null> get done => _peer.done;

Future<Null> get done async {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Future<void>?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really don't know if we're returning null or void here.

@devoncarew devoncarew merged commit 9d9836f into flutter:master Jul 9, 2018
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants