JuliaLang / julia

The Julia Programming Language
https://julialang.org/
MIT License
45.71k stars 5.49k forks source link

`IOContext` does not follow the required interface of `AbstractPipe` #51808

Open Seelengrab opened 1 year ago

Seelengrab commented 1 year ago

IOContext claims to be an AbstractPipe:

julia> a = IOContext(IOBuffer())
IOContext(IOBuffer(data=UInt8[...], readable=true, writable=true, seekable=true, append=false, size=0, maxsize=Inf, ptr=1, mark=-1))

julia> a isa Base.AbstractPipe
true

But doesn't follow that interface:

help?> Base.AbstractPipe
  │ Warning
  │
  │  The following bindings may be internal; they may change or be removed in future versions:
  │
  │    •  Base.AbstractPipe

  AbstractPipe

  AbstractPipe is the abstract supertype for IO pipes that provide for communication between processes.

  If pipe isa AbstractPipe, it must obey the following interface:

    •  pipe.in or pipe.in_stream, if present, must be of type IO and be used to provide input to the pipe

    •  pipe.out or pipe.out_stream, if present, must be of type IO and be used for output from the pipe

    •  pipe.err or pipe.err_stream, if present, must be of type IO and be used for writing errors from the pipe

julia> a.in
ERROR: type IOContext has no field in
Stacktrace:
 [1] getproperty(pipe::IOContext{IOBuffer}, name::Symbol)
   @ Base ./io.jl:418
 [2] top-level scope
   @ REPL[8]:1

julia> a.in_stream
ERROR: type IOContext has no field in_stream
Stacktrace:
 [1] getproperty(pipe::IOContext{IOBuffer}, name::Symbol)
   @ Base ./io.jl:418
 [2] top-level scope
   @ REPL[9]:1

I'm not 100% sure the "if present" means "if the field exists" or "if the field is set", but either way, this should be cleared up somehow.

vtjnash commented 1 year ago

It can mean either of those, as they are functionally equivalent. Following the interface merely means the type should not have a value in a field of those names and not be the expected value. Perhaps it would be clearer to say 'is defined', since that is an existing functional predicate and therefore a bit more precise than 'is present'?

Seelengrab commented 1 year ago

That makes sense, yes; mentioning isdefined is an improvement.

Still, is IOContext actually expected to be used for "communication between processes"? So far I've only encountered it in cases where the printing of something based on where the environment of the printing matters, e.g. different terminal sizes necessitating linebreaks and such.

vtjnash commented 1 year ago

The "between processes" should also be removed, as that is incorrect. In Julia, the phrase "between processes" means the IO is required to be thread-safe for consistency, and that is not true of the example here.

If we don't already, it might be worth mentioning that the interface for this subtype is to have one of .in, .in_stream, or Base.pipe_reader implemented and/or .out, .out_stream, or Base.pipe_writer and optionally .err or .err_stream

tecosaur commented 1 year ago

Hmm, should AnnotatedIOBuffer be an AbstractPipe not an IO?