Add read/write specialisation for IOContext{AnnIO}

[tecosaur]

Now that we have AnnotatedIOBuffer and StyledStrings, I decided to revisit my "tell me about this struct" utility, and make it less of a kludge.

Along the way I ran into what I think is essentially a rather rough edge that can be smoothed over with a few extra method specialisations.

See the commit message for details on this what's been done, it's changed a few times now.

[vtjnash]

Two of those seem like what I would have expected the AbstractPipe would already do for forwarding these to the underlying implementation

[tecosaur]

Running into this behaviour took me by surprise, but I think it's because AnnotatedIOBuffer implements pipe_reader and pipe_writer to point to the IOBuffer inside it. The generic write method for IOContext then writes to the pipe_writer, hence the need for this specialisation.

[vtjnash]

Okay, I see this calls instead StyledStrings._ansi_writer because of overloading StyledStrings, rather than first unwrapping the IOContext. I think that suggests that it should unwrap any IOContext before dispatching to StyledStrings._ansi_writer for these types? Otherwise this looks like just too much of a narrow special case, that can be tricked simply by cases such as IOContext{Any}

[tecosaur]

Thanks for taking a further look into this. Do I read you correctly in that you're suggesting we resolve this issue by tackling the general case where reads/writes with an IOContext behave differently from reads/writes to the wrapped IO?

[vtjnash]

yes

[tecosaur]

Cool, I'll have a look at doing that and see about updating this PR to take that approach instead.

[tecosaur]

Hmmm, my first attempt at this hasn't gone well

# We want reads/writes involving an `IOContext` to dispatch to to underlying `IO`,
# rather than on `IOContext`.
write(io::IOContext, x::Any) = write(io.io, x)
read(io::IOContext, x::Any)  = read(io.io, x)
# To resolve method ambiguities
write(io::IO, x::AbstactChar) = write(io.io, x)
write(io::IO, x::AbstactString) = write(io.io, x)
write(io::IO, x::Union{String,SubString{String}}) = write(io.io, x)
write(io::IO, x::UInt8) = write(io.io, x)

(when I take julia +nightly and eval the above in a begin ... end block) produces

SYSTEM (REPL): showing an error caused an error

SYSTEM (REPL): caught exception of type Unreachable reached at 0x7f0b8cb176c6

[28873] signal 4 (2): Illegal instruction
in expression starting at none:0
print at ./strings/io.jl:250
unknown function (ip: 0x7f0b8cb176f6)
print at ./strings/io.jl:46
...

zsh: illegal hardware instruction (core dumped)  julia +nightly
18:09:05 ❯ 

I'm also not sure if there's a good general method do deal with all the potential method ambiguities introduced.

I'm confident about the methods for the IOContext{AnnotatedIOBuffer} case since we can know ahead of time which methods need to be implemented and that no ambiguities are introduced, but I'm struggling to see how to make a general solution work here.

If anybody has thoughts on this, I'd love to hear them.

[Seelengrab]

I'm also not sure if there's a good general method do deal with all the potential method ambiguities introduced.

Possibly related: #47385, #47325

[tecosaur]

After a rather productive chat with Nathan Zimmerberg on slack, I'm thinking that:

• It probably wouldn't be a good idea to always pass through to the underlying IO of an IOContext
• We can fix this headache just as effectively by making a "pass through to the pipe_writer/pipe_reader" method in annotated.jl

Let me know what you think!

[tecosaur]

Update: this breaks the behaviour of IOContext{IOBuffer} with :color => true. Maybe we should just go with the IOContext{AnnotatedIOBuffer} specialisation after all?

[tecosaur]

I suppose the alterative would be that we try to shoehorn the type of pipe_writer(io) into the dispatch with a method like so:

# So that read/writes with `IOContext` (and any similar `AbstractPipe` wrappers)
# work as expected.
function write(io::AbstractPipe, s::Union{AnnotatedString, SubString{<:AnnotatedString}})
    if pipe_writer(io) isa AnnotatedIOBuffer
        write(pipe_writer(io), s)
    else
        invoke(write, Tuple{IO, typeof(s)}, io, s)
    end
end
# Can't be part of the `Union` above because it introduces method ambiguities
function write(io::AbstractPipe, c::AnnotatedChar)
    if pipe_writer(io) isa AnnotatedIOBuffer
        write(pipe_writer(io), c)
    else
        invoke(write, Tuple{IO, typeof(c)}, io, c)
    end
end

I don't think this can work for read though, but that is just a convenience method (in other words: it's nice for completeness, but not alleviating a headache the way write is).

[tecosaur]

If anyone has thoughts on this, I'd love to hear them. Currently, I find the alternative mentioned in my last comment a bit ugly, but if there's no other input I'll just go with that.

[tecosaur]

I've updated this PR. @vtjnash please let me know what you think of the current state.

[tecosaur]

Let me know if I'm being too ambitious, but I'm going to speculatively tag this with backport-1.11 since it (IMO) improves the state of this feature in the 1.11 release.

[tecosaur]

I've just resolved a recently-introduced merge conflict.

More broadly, I still think this code looks a bit ugly, but it seems to work, and it's functionality that I think is rather worth having.

Should we go ahead and merge this, and let any improvements/nicer implementations come along in the future?

[fingolfin]

The following is a 100% optional suggestion that is ultimately a matter of taste

As you are most likely aware, you could avoid the code duplication by using @eval, i.e.

Suggested change

	function write(io::AbstractPipe, s::Union{AnnotatedString, SubString{<:AnnotatedString}})
	if pipe_writer(io) isa AnnotatedIOBuffer
	write(pipe_writer(io), s)
	else
	invoke(write, Tuple{IO, typeof(s)}, io, s)
	end::Int
	end
	# Can't be part of the `Union` above because it introduces method ambiguities
	function write(io::AbstractPipe, c::AnnotatedChar)
	if pipe_writer(io) isa AnnotatedIOBuffer
	write(pipe_writer(io), c)
	else
	invoke(write, Tuple{IO, typeof(c)}, io, c)
	end::Int
	end
	# (can't do this via a single method with a 'Union' argument due to method ambiguities)
	for T in (AnnotatedChar, AnnotatedString, SubString{<:AnnotatedString}) do
	@eval function write(io::AbstractPipe, arg::$T)
	if pipe_writer(io) isa AnnotatedIOBuffer
	write(pipe_writer(io), arg)
	else
	invoke(write, Tuple{IO, typeof(arg)}, io, arg)
	end::Int
	end
	end

[fingolfin]

Still trying to wrap my brain around this:

• by default, unsafe_write(io::AbstractPipe, ...) for simply redispatches to unsafe_write(pipe_writer(io)::IO, ...)
• by default, all write(io, ...) methods in the end go through unsafe_write(io, ...)
• so naively write(pipe_writer(io), s) seems pointless
• but then there are custom write methods ... such as your write(io::AnnotatedIOBuffer, astr::Union{AnnotatedString, SubString{<:AnnotatedString}}) method...

And now we have a problem if AbstractPipe type, such as IOContext, wraps an AnnotatedIOBuffer: instead of dispatching from write(io, ...) to unsafe_write(io, ...) you need it to go through pipe_writer(io) so that the annotations can be handled by that.

What if there is a sandwich of three AbstractPipe types, say an IOContext outermost, then some other AbstractPipe type, then the AnnotatedIOBuffer -- it wouldn't work out then either, right?

This then makes me wonder: why not "simply" something like

write(io::AbstractPipe, s::AnnotatedString) = write(pipe_writer(io), s)

I bet you considered this and ruled it out for some reason, but I didn't see it in the PR discussion.

None of this is meant as fundamental objection to this PR, but I feel at least this comment should be a bit more elaborate (which, if you happen to agree with this POV, could certainly wait for a future PR -- I don't mind to hold this one up). I'd love to be helpful and suggest something, but for that I'd first have to really understand it, hence my questions :-).

[fingolfin]

Just had a chat with @tecosaur about this over on Slack, and he pointed out the commit message here which is excellent and provides further details.

All in all, I share the mild unease about the code in here, and I think there may be need to change some things in the IO system, but this PR is not the place for it. And if this code causes any problems down the road, well, the only way to find out is to try it in the wild?

With that I'd be happy to have this merged at any time.

[tecosaur]

(just pushed a typo fix, no changes to the code)