remove length from Stateful

[vtjnash]

Stateful iterators do not have a consistent notion of length, as it is continuously changing as elements are removed. As the main purpose of Stateful is to take elements from multiple places, any notion of HaveShape is invalid for those cases, and thus not useful in general.

Fix #47790
Blocked on #51869

[PallHaraldsson]

As you state you're removing:

function length(s::Stateful)

and thus:

@test length(Iterators.Stateful(Iterators.Stateful(1:5))) == 5

and I'm thinking, is this a "breaking" change? I think you're right, and this would be an "improvement" in some sense, but you would get an ERROR at runtime (e.g. in packages)? Instead of what, misleading or wrong answers... with status quo? I'm conflicted here, because of the principle (of not removing/changing/semver) vs. better API.

I'm all for trying this on master (for 1.11), what do do for 1.10? Deprecate this and NOT remove?

If relying on this function is a technically breaking change, i.e. a bug, (almost) always, then change this in some way on 1.10, i.e. backport soon?

Would it help to deprecate and do something like length(s::Stateful) = error("This doesn't make sense [and why]")? If it helps to do that, or simply remove, then ideally from 1.10.0, i.e. add to 1.10 milestone. It could always be undone in 1.10.x, while removing in .x non-ideal...

I suppose PkgEval should be run with this simply removed to see potential practical consequences, but even if none turn up, it wouldn't rule out breaking user code. So what's best to do for 1.10?

[jakobnissen]

This will not be backported to 1.10, it's too late for that kind of changes.
This is a breaking change, but the only alternative I can see is incorrect behaviour, and while breaking changes are very bad, silently getting incorrect answers is worse.

I can't help but feel that the fix here is a bandaid over a larger problem, namely that Julia does not have any functionality to distinguish mutable from immutable iterators, and as a result of that, consistently conflate them even in Base. However, this is a rather fundamental problem, and as such, is unlikely to be fixed. So solutions like the ones here may be the best approach.

[KristofferC]

@PallHaraldsson You keep commenting in various PRs (that have semi-breaking or invasive changes) about backporting them to 1.10 and every time it has to be pointed out that 1.10 is not in that stage. At this point, I would maybe stop worrying so much about 1.10 and just assume that the backports will be handled in an ok way.

[PallHaraldsson]

Should we at least add a "breaking" label? I think we should take breaking changes seriously, why I bring this up more than 1.10.

[oscardssmith]

triage says this is a (slightly breaking) bugfix.

[oscardssmith]

@nanosoldier runtests()

[nanosoldier]

Your job failed. Consult the server logs for more details (cc @maleadt).

[maleadt]

MethodError: no method matching length(::Base.Iterators.Stateful{Base.Iterators.Zip{Tuple{Base.EachStringIndex{String}, String}}, Union{Nothing, Tuple{Tuple{Int64, Char}, Tuple{Int64, Int64}}}})    JULIA stdlib/Distributed.release.image
Closest candidates are:
  length(!Matched::Base.MethodSpecializations)
   @ Base reflection.jl:1261
  length(!Matched::CompositeException)
   @ Base task.jl:51
  length(!Matched::Core.MethodTable)
   @ Base reflection.jl:1263
  ...
Stacktrace:
  [1]     JULIA stdlib/LibGit2_jll.release.image
    JULIA stdlib/LibCURL_jll.release.image
(::StyledStrings.var"#readexpr!#43")(state::@NamedTuple{content::String, bytes::Vector{UInt8}, s::Base.Iterators.Stateful{Base.Iterators.Zip{Tuple{Base.EachStringIndex{String}, String}}, Union{Nothing, Tuple{Tuple{Int64, Char}, Tuple{Int64, Int64}}}}, parts::Vector{Any}, active_styles::Vector{Vector{Tuple{Int64, Int64, Union{Expr, Pair{Symbol, Any}, Symbol}}}}, pending_styles::Vector{Tuple{UnitRange{Int64}, Union{Expr, Pair{Symbol, Any}, Symbol}}}, offset::Base.RefValue{Int64}, point::Base.RefValue{Int64}, escape::Base.RefValue{Bool}, interpolated::Base.RefValue{Bool}, errors::Vector{@NamedTuple{message::Base.AnnotatedString{String}, position::Union{Nothing, Int64}, hint::String}}}, pos::Int64)
    @ StyledStrings /source/usr/share/julia/stdlib/v1.11/StyledStrings/src/stylemacro.jl:243
  [2] (::StyledStrings.var"#interpolated!#42")(state::@NamedTuple{content::String, bytes::Vector{UInt8}, s::Base.Iterators.Stateful{Base.Iterators.Zip{Tuple{Base.EachStringIndex{String}, String}}, Union{Nothing, Tuple{Tuple{Int64, Char}, Tuple{Int64, Int64}}}}, parts::Vector{Any}, active_styles::Vector{Vector{Tuple{Int64, Int64, Union{Expr, Pair{Symbol, Any}, Symbol}}}}, pending_styles::Vector{Tuple{UnitRange{Int64}, Union{Expr, Pair{Symbol, Any}, Symbol}}}, offset::Base.RefValue{Int64}, point::Base.RefValue{Int64}, escape::Base.RefValue{Bool}, interpolated::Base.RefValue{Bool}, errors::Vector{@NamedTuple{message::Base.AnnotatedString{String}, position::Union{Nothing, Int64}, hint::String}}}, i::Int64, ::Char)
    @ StyledStrings /source/usr/share/julia/stdlib/v1.11/StyledStrings/src/stylemacro.jl:225
  [3] (::StyledStrings.var"#run_state_machine!#63"{StyledStrings.var"#end_style!#46", StyledStrings.var"#begin_style!#45", StyledStrings.var"#interpolated!#42", StyledStrings.var"#escaped!#41"})(state::@NamedTuple{content::String, bytes::Vector{UInt8}, s::Base.Iterators.Stateful{Base.Iterators.Zip{Tuple{Base.EachStringIndex{String}, String}}, Union{Nothing, Tuple{Tuple{Int64, Char}, Tuple{Int64, Int64}}}}, parts::Vector{Any}, active_styles::Vector{Vector{Tuple{Int64, Int64, Union{Expr, Pair{Symbol, Any}, Symbol}}}}, pending_styles::Vector{Tuple{UnitRange{Int64}, Union{Expr, Pair{Symbol, Any}, Symbol}}}, offset::Base.RefValue{Int64}, point::Base.RefValue{Int64}, escape::Base.RefValue{Bool}, interpolated::Base.RefValue{Bool}, errors::Vector{@NamedTuple{message::Base.AnnotatedString{String}, position::Union{Nothing, Int64}, hint::String}}})
    @ StyledStrings /source/usr/share/julia/stdlib/v1.11/StyledStrings/src/stylemacro.jl:638
  [4] var"@styled_str"(__source__::LineNumberNode, __module__::Module, raw_content::String)
    @ StyledStrings /source/usr/share/julia/stdlib/v1.11/StyledStrings/src/stylemacro.jl:666
  [5] include(mod::Module, _path::String)
    @ Base ./Base.jl:497
  [6] include(x::String)
    @ StyledStrings /source/usr/share/julia/stdlib/v1.11/StyledStrings/src/StyledStrings.jl:3
  [7] top-level scope
    @ /source/usr/share/julia/stdlib/v1.11/StyledStrings/src/StyledStrings.jl:14
  [8] include
    @ Base ./Base.jl:497 [inlined]
  [9] include_package_for_output(pkg::Base.PkgId, input::String, depot_path::Vector{String}, dl_load_path::Vector{String}, load_path::Vector{String}, concrete_deps::Vector{Pair{Base.PkgId, UInt128}}, source::Nothing)
    @ Base ./loading.jl:2311
 [10] top-level scope
    @ stdin:3
in expression starting at /source/usr/share/julia/stdlib/v1.11/StyledStrings/src/stylemacro.jl:687
in expression starting at /source/usr/share/julia/stdlib/v1.11/StyledStrings/src/stylemacro.jl:681
in expression starting at /source/usr/share/julia/stdlib/v1.11/StyledStrings/src/StyledStrings.jl:3
in expression starting at stdin:3

[tecosaur]

Mea culpa, thanks for the PR to fix my usage of this Jameson.

[vtjnash]

@nanosoldier runbenchmarks("strings" || "string", vs=":master")

[nanosoldier]

Your job failed.

[IanButterworth]

CI isn't happy and there are some open questions so switching labels here

[vtjnash]

@nanosoldier runbenchmarks("strings" || "string", vs=":master")

[nanosoldier]

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here.

[rafaqz]

Turns out this is kinda breaking

[jakobnissen]

That's right. However, status quo (before this PR) have wrong answers in several cases. I would argue that breaking code is better than having it silently be wrong.
Ideally, the correctness bugs would be fixed, but it's proven to be difficult to do.

[rafaqz]

The problem is where Stateful was used internally in controlled conditions (and using length was totally fine)

In DiskArrays.jl we will have to make our own identical DiskArrays.Stateful with length defined.

[vtjnash]

Yeah, I can see that use, but it is only fine because you are relying on it being a monotonic counter to see if the iterator has advanced, and not actually using it as a length. It seems like that code might be more correct with a call to peek() there instead, as a drop in replacement? (or perhaps needs all of s.nextvalstate)
https://github.com/meggart/DiskArrays.jl/blob/9fc52e23a1f3e46267f9377d45dbd832e8c83750/src/iterator.jl#L45-L52

[vtjnash]

All cases so far of length in action have been to test if the length is zero, which is better written as isempty anyways (since length may be expensive or incorrect to compute). I have made PRs to fix the ones that I know about.