Tighten types in test.dart even more.

Tighten types in test.dart even more.

This removes all implicit casts and many implicit uses of dynamic. It
adds a gratuitous number of explicit "as" casts and arguably makes the
code worse.

This is an interim step towards replacing the big configuration map
with an actual typed object. These "as" casts should help catch places
where the configuration object is being used and where the code will
need to be changed to use a new object.

R=sigmund@google.com

Committed: https://github.com/dart-lang/sdk/commit/7ce978bf91ad550610e310dd1032e10ba7d304f2

[Bob Nystrom, 2017-05-23 21:45:03]

rnystrom@google.com changed reviewers:
+ whesse@google.com

[Bob Nystrom, 2017-05-23 21:45:04]

Thanks for reviewing all these changes!

[Bob Nystrom, 2017-05-26 18:11:00]

rnystrom@google.com changed reviewers:
+ sigmund@google.com
- whesse@google.com

[Bob Nystrom, 2017-05-26 18:11:01]

Would you mind taking a look at this too since it's an AAR holiday?

Thanks!

[Bob Nystrom, 2017-05-26 18:11:39]

Note that this also has the changes I just uploaded to the previous patch. You
can ignore those here. I'll land the other one first.

[Siggi Cherem (dart-lang), 2017-05-30 20:48:40]

lgtm after addressing a few small comments/questions.

https://codereview.chromium.org/2903703002/diff/40001/tools/testing/dart/comp...
File tools/testing/dart/compiler_configuration.dart (right):

https://codereview.chromium.org/2903703002/diff/40001/tools/testing/dart/comp...
tools/testing/dart/compiler_configuration.dart:66: var isChecked =
configuration['checked'] as bool;
Just to double check - these are cases that will eventually look more like:
   var isChecked == configuration.isChecked;

correct? (and you are only doing these changes to help you catch problems on a
subsequent CL?)

If not and you intend to keep the cast for a while, then I honestly prefer the
style as it was before. (and you could still get validation of types when
running in checked mode.)

https://codereview.chromium.org/2903703002/diff/40001/tools/testing/dart/html...
File tools/testing/dart/html_test.dart (right):

https://codereview.chromium.org/2903703002/diff/40001/tools/testing/dart/html...
tools/testing/dart/html_test.dart:43: annotation['expectedMessages'] as
List<String>,
It seems this could fail in strong mode: the underlying json List is probably
List<dynamic> and not List<String>, correct?

https://codereview.chromium.org/2903703002/diff/40001/tools/testing/dart/reco...
File tools/testing/dart/record_and_replay.dart (right):

https://codereview.chromium.org/2903703002/diff/40001/tools/testing/dart/reco...
tools/testing/dart/record_and_replay.dart:95: as List<Map<String, Map<String,
dynamic>>>;
same here: looks to me like this would fail in strong mode.

https://codereview.chromium.org/2903703002/diff/40001/tools/testing/dart/test...
File tools/testing/dart/test_progress.dart (right):

https://codereview.chromium.org/2903703002/diff/40001/tools/testing/dart/test...
tools/testing/dart/test_progress.dart:668:
.addAll(test.configuration['_reproducing_arguments_'] as List<String>);
here too?

[Bob Nystrom, 2017-05-30 21:01:47]

Thanks!

https://codereview.chromium.org/2903703002/diff/40001/tools/testing/dart/comp...
File tools/testing/dart/compiler_configuration.dart (right):

https://codereview.chromium.org/2903703002/diff/40001/tools/testing/dart/comp...
tools/testing/dart/compiler_configuration.dart:66: var isChecked =
configuration['checked'] as bool;
On 2017/05/30 20:48:39, Siggi Cherem (dart-lang) wrote:
> Just to double check - these are cases that will eventually look more like:
>    var isChecked == configuration.isChecked;
> 
> correct? (and you are only doing these changes to help you catch problems on a
> subsequent CL?)

That's right. That CL is: https://codereview.chromium.org/2901923003/

https://codereview.chromium.org/2903703002/diff/40001/tools/testing/dart/html...
File tools/testing/dart/html_test.dart (right):

https://codereview.chromium.org/2903703002/diff/40001/tools/testing/dart/html...
tools/testing/dart/html_test.dart:43: annotation['expectedMessages'] as
List<String>,
On 2017/05/30 20:48:39, Siggi Cherem (dart-lang) wrote:
> It seems this could fail in strong mode: the underlying json List is probably
> List<dynamic> and not List<String>, correct?

Ah, yes. Good catch. Fixed.

https://codereview.chromium.org/2903703002/diff/40001/tools/testing/dart/reco...
File tools/testing/dart/record_and_replay.dart (right):

https://codereview.chromium.org/2903703002/diff/40001/tools/testing/dart/reco...
tools/testing/dart/record_and_replay.dart:95: as List<Map<String, Map<String,
dynamic>>>;
On 2017/05/30 20:48:39, Siggi Cherem (dart-lang) wrote:
> same here: looks to me like this would fail in strong mode.

Done.

https://codereview.chromium.org/2903703002/diff/40001/tools/testing/dart/test...
File tools/testing/dart/test_progress.dart (right):

https://codereview.chromium.org/2903703002/diff/40001/tools/testing/dart/test...
tools/testing/dart/test_progress.dart:668:
.addAll(test.configuration['_reproducing_arguments_'] as List<String>);
On 2017/05/30 20:48:39, Siggi Cherem (dart-lang) wrote:
> here too?

This one is OK. It comes from _reproducingCommand(), which does return a reified
List<String>.

[Bob Nystrom, 2017-05-30 21:02:26]

Description was changed from

==========
Tighten types in test.dart even more.

This removes all implicit casts and many implicit uses of dynamic. It
adds a gratuitous number of explicit "as" casts and arguably makes the
code worse.

This is an interim step towards replacing the big configuration map
with an actual typed object. These "as" casts should help catch places
where the configuration object is being used and where the code will
need to be changed to use a new object.
==========

to

==========
Tighten types in test.dart even more.

This removes all implicit casts and many implicit uses of dynamic. It
adds a gratuitous number of explicit "as" casts and arguably makes the
code worse.

This is an interim step towards replacing the big configuration map
with an actual typed object. These "as" casts should help catch places
where the configuration object is being used and where the code will
need to be changed to use a new object.

R=sigmund@google.com

Committed:
https://github.com/dart-lang/sdk/commit/7ce978bf91ad550610e310dd1032e10ba7d304f2
==========

[Bob Nystrom, 2017-05-30 21:02:27]

Committed patchset #4 (id:60001) manually as
7ce978bf91ad550610e310dd1032e10ba7d304f2 (presubmit successful).