Type-stabilize eachline

[pabloferz]

Fix #20351

[stevengj]

Why is the typeassert needed? Is this an inference bug, or could the EachLine constructor be written differently to be type-stable?

[pabloferz]

eachline is calling invoke a type constructor with keyword arguments right now because of the resent changes/deprecations. And I think inference has problems with invoke and methods with keyword arguments. This is a temporal measure (I should probably add a comment).

[pabloferz]

@stevengj It seems that the problem is simply with type constructors with keyword arguments

type Foo
    i::Int
    b::Bool
    Foo(i::Int=1, b::Bool=true) = new(i, b)
end

type Bar
    i::Int
    b::Bool
    Bar(i::Int=1; b::Bool=true) = new(i, b)
end

@code_warntype Foo(1)
# ...
end::Foo

@code_warntype Bar(1)
# ...
end::Any

This PRs works around that in case there is no easy/soon-to-be fix.

[pabloferz]

Another way to fix this is to change the EachLine constructor to use positional arguments, but leave eachline with keyword arguments. That would also need to remove the just added deprecation for EachLine(stream, ondone).

EDIT: Added a second commit with the other possible solution.

[martinholters]

Well, that the return type of the constructors with keyword arguments cannot be inferred is rather unfortunate (do we an issue for that?), but with that explanation, just type-asserting their usages seems to be a valid work-around.

[nalimilan]

Since people are supposed to use eachline in most cases, it shouldn't be an issue and it will be clearer to have the EachLine constructor take two positional arguments that correspond to the type's fields. Do we have other keyword argument constructors like that in Base?

[StefanKarpinski]

We export EachLine as a public API. As such, I think that using positional arguments for this sort of thing is really not ideal: it doesn't match eachline, which people will expect, and who can remember which of the arguments goes first – chomp or the ondone callback? Which do we think it's more common to pass? I'd be more ok with unexporting EachLine since I'm not entirely sure why we export it in the first place – in that case we can make the API anything we want without worrying about ergonomics.

[StefanKarpinski]

Unfortunate as it may be that we can't type infer constructors with keyword arguments at the moment, that will be fixed and those type assertions can be deleted without affecting anyone's code. Committing to a public API based on a temporary performance issue is not a good move.

[nalimilan]

RegexMatchIterator (returned by eachmatch) isn't exported either, so +1 for unexporting EachLine. A similar situation happens with zip and Zip/Zip2.

[StefanKarpinski]

In any case, let's leave this PR as the simplest fix that addresses the type issue: sticking some type annotations on calls to the EachLine constructor.

[pabloferz]

I left just the type-assertion work-around, so the changes on the type constructor and unexporting can be done separately.

[martinholters]

A bit late now, but I guess we could just have made the constructor an outer constructor, replacing the new with a call to the default inner constructor.