Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Expose AutoStream getters for the wrapped RawStream #222

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

DaniPopes
Copy link
Contributor

@DaniPopes DaniPopes commented Oct 24, 2024

AutoStream can be consumed to get the wrapped S: RawStream, but there is currently no way to get a reference to it.

My use case for this is being able to get the &mut dyn Write while storing AutoStream<Box<dyn Write>>, and to either write directly to it or downcast it to Vec<u8> to get the access to the written bytes.

crates/anstream/src/auto.rs Outdated Show resolved Hide resolved
@epage
Copy link
Collaborator

epage commented Oct 24, 2024

AutoStream can be consumed to get the wrapped S: RawStream, but there is currently no way to get a reference to it.

Could you add to the PR description the use case for why this would be needed?

@coveralls
Copy link

coveralls commented Oct 24, 2024

Pull Request Test Coverage Report for Build 11501103511

Details

  • 0 of 18 (0.0%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.5%) to 51.454%

Changes Missing Coverage Covered Lines Changed/Added Lines %
crates/anstream/src/strip.rs 0 4 0.0%
crates/anstream/src/wincon.rs 0 4 0.0%
crates/anstream/src/auto.rs 0 10 0.0%
Totals Coverage Status
Change from base Build 11282311767: -0.5%
Covered Lines: 1221
Relevant Lines: 2373

💛 - Coveralls

@DaniPopes DaniPopes force-pushed the raw-stream-getters branch 2 times, most recently from ebb2421 to 900d738 Compare October 24, 2024 14:11
@DaniPopes
Copy link
Contributor Author

Could you add to the PR description the use case for why this would be needed?

Done.

@epage
Copy link
Collaborator

epage commented Oct 24, 2024

My use case for this is being able to get the &mut dyn Write while storing AutoStream<Box>, and to either write directly to it or downcast it to Vec to get the access to the written bytes.

Please expand on why

note: generally I require Issues first where this is all figured out. It isn't always ideal to have these conversations in PRs.

`AutoStream` can be consumed to get the wrapped `S: RawStream`, but there
is currently no way to get a reference to it.
@DaniPopes
Copy link
Contributor Author

DaniPopes commented Oct 24, 2024

Figured it would be straight-forward enough that it wouldn't require an issue.

I have a potentially Vec<u8> buffer inside of a Box<dyn Write>, I want a reference to it so I can copy/take the written bytes out of it to return them as a stand-alone String or stored somewhere else separately, while keeping the AutoStream instance.

Currently I have to either store a raw pointer to the Box<dyn Write> separately so I can get a reference to it later, or have something like two instances of struct SharedBuffer(Arc<Mutex<Vec<u8>>>) so I can access my buffer just because it's not exposed through the AutoStream API.

Correct me if I'm wrong, but given that AutoStream already guarantees that it stores an owned S because of the into_inner method, it should also be fine to provide getters to this instance.

@epage
Copy link
Collaborator

epage commented Oct 30, 2024

Figured it would be straight-forward enough that it wouldn't require an issue.

Correct me if I'm wrong, but given that AutoStream already guarantees that it stores an owned S because of the into_inner method, it should also be fine to provide getters to this instance.

AutoStream is stateful and care was taken into the design to try to minimize the chance that the inner would be read from or written to mid-state. into_inner is an explicit acknowledgement that you are abandoning that state while references, particularly mutable references, can muck this up.

I need to weigh out your use case and the complexity foisted on you against helping to prevent bugs for others. The more generally applicable it seems, the more it weighs in favor of making the change.

I have a potentially Vec buffer inside of a Box, I want a reference to it so I can copy/take the written bytes out of it to return them as a stand-alone String or stored somewhere else separately, while keeping the AutoStream instance.

Maybe asked a different way, what problem are you trying to solve? Why do you need a single AutoStream alive while pulling its state out?

I can get a reference to it later, or have something like two instances of struct SharedBuffer(Arc<Mutex<Vec>>) so I can access my buffer just because it's not exposed through the AutoStream API.

Depending on your use case, you may be able to use RefCell.

@epage
Copy link
Collaborator

epage commented Oct 30, 2024

AutoStream is stateful and care was taken into the design to try to minimize the chance that the inner would be read from or written to mid-state. into_inner is an explicit acknowledgement that you are abandoning that state while references, particularly mutable references, can muck this up.

Hmm, another interesting design idea that I don't think of enough is the use of extension traits to opt-in to potentially dangerious (but not unsafe) behavior, like indexmap does with https://docs.rs/indexmap/latest/indexmap/map/raw_entry_v1/trait.RawEntryApiV1.html

@DaniPopes
Copy link
Contributor Author

DaniPopes commented Oct 30, 2024

Maybe asked a different way, what problem are you trying to solve? Why do you need a single AutoStream alive while pulling its state out?

I am using similar setup to the rustc diagnostics system:

My goal is to obtain the &[u8] to the buffer of emitted diagnostics. I have thought of 3 possible ways of achieving this:

  1. pass the output by mutable reference, &mut Vec<u8>; this requires a. making HumanEmitter generic over the writer type and b. a non-static inner writer type, which only works in the temporary JsonEmitter case, and not when I want to query for this buffer through dyn Emitter or HumanEmitter<dyn...>
  2. use Box<dyn WriteExt>, which has extra methods for what I need from the writer, namely something like fn buffer(&self) -> Option<&[u8]>; this is not possible through AutoStream because the inner writer type must be one of the predefined RawStream, and requires access to it
  3. store a TypeId alongside the writer on creation so that the original type can be retrieved later with a dynamic downcast; this also requires access to a pointer to the inner writer

Hmm, another interesting design idea that I don't think of enough is the use of extension traits to opt-in to potentially dangerious (but not unsafe) behavior

That would be fine by me if it is deemed necessary for this API. I would imagine that it wouldn't be required for immutable references.

@epage
Copy link
Collaborator

epage commented Oct 31, 2024

Thanks for explanation.

Are you always using Vec<u8> Is there a reason not to just use AutoStream<Vec<u8>> or even just Vec<u8> and adapt it later when the read the Vec<u8> and then do something with it? With deferred processing, that was mostly my intent of how to handle this. The design is focused on keeping ANSI escape codes until you present it to the user.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants