Steps towards making the convert library strong-mode compliant.

Steps towards making the convert library strong-mode compliant.

This version has a few deprecated methods. In a future release they will be removed.

See https://codereview.chromium.org/1827803002 for the final patch (once the deprecated methods have been removed).

R=leafp@google.com, lrn@google.com

Committed: https://github.com/dart-lang/sdk/commit/4ab1219bf93a33a2a3d4803e053ccd70ce190371
Reverted: https://github.com/dart-lang/sdk/commit/045a3753665268761508c8053e0ee84372d9ef35

Committed: https://github.com/dart-lang/sdk/commit/50bdab3841dafd9b056b1dd013c103ce6a566f60

[floitsch, 2016-03-31 16:48:14]

Description was changed from

==========
Steps towards making the convert library strong-mode compliant.

This version has a few deprecated methods. In a future release they will be
removed.
==========

to

==========
Steps towards making the convert library strong-mode compliant.

This version has a few deprecated methods. In a future release they will be
removed.

See https://codereview.chromium.org/1827803002 for the final patch (once the
deprecated methods have been removed).
==========

[floitsch, 2016-03-31 16:49:04]

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

[Lasse Reichstein Nielsen, 2016-03-31 20:20:20]

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

[Lasse Reichstein Nielsen, 2016-03-31 20:20:21]

Seem correct (doesn't exactly look *good*, but LGTM)

https://chromiumcodereview.appspot.com/1847843002/diff/1/sdk/lib/convert/chun...
File sdk/lib/convert/chunked_conversion.dart (right):

https://chromiumcodereview.appspot.com/1847843002/diff/1/sdk/lib/convert/chun...
sdk/lib/convert/chunked_conversion.dart:12: // This class may be used as Mixin.
If this comments means anything, put it in the main comment.
That makes it public that the class is intended to be used as a mixin too.

https://chromiumcodereview.appspot.com/1847843002/diff/1/sdk/lib/convert/chun...
sdk/lib/convert/chunked_conversion.dart:13: abstract class ChunkedConverter<S,
T, S2, T2> extends Converter<S, T> {
Document S, T, S2 and T2 - what are they, how are they used.

https://chromiumcodereview.appspot.com/1847843002/diff/1/sdk/lib/convert/chun...
sdk/lib/convert/chunked_conversion.dart:15: const ChunkedConverter();
Hmm, are you sure it can be used as a mixin when it has a const constructor (or
any constructor)?

https://chromiumcodereview.appspot.com/1847843002/diff/1/sdk/lib/convert/chun...
sdk/lib/convert/chunked_conversion.dart:123: * For a non-chunked conversion
converts the input in sequence.
Something seems to be missing from this sentence.

https://chromiumcodereview.appspot.com/1847843002/diff/1/tests/lib/convert/ch...
File tests/lib/convert/chunked_conversion1_test.dart (right):

https://chromiumcodereview.appspot.com/1847843002/diff/1/tests/lib/convert/ch...
tests/lib/convert/chunked_conversion1_test.dart:113: return new
IdentitySink(sink);
Just return the input sink?

[floitsch, 2016-04-01 10:06:00]

Thanks Lasse.

https://chromiumcodereview.appspot.com/1847843002/diff/1/sdk/lib/convert/chun...
File sdk/lib/convert/chunked_conversion.dart (right):

https://chromiumcodereview.appspot.com/1847843002/diff/1/sdk/lib/convert/chun...
sdk/lib/convert/chunked_conversion.dart:12: // This class may be used as Mixin.
On 2016/03/31 20:20:21, Lasse Reichstein Nielsen wrote:
> If this comments means anything, put it in the main comment.
> That makes it public that the class is intended to be used as a mixin too.

Removed.

https://chromiumcodereview.appspot.com/1847843002/diff/1/sdk/lib/convert/chun...
sdk/lib/convert/chunked_conversion.dart:13: abstract class ChunkedConverter<S,
T, S2, T2> extends Converter<S, T> {
On 2016/03/31 20:20:21, Lasse Reichstein Nielsen wrote:
> Document S, T, S2 and T2 - what are they, how are they used.

Done.

https://chromiumcodereview.appspot.com/1847843002/diff/1/sdk/lib/convert/chun...
sdk/lib/convert/chunked_conversion.dart:15: const ChunkedConverter();
On 2016/03/31 20:20:21, Lasse Reichstein Nielsen wrote:
> Hmm, are you sure it can be used as a mixin when it has a const constructor
(or
> any constructor)?

My mistake. Of course it cannot.
That was my first (failed) attempt.

https://chromiumcodereview.appspot.com/1847843002/diff/1/sdk/lib/convert/chun...
sdk/lib/convert/chunked_conversion.dart:123: * For a non-chunked conversion
converts the input in sequence.
On 2016/03/31 20:20:21, Lasse Reichstein Nielsen wrote:
> Something seems to be missing from this sentence.

Removed this sentence.

https://chromiumcodereview.appspot.com/1847843002/diff/1/tests/lib/convert/ch...
File tests/lib/convert/chunked_conversion1_test.dart (right):

https://chromiumcodereview.appspot.com/1847843002/diff/1/tests/lib/convert/ch...
tests/lib/convert/chunked_conversion1_test.dart:113: return new
IdentitySink(sink);
On 2016/03/31 20:20:21, Lasse Reichstein Nielsen wrote:
> Just return the input sink?

Removed it, but then the test failed. I had added checks to make sure that the
sink is correctly wrapped.
I will keep as is. (Since it doesn't really change the test in any way).

[Leaf, 2016-04-01 15:58:03]

Couple of suggestions for consideration, but LGTM .

https://chromiumcodereview.appspot.com/1847843002/diff/20001/sdk/lib/convert/...
File sdk/lib/convert/chunked_conversion.dart (right):

https://chromiumcodereview.appspot.com/1847843002/diff/20001/sdk/lib/convert/...
sdk/lib/convert/chunked_conversion.dart:23: abstract class ChunkedConverter<S,
T, S2, T2> extends Converter<S, T> {
As far as I can tell, LineSplitter is the only example that uses the full
generality.  Would it be worth splitting this into two classes, something like:

GeneralChunkedConverter<S, T, S2, T2> extends Converter<S, T> etc
ChunkedConverter<S, T> extends Converter<S, T, S, T> {};

Or call the second one RegularChunkedConverter if preferred?    Would that work?
 Seems like then almost nobody needs to care about all the extra type machinery.

https://chromiumcodereview.appspot.com/1847843002/diff/20001/sdk/lib/convert/...
sdk/lib/convert/chunked_conversion.dart:51: Converter<S, dynamic>
fuse(Converter<T, dynamic> other) {
Should this be generic now?

Converter<S, dynamic /*=M*/> fuse/*<M>*/(Converter<T, dynamic/*=M*/> other)
{...?

[floitsch, 2016-04-01 16:27:31]

https://chromiumcodereview.appspot.com/1847843002/diff/20001/sdk/lib/convert/...
File sdk/lib/convert/chunked_conversion.dart (right):

https://chromiumcodereview.appspot.com/1847843002/diff/20001/sdk/lib/convert/...
sdk/lib/convert/chunked_conversion.dart:23: abstract class ChunkedConverter<S,
T, S2, T2> extends Converter<S, T> {
On 2016/04/01 15:58:03, Leaf wrote:
> As far as I can tell, LineSplitter is the only example that uses the full
> generality.  Would it be worth splitting this into two classes, something
like:
> 
> GeneralChunkedConverter<S, T, S2, T2> extends Converter<S, T> etc
> ChunkedConverter<S, T> extends Converter<S, T, S, T> {};
> 
> Or call the second one RegularChunkedConverter if preferred?    Would that
work?
>  Seems like then almost nobody needs to care about all the extra type
machinery.

I don't want to add a new class just because the converters we have happen to
follow a specific pattern.

https://chromiumcodereview.appspot.com/1847843002/diff/20001/sdk/lib/convert/...
sdk/lib/convert/chunked_conversion.dart:51: Converter<S, dynamic>
fuse(Converter<T, dynamic> other) {
On 2016/04/01 15:58:03, Leaf wrote:
> Should this be generic now?
> 
> Converter<S, dynamic /*=M*/> fuse/*<M>*/(Converter<T, dynamic/*=M*/> other)
> {...?

Yes. But not in this CL.

[floitsch, 2016-04-11 17:16:09]

Description was changed from

==========
Steps towards making the convert library strong-mode compliant.

This version has a few deprecated methods. In a future release they will be
removed.

See https://codereview.chromium.org/1827803002 for the final patch (once the
deprecated methods have been removed).
==========

to

==========
Steps towards making the convert library strong-mode compliant.

This version has a few deprecated methods. In a future release they will be
removed.

See https://codereview.chromium.org/1827803002 for the final patch (once the
deprecated methods have been removed).

R=leafp@google.com, lrn@google.com

Committed:
https://github.com/dart-lang/sdk/commit/4ab1219bf93a33a2a3d4803e053ccd70ce190371
==========

[floitsch, 2016-04-11 17:16:11]

Committed patchset #2 (id:20001) manually as
4ab1219bf93a33a2a3d4803e053ccd70ce190371 (presubmit successful).

[floitsch, 2016-04-11 17:54:27]

Description was changed from

==========
Steps towards making the convert library strong-mode compliant.

This version has a few deprecated methods. In a future release they will be
removed.

See https://codereview.chromium.org/1827803002 for the final patch (once the
deprecated methods have been removed).

R=leafp@google.com, lrn@google.com

Committed:
https://github.com/dart-lang/sdk/commit/4ab1219bf93a33a2a3d4803e053ccd70ce190371
==========

to

==========
Steps towards making the convert library strong-mode compliant.

This version has a few deprecated methods. In a future release they will be
removed.

See https://codereview.chromium.org/1827803002 for the final patch (once the
deprecated methods have been removed).

R=leafp@google.com, lrn@google.com

Committed:
https://github.com/dart-lang/sdk/commit/4ab1219bf93a33a2a3d4803e053ccd70ce190371
Reverted:
https://github.com/dart-lang/sdk/commit/045a3753665268761508c8053e0ee84372d9ef35
==========

[floitsch, 2016-04-11 18:37:53]

Had to revert.

Please have a look at the code that is making problems. I'm not yet sure how to
solve it.

https://codereview.chromium.org/1847843002/diff/40001/sdk/lib/convert/chunked...
File sdk/lib/convert/chunked_conversion.dart (right):

https://codereview.chromium.org/1847843002/diff/40001/sdk/lib/convert/chunked...
sdk/lib/convert/chunked_conversion.dart:52: if (other is ChunkedConverter<T,
dynamic, T2, dynamic>) {
Looking at this again, I guess there is no way to make this strong-mode
compliant?
I would like to create the FusedChunkedConverter based on the dynamic type. The
only way I can see this working is using double-dispatch...

@Leaf: is there a way to do an 'is' check that doesn't look at the generic
types?
In that case I could write:
if (other is ChunkedConverter) {
  return other._fuseFromOther<S, T, S2, T2>(this);
}

_fuseFromOther<otherS, otherT, otherS2, otherT2>(ChunkedConverter<otherS,
otherT, otherS2, otherT2> other) {
  if (this is ChunkedConverter<otherT, T, otherT2, T2>) {
    return new _FusedChunkedConverter<otherS, T, otherS2, T2>(other, this);
  }
}

This would require users to extend the ChunkedConverter (instead of just
implementing the interface). Otherwise we could make that function public.

https://codereview.chromium.org/1847843002/diff/40001/sdk/lib/convert/chunked...
sdk/lib/convert/chunked_conversion.dart:128: ChunkedConverter<dynamic, dynamic,
S, T> converter, EventSink<T> sink)
This is the line that broke the build. Since not all converters have been
migrated yet external converters weren't implementing the ChunkedConverter
interface.

I had to revert to "Converter" here (as is on the left).
I could, for strong mode, make it a `Converter/*=ChunkedConverter<dynamic,
dynamic, S, T>*/ instead.

However, I just realized that the 'dynamic' would break in strong mode (even if
it's unused). I guess I will have to refactor this library again...

[Leaf, 2016-04-11 19:46:28]

https://codereview.chromium.org/1847843002/diff/40001/sdk/lib/convert/chunked...
File sdk/lib/convert/chunked_conversion.dart (right):

https://codereview.chromium.org/1847843002/diff/40001/sdk/lib/convert/chunked...
sdk/lib/convert/chunked_conversion.dart:52: if (other is ChunkedConverter<T,
dynamic, T2, dynamic>) {
On 2016/04/11 18:37:53, floitsch wrote:
> Looking at this again, I guess there is no way to make this strong-mode
> compliant?
> I would like to create the FusedChunkedConverter based on the dynamic type.
The
> only way I can see this working is using double-dispatch...
> 
> @Leaf: is there a way to do an 'is' check that doesn't look at the generic
> types?
> In that case I could write:
> if (other is ChunkedConverter) {
>   return other._fuseFromOther<S, T, S2, T2>(this);
> }
> 
> _fuseFromOther<otherS, otherT, otherS2, otherT2>(ChunkedConverter<otherS,
> otherT, otherS2, otherT2> other) {
>   if (this is ChunkedConverter<otherT, T, otherT2, T2>) {
>     return new _FusedChunkedConverter<otherS, T, otherS2, T2>(other, this);
>   }
> }
> 
> This would require users to extend the ChunkedConverter (instead of just
> implementing the interface). Otherwise we could make that function public.

I don't see anything in this code that is directly not strong-mode compliant
(but may be missing something).  Is the concern that this returns something
whose dynamic type is Converter<S, dynamic>, and that consequently strong mode
will not allow the result to be used as a Converter<S, SomethingConcrete>? 
That, I think is an issue.

I'm not sure quite what you mean by and 'is' check that doesn't look at the
generic types.  "other is ChunkedConverter" in some sense doesn't look at the
generic types, in that it will return true for any instantiation of
ChunkedConverter.  Strong mode generic classes are still covariant.  Is that not
what you want?

https://codereview.chromium.org/1847843002/diff/40001/sdk/lib/convert/chunked...
sdk/lib/convert/chunked_conversion.dart:128: ChunkedConverter<dynamic, dynamic,
S, T> converter, EventSink<T> sink)
On 2016/04/11 18:37:53, floitsch wrote:
> This is the line that broke the build. Since not all converters have been
> migrated yet external converters weren't implementing the ChunkedConverter
> interface.
> 
> I had to revert to "Converter" here (as is on the left).
> I could, for strong mode, make it a `Converter/*=ChunkedConverter<dynamic,
> dynamic, S, T>*/ instead.
> 
> However, I just realized that the 'dynamic' would break in strong mode (even
if
> it's unused). I guess I will have to refactor this library again...

I'm probably just off to a slow start this week, but not sure I see the issue
with dynamic here.  Can you elaborate?

[floitsch, 2016-04-12 15:28:57]

https://codereview.chromium.org/1847843002/diff/40001/sdk/lib/convert/chunked...
File sdk/lib/convert/chunked_conversion.dart (right):

https://codereview.chromium.org/1847843002/diff/40001/sdk/lib/convert/chunked...
sdk/lib/convert/chunked_conversion.dart:52: if (other is ChunkedConverter<T,
dynamic, T2, dynamic>) {
On 2016/04/11 19:46:27, Leaf wrote:
> On 2016/04/11 18:37:53, floitsch wrote:
> > Looking at this again, I guess there is no way to make this strong-mode
> > compliant?
> > I would like to create the FusedChunkedConverter based on the dynamic type.
> The
> > only way I can see this working is using double-dispatch...
> > 
> > @Leaf: is there a way to do an 'is' check that doesn't look at the generic
> > types?
> > In that case I could write:
> > if (other is ChunkedConverter) {
> >   return other._fuseFromOther<S, T, S2, T2>(this);
> > }
> > 
> > _fuseFromOther<otherS, otherT, otherS2, otherT2>(ChunkedConverter<otherS,
> > otherT, otherS2, otherT2> other) {
> >   if (this is ChunkedConverter<otherT, T, otherT2, T2>) {
> >     return new _FusedChunkedConverter<otherS, T, otherS2, T2>(other, this);
> >   }
> > }
> > 
> > This would require users to extend the ChunkedConverter (instead of just
> > implementing the interface). Otherwise we could make that function public.
> 
> I don't see anything in this code that is directly not strong-mode compliant
> (but may be missing something).  Is the concern that this returns something
> whose dynamic type is Converter<S, dynamic>, and that consequently strong mode
> will not allow the result to be used as a Converter<S, SomethingConcrete>? 
> That, I think is an issue.
> 
> I'm not sure quite what you mean by and 'is' check that doesn't look at the
> generic types.  "other is ChunkedConverter" in some sense doesn't look at the
> generic types, in that it will return true for any instantiation of
> ChunkedConverter.  Strong mode generic classes are still covariant.  Is that
not
> what you want?
> 

My mistake. Somehow I was convinced that strong mode was invariant.

https://codereview.chromium.org/1847843002/diff/40001/sdk/lib/convert/chunked...
sdk/lib/convert/chunked_conversion.dart:128: ChunkedConverter<dynamic, dynamic,
S, T> converter, EventSink<T> sink)
On 2016/04/11 19:46:27, Leaf wrote:
> On 2016/04/11 18:37:53, floitsch wrote:
> > This is the line that broke the build. Since not all converters have been
> > migrated yet external converters weren't implementing the ChunkedConverter
> > interface.
> > 
> > I had to revert to "Converter" here (as is on the left).
> > I could, for strong mode, make it a `Converter/*=ChunkedConverter<dynamic,
> > dynamic, S, T>*/ instead.
> > 
> > However, I just realized that the 'dynamic' would break in strong mode (even
> if
> > it's unused). I guess I will have to refactor this library again...
> 
> I'm probably just off to a slow start this week, but not sure I see the issue
> with dynamic here.  Can you elaborate?

ditto.

[floitsch, 2016-04-12 16:32:05]

Relaxed the type and fixed the missed converters.
If you want, PTAL. I will commit soonish (but would do a follow-up CL if there
are comments).

[Leaf, 2016-04-12 17:44:31]

lgtm

https://codereview.chromium.org/1847843002/diff/40001/sdk/lib/convert/chunked...
File sdk/lib/convert/chunked_conversion.dart (right):

https://codereview.chromium.org/1847843002/diff/40001/sdk/lib/convert/chunked...
sdk/lib/convert/chunked_conversion.dart:52: if (other is ChunkedConverter<T,
dynamic, T2, dynamic>) {
On 2016/04/12 15:28:57, floitsch wrote:
> On 2016/04/11 19:46:27, Leaf wrote:
> > On 2016/04/11 18:37:53, floitsch wrote:
> > > Looking at this again, I guess there is no way to make this strong-mode
> > > compliant?
> > > I would like to create the FusedChunkedConverter based on the dynamic
type.
> > The
> > > only way I can see this working is using double-dispatch...
> > > 
> > > @Leaf: is there a way to do an 'is' check that doesn't look at the generic
> > > types?
> > > In that case I could write:
> > > if (other is ChunkedConverter) {
> > >   return other._fuseFromOther<S, T, S2, T2>(this);
> > > }
> > > 
> > > _fuseFromOther<otherS, otherT, otherS2, otherT2>(ChunkedConverter<otherS,
> > > otherT, otherS2, otherT2> other) {
> > >   if (this is ChunkedConverter<otherT, T, otherT2, T2>) {
> > >     return new _FusedChunkedConverter<otherS, T, otherS2, T2>(other,
this);
> > >   }
> > > }
> > > 
> > > This would require users to extend the ChunkedConverter (instead of just
> > > implementing the interface). Otherwise we could make that function public.
> > 
> > I don't see anything in this code that is directly not strong-mode compliant
> > (but may be missing something).  Is the concern that this returns something
> > whose dynamic type is Converter<S, dynamic>, and that consequently strong
mode
> > will not allow the result to be used as a Converter<S, SomethingConcrete>? 
> > That, I think is an issue.
> > 
> > I'm not sure quite what you mean by and 'is' check that doesn't look at the
> > generic types.  "other is ChunkedConverter" in some sense doesn't look at
the
> > generic types, in that it will return true for any instantiation of
> > ChunkedConverter.  Strong mode generic classes are still covariant.  Is that
> not
> > what you want?
> > 
> 
> My mistake. Somehow I was convinced that strong mode was invariant.

We tried... but without generic methods it's hard to get off the ground without
covariance.  There probably is a long term strong mode/DDC issue here where
people will expect the result of fuse on two typed converters to be typed, but
it won't be (and casting it to a concrete Converter type will fail).

[floitsch, 2016-04-12 17:55:29]

https://codereview.chromium.org/1847843002/diff/40001/sdk/lib/convert/chunked...
File sdk/lib/convert/chunked_conversion.dart (right):

https://codereview.chromium.org/1847843002/diff/40001/sdk/lib/convert/chunked...
sdk/lib/convert/chunked_conversion.dart:52: if (other is ChunkedConverter<T,
dynamic, T2, dynamic>) {
On 2016/04/12 17:44:30, Leaf wrote:
> On 2016/04/12 15:28:57, floitsch wrote:
> > On 2016/04/11 19:46:27, Leaf wrote:
> > > On 2016/04/11 18:37:53, floitsch wrote:
> > > > Looking at this again, I guess there is no way to make this strong-mode
> > > > compliant?
> > > > I would like to create the FusedChunkedConverter based on the dynamic
> type.
> > > The
> > > > only way I can see this working is using double-dispatch...
> > > > 
> > > > @Leaf: is there a way to do an 'is' check that doesn't look at the
generic
> > > > types?
> > > > In that case I could write:
> > > > if (other is ChunkedConverter) {
> > > >   return other._fuseFromOther<S, T, S2, T2>(this);
> > > > }
> > > > 
> > > > _fuseFromOther<otherS, otherT, otherS2,
otherT2>(ChunkedConverter<otherS,
> > > > otherT, otherS2, otherT2> other) {
> > > >   if (this is ChunkedConverter<otherT, T, otherT2, T2>) {
> > > >     return new _FusedChunkedConverter<otherS, T, otherS2, T2>(other,
> this);
> > > >   }
> > > > }
> > > > 
> > > > This would require users to extend the ChunkedConverter (instead of just
> > > > implementing the interface). Otherwise we could make that function
public.
> > > 
> > > I don't see anything in this code that is directly not strong-mode
compliant
> > > (but may be missing something).  Is the concern that this returns
something
> > > whose dynamic type is Converter<S, dynamic>, and that consequently strong
> mode
> > > will not allow the result to be used as a Converter<S, SomethingConcrete>?

> > > That, I think is an issue.
> > > 
> > > I'm not sure quite what you mean by and 'is' check that doesn't look at
the
> > > generic types.  "other is ChunkedConverter" in some sense doesn't look at
> the
> > > generic types, in that it will return true for any instantiation of
> > > ChunkedConverter.  Strong mode generic classes are still covariant.  Is
that
> > not
> > > what you want?
> > > 
> > 
> > My mistake. Somehow I was convinced that strong mode was invariant.
> 
> We tried... but without generic methods it's hard to get off the ground
without
> covariance.  There probably is a long term strong mode/DDC issue here where
> people will expect the result of fuse on two typed converters to be typed, but
> it won't be (and casting it to a concrete Converter type will fail).

Yes. This is not perfect yet...
I will make 'fuse' generic (in a future CL), but it won't help for the chunked
conversion case. Unless we add support for reading the generic types of
instances we will have to do a double-dispatch to get the full type. Not yet
sure what we want to do.

[floitsch, 2016-04-12 18:59:26]

Description was changed from

==========
Steps towards making the convert library strong-mode compliant.

This version has a few deprecated methods. In a future release they will be
removed.

See https://codereview.chromium.org/1827803002 for the final patch (once the
deprecated methods have been removed).

R=leafp@google.com, lrn@google.com

Committed:
https://github.com/dart-lang/sdk/commit/4ab1219bf93a33a2a3d4803e053ccd70ce190371
Reverted:
https://github.com/dart-lang/sdk/commit/045a3753665268761508c8053e0ee84372d9ef35
==========

to

==========
Steps towards making the convert library strong-mode compliant.

This version has a few deprecated methods. In a future release they will be
removed.

See https://codereview.chromium.org/1827803002 for the final patch (once the
deprecated methods have been removed).

R=leafp@google.com, lrn@google.com

Committed:
https://github.com/dart-lang/sdk/commit/4ab1219bf93a33a2a3d4803e053ccd70ce190371
Reverted:
https://github.com/dart-lang/sdk/commit/045a3753665268761508c8053e0ee84372d9ef35

Committed:
https://github.com/dart-lang/sdk/commit/50bdab3841dafd9b056b1dd013c103ce6a566f60
==========

[floitsch, 2016-04-12 18:59:28]

Committed patchset #4 (id:60001) manually as
50bdab3841dafd9b056b1dd013c103ce6a566f60 (presubmit successful).