Fix all strong mode warnings in protoc-plugin

Fix all strong mode warnings in protoc-plugin

Includes changes to the plugin, the generated code, the
benchmarks, and the tests.

Also, fix the benchmarks to work again. It has a hacky parser
for .pubspec-lock files that didn't work after the format changed.

Also, dartfmt modified files.

Committed: https://github.com/dart-lang/dart-protoc-plugin/commit/23a67b48fc1ad532e7786345ce8b7b3adebd8f14

[skybrian, 2016-03-23 02:23:09]

Description was changed from

==========
Fix all strong mode warnings in protoc-plugin

Includes changes to the plugin, the generated code, the
benchmarks, and the tests.
==========

to

==========
Fix all strong mode warnings in protoc-plugin

Includes changes to the plugin, the generated code, the
benchmarks, and the tests.

Also, fix the benchmarks to work again. It has a hacky parser
for .pubspec-lock files that didn't work after the format changed.
==========

[skybrian, 2016-03-23 02:31:32]

Description was changed from

==========
Fix all strong mode warnings in protoc-plugin

Includes changes to the plugin, the generated code, the
benchmarks, and the tests.

Also, fix the benchmarks to work again. It has a hacky parser
for .pubspec-lock files that didn't work after the format changed.
==========

to

==========
Fix all strong mode warnings in protoc-plugin

Includes changes to the plugin, the generated code, the
benchmarks, and the tests.

Also, fix the benchmarks to work again. It has a hacky parser
for .pubspec-lock files that didn't work after the format changed.

Also, dartfmt modified files.
==========

[skybrian, 2016-03-23 02:43:31]

skybrian@google.com changed reviewers:
+ vsm@google.com

[vsm, 2016-03-23 17:20:08]

vsm@google.com changed reviewers:
+ leafp@google.com

[vsm, 2016-03-23 17:20:24]

https://chromiumcodereview.appspot.com/1829573002/diff/1/benchmark/lib/dashbo...
File benchmark/lib/dashboard_model.dart (right):

https://chromiumcodereview.appspot.com/1829573002/diff/1/benchmark/lib/dashbo...
benchmark/lib/dashboard_model.dart:47: Iterator it = report == null ?
[].iterator : report.responses.iterator;
Interesting that we're not inferring this type.

https://chromiumcodereview.appspot.com/1829573002/diff/1/benchmark/lib/dashbo...
File benchmark/lib/dashboard_view.dart (right):

https://chromiumcodereview.appspot.com/1829573002/diff/1/benchmark/lib/dashbo...
benchmark/lib/dashboard_view.dart:100: _menu.onChange.map((item) => item ==
noBaseline ? null : item)
Stream.map is a generic method.  Strong mode will try to infer the type, but
it's failing here.

Can you try map<String>(...) and dropping the cast?  The cast will fail at
runtime in DDC (it'd try to cast Stream<Object> to Stream<String>).

https://chromiumcodereview.appspot.com/1829573002/diff/1/benchmark/lib/dashbo...
benchmark/lib/dashboard_view.dart:246: Stream<String> get onChange =>
_changes.stream as Stream<String>;
This will fail at runtime if the runtime type of _changes.stream is not a
Stream<String>.  Instead, I think we need above:

final _changes = new StreamController<String>.broadcast();

and drop the cast.

https://chromiumcodereview.appspot.com/1829573002/diff/1/lib/src/descriptor.p...
File lib/src/descriptor.pb.dart (right):

https://chromiumcodereview.appspot.com/1829573002/diff/1/lib/src/descriptor.p...
lib/src/descriptor.pb.dart:30: List<FileDescriptorProto> get file => $_get(0, 1,
null) as List<FileDescriptorProto>;
This is going to be problematic at runtime if $_get returns a List<Object> or
List<dynamic>.

A possible solution is to make $_get a generic function?  Something like:

$_getList<FileDescriptorProto>(0, 1, null)

Would that make sense?

https://chromiumcodereview.appspot.com/1829573002/diff/1/lib/src/descriptor.p...
lib/src/descriptor.pb.dart:76: List<String> get dependency => $_get(2, 3, null)
as List<String>;
ditto here and the other List casts below

https://chromiumcodereview.appspot.com/1829573002/diff/1/lib/src/plugin.pb.dart
File lib/src/plugin.pb.dart (right):

https://chromiumcodereview.appspot.com/1829573002/diff/1/lib/src/plugin.pb.da...
lib/src/plugin.pb.dart:32: List<String> get fileToGenerate => $_get(0, 1, null)
as List<String>;
here as well

[skybrian, 2016-03-23 18:02:39]

Thanks, looks like this is going to take more work. I'll get back to this after
summary stuff is done.

https://chromiumcodereview.appspot.com/1829573002/diff/1/lib/src/descriptor.p...
File lib/src/descriptor.pb.dart (right):

https://chromiumcodereview.appspot.com/1829573002/diff/1/lib/src/descriptor.p...
lib/src/descriptor.pb.dart:30: List<FileDescriptorProto> get file => $_get(0, 1,
null) as List<FileDescriptorProto>;
On 2016/03/23 17:20:24, vsm wrote:
> This is going to be problematic at runtime if $_get returns a List<Object> or
> List<dynamic>.
> 
> A possible solution is to make $_get a generic function?  Something like:
> 
> $_getList<FileDescriptorProto>(0, 1, null)
> 
> Would that make sense?

It's going to be trickier than that. Ultimately the list for a repeated field is
created here:

https://github.com/dart-lang/dart-protobuf/blob/master/lib/src/protobuf/gener...

I could make this a generic method, but I'm not sure I can pass the type all the
way down.

[Leaf, 2016-03-23 18:10:00]

https://chromiumcodereview.appspot.com/1829573002/diff/1/benchmark/lib/dashbo...
File benchmark/lib/dashboard_model.dart (right):

https://chromiumcodereview.appspot.com/1829573002/diff/1/benchmark/lib/dashbo...
benchmark/lib/dashboard_model.dart:47: Iterator it = report == null ?
[].iterator : report.responses.iterator;
On 2016/03/23 17:20:24, vsm wrote:
> Interesting that we're not inferring this type.

Probably LUB.

https://chromiumcodereview.appspot.com/1829573002/diff/1/benchmark/lib/dashbo...
File benchmark/lib/dashboard_view.dart (right):

https://chromiumcodereview.appspot.com/1829573002/diff/1/benchmark/lib/dashbo...
benchmark/lib/dashboard_view.dart:100: _menu.onChange.map((item) => item ==
noBaseline ? null : item)
On 2016/03/23 17:20:24, vsm wrote:
> Stream.map is a generic method.  Strong mode will try to infer the type, but
> it's failing here.
> 
> Can you try map<String>(...) and dropping the cast?  The cast will fail at
> runtime in DDC (it'd try to cast Stream<Object> to Stream<String>).

I don't understand why inference is failing here.  In a standalone repro it
works.  What version are you using?  Command line or Intellij?  There's an
intellij regression in 1.16.0.dev-2.0 that breaks strong mode generics - be sure
you're not using that.

https://chromiumcodereview.appspot.com/1829573002/diff/1/benchmark/lib/dashbo...
benchmark/lib/dashboard_view.dart:246: Stream<String> get onChange =>
_changes.stream as Stream<String>;
On 2016/03/23 17:20:24, vsm wrote:
> This will fail at runtime if the runtime type of _changes.stream is not a
> Stream<String>.  Instead, I think we need above:
> 
> final _changes = new StreamController<String>.broadcast();
> 
> and drop the cast.

Agreed.

https://chromiumcodereview.appspot.com/1829573002/diff/1/lib/src/descriptor.p...
File lib/src/descriptor.pb.dart (right):

https://chromiumcodereview.appspot.com/1829573002/diff/1/lib/src/descriptor.p...
lib/src/descriptor.pb.dart:30: List<FileDescriptorProto> get file => $_get(0, 1,
null) as List<FileDescriptorProto>;
On 2016/03/23 18:02:39, skybrian wrote:
> On 2016/03/23 17:20:24, vsm wrote:
> > This is going to be problematic at runtime if $_get returns a List<Object>
or
> > List<dynamic>.
> > 
> > A possible solution is to make $_get a generic function?  Something like:
> > 
> > $_getList<FileDescriptorProto>(0, 1, null)
> > 
> > Would that make sense?
> 
> It's going to be trickier than that. Ultimately the list for a repeated field
is
> created here:
> 
>
https://github.com/dart-lang/dart-protobuf/blob/master/lib/src/protobuf/gener...
> 
> I could make this a generic method, but I'm not sure I can pass the type all
the
> way down.

We don't support generic getters and setters.  Interesting that we're
encountering a use case for them.  What about new
List<FileDescriptor>.from($_get(0, 1, null)?

[skybrian, 2016-03-23 18:34:17]

Apparently fixing analyzer warnings and running tests isn't enough to verify
that strong mode works. We also need to make sure the behavior is correct at
runtime.  So I'm wondering how I can verify that the issues you pointed out in
code review are fixed?

It seems like either we need a "strong checked mode" or I should be running
protobuf's tests using DDC. Otherwise, I can fix all the analyzer warnings but
it still might not work.

https://chromiumcodereview.appspot.com/1829573002/diff/1/benchmark/lib/dashbo...
File benchmark/lib/dashboard_view.dart (right):

https://chromiumcodereview.appspot.com/1829573002/diff/1/benchmark/lib/dashbo...
benchmark/lib/dashboard_view.dart:100: _menu.onChange.map((item) => item ==
noBaseline ? null : item)
On 2016/03/23 18:09:59, Leaf wrote:
> On 2016/03/23 17:20:24, vsm wrote:
> > Stream.map is a generic method.  Strong mode will try to infer the type, but
> > it's failing here.
> > 
> > Can you try map<String>(...) and dropping the cast?  The cast will fail at
> > runtime in DDC (it'd try to cast Stream<Object> to Stream<String>).
> 
> I don't understand why inference is failing here.  In a standalone repro it
> works.  What version are you using?  Command line or Intellij?  There's an
> intellij regression in 1.16.0.dev-2.0 that breaks strong mode generics - be
sure
> you're not using that.

I'm using Atom with an SDK built at HEAD. I think this is another instance of:
https://github.com/dart-lang/sdk/issues/26062

Hopefully by the time I get back to this CL it will be fixed.

https://chromiumcodereview.appspot.com/1829573002/diff/1/lib/src/descriptor.p...
File lib/src/descriptor.pb.dart (right):

https://chromiumcodereview.appspot.com/1829573002/diff/1/lib/src/descriptor.p...
lib/src/descriptor.pb.dart:30: List<FileDescriptorProto> get file => $_get(0, 1,
null) as List<FileDescriptorProto>;
> We don't support generic getters and setters.

I don't know what you mean by that? _get isn't a getter.

> List<FileDescriptor>.from($_get(0, 1, null)?

Doesn't work - the whole point is that we're returning a mutable  list. Also,
it's important to preserve the subtype of List.

[vsm, 2016-03-23 19:30:33]

On 2016/03/23 18:34:17, skybrian wrote:
> Apparently fixing analyzer warnings and running tests isn't enough to verify
> that strong mode works. We also need to make sure the behavior is correct at
> runtime.  So I'm wondering how I can verify that the issues you pointed out in
> code review are fixed?

Right, certain unsafe operations need to be checked at runtime (down casts,
dynamic operations).  Static analysis can't tell us whether those will fail.

> It seems like either we need a "strong checked mode" or I should be running
> protobuf's tests using DDC. Otherwise, I can fix all the analyzer warnings but
> it still might not work.

We'll have this soon.  We'll need to revisit when we do.

> 
>
https://chromiumcodereview.appspot.com/1829573002/diff/1/benchmark/lib/dashbo...
> File benchmark/lib/dashboard_view.dart (right):
> 
>
https://chromiumcodereview.appspot.com/1829573002/diff/1/benchmark/lib/dashbo...
> benchmark/lib/dashboard_view.dart:100: _menu.onChange.map((item) => item ==
> noBaseline ? null : item)
> On 2016/03/23 18:09:59, Leaf wrote:
> > On 2016/03/23 17:20:24, vsm wrote:
> > > Stream.map is a generic method.  Strong mode will try to infer the type,
but
> > > it's failing here.
> > > 
> > > Can you try map<String>(...) and dropping the cast?  The cast will fail at
> > > runtime in DDC (it'd try to cast Stream<Object> to Stream<String>).
> > 
> > I don't understand why inference is failing here.  In a standalone repro it
> > works.  What version are you using?  Command line or Intellij?  There's an
> > intellij regression in 1.16.0.dev-2.0 that breaks strong mode generics - be
> sure
> > you're not using that.
> 
> I'm using Atom with an SDK built at HEAD. I think this is another instance of:
> https://github.com/dart-lang/sdk/issues/26062
> 
> Hopefully by the time I get back to this CL it will be fixed.
> 
>
https://chromiumcodereview.appspot.com/1829573002/diff/1/lib/src/descriptor.p...
> File lib/src/descriptor.pb.dart (right):
> 
>
https://chromiumcodereview.appspot.com/1829573002/diff/1/lib/src/descriptor.p...
> lib/src/descriptor.pb.dart:30: List<FileDescriptorProto> get file => $_get(0,
1,
> null) as List<FileDescriptorProto>;
> > We don't support generic getters and setters.
> 
> I don't know what you mean by that? _get isn't a getter.
> 
> > List<FileDescriptor>.from($_get(0, 1, null)?
> 
> Doesn't work - the whole point is that we're returning a mutable  list. Also,
> it's important to preserve the subtype of List.

[vsm, 2016-03-23 19:34:22]

On 2016/03/23 19:30:33, vsm wrote:
> On 2016/03/23 18:34:17, skybrian wrote:
> > Apparently fixing analyzer warnings and running tests isn't enough to verify
> > that strong mode works. We also need to make sure the behavior is correct at
> > runtime.  So I'm wondering how I can verify that the issues you pointed out
in
> > code review are fixed?
> 
> Right, certain unsafe operations need to be checked at runtime (down casts,
> dynamic operations).  Static analysis can't tell us whether those will fail.
> 
> > It seems like either we need a "strong checked mode" or I should be running
> > protobuf's tests using DDC. Otherwise, I can fix all the analyzer warnings
but
> > it still might not work.
> 
> We'll have this soon.  We'll need to revisit when we do.

Actually, I think you meant command line tests?  We need to think about how to
make that work. 

> > 
> >
>
https://chromiumcodereview.appspot.com/1829573002/diff/1/benchmark/lib/dashbo...
> > File benchmark/lib/dashboard_view.dart (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/1829573002/diff/1/benchmark/lib/dashbo...
> > benchmark/lib/dashboard_view.dart:100: _menu.onChange.map((item) => item ==
> > noBaseline ? null : item)
> > On 2016/03/23 18:09:59, Leaf wrote:
> > > On 2016/03/23 17:20:24, vsm wrote:
> > > > Stream.map is a generic method.  Strong mode will try to infer the type,
> but
> > > > it's failing here.
> > > > 
> > > > Can you try map<String>(...) and dropping the cast?  The cast will fail
at
> > > > runtime in DDC (it'd try to cast Stream<Object> to Stream<String>).
> > > 
> > > I don't understand why inference is failing here.  In a standalone repro
it
> > > works.  What version are you using?  Command line or Intellij?  There's an
> > > intellij regression in 1.16.0.dev-2.0 that breaks strong mode generics -
be
> > sure
> > > you're not using that.
> > 
> > I'm using Atom with an SDK built at HEAD. I think this is another instance
of:
> > https://github.com/dart-lang/sdk/issues/26062
> > 
> > Hopefully by the time I get back to this CL it will be fixed.
> > 
> >
>
https://chromiumcodereview.appspot.com/1829573002/diff/1/lib/src/descriptor.p...
> > File lib/src/descriptor.pb.dart (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/1829573002/diff/1/lib/src/descriptor.p...
> > lib/src/descriptor.pb.dart:30: List<FileDescriptorProto> get file =>
$_get(0,
> 1,
> > null) as List<FileDescriptorProto>;
> > > We don't support generic getters and setters.
> > 
> > I don't know what you mean by that? _get isn't a getter.
> > 
> > > List<FileDescriptor>.from($_get(0, 1, null)?
> > 
> > Doesn't work - the whole point is that we're returning a mutable  list.
Also,
> > it's important to preserve the subtype of List.

[Søren Gjesse, 2016-03-29 09:04:12]

sgjesse@google.com changed reviewers:
+ sgjesse@google.com

[Søren Gjesse, 2016-03-29 09:04:13]

dbc.

https://chromiumcodereview.appspot.com/1829573002/diff/1/lib/file_generator.dart
File lib/file_generator.dart (right):

https://chromiumcodereview.appspot.com/1829573002/diff/1/lib/file_generator.d...
lib/file_generator.dart:123: throw ("FAILURE: File with an absolute path is not
supported");
Remove the () here.

https://chromiumcodereview.appspot.com/1829573002/diff/1/lib/file_generator.d...
lib/file_generator.dart:207: throw ("FAILURE: Import with absolute path is not
supported");
Remove the () here.

https://chromiumcodereview.appspot.com/1829573002/diff/1/lib/options.dart
File lib/options.dart (right):

https://chromiumcodereview.appspot.com/1829573002/diff/1/lib/options.dart#new...
lib/options.dart:23: }
???

https://chromiumcodereview.appspot.com/1829573002/diff/1/lib/src/descriptor.p...
File lib/src/descriptor.pb.dart (right):

https://chromiumcodereview.appspot.com/1829573002/diff/1/lib/src/descriptor.p...
lib/src/descriptor.pb.dart:4: library proto2_descriptor;
What is the reason for editing the generated code? Shouldn't the generator be
fixed instead?

[skybrian, 2016-04-01 02:00:15]

Updated. This now depends on some more changes to dart protobuf [1]. I bumped
the version numbers.

[1] https://chromiumcodereview.appspot.com/1844293003/

https://chromiumcodereview.appspot.com/1829573002/diff/1/benchmark/lib/dashbo...
File benchmark/lib/dashboard_model.dart (right):

https://chromiumcodereview.appspot.com/1829573002/diff/1/benchmark/lib/dashbo...
benchmark/lib/dashboard_model.dart:47: Iterator it = report == null ?
[].iterator : report.responses.iterator;
On 2016/03/23 18:09:59, Leaf wrote:
> On 2016/03/23 17:20:24, vsm wrote:
> > Interesting that we're not inferring this type.
> 
> Probably LUB.

What does LUB mean?

https://chromiumcodereview.appspot.com/1829573002/diff/1/benchmark/lib/dashbo...
File benchmark/lib/dashboard_view.dart (right):

https://chromiumcodereview.appspot.com/1829573002/diff/1/benchmark/lib/dashbo...
benchmark/lib/dashboard_view.dart:100: _menu.onChange.map((item) => item ==
noBaseline ? null : item)
On 2016/03/23 18:34:17, skybrian wrote:
> On 2016/03/23 18:09:59, Leaf wrote:
> > On 2016/03/23 17:20:24, vsm wrote:
> > > Stream.map is a generic method.  Strong mode will try to infer the type,
but
> > > it's failing here.
> > > 
> > > Can you try map<String>(...) and dropping the cast?  The cast will fail at
> > > runtime in DDC (it'd try to cast Stream<Object> to Stream<String>).
> > 
> > I don't understand why inference is failing here.  In a standalone repro it
> > works.  What version are you using?  Command line or Intellij?  There's an
> > intellij regression in 1.16.0.dev-2.0 that breaks strong mode generics - be
> sure
> > you're not using that.
> 
> I'm using Atom with an SDK built at HEAD. I think this is another instance of:
> https://github.com/dart-lang/sdk/issues/26062
> 
> Hopefully by the time I get back to this CL it will be fixed.
> 
> 

It works now. Reverted.

https://chromiumcodereview.appspot.com/1829573002/diff/1/benchmark/lib/dashbo...
benchmark/lib/dashboard_view.dart:246: Stream<String> get onChange =>
_changes.stream as Stream<String>;
On 2016/03/23 18:09:59, Leaf wrote:
> On 2016/03/23 17:20:24, vsm wrote:
> > This will fail at runtime if the runtime type of _changes.stream is not a
> > Stream<String>.  Instead, I think we need above:
> > 
> > final _changes = new StreamController<String>.broadcast();
> > 
> > and drop the cast.
> 
> Agreed.

Done

https://chromiumcodereview.appspot.com/1829573002/diff/1/lib/file_generator.dart
File lib/file_generator.dart (right):

https://chromiumcodereview.appspot.com/1829573002/diff/1/lib/file_generator.d...
lib/file_generator.dart:123: throw ("FAILURE: File with an absolute path is not
supported");
On 2016/03/29 09:04:13, Søren Gjesse wrote:
> Remove the () here.

Done.

https://chromiumcodereview.appspot.com/1829573002/diff/1/lib/file_generator.d...
lib/file_generator.dart:207: throw ("FAILURE: Import with absolute path is not
supported");
On 2016/03/29 09:04:13, Søren Gjesse wrote:
> Remove the () here.

Done.

https://chromiumcodereview.appspot.com/1829573002/diff/1/lib/src/descriptor.p...
File lib/src/descriptor.pb.dart (right):

https://chromiumcodereview.appspot.com/1829573002/diff/1/lib/src/descriptor.p...
lib/src/descriptor.pb.dart:4: library proto2_descriptor;
On 2016/03/29 09:04:13, Søren Gjesse wrote:
> What is the reason for editing the generated code? Shouldn't the generator be
> fixed instead?

Yes, this is generated code, but it's also checked in. I regenerated it.

https://chromiumcodereview.appspot.com/1829573002/diff/1/lib/src/descriptor.p...
lib/src/descriptor.pb.dart:30: List<FileDescriptorProto> get file => $_get(0, 1,
null) as List<FileDescriptorProto>;
On 2016/03/23 18:34:17, skybrian wrote:
> > We don't support generic getters and setters.
> 
> I don't know what you mean by that? _get isn't a getter.
> 
> > List<FileDescriptor>.from($_get(0, 1, null)?
> 
> Doesn't work - the whole point is that we're returning a mutable  list. Also,
> it's important to preserve the subtype of List.

$_get() is now a generic method.  At runtime the list should have the right
(reified) type, but this is hard to prove statically.

[Leaf, 2016-04-01 18:13:52]

https://chromiumcodereview.appspot.com/1829573002/diff/1/benchmark/lib/dashbo...
File benchmark/lib/dashboard_model.dart (right):

https://chromiumcodereview.appspot.com/1829573002/diff/1/benchmark/lib/dashbo...
benchmark/lib/dashboard_model.dart:47: Iterator it = report == null ?
[].iterator : report.responses.iterator;
On 2016/04/01 02:00:15, skybrian wrote:
> On 2016/03/23 18:09:59, Leaf wrote:
> > On 2016/03/23 17:20:24, vsm wrote:
> > > Interesting that we're not inferring this type.
> > 
> > Probably LUB.
> 
> What does LUB mean?

Sorry - Least Upper Bound.  Dart's least upper bound computation isn't very
useful to us, and often gives surprising results.  I've recently checked in
something which fixes the low-hanging fruit - you could try and see if we now
infer a useful type.

https://chromiumcodereview.appspot.com/1829573002/diff/40001/lib/src/descript...
File lib/src/descriptor.pb.dart (right):

https://chromiumcodereview.appspot.com/1829573002/diff/40001/lib/src/descript...
lib/src/descriptor.pb.dart:66: String get name => $_get/*<String>*/(0, 1, '');
It's fine to make this explicit if preferred, but inference should fill in the
type argument for almost all of these calls if you want to leave it off.

[skybrian, 2016-04-01 19:47:48]

https://chromiumcodereview.appspot.com/1829573002/diff/40001/lib/src/descript...
File lib/src/descriptor.pb.dart (right):

https://chromiumcodereview.appspot.com/1829573002/diff/40001/lib/src/descript...
lib/src/descriptor.pb.dart:66: String get name => $_get/*<String>*/(0, 1, '');
On 2016/04/01 18:13:51, Leaf wrote:
> It's fine to make this explicit if preferred, but inference should fill in the
> type argument for almost all of these calls if you want to leave it off.

I started up Atom today and I'm getting "Unsound implicit cast from dynamic to
List<Request>" for code like this:

  List<Request> get requests => $_get/*<List<Request>>*/(0, 1, null);

It appears that there are analyzer bugs such that strong mode warnings don't
show up until you run "reanalyze sources" (or sometimes they disappear when you
do that) so I'm not sure what to think.

Since this error doesn't go away when I reanalyze sources, I think I need to use
"as" here?

[Leaf, 2016-04-01 19:57:22]

https://chromiumcodereview.appspot.com/1829573002/diff/40001/lib/src/descript...
File lib/src/descriptor.pb.dart (right):

https://chromiumcodereview.appspot.com/1829573002/diff/40001/lib/src/descript...
lib/src/descriptor.pb.dart:66: String get name => $_get/*<String>*/(0, 1, '');
On 2016/04/01 19:47:47, skybrian wrote:
> On 2016/04/01 18:13:51, Leaf wrote:
> > It's fine to make this explicit if preferred, but inference should fill in
the
> > type argument for almost all of these calls if you want to leave it off.
> 
> I started up Atom today and I'm getting "Unsound implicit cast from dynamic to
> List<Request>" for code like this:
> 
>   List<Request> get requests => $_get/*<List<Request>>*/(0, 1, null);
> 
> It appears that there are analyzer bugs such that strong mode warnings don't
> show up until you run "reanalyze sources" (or sometimes they disappear when
you
> do that) so I'm not sure what to think.
> 
> Since this error doesn't go away when I reanalyze sources, I think I need to
use
> "as" here?

This doesn't seem right.  As I recall the new $_get implementation, it returns
something of the same type as what its generic parameter is.  So there should be
no need for a cast here, and no warning.

The warning you are describing is a strong mode mode warning, so strong mode is
on... I'm not sure what's going on.  Could you possibly have the wrong version
of the protobuf library that defines $_get?  That is, one without the generic
method?  I would expect that if you did you would also see a warning about
passing 1 type argument where 0 was expected.

[skybrian, 2016-04-01 20:11:45]

https://chromiumcodereview.appspot.com/1829573002/diff/40001/lib/src/descript...
File lib/src/descriptor.pb.dart (right):

https://chromiumcodereview.appspot.com/1829573002/diff/40001/lib/src/descript...
lib/src/descriptor.pb.dart:66: String get name => $_get/*<String>*/(0, 1, '');
On 2016/04/01 19:57:22, Leaf wrote:
> On 2016/04/01 19:47:47, skybrian wrote:
> > On 2016/04/01 18:13:51, Leaf wrote:
> > > It's fine to make this explicit if preferred, but inference should fill in
> the
> > > type argument for almost all of these calls if you want to leave it off.
> > 
> > I started up Atom today and I'm getting "Unsound implicit cast from dynamic
to
> > List<Request>" for code like this:
> > 
> >   List<Request> get requests => $_get/*<List<Request>>*/(0, 1, null);
> > 
> > It appears that there are analyzer bugs such that strong mode warnings don't
> > show up until you run "reanalyze sources" (or sometimes they disappear when
> you
> > do that) so I'm not sure what to think.
> > 
> > Since this error doesn't go away when I reanalyze sources, I think I need to
> use
> > "as" here?
> 
> This doesn't seem right.  As I recall the new $_get implementation, it returns
> something of the same type as what its generic parameter is.  So there should
be
> no need for a cast here, and no warning.
> 
> The warning you are describing is a strong mode mode warning, so strong mode
is
> on... I'm not sure what's going on.  Could you possibly have the wrong version
> of the protobuf library that defines $_get?  That is, one without the generic
> method?  I would expect that if you did you would also see a warning about
> passing 1 type argument where 0 was expected.  

No, it's the right one. Code navigation takes me to:

  /*T*/ $_get/*<T>*/(int index, int tagNumber, /*T*/ defaultValue) =>
      _fieldSet._$get/*<T>*/(index, tagNumber, defaultValue);

The third argument in this case is null, so it couldn't infer anything from
that. I tried changing it to "null as List<Request>" but it doesn't help. I
think I'm going to use "as".

[Leaf, 2016-04-01 20:22:18]

https://chromiumcodereview.appspot.com/1829573002/diff/40001/lib/src/descript...
File lib/src/descriptor.pb.dart (right):

https://chromiumcodereview.appspot.com/1829573002/diff/40001/lib/src/descript...
lib/src/descriptor.pb.dart:66: String get name => $_get/*<String>*/(0, 1, '');
On 2016/04/01 20:11:45, skybrian wrote:
> On 2016/04/01 19:57:22, Leaf wrote:
> > On 2016/04/01 19:47:47, skybrian wrote:
> > > On 2016/04/01 18:13:51, Leaf wrote:
> > > > It's fine to make this explicit if preferred, but inference should fill
in
> > the
> > > > type argument for almost all of these calls if you want to leave it off.
> > > 
> > > I started up Atom today and I'm getting "Unsound implicit cast from
dynamic
> to
> > > List<Request>" for code like this:
> > > 
> > >   List<Request> get requests => $_get/*<List<Request>>*/(0, 1, null);
> > > 
> > > It appears that there are analyzer bugs such that strong mode warnings
don't
> > > show up until you run "reanalyze sources" (or sometimes they disappear
when
> > you
> > > do that) so I'm not sure what to think.
> > > 
> > > Since this error doesn't go away when I reanalyze sources, I think I need
to
> > use
> > > "as" here?
> > 
> > This doesn't seem right.  As I recall the new $_get implementation, it
returns
> > something of the same type as what its generic parameter is.  So there
should
> be
> > no need for a cast here, and no warning.
> > 
> > The warning you are describing is a strong mode mode warning, so strong mode
> is
> > on... I'm not sure what's going on.  Could you possibly have the wrong
version
> > of the protobuf library that defines $_get?  That is, one without the
generic
> > method?  I would expect that if you did you would also see a warning about
> > passing 1 type argument where 0 was expected.  
> 
> No, it's the right one. Code navigation takes me to:
> 
>   /*T*/ $_get/*<T>*/(int index, int tagNumber, /*T*/ defaultValue) =>
>       _fieldSet._$get/*<T>*/(index, tagNumber, defaultValue);
> 
> The third argument in this case is null, so it couldn't infer anything from
> that. I tried changing it to "null as List<Request>" but it doesn't help. I
> think I'm going to use "as".

I'm confused - if the code looks like this:

  List<Request> get requests => $_get/*<List<Request>>*/(0, 1, null);

then there's no inference happening.  There definitely shouldn't be a need for a
cast.

[skybrian, 2016-04-01 20:35:03]

On 2016/04/01 20:22:18, Leaf wrote:
>
https://chromiumcodereview.appspot.com/1829573002/diff/40001/lib/src/descript...
> File lib/src/descriptor.pb.dart (right):
> 
>
https://chromiumcodereview.appspot.com/1829573002/diff/40001/lib/src/descript...
> lib/src/descriptor.pb.dart:66: String get name => $_get/*<String>*/(0, 1, '');
> On 2016/04/01 20:11:45, skybrian wrote:
> > On 2016/04/01 19:57:22, Leaf wrote:
> > > On 2016/04/01 19:47:47, skybrian wrote:
> > > > On 2016/04/01 18:13:51, Leaf wrote:
> > > > > It's fine to make this explicit if preferred, but inference should
fill
> in
> > > the
> > > > > type argument for almost all of these calls if you want to leave it
off.
> > > > 
> > > > I started up Atom today and I'm getting "Unsound implicit cast from
> dynamic
> > to
> > > > List<Request>" for code like this:
> > > > 
> > > >   List<Request> get requests => $_get/*<List<Request>>*/(0, 1, null);
> > > > 
> > > > It appears that there are analyzer bugs such that strong mode warnings
> don't
> > > > show up until you run "reanalyze sources" (or sometimes they disappear
> when
> > > you
> > > > do that) so I'm not sure what to think.
> > > > 
> > > > Since this error doesn't go away when I reanalyze sources, I think I
need
> to
> > > use
> > > > "as" here?
> > > 
> > > This doesn't seem right.  As I recall the new $_get implementation, it
> returns
> > > something of the same type as what its generic parameter is.  So there
> should
> > be
> > > no need for a cast here, and no warning.
> > > 
> > > The warning you are describing is a strong mode mode warning, so strong
mode
> > is
> > > on... I'm not sure what's going on.  Could you possibly have the wrong
> version
> > > of the protobuf library that defines $_get?  That is, one without the
> generic
> > > method?  I would expect that if you did you would also see a warning about
> > > passing 1 type argument where 0 was expected.  
> > 
> > No, it's the right one. Code navigation takes me to:
> > 
> >   /*T*/ $_get/*<T>*/(int index, int tagNumber, /*T*/ defaultValue) =>
> >       _fieldSet._$get/*<T>*/(index, tagNumber, defaultValue);
> > 
> > The third argument in this case is null, so it couldn't infer anything from
> > that. I tried changing it to "null as List<Request>" but it doesn't help. I
> > think I'm going to use "as".
> 
> I'm confused - if the code looks like this:
> 
>   List<Request> get requests => $_get/*<List<Request>>*/(0, 1, null);
> 
> then there's no inference happening.  There definitely shouldn't be a need for
a
> cast.

It seems to have something to do with the getter's return type not being within
a comment.
(That is, it's also the return type in standards mode.) If I put the return type
in a comment,
the warning goes away.

(But I'm not sure whether I've just commented out the type.)

[skybrian, 2016-04-01 20:45:51]

On 2016/04/01 20:35:03, skybrian wrote:
> On 2016/04/01 20:22:18, Leaf wrote:
> >
>
https://chromiumcodereview.appspot.com/1829573002/diff/40001/lib/src/descript...
> > File lib/src/descriptor.pb.dart (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/1829573002/diff/40001/lib/src/descript...
> > lib/src/descriptor.pb.dart:66: String get name => $_get/*<String>*/(0, 1,
'');
> > On 2016/04/01 20:11:45, skybrian wrote:
> > > On 2016/04/01 19:57:22, Leaf wrote:
> > > > On 2016/04/01 19:47:47, skybrian wrote:
> > > > > On 2016/04/01 18:13:51, Leaf wrote:
> > > > > > It's fine to make this explicit if preferred, but inference should
> fill
> > in
> > > > the
> > > > > > type argument for almost all of these calls if you want to leave it
> off.
> > > > > 
> > > > > I started up Atom today and I'm getting "Unsound implicit cast from
> > dynamic
> > > to
> > > > > List<Request>" for code like this:
> > > > > 
> > > > >   List<Request> get requests => $_get/*<List<Request>>*/(0, 1, null);
> > > > > 
> > > > > It appears that there are analyzer bugs such that strong mode warnings
> > don't
> > > > > show up until you run "reanalyze sources" (or sometimes they disappear
> > when
> > > > you
> > > > > do that) so I'm not sure what to think.
> > > > > 
> > > > > Since this error doesn't go away when I reanalyze sources, I think I
> need
> > to
> > > > use
> > > > > "as" here?
> > > > 
> > > > This doesn't seem right.  As I recall the new $_get implementation, it
> > returns
> > > > something of the same type as what its generic parameter is.  So there
> > should
> > > be
> > > > no need for a cast here, and no warning.
> > > > 
> > > > The warning you are describing is a strong mode mode warning, so strong
> mode
> > > is
> > > > on... I'm not sure what's going on.  Could you possibly have the wrong
> > version
> > > > of the protobuf library that defines $_get?  That is, one without the
> > generic
> > > > method?  I would expect that if you did you would also see a warning
about
> > > > passing 1 type argument where 0 was expected.  
> > > 
> > > No, it's the right one. Code navigation takes me to:
> > > 
> > >   /*T*/ $_get/*<T>*/(int index, int tagNumber, /*T*/ defaultValue) =>
> > >       _fieldSet._$get/*<T>*/(index, tagNumber, defaultValue);
> > > 
> > > The third argument in this case is null, so it couldn't infer anything
from
> > > that. I tried changing it to "null as List<Request>" but it doesn't help.
I
> > > think I'm going to use "as".
> > 
> > I'm confused - if the code looks like this:
> > 
> >   List<Request> get requests => $_get/*<List<Request>>*/(0, 1, null);
> > 
> > then there's no inference happening.  There definitely shouldn't be a need
for
> a
> > cast.
> 
> It seems to have something to do with the getter's return type not being
within
> a comment.
> (That is, it's also the return type in standards mode.) If I put the return
type
> in a comment,
> the warning goes away.
> 
> (But I'm not sure whether I've just commented out the type.)

For what it's worth, if I change it to:

  List<Request> get requests => $_get(0, 1, null);

Or even:

  List<Request> get requests => $_get/*<int>*/(0, 1, null);

I get the same warning. So I don't think the type declaration on the $_get call
is having any effect.

I also tried changing the $_get declaration to:

  /*XYZ*/ $_get/*<T>*/(int index, int tagNumber, /*T*/ defaultValue) =>
      _fieldSet._$get/*<T>*/(index, tagNumber, defaultValue);

And no warning is reported even though XYZ isn't defined.

[Leaf, 2016-04-01 20:50:19]

https://chromiumcodereview.appspot.com/1829573002/diff/40001/lib/src/descript...
File lib/src/descriptor.pb.dart (right):

https://chromiumcodereview.appspot.com/1829573002/diff/40001/lib/src/descript...
lib/src/descriptor.pb.dart:66: String get name => $_get/*<String>*/(0, 1, '');
On 2016/04/01 20:22:18, Leaf wrote:
> On 2016/04/01 20:11:45, skybrian wrote:
> > On 2016/04/01 19:57:22, Leaf wrote:
> > > On 2016/04/01 19:47:47, skybrian wrote:
> > > > On 2016/04/01 18:13:51, Leaf wrote:
> > > > > It's fine to make this explicit if preferred, but inference should
fill
> in
> > > the
> > > > > type argument for almost all of these calls if you want to leave it
off.
> > > > 
> > > > I started up Atom today and I'm getting "Unsound implicit cast from
> dynamic
> > to
> > > > List<Request>" for code like this:
> > > > 
> > > >   List<Request> get requests => $_get/*<List<Request>>*/(0, 1, null);
> > > > 
> > > > It appears that there are analyzer bugs such that strong mode warnings
> don't
> > > > show up until you run "reanalyze sources" (or sometimes they disappear
> when
> > > you
> > > > do that) so I'm not sure what to think.
> > > > 
> > > > Since this error doesn't go away when I reanalyze sources, I think I
need
> to
> > > use
> > > > "as" here?
> > > 
> > > This doesn't seem right.  As I recall the new $_get implementation, it
> returns
> > > something of the same type as what its generic parameter is.  So there
> should
> > be
> > > no need for a cast here, and no warning.
> > > 
> > > The warning you are describing is a strong mode mode warning, so strong
mode
> > is
> > > on... I'm not sure what's going on.  Could you possibly have the wrong
> version
> > > of the protobuf library that defines $_get?  That is, one without the
> generic
> > > method?  I would expect that if you did you would also see a warning about
> > > passing 1 type argument where 0 was expected.  
> > 
> > No, it's the right one. Code navigation takes me to:
> > 
> >   /*T*/ $_get/*<T>*/(int index, int tagNumber, /*T*/ defaultValue) =>
> >       _fieldSet._$get/*<T>*/(index, tagNumber, defaultValue);
> > 
> > The third argument in this case is null, so it couldn't infer anything from
> > that. I tried changing it to "null as List<Request>" but it doesn't help. I
> > think I'm going to use "as".
> 
> I'm confused - if the code looks like this:
> 
>   List<Request> get requests => $_get/*<List<Request>>*/(0, 1, null);
> 
> then there's no inference happening.  There definitely shouldn't be a need for
a
> cast.

Aargh.  This is wrong, sorry I missed this:

  /*T*/ $_get/*<T>*/(int index, int tagNumber, /*T*/ defaultValue) =>
      _fieldSet._$get/*<T>*/(index, tagNumber, defaultValue);

All of the /*T*/ instances need to be /*=T*/

This is the problem.

[skybrian, 2016-04-01 21:47:35]

Thanks. As you'd expect, I'm getting a lot more warnings now.

I don't know how to write this code:

  /// The implementation of a generated getter.
  /*=T*/ _$get/*<T>*/(int index, int tagNumber, /*=T*/ defaultValue) {
    assert(_nonExtensionInfo(tagNumber).index == index);
    var value = _values[index];
    if (value != null) return value;
    if (defaultValue != null) return defaultValue;
    return _getDefault(_nonExtensionInfo(tagNumber));
  }

I'm getting "Unsound implicit cast from dynamic to T". The "value" variable has
type dynamic. I know that at runtime it will always have the correct type. How
do I cast it to T?

[Leaf, 2016-04-01 22:08:50]

On 2016/04/01 21:47:35, skybrian wrote:
> Thanks. As you'd expect, I'm getting a lot more warnings now.
> 
> I don't know how to write this code:
> 
>   /// The implementation of a generated getter.
>   /*=T*/ _$get/*<T>*/(int index, int tagNumber, /*=T*/ defaultValue) {
>     assert(_nonExtensionInfo(tagNumber).index == index);
>     var value = _values[index];
>     if (value != null) return value;
>     if (defaultValue != null) return defaultValue;
>     return _getDefault(_nonExtensionInfo(tagNumber));
>   }
> 
> I'm getting "Unsound implicit cast from dynamic to T". The "value" variable
has
> type dynamic. I know that at runtime it will always have the correct type. How
> do I cast it to T?

(value as T) should work.

Or 

var value = _values[index] as T;

[skybrian, 2016-04-01 22:12:39]

On 2016/04/01 22:08:50, Leaf wrote:
> On 2016/04/01 21:47:35, skybrian wrote:
> > Thanks. As you'd expect, I'm getting a lot more warnings now.
> > 
> > I don't know how to write this code:
> > 
> >   /// The implementation of a generated getter.
> >   /*=T*/ _$get/*<T>*/(int index, int tagNumber, /*=T*/ defaultValue) {
> >     assert(_nonExtensionInfo(tagNumber).index == index);
> >     var value = _values[index];
> >     if (value != null) return value;
> >     if (defaultValue != null) return defaultValue;
> >     return _getDefault(_nonExtensionInfo(tagNumber));
> >   }
> > 
> > I'm getting "Unsound implicit cast from dynamic to T". The "value" variable
> has
> > type dynamic. I know that at runtime it will always have the correct type.
How
> > do I cast it to T?
> 
> (value as T) should work.
> 
> Or 
> 
> var value = _values[index] as T;

I don't think that will work when not in strong mode (T won't be defined).

It seems this will do it though:

// ignore: strong_mode_down_cast_composite

[skybrian, 2016-04-01 23:09:17]

https://chromiumcodereview.appspot.com/1829573002/diff/40001/lib/src/descript...
File lib/src/descriptor.pb.dart (right):

https://chromiumcodereview.appspot.com/1829573002/diff/40001/lib/src/descript...
lib/src/descriptor.pb.dart:66: String get name => $_get/*<String>*/(0, 1, '');
On 2016/04/01 18:13:51, Leaf wrote:
> It's fine to make this explicit if preferred, but inference should fill in the
> type argument for almost all of these calls if you want to leave it off.

Removed it and regenerated the .pb.dart files

[Leaf, 2016-04-01 23:19:42]

On 2016/04/01 22:12:39, skybrian wrote:
> On 2016/04/01 22:08:50, Leaf wrote:
> > On 2016/04/01 21:47:35, skybrian wrote:
> > > Thanks. As you'd expect, I'm getting a lot more warnings now.
> > > 
> > > I don't know how to write this code:
> > > 
> > >   /// The implementation of a generated getter.
> > >   /*=T*/ _$get/*<T>*/(int index, int tagNumber, /*=T*/ defaultValue) {
> > >     assert(_nonExtensionInfo(tagNumber).index == index);
> > >     var value = _values[index];
> > >     if (value != null) return value;
> > >     if (defaultValue != null) return defaultValue;
> > >     return _getDefault(_nonExtensionInfo(tagNumber));
> > >   }
> > > 
> > > I'm getting "Unsound implicit cast from dynamic to T". The "value"
variable
> > has
> > > type dynamic. I know that at runtime it will always have the correct type.
> How
> > > do I cast it to T?
> > 
> > (value as T) should work.
> > 
> > Or 
> > 
> > var value = _values[index] as T;
> 
> I don't think that will work when not in strong mode (T won't be defined).
> 
> It seems this will do it though:
> 
> // ignore: strong_mode_down_cast_composite

Should have been:

(value as dynamic /*=T*/) 

or similar for _values[index'.

[skybrian, 2016-04-04 17:09:15]

Description was changed from

==========
Fix all strong mode warnings in protoc-plugin

Includes changes to the plugin, the generated code, the
benchmarks, and the tests.

Also, fix the benchmarks to work again. It has a hacky parser
for .pubspec-lock files that didn't work after the format changed.

Also, dartfmt modified files.
==========

to

==========
Fix all strong mode warnings in protoc-plugin

Includes changes to the plugin, the generated code, the
benchmarks, and the tests.

Also, fix the benchmarks to work again. It has a hacky parser
for .pubspec-lock files that didn't work after the format changed.

Also, dartfmt modified files.

Committed:
https://github.com/dart-lang/dart-protoc-plugin/commit/23a67b48fc1ad532e77863...
==========

[skybrian, 2016-04-04 17:09:17]

Committed patchset #5 (id:80001) manually as
23a67b48fc1ad532e7786345ce8b7b3adebd8f14 (presubmit successful).