DDC fixes for path

DDC fixes for path

R=rnystrom@google.com

Committed: https://github.com/dart-lang/path/commit/29a3784b7fa4b6db37f97f365a37b8790288e0da

[vsm, 2015-03-26 19:32:53]

vsm@google.com changed reviewers:
+ nweiz@google.com, rnystrom@google.com

[vsm, 2015-03-26 19:33:31]

https://codereview.chromium.org/1038063002/diff/1/lib/src/context.dart
File lib/src/context.dart (right):

https://codereview.chromium.org/1038063002/diff/1/lib/src/context.dart#newcode43
lib/src/context.dart:43: return new Context._(style, current);
Note, we get an implicit down cast warning on style here (to InternalStyle).  It
will always succeed though.

https://codereview.chromium.org/1038063002/diff/1/lib/src/context.dart#newcode48
lib/src/context.dart:48: : style = Style.platform,
Ditto as Style.platform is typed to return a Style, not an InternalStyle.

https://codereview.chromium.org/1038063002/diff/1/lib/src/context.dart#newcod...
lib/src/context.dart:200: var parts = <String>[part1, part2, part3, part4,
part5, part6, part7, part8];
Note: 
  List<String> parts = [ ... ]
would work as well here and elsewhere.

[nweiz, 2015-03-26 19:52:16]

I'm down with some "as" additions when downcasting, but adding type annotations
to every single list literal is a serious burden, as well as inconsistent with
our style guide that emphasizes no type annotations in function bodies. Can we
not do some sort of inference there?

[vsm, 2015-03-26 20:00:28]

On 2015/03/26 19:52:16, nweiz wrote:
> I'm down with some "as" additions when downcasting, but adding type
annotations
> to every single list literal is a serious burden, as well as inconsistent with
> our style guide that emphasizes no type annotations in function bodies. Can we
> not do some sort of inference there?

Not easily in these cases.  If we know it from immediate context, we'll infer
(e.g., List<String> l = ...), but that doesn't apply to these cases.  The
annotation here fixes a type issue downstream.

A more complex inference could determine this (here but not always), but we're
keeping any inference we're doing pretty simple.  It needs to be fast and
modular enough for a dev iteration cycle.

[Bob Nystrom, 2015-03-27 20:23:13]

> adding type annotations
to every single list literal is a serious burden, as well as inconsistent with
our style guide that emphasizes no type annotations in function bodies

I'm personally OK with typing my collection literals if it gets me better static
checking. This isn't technically a type annotation, it's specifying a property
of the *value*. I'd rather do that than annotate the local variable. I like
having a clean model that inference flows outwards from values into variables
instead of inwards from annotated variables to their initializers.

https://codereview.chromium.org/1038063002/diff/1/lib/src/context.dart
File lib/src/context.dart (right):

https://codereview.chromium.org/1038063002/diff/1/lib/src/context.dart#newcode43
lib/src/context.dart:43: return new Context._(style, current);
On 2015/03/26 19:33:31, vsm wrote:
> Note, we get an implicit down cast warning on style here (to InternalStyle). 
It
> will always succeed though.

Interesting. I'd be fine with explicit downcasting here. It's notable that this
is one of the known more odd corners of the path public API. We take in a public
type and require it to be an internal subclass.

So it's nice that ddc draws our attention to it.

[Bob Nystrom, 2015-03-27 20:24:04]

Do you intend to land this? If so, you'll need to update the changelog, bump the
version in the pubspec, etc.

https://codereview.chromium.org/1038063002/diff/1/lib/src/context.dart
File lib/src/context.dart (right):

https://codereview.chromium.org/1038063002/diff/1/lib/src/context.dart#newcod...
lib/src/context.dart:200: var parts = <String>[part1, part2, part3, part4,
part5, part6, part7, part8];
On 2015/03/26 19:33:31, vsm wrote:
> Note: 
>   List<String> parts = [ ... ]
> would work as well here and elsewhere.

I prefer how you did this.

[nweiz, 2015-03-27 21:30:36]

On 2015/03/27 20:23:13, Bob Nystrom wrote:
> I'm personally OK with typing my collection literals if it gets me better
static
> checking. This isn't technically a type annotation, it's specifying a property
> of the *value*. I'd rather do that than annotate the local variable. I like
> having a clean model that inference flows outwards from values into variables
> instead of inwards from annotated variables to their initializers.

Regardless of which way this goes, it won't affect the quality of the static
checking; the lists definitely need to have the correct type. What I'm
advocating for is getting good-quality static checking without needing extra
annotations.

[vsm, 2015-03-27 21:42:51]

PTAL

Issue opened on Natalie's suggestion:
 https://github.com/dart-lang/dev_compiler/issues/108

https://codereview.chromium.org/1038063002/diff/1/lib/src/context.dart
File lib/src/context.dart (right):

https://codereview.chromium.org/1038063002/diff/1/lib/src/context.dart#newcode43
lib/src/context.dart:43: return new Context._(style, current);
On 2015/03/27 20:23:13, Bob Nystrom wrote:
> On 2015/03/26 19:33:31, vsm wrote:
> > Note, we get an implicit down cast warning on style here (to InternalStyle).

> It
> > will always succeed though.
> 
> Interesting. I'd be fine with explicit downcasting here. It's notable that
this
> is one of the known more odd corners of the path public API. We take in a
public
> type and require it to be an internal subclass.
> 
> So it's nice that ddc draws our attention to it.

Done.

https://codereview.chromium.org/1038063002/diff/1/lib/src/context.dart#newcode48
lib/src/context.dart:48: : style = Style.platform,
On 2015/03/26 19:33:31, vsm wrote:
> Ditto as Style.platform is typed to return a Style, not an InternalStyle.

Added explicit cast.

[Bob Nystrom, 2015-03-27 22:05:41]

LGTM! Don't forget to publish it and tag it in Git. Let me know if you need
details on the latter.

https://codereview.chromium.org/1038063002/diff/20001/lib/src/context.dart
File lib/src/context.dart (right):

https://codereview.chromium.org/1038063002/diff/20001/lib/src/context.dart#ne...
lib/src/context.dart:200: var parts = <String>[part1, part2, part3, part4,
part5, part6, part7, part8];
This is *just* a bit too long now, so you'll have to make it all multi-line:

var parts = <String>[
  part1,
  part2,
 ...
];

[vsm, 2015-03-27 22:30:02]

https://codereview.chromium.org/1038063002/diff/20001/lib/src/context.dart
File lib/src/context.dart (right):

https://codereview.chromium.org/1038063002/diff/20001/lib/src/context.dart#ne...
lib/src/context.dart:200: var parts = <String>[part1, part2, part3, part4,
part5, part6, part7, part8];
On 2015/03/27 22:05:41, Bob Nystrom wrote:
> This is *just* a bit too long now, so you'll have to make it all multi-line:
> 
> var parts = <String>[
>   part1,
>   part2,
>  ...
> ];

Done.

[vsm, 2015-03-27 22:30:17]

Committed patchset #3 (id:40001) manually as
29a3784b7fa4b6db37f97f365a37b8790288e0da (presubmit successful).