Sdk: LineSplitter not assignable to Converter<String, List<String>>

[tvolkert]

    Converter<String, List<String>> lineSplitter = const LineSplitter();

That line yields the following analyzer error:

[error] A value of type 'LineSplitter' can't be assigned to a variable of type 'Converter<String, List<String>>'

And yet, LineSplitter extends Converter<String, List<String>>

Huh??

[leafpetersen]

What version are you seeing this in? There was a temporary regression in the analyzer configuration logic that broke this, see here: https://github.com/dart-lang/sdk/issues/28507 .

This is fixed in bleeding edge, and I believe in more recent 1.22 dev releases. Closing the issues, but if you're seeing this in the latest dev release (or the next release candidate) please re-open.

[tvolkert]

This was in 1.22.0-dev.9.1

[leafpetersen]

Yes, it was broken in that dev release, fixed as of (at least) dev.10.1.

[tvolkert]

Actually I'm still seeing this in 1.22.0-dev.10.6 -- see the failures in https://github.com/google/file.dart/pull/38

[tvolkert]

Still seeing this in 1.22.0-dev.10.6 (along with an inexplicable abundance of unawaited_futures false positive lint warnings, but that's a different issue).

I don't seem to have permission to re-open the issue; @leafpetersen, can you?

[leafpetersen]

I can reproduce. This was fixed from (at least) dev.10.1 through dev.10.3, but I see a regression again in dev.10.4 . As a workaround, you can use dev.10.3 for now.

[leafpetersen]

Sorry, I didn't look through your original example closely enough. This is correct behavior. Linesplitter is not really type safe as originally written. To make it strong mode clean, the comment syntax was used to change its base class in strong mode.

class LineSplitter
    extends Converter<String, List<String>>/*=Object*/
    implements ChunkedConverter<String, List<String>, String, String>
        /*=StreamTransformer<String, String>*/ {

In strong mode, this is treated as extending Object.

So the error is correct, the regression was the temporary set of dev releases that used the old signature in strong mode.

[tvolkert]

Linesplitter is not really type safe as originally written

@leafpetersen so is this code block even safe?

Converter<String, List<String>> lineSplitter =
    const LineSplitter() as Converter<String, List<String>>;

[leafpetersen]

No. In strong mode that cast will fail. I believe that you can get from a StreamTransformer to a Converter via some path, but I'm not super familiar with the APIs here. @nex3 @kevmoo or @lrhn might be able to give some guidance.

[kevmoo]

@lrhn @floitschG Should really look at this.

Now that we've shipped generic syntax, this really should be cleaned up.

[tvolkert]

I guess a meta-question is: can we make LineSplitter actually be a Converter<String, List<String>>?

[nex3]

I guess a meta-question is: can we make LineSplitter actually be a Converter<String, List<String>>?

Not really. The Converter interface requires that the convert() method goes from S → T and that the bind() method goes from Stream<S> → Stream<T>. But LinesSplitter doesn't follow that. LineSplitter.convert() goes from String → List<String> (because it splits a single string into lines), whereas LineSplitter.bind() goes from Stream<String> → Stream<String> (because it splits lines across chunks).

Semantically speaking, I think LineSplitter is best modeled the way it's typed in strong mode right now: as a plain old StreamTransformer<String, String>. I'd even push for deprecating its existing convert() method since it's redundant with the static split() method.

[kevmoo]

What @nex3 said.

@lrhn @floitschG Reasonable?

[floitschG]

Yes.
That was the intention behind using the comment syntax. It is basically used as an ifdef for strong mode.

I don't think we should remove the convert method. LineSplitter isn't a converter anymore, but it definitely feels like one, and having similar APIs thus makes sense.

I would suggest closing this bug. We already have plans to completely remove the comment syntax which would make the transition from LineSplitter to a non-converter complete.

[kevmoo]

@floitschG but that will be a breaking change, right?

For which milestone?

[floitschG]

When migrating to strong mode, programs need to adapt (but they need to do that anyway). After that there is no breaking change.

[srawlins]

So it sounds like the resolution to this bug is changing LineSplitter's signature from:

class LineSplitter extends Converter<String, List<String>> /*=Object*/
    implements
        ChunkedConverter<String, List<String>, String, String>
/*=StreamTransformer<String, String>*/ {

to

class LineSplitter implements StreamTransformer<String, String> {

This is strange enough that I think it warrants its own bug (this bug, which blocks #28796 and #28773 rather than is a duplicate of), and warrants its own line in the CHANGELOG.

[lrhn]

Removing the comment syntax is a breaking change, which is why we were expecting to not do it until Dart 2.0. Until then, strong mode uses the Dart 2 signature and Dart 1 programs use the original one. It is annoying to maintain the comment syntax, and it would be great to not need it any more.
(There are other cases where comment syntax is still used for performance, but I believe this is the only one where removing it is a breaking change).

[kevmoo]

Are we saying this won't be in 1.24?

[floitschG]

Removed the milestone. There is no reason to force this change. It will naturally happen once we discontinue support for comment syntax.

[lrhn]

Making the change is a breaking change for the LineSplitter since the new declaration, expressed using comments, isn't backwards compatible with the existing class declaration.

[zoechi]

Comment syntax was clearly communicated as experimental and
breaking changes in experimental features are expected and should be allowed?
It's always a great idea to try to avoid breakage of course, but what's the point of marking something experimental?
Just my 2c.

[lrhn]

The problem here is that we don't just want to remove the comments, which would only be affecting the experimental version, but actually change the existing declaration to be the one that is currently in comments.
That breaks the existing, non-experimental version of the class, and any library that depends on it. It'll be a necessary change form Dart 2/Strong mode, but a breaking change for Dart 1. That's why we have been delaying the change.

[zoechi]

I see. Thanks for clarification.

[nex3]

I don't think this was actually supposed to be closed by google/file.dart#62.

[leafpetersen]

@nex3 I had actually intended to close this off, but am happy to keep it open if there's something actionable we still intend to do? As of now though, the use of the comment syntax in line_splitter.dart has been remove, LineSplitter works as you describe here: https://github.com/dart-lang/sdk/issues/28748#issuecomment-279831950 , and the original example from this issue is fixed.

[nex3]

Oh okay, I just assumed that the "fixes" line in the file.dart commit accidentally closed this issue. It sounds like it was intentional, though, and I don't have anything else I think needs to be done.