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

[API Proposal]: Add File.WriteAllBytes taking ReadOnlySpan<byte> #99823

Closed
KrzysztofCwalina opened this issue Mar 15, 2024 · 9 comments · Fixed by #103308
Closed

[API Proposal]: Add File.WriteAllBytes taking ReadOnlySpan<byte> #99823

KrzysztofCwalina opened this issue Mar 15, 2024 · 9 comments · Fixed by #103308
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.IO in-pr There is an active PR which will close this issue when it is merged
Milestone

Comments

@KrzysztofCwalina
Copy link
Member

KrzysztofCwalina commented Mar 15, 2024

EDITED by @stephentoub on 4/21/2024:
Exact same shape as existing APIs that take byte[]/string, just taking ReadOnlySpan<byte/char> instead for sync and ReadOnlyMemory<byte/char> for async.

public static class File
{
+   public static void WriteAllBytes(string path, ReadOnlySpan<byte> bytes);
+   public static Task WriteAllBytesAsync(string path, ReadOnlyMemory<byte> bytes, CancellationToken cancellationToken = default);

+   public static void WriteAllText(string path, ReadOnlySpan<char> contents);
+   public static void WriteAllText(string path, ReadOnlySpan<char> contents, Encoding encoding);
+   public static Task WriteAllTextAsync(string path, ReadOnlyMemory<byte> contents, CancellationToken cancellationToken = default);
+   public static Task WriteAllTextAsync(string path, ReadOnlyMemory<byte> contents, Encoding encoding, CancellationToken cancellationToken = default);

+   public static void AppendAllBytes(string path, ReadOnlySpan<byte> bytes);
+   public static Task AppendAllBytesAsync(string path, ReadOnlyMemory<byte> bytes, CancellationToken cancellationToken = default);

+   public static void AppendAllText(string path, ReadOnlySpan<char> contents);
+   public static void AppendAllText(string path, ReadOnlySpan<char> contents, Encoding encoding);
+   public static Task AppendAllTextAsync(string path, ReadOnlyMemory<char> contents, CancellationToken cancellationToken = default);
+   public static Task AppendAllTextAsync(string path, ReadOnlyMemory<char> contents, Encoding encoding, CancellationToken cancellationToken = default);
}

Background and motivation

Today, File.ReadAllBytes takes byte[] representing the bytes. It would be good to have an overload that takes ReadOnlySpan (sync ReadAllBytes) and ReadOnlyMemory (async ReadAllBytesAsync)

API Proposal

namespace System.IO;

public static class File
{
    public static void WriteAllBytes(string path, ReadOnlySpan<byte> bytes);
    public static void WriteAllBytes(string path, ReadOnlyMemory<byte> bytes);
    public static Task WriteAllBytesAsync(string path, ReadOnlyMemory<byte> bytes, CancellationToken cancellationToken = default);

    public static void AppendAllBytes(string path, ReadOnlySpan<byte> bytes);
    public static void AppendAllBytes(string path, ReadOnlyMemory<byte> bytes);
    public static Task AppendAllBytesAsync(string path, ReadOnlyMemory<byte> bytes, CancellationToken cancellationToken = default);
}

API Usage

File.WriteAllBytes(path, "Hello World!"u8);

Alternative Designs

Risks

No response

@KrzysztofCwalina KrzysztofCwalina added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Mar 15, 2024
Copy link
Contributor

Tagging subscribers to this area: @dotnet/area-system-io
See info in area-owners.md if you want to be subscribed.

@dotnet-policy-service dotnet-policy-service bot added the untriaged New issue has not been triaged by the area owner label Mar 15, 2024
@stephentoub
Copy link
Member

I'm actually surprised we don't have these. Went searching and found #35054 (comment). @terrajobst, @bartonjs, those concerns seem a bit stale; do we still have such objections?

@tannergooding
Copy link
Member

I'd personally like to see us add span APIs here and to other places where similar objections had been made in the past (like Console) as well.

The desire to reduce allocations exists no matter where you are and while users can achieve this by creating their own FileStream, that's less convenient and more error prone than just providing the same convenience API we're already providing.

Particularly with params ROSpan<T> now existing as well as newer features like collection expressions or InlineArrays preferring span as the target type, I imagine we'll be adding new public APIs to many of these places anyways and users will want the compiler+BCL to just do the efficient thing, so they don't need to think about it.

@bartonjs
Copy link
Member

My only hesitation is the ad infinitum pause. That is, is doing this enough? Should we be doing more at the same time? Where does it end?

I don't see anything wrong with the specific segment of the proposal, personally.

@neon-sunset
Copy link
Contributor

neon-sunset commented Mar 16, 2024

Synchronous ReadOnlyMemory<byte> overloads will cause ambiguous call issue for types that are implicitly convertible to both ROM and ROS but are not byte[]. Given that such overloads will be forwarding to ReadOnlySpan anyway, and users can just write memory.Span, is there a need for them?

@terrajobst
Copy link
Member

Where does it end?

Virtually all new APIs taking in buffers should be using ROS (sync) or ROM (async). If the API is popular, it should also takes arrays. For existing APIs this largely means adding overloads. It ends when we stop caring ;-)

@terrajobst
Copy link
Member

@neon-sunset I think we should only have ROS for the sync APIs and no ROM overloads:

namespace System.IO;

public static class File
{
    public static void WriteAllBytes(string path, ReadOnlySpan<byte> bytes);
    public static Task WriteAllBytesAsync(string path, ReadOnlyMemory<byte> bytes, CancellationToken cancellationToken = default);

    public static void AppendAllBytes(string path, ReadOnlySpan<byte> bytes);
    public static Task AppendAllBytesAsync(string path, ReadOnlyMemory<byte> bytes, CancellationToken cancellationToken = default);
}

@stephentoub stephentoub added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation untriaged New issue has not been triaged by the area owner labels Apr 22, 2024
@stephentoub stephentoub added this to the 9.0.0 milestone Apr 22, 2024
@stephentoub stephentoub self-assigned this May 5, 2024
@ramonsmits
Copy link

So yes its a duplicate of:

But this convenience method would be very welcome. Its a bit strange that it is to convert to a byte array. You could also just consider to drop the previous byte[] and string based overloads. Yes, a breaking changes but I don't know how .NET deals with droppen such legacy overloads. Needs to happen sometimes anyway :-)

@terrajobst
Copy link
Member

terrajobst commented Jun 11, 2024

Video

  • Looks good as proposed
public static class File
{
    public static void WriteAllBytes(string path, ReadOnlySpan<byte> bytes);
    public static Task WriteAllBytesAsync(string path, ReadOnlyMemory<byte> bytes, CancellationToken cancellationToken = default);

    public static void WriteAllText(string path, ReadOnlySpan<char> contents);
    public static void WriteAllText(string path, ReadOnlySpan<char> contents, Encoding encoding);
    public static Task WriteAllTextAsync(string path, ReadOnlyMemory<byte> contents, CancellationToken cancellationToken = default);
    public static Task WriteAllTextAsync(string path, ReadOnlyMemory<byte> contents, Encoding encoding, CancellationToken cancellationToken = default);

    public static void AppendAllBytes(string path, ReadOnlySpan<byte> bytes);
    public static Task AppendAllBytesAsync(string path, ReadOnlyMemory<byte> bytes, CancellationToken cancellationToken = default);

    public static void AppendAllText(string path, ReadOnlySpan<char> contents);
    public static void AppendAllText(string path, ReadOnlySpan<char> contents, Encoding encoding);
    public static Task AppendAllTextAsync(string path, ReadOnlyMemory<char> contents, CancellationToken cancellationToken = default);
    public static Task AppendAllTextAsync(string path, ReadOnlyMemory<char> contents, Encoding encoding, CancellationToken cancellationToken = default);
}

@terrajobst terrajobst added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Jun 11, 2024
@stephentoub stephentoub added the in-pr There is an active PR which will close this issue when it is merged label Jun 11, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Jul 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.IO in-pr There is an active PR which will close this issue when it is merged
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants