batch 2: IO and streams

This commit is contained in:
Adam Hathcock
2026-07-28 12:19:33 +01:00
parent d6f79a5467
commit 7ea660c049
14 changed files with 121 additions and 211 deletions

View File

@@ -23,8 +23,9 @@ the verification workflow, and the record of what is blocked and why.
| Batch | Area | Status |
| --- | --- | --- |
| 0 | `Archives/IArchiveEntryExtensions` (5 methods) | done — `c303856c` |
| 1 | `Compressors/Filters/Filter` + 6 XZ branch filters + `Lzma2Filter` (9 methods, 147 lines) | done — verified token-identical on net48 + net10.0 |
| 212 | below | to do |
| 1 | `Compressors/Filters/Filter` + 6 XZ branch filters + `Lzma2Filter` (9 methods, 147 lines) | done — verified identical on net48 + net10.0 |
| 2 | `IO/` leaf stream shims + `Common/EntryStream` (17 methods, 190 lines) | done — verified identical on net48 + net10.0 |
| 312 | below | to do |
| Rar reader unification | below | to do, needs its own design review |
---
@@ -82,14 +83,29 @@ Generated source lands in
`src/SharpCompress/obj/Release/<tfm>/generated/Zomp.SyncMethodGenerator/Zomp.SyncMethodGenerator.SyncMethodSourceGenerator/<Namespace>.<Type>.<Method>.g.cs`
— already TFM-scoped, already gitignored, and invisible to csharpier.
For each attributed method, compare the generated body against the body deleted from `HEAD`
(`git show HEAD:<path>`), normalising whitespace **and** the generator's full qualification: it emits
`global::System.Buffer.BlockCopy` where the hand-written code said `Buffer.BlockCopy`, so collapse
`global::A.B.C.Type.Member` to `Type.Member` before comparing (a greedy
`global::(?:[A-Za-z0-9_]+\.)*([A-Za-z0-9_]+\.[A-Za-z0-9_]+)` → `$1` does it). Batch 1 was verified
this way with a throwaway Python script — 18/18 bodies token-identical. **Expected result is an empty
For each attributed method, compare the generated body against the body deleted from a baseline commit
(`git show <rev>:<path>`). Batches 1 and 2 were verified this way with a throwaway Python script that
walks the generated directory, parses each generated declaration, finds the same-signature method in
the baseline and diffs the bodies — 48/48 identical across both TFMs. **Expected result is an empty
diff**; itemise any non-empty hunk in the commit message.
Four things that will otherwise produce false results, all learned the hard way:
- **`obj/` is not cleaned by an incremental build.** If nothing changed the generator does not re-run,
so you compare against stale `.g.cs` from a previous attempt. Use `-t:Rebuild`, or delete
`obj/Release/<tfm>/generated` first.
- **The baseline is per batch.** Once a batch is committed `HEAD` no longer contains the code it
deleted, so earlier batches must be compared against the commit *before* that batch.
- **The generator fully qualifies types and reflows fluent calls.** It emits
`new global::System.ObjectDisposedException(...)` and splits `BaseStream\n.Read(...)` over lines.
Strip `global::` and type qualifiers **both before and after** collapsing whitespace — each form
only matches in one of the two passes.
- **Extension invocations become static calls.** `this.Skip()` is emitted as
`StreamExtensions.Skip(this)`. Canonicalise one form to the other before comparing.
A generated method whose signature is *absent* from the baseline is the important signal: the batch
created a new member instead of replacing a duplicate. Report those separately and fail on them.
If a dump has to survive `clean`, override `CompilerGeneratedFilesOutputPath` — but build one TFM at
a time (it is a global property with no `$(TargetFramework)` expansion, so a multi-TFM build races all
six inner builds into one directory) and point it **outside the repo tree** so csharpier never sees it.
@@ -127,18 +143,31 @@ hand-written — not noise to override.
Ordered by (duplication removed) / risk.
### 2 — `IO/` leaf stream shims · ~150 lines · low risk
### 2 — `IO/` leaf stream shims — **done**, 17 methods, 190 lines
`ReadOnlySubStream`, `BufferedSubStream`, `SourceStream`, `SeekableSharpCompressStream`,
`ProgressReportingStream`, `Common/EntryStream`.
`ProgressReportingStream`, `CountingStream`, `Common/EntryStream`.
- `ReadOnlySubStream` is the one clean case where **both** overloads are attributed — a hand-written
`Read(Span<byte>)` exists (`ReadOnlySubStream.cs:78`) and matches the generated form.
- `[SkipSyncVersion]` on every `Stream.DisposeAsync`.
- `IO/CountingStream` needs `partial` added; then its `FlushAsync`/`ReadAsync`/`WriteAsync` map onto
existing hand-written members.
- Leave `SharpCompressStream` for later (stateful, 274-line async partial; also needs the
`ReadAsyncCore` rename and the `ReadAsync(Memory<byte>)` rewrite, below).
What it removed: `Read(byte[],int,int)` from all seven; `Read(Span<byte>)` from `CountingStream`,
`ProgressReportingStream` and `SeekableSharpCompressStream`; `Write(byte[],int,int)` and
`Write(ReadOnlySpan<byte>)` from `SeekableSharpCompressStream`/`CountingStream`; `Flush()` from those
two; `BufferedSubStream.RefillCache()`; `EntryStream.SkipEntry()`.
Notes from doing it:
- `IO/CountingStream` needed `partial` added (it has no `.Async.cs` — both halves live in one file).
- `Flush`/`FlushAsync` **can** be generated when the body is a pure delegation
(`SeekableSharpCompressStream`, `CountingStream`); the "leave hand-written" rule below is about
pairs whose semantics differ, not about the method name.
- `ReadOnlySubStream.Read(Span<byte>)` **stays hand-written** — see the generator limitation below.
Its `Read(byte[],int,int)` is generated.
- Not attributed, deliberately: every `DisposeAsync`; `SeekableSharpCompressStream.CopyToAsync`
(no hand-written `CopyTo(Stream,int)` twin); and the `Memory<byte>` overloads of
`BufferedSubStream`, `SourceStream` and `EntryStream`, which have no `Read(Span<byte>)` twin.
- `EntryStream`: the informative doc comment lived on the sync `SkipEntry`; it moved to
`SkipEntryAsync`, which is now the single source of truth.
- `SharpCompressStream` was left for later (stateful, 274-line async partial; also needs the
`ReadAsyncCore` rename and the `ReadAsync(Memory<byte>)` rewrite listed below).
### 3 — XZ reader family · ~120 lines · low-med risk
@@ -279,18 +308,68 @@ it when the async method awaits several operations at once.
**Budget: at most two `SYNC_ONLY` sites per method, and never more than ~15% of its lines.** Past
that, two honest files beat one half-preprocessor file.
## Known generator limitations
- **`Memory<T>` overloads only convert when the buffer is passed through unmodified.** Translating
`stream.ReadAsync(memory, ct)` to `stream.Read(span)` involves appending `.Span` to the argument;
if the async body slices first (`_stream.ReadAsync(buffer.Slice(0, n), ct)`), the generated code is
`_stream.Read(buffer.Slice(0, n).Span)` — and `buffer` is already a `Span<byte>` once the parameter
is converted, so it fails with `CS1061: 'Span<byte>' does not contain a definition for 'Span'`.
That is why `ReadOnlySubStream.Read(Span<byte>)` stays hand-written (with a comment saying so),
while the direct-pass-through cases (`CountingStream`, `ProgressReportingStream`,
`SeekableSharpCompressStream`) generate fine. It is a compile error, not a silent miscompile.
- **Only a trailing `Async` is stripped** — `FooAsyncCore` and `OpenAsyncReader` are not renamed.
- **Task-returning expressions that aren't awaited** are not rewritten: `new ValueTask<T>(x)` and
`ValueTask.FromResult(x)` need an `async`/`await` rewrite first. A non-`async` method that simply
*returns* an `XAsync(...)` call is fine (`Lzma2Filter` and `SeekableSharpCompressStream` prove it).
## Leave hand-written
Record the decision once, as a comment at the method, so it is not re-litigated.
- `Dispose`/`DisposeAsync`, `Flush`/`FlushAsync`, `CopyTo`/`CopyToAsync` — framework semantics
genuinely differ, and on a `Stream` the generated `Dispose()` cannot override the non-virtual
`Stream.Dispose()` (`CS0506`); the real override is `Dispose(bool)`.
- `Dispose`/`DisposeAsync` — on a `Stream` the generated `Dispose()` cannot override the non-virtual
`Stream.Dispose()` (`CS0506`); the real override is `Dispose(bool)`. (On a plain `IDisposable` such
as `OutWindow` or LZMA's `Decoder`, `DisposeAsync`→`Dispose()` is legal.)
- `ReadByteAsync`/`WriteByteAsync` on a `Stream` — generated without `override`, hides
`Stream.ReadByte`/`WriteByte` (`CS0108`).
- Pairs whose difference is an optimisation spread over >2 sites or >20% of the body — e.g.
`Xz/BinaryUtils`.
- `Memory`/`ReadOnlyMemory` overloads with no sync twin.
- `Memory`/`ReadOnlyMemory` overloads with no sync twin — see the symmetry question below.
- `Flush`/`FlushAsync` and `CopyTo`/`CopyToAsync` **only** where the two bodies genuinely differ.
Pure delegations convert cleanly and were converted in batch 2.
## Should the sync and async surfaces match?
Worth settling deliberately, because the answer decides whether some of the remaining asymmetries are
"leave alone" or "a batch of their own". `Stream`'s base-class defaults mean an asymmetric type is not
neutral:
| Not overridden | What the base class does |
| --- | --- |
| `ReadAsync(byte[],int,int,ct)` | `BeginRead`/`EndRead`, i.e. runs the **sync** `Read` on a thread-pool thread |
| `FlushAsync(ct)` | runs the **sync** `Flush` on a thread-pool thread |
| `ReadAsync(Memory<byte>,ct)` | delegates to the `byte[]` overload, renting + copying when the memory is not array-backed |
| `Read(Span<byte>)` | rents an array, calls `Read(byte[],int,int)`, copies back |
| `DisposeAsync()` | calls the sync `Dispose()` |
| `CopyToAsync(Stream,int,ct)` | generic `ReadAsync`/`WriteAsync` loop, skipping any inner fast path |
So: **for wrapper/delegating streams the surfaces should match**, and the generator makes that nearly
free — the async body is written once and the sync twin is emitted. Two distinct asymmetries exist
today:
1. **Async present, sync missing** — `ReadAsync(Memory<byte>)` with no `Read(Span<byte>)`
(`Filter`, `BufferedSubStream`, `SourceStream`, `EntryStream`, all six XZ branch filters, and
more). Attributing these *creates* the missing `Read(Span<byte>)`, replacing the base rent-and-copy
shim. That is a real improvement and makes the surfaces symmetric, but it is **not** a
deduplication: it changes behaviour, so it does not belong in a batch advertised as a no-op. Do it
as its own "symmetry pass" commit, one type at a time, and let the benchmark job see it.
2. **Sync present, async missing** — the async path then blocks a thread-pool thread. On a
delegating wrapper this is worth fixing on its own merits (write the async override, attribute it,
delete the sync one), and it is the only case where new *async* code should be written as part of
this campaign.
`DisposeAsync` is the deliberate exception in both directions: it cannot be generated on a `Stream`,
so its symmetry stays hand-maintained.
## Hard-blocked

View File

@@ -9,8 +9,9 @@ namespace SharpCompress.Common;
public partial class EntryStream
{
/// <summary>
/// Asynchronously skip the rest of the entry stream.
/// When reading a stream from OpenEntryStream, the stream must be completed so use this to finish reading the entire entry.
/// </summary>
[Zomp.SyncMethodGenerator.CreateSyncVersion]
public async ValueTask SkipEntryAsync(CancellationToken cancellationToken = default)
{
await this.SkipAsync(cancellationToken).ConfigureAwait(false);
@@ -47,6 +48,7 @@ public partial class EntryStream
}
#endif
[Zomp.SyncMethodGenerator.CreateSyncVersion]
public override async Task<int> ReadAsync(
byte[] buffer,
int offset,

View File

@@ -21,15 +21,6 @@ public partial class EntryStream : Stream
_stream = stream;
}
/// <summary>
/// When reading a stream from OpenEntryStream, the stream must be completed so use this to finish reading the entire entry.
/// </summary>
public void SkipEntry()
{
this.Skip();
_completed = true;
}
protected override void Dispose(bool disposing)
{
if (_isDisposed)
@@ -93,16 +84,6 @@ public partial class EntryStream : Stream
set => throw new NotSupportedException();
}
public override int Read(byte[] buffer, int offset, int count)
{
var read = _stream.Read(buffer, offset, count);
if (read <= 0)
{
_completed = true;
}
return read;
}
public override int ReadByte()
{
var value = _stream.ReadByte();

View File

@@ -8,6 +8,7 @@ namespace SharpCompress.IO;
internal partial class BufferedSubStream
{
[Zomp.SyncMethodGenerator.CreateSyncVersion]
private async ValueTask RefillCacheAsync(CancellationToken cancellationToken)
{
if (_isDisposed)
@@ -35,6 +36,7 @@ internal partial class BufferedSubStream
BytesLeftToRead -= _cacheLength;
}
[Zomp.SyncMethodGenerator.CreateSyncVersion]
public override async Task<int> ReadAsync(
byte[] buffer,
int offset,

View File

@@ -59,55 +59,6 @@ internal partial class BufferedSubStream : Stream, IStreamStack
set => throw new NotSupportedException();
}
private void RefillCache()
{
if (_isDisposed)
{
throw new ObjectDisposedException(nameof(BufferedSubStream));
}
var count = (int)Math.Min(BytesLeftToRead, _cache!.Length);
_cacheOffset = 0;
if (count == 0)
{
_cacheLength = 0;
return;
}
// Only seek if we're not already at the correct position
// This avoids expensive seek operations when reading sequentially
if (_stream.CanSeek && _stream.Position != origin)
{
_stream.Position = origin;
}
_cacheLength = _stream.Read(_cache, 0, count);
origin += _cacheLength;
BytesLeftToRead -= _cacheLength;
}
public override int Read(byte[] buffer, int offset, int count)
{
if (count > Length)
{
count = (int)Length;
}
if (count > 0)
{
if (_cacheOffset == _cacheLength)
{
RefillCache();
}
count = Math.Min(count, _cacheLength - _cacheOffset);
Buffer.BlockCopy(_cache!, _cacheOffset, buffer, offset, count);
_cacheOffset += count;
}
return count;
}
public override int ReadByte()
{
if (_cacheOffset == _cacheLength)

View File

@@ -8,7 +8,7 @@ namespace SharpCompress.IO;
/// <summary>
/// A simple stream wrapper that counts bytes read and written without buffering.
/// </summary>
internal class CountingStream : Stream
internal partial class CountingStream : Stream
{
private readonly Stream _stream;
private long _bytesRead;
@@ -45,18 +45,10 @@ internal class CountingStream : Stream
set => _stream.Position = value;
}
public override void Flush() => _stream.Flush();
[Zomp.SyncMethodGenerator.CreateSyncVersion]
public override async Task FlushAsync(CancellationToken cancellationToken) =>
await _stream.FlushAsync(cancellationToken).ConfigureAwait(false);
public override int Read(byte[] buffer, int offset, int count)
{
var read = _stream.Read(buffer, offset, count);
_bytesRead += read;
return read;
}
public override int ReadByte()
{
var value = _stream.ReadByte();
@@ -68,31 +60,17 @@ internal class CountingStream : Stream
return value;
}
#if !LEGACY_DOTNET
public override int Read(Span<byte> buffer)
{
var read = _stream.Read(buffer);
_bytesRead += read;
return read;
}
#endif
public override long Seek(long offset, SeekOrigin origin) => _stream.Seek(offset, origin);
public override void SetLength(long value) => _stream.SetLength(value);
public override void Write(byte[] buffer, int offset, int count)
{
_stream.Write(buffer, offset, count);
_bytesWritten += count;
}
public override void WriteByte(byte value)
{
_stream.WriteByte(value);
_bytesWritten++;
}
[Zomp.SyncMethodGenerator.CreateSyncVersion]
public override async Task WriteAsync(
byte[] buffer,
int offset,
@@ -104,6 +82,7 @@ internal class CountingStream : Stream
_bytesWritten += count;
}
[Zomp.SyncMethodGenerator.CreateSyncVersion]
public override async Task<int> ReadAsync(
byte[] buffer,
int offset,
@@ -119,6 +98,7 @@ internal class CountingStream : Stream
}
#if !LEGACY_DOTNET
[Zomp.SyncMethodGenerator.CreateSyncVersion]
public override async ValueTask<int> ReadAsync(
Memory<byte> buffer,
CancellationToken cancellationToken = default

View File

@@ -7,6 +7,7 @@ namespace SharpCompress.IO;
internal sealed partial class ProgressReportingStream
{
[Zomp.SyncMethodGenerator.CreateSyncVersion]
public override async Task<int> ReadAsync(
byte[] buffer,
int offset,
@@ -26,6 +27,7 @@ internal sealed partial class ProgressReportingStream
}
#if !LEGACY_DOTNET
[Zomp.SyncMethodGenerator.CreateSyncVersion]
public override async ValueTask<int> ReadAsync(
Memory<byte> buffer,
CancellationToken cancellationToken = default

View File

@@ -51,30 +51,6 @@ internal sealed partial class ProgressReportingStream : Stream
public override void Flush() => _baseStream.Flush();
public override int Read(byte[] buffer, int offset, int count)
{
var bytesRead = _baseStream.Read(buffer, offset, count);
if (bytesRead > 0)
{
_bytesTransferred += bytesRead;
ReportProgress();
}
return bytesRead;
}
#if !LEGACY_DOTNET
public override int Read(Span<byte> buffer)
{
var bytesRead = _baseStream.Read(buffer);
if (bytesRead > 0)
{
_bytesTransferred += bytesRead;
ReportProgress();
}
return bytesRead;
}
#endif
public override int ReadByte()
{
var value = _baseStream.ReadByte();

View File

@@ -7,6 +7,7 @@ namespace SharpCompress.IO;
internal partial class ReadOnlySubStream
{
[Zomp.SyncMethodGenerator.CreateSyncVersion]
public override async Task<int> ReadAsync(
byte[] buffer,
int offset,

View File

@@ -45,21 +45,6 @@ internal partial class ReadOnlySubStream : Stream, IStreamStack
set => throw new NotSupportedException();
}
public override int Read(byte[] buffer, int offset, int count)
{
if (BytesLeftToRead < count)
{
count = (int)BytesLeftToRead;
}
var read = _stream.Read(buffer, offset, count);
if (read > 0)
{
BytesLeftToRead -= read;
_position += read;
}
return read;
}
public override int ReadByte()
{
if (BytesLeftToRead <= 0)
@@ -76,6 +61,8 @@ internal partial class ReadOnlySubStream : Stream, IStreamStack
}
#if !LEGACY_DOTNET
// Not generated from ReadAsync(Memory<byte>): the generator emits `.Span` on the sliced
// argument, which is already a Span<byte> once the parameter is converted.
public override int Read(Span<byte> buffer)
{
var sliceLen = BytesLeftToRead < buffer.Length ? BytesLeftToRead : buffer.Length;

View File

@@ -8,6 +8,7 @@ namespace SharpCompress.IO;
internal sealed partial class SeekableSharpCompressStream
{
[Zomp.SyncMethodGenerator.CreateSyncVersion]
public override Task<int> ReadAsync(
byte[] buffer,
int offset,
@@ -16,11 +17,13 @@ internal sealed partial class SeekableSharpCompressStream
) => _stream.ReadAsync(buffer, offset, count, cancellationToken);
#if !LEGACY_DOTNET
[Zomp.SyncMethodGenerator.CreateSyncVersion]
public override ValueTask<int> ReadAsync(
Memory<byte> buffer,
CancellationToken cancellationToken = default
) => _stream.ReadAsync(buffer, cancellationToken);
[Zomp.SyncMethodGenerator.CreateSyncVersion]
public override ValueTask WriteAsync(
ReadOnlyMemory<byte> buffer,
CancellationToken cancellationToken = default
@@ -47,6 +50,7 @@ internal sealed partial class SeekableSharpCompressStream
}
#endif
[Zomp.SyncMethodGenerator.CreateSyncVersion]
public override Task WriteAsync(
byte[] buffer,
int offset,
@@ -54,6 +58,7 @@ internal sealed partial class SeekableSharpCompressStream
CancellationToken cancellationToken
) => _stream.WriteAsync(buffer, offset, count, cancellationToken);
[Zomp.SyncMethodGenerator.CreateSyncVersion]
public override Task FlushAsync(CancellationToken cancellationToken) =>
_stream.FlushAsync(cancellationToken);

View File

@@ -46,26 +46,10 @@ internal sealed partial class SeekableSharpCompressStream : SharpCompressStream
internal override bool IsRecording => _recordedPosition.HasValue;
public override void Flush() => _stream.Flush();
public override int Read(byte[] buffer, int offset, int count) =>
_stream.Read(buffer, offset, count);
#if !LEGACY_DOTNET
public override int Read(Span<byte> buffer) => _stream.Read(buffer);
#endif
public override long Seek(long offset, SeekOrigin origin) => _stream.Seek(offset, origin);
public override void SetLength(long value) => _stream.SetLength(value);
public override void Write(byte[] buffer, int offset, int count) =>
_stream.Write(buffer, offset, count);
#if !LEGACY_DOTNET
public override void Write(ReadOnlySpan<byte> buffer) => _stream.Write(buffer);
#endif
public override void Rewind(bool stopRecording = false)
{
if (!_recordedPosition.HasValue)

View File

@@ -9,6 +9,7 @@ namespace SharpCompress.IO;
public partial class SourceStream
{
[Zomp.SyncMethodGenerator.CreateSyncVersion]
public override async Task<int> ReadAsync(
byte[] buffer,
int offset,

View File

@@ -138,47 +138,6 @@ public partial class SourceStream : Stream, IStreamStack
public override void Flush() => Current.Flush();
public override int Read(byte[] buffer, int offset, int count)
{
if (count <= 0)
{
return 0;
}
var total = count;
var r = -1;
while (count != 0 && r != 0)
{
r = Current.Read(
buffer,
offset,
(int)Math.Min(count, Current.Length - Current.Position)
);
count -= r;
offset += r;
if (!IsVolumes && count != 0 && Current.Position == Current.Length)
{
var length = Current.Length;
// Load next file if present
if (!SetStream(_stream + 1))
{
break;
}
// Current stream switched
// Add length of previous stream
_prevSize += length;
Current.Seek(0, SeekOrigin.Begin);
r = -1; //BugFix: reset to allow loop if count is still not 0 - was breaking split zipx (lzma xz etc)
}
}
return total - count;
}
public override long Seek(long offset, SeekOrigin origin)
{
var pos = Position;