[Urgent] v1.10.3 breaks AnnotatedString inputs (affects downstream packages)

[MilesCranmer]

Version v1.10.3 inadvertently introduced a bug by calling IOBuffer on an AnnotatedString. This breaks multiple downstream packages including SymbolicRegression.jl and PySR, causing them to fail with errors like:

ERROR: MethodError: no method matching IOBuffer(::Base.AnnotatedString{String})
Closest candidates are:
  IOBuffer(!Matched::SubString{String})
    @ Base strings/io.jl:309
  IOBuffer(!Matched::String)
    @ Base strings/io.jl:308
  IOBuffer(; read, write, append, truncate, maxsize, sizehint)
    @ Base iobuffer.jl:119

Introduced in #341 (cc @MasonProtter), specifically this line:

ProgressMeter.jl/src/ProgressMeter.jl

Line 591 in 81362ec

if countlines(IOBuffer(string_value)) > 1

Could you please revert or yank v1.10.3 ASAP, and issue a patch version without this breaking change? In any case, I don't think a patch version should have introduced this kind of major code change.

In the meantime I need to issue a bunch of patch releases for SR.jl to lock to the old versions as this breaks everything. But please let me know as soon as this is fixed so I can unlock the versions again.

[MilesCranmer]

I think JET.jl would have flagged this kind of bug. It's probably worth adding JET to the CI to prevent such issues in the future.

[MilesCranmer]

Thanks for the quick turnaround @MarcMush!

[MarcMush]

you're welcome, hopefully it's all good now

[MilesCranmer]

I should say that I think v1.10.3 and v1.10.4 should both be yanked. See https://github.com/JuliaRegistries/General?tab=readme-ov-file#when-is-yanking-a-release-appropriate for how to do this.

The reason is that they do not follow semver (https://semver.org/) and introduce new features and changes in behavior. I think that v1.10.4 should be released under v2.0.0. This is so that downstream maintainers have a chance to consider the new behavior before deciding to incorporate it in their Project.toml. For example, both #330 and #341 cause different behavior in the progress bar, and thus need a new major release rather than minor release (or patch release, as is currently done).

[MarcMush]

I'm yanking 1.10.3 (JuliaRegistries/General#128480)
I don't think alining values differently should be considered breaking but I see what you're saying

[MasonProtter]

Ugh, that's super annoying. I guess StyledStrings didn't know to implement this method.

Demanding that a change like not breaking the display on multiline values be listed as a v2.0 is pretty silly. It was literally a bugfix, that apparently hits a different bug caused by StyledStrings.

The documented way to count the number of lines in a string is to use IOBuffer:

help?> countlines
search: countlines count_ones contains continue copyline ColumnSlices count_zeros RowSlices count rounding readlines sortslices count!

  countlines(io::IO; eol::AbstractChar = '\n')
  countlines(filename::AbstractString; eol::AbstractChar = '\n')

  Read io until the end of the stream/file and count the number of lines. To specify a file pass the filename as the first argument. EOL
  markers other than '\n' are supported by passing them as the second argument. The last non-empty line of io is counted even if it does
  not end with the EOL, matching the length returned by eachline and readlines.

  To count lines of a String, countlines(IOBuffer(str)) can be used. 

[MilesCranmer]

@MasonProtter maybe you missed it but I was referring to #330 which changes the default printing behavior. It should definitely not be a patch release, I hope we can agree on that. You could argue that it is okay to be a minor release, but given that ProgressMeter's main purpose is printing, and it changes the printing format, I think the proper way to introduce it is via an opt-in flag on 1.x.y, and then as a default in 2.0.0. I think the semver page is fairly clear about this recommendation.

At the end of the day when you have 100s of dependencies, it just makes life easier when people are strict about this kinda stuff.

[MasonProtter]

Yes I did miss that. I don't feel the same, but I see why you might feel that way.

[MilesCranmer]

Btw regarding

The documented way to count the number of lines in a string is to use IOBuffer:

It looks like the docstring refers to String (rather than "string" lower-case implying AbstractString) which is why.

To count lines of a String, countlines(IOBuffer(str)) can be used.

For a Base.AnnotatedString there is a corresponding Base.AnnotatedIOBuffer but probably safer to use something more generic?

[MarcMush]

a few other PRs that changed printing: #301, #233, #211
not really an argument either way, but we should clarifiy what we are allowed to change or not