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 class method overloads for ReadOnlyMemory and ReadOnlySpan #35054

Closed
Tan90909090 opened this issue Apr 16, 2020 · 10 comments
Labels
api-ready-for-review API is ready for review, it is NOT ready for implementation area-System.IO
Milestone

Comments

@Tan90909090
Copy link

Tan90909090 commented Apr 16, 2020

Related to #30305.

Motivation

Currently, File APIs for writing a file only accept byte[] and string.
For instance, File.WriteAllBytesAsync only accepts byte[]. When I want to write a ReadOnlyMemory<byte> to file, I have two choises.

  1. Call ReadOnlyMemory<byte>.ToArray() then call File.WriteAllBytesAsync(string, byte[], CancellationToken). But this allocates extra memory.
  2. Create a FileStream instance with an appropriate FileOptions and call FileStream.WriteAsync(ReadOnlyMemory<byte>, CancellationToken). But this choise is verbose.

It is convenient to be able to pass ReadOnlyMemory<byte> to File.WriteAllBytesAsync() directly.

Proposal

namespace System.IO
{
    public static class File
    {
        // it seems like path might be better as ReadOnlyMemory<char>/ReadOnlySpan<char>
        // in these methods, but FileStream doesn't currently support that.
        public static ValueTask AppendAllLinesAsync(string path, IEnumerable<ReadOnlyMemory<char>> contents, CancellationToken cancellationToken = default);
        public static ValueTask AppendAllLinesAsync(string path, IEnumerable<ReadOnlyMemory<char>> contents, Encoding encoding, 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 ValueTask AppendAllTextAsync(string path, ReadOnlyMemory<char> contents, CancellationToken cancellationToken = default);
        public static ValueTask AppendAllTextAsync(string path, ReadOnlyMemory<char> contents, Encoding encoding, CancellationToken cancellationToken = default);
        public static ValueTask WriteAllLinesAsync(string path, IEnumerable<ReadOnlyMemory<char>> contents, CancellationToken cancellationToken = default);
        public static ValueTask WriteAllLinesAsync(string path, IEnumerable<ReadOnlyMemory<char>> contents, Encoding encoding, 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 ValueTask WriteAllTextAsync(string path, ReadOnlyMemory<char> contents, CancellationToken cancellationToken = default);
        public static ValueTask WriteAllTextAsync(string path, ReadOnlyMemory<char> contents, Encoding encoding, CancellationToken cancellationToken = default);
        public static void WriteAllBytes(string path, ReadOnlySpan<byte> bytes);
        public static ValueTask WriteAllBytesAsync(string path, ReadOnlyMemory<byte> bytes, CancellationToken cancellationToken = default);
    }
}
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added area-System.IO untriaged New issue has not been triaged by the area owner labels Apr 16, 2020
@ghost
Copy link

ghost commented Apr 16, 2020

Tagging subscribers to this area: @carlossanlop, @jozkee
Notify danmosemsft if you want to be subscribed.

@Tan90909090 Tan90909090 changed the title API proposal: Add File.WriteAllBytesAsync for ReadOnlyMemory<byte> API proposal: Add File.WriteAllBytesAsync overload for ReadOnlyMemory<byte> Apr 16, 2020
@carlossanlop
Copy link
Member

Thanks for the contribution, @Tan90909090.

I noticed that in the source code for File.WriteAllBytesAsync, we create a FileStream and call WriteAsync, converting the byte[] to a ReadOnlyMemory<byte>. So I see the value of having the proposed API.

@carlossanlop carlossanlop added api-suggestion Early API idea and discussion, it is NOT ready for implementation and removed untriaged New issue has not been triaged by the area owner labels Apr 17, 2020
@GrabYourPitchforks
Copy link
Member

@carlossanlop Should we mark this as ready for review? Seems like it's actionable by the API reviewers.

@Tan90909090
Copy link
Author

Tan90909090 commented Apr 20, 2020

Oh! I forget File APIs targeting string!
In the source, File.AppendAllTextAsync and File.WriteAllTextAsync call InternalWriteAllTextAsync, which a create StreamWriter and call WriteAsync, converting the string to a ReadOnlyMemory<char>.
Currently File.AppendAllLinesAsync and File.WriteAllLinesAsync don't use ReadOnlyMemory<char> but I think they are also able to use ReadOnlyMemory<char>.
More proposal:

namespace System.IO
{
    public static class File
    {
+       public static ValueTask AppendAllLinesAsync(string path, IEnumerable<ReadOnlyMemory<char>> contents, CancellationToken cancellationToken = default);
+       public static ValueTask AppendAllLinesAsync(string path, IEnumerable<ReadOnlyMemory<char>> contents, Encoding encoding, CancellationToken cancellationToken = default);
+       public static ValueTask AppendAllTextAsync(string path, ReadOnlyMemory<char> contents, CancellationToken cancellationToken = default);
+       public static ValueTask AppendAllTextAsync(string path, ReadOnlyMemory<char> contents, Encoding encoding, CancellationToken cancellationToken = default);
+       public static ValueTask WriteAllLinesAsync(string path, IEnumerable<ReadOnlyMemory<char>> contents, CancellationToken cancellationToken = default);
+       public static ValueTask WriteAllLinesAsync(string path, IEnumerable<ReadOnlyMemory<char>> contents, Encoding encoding, CancellationToken cancellationToken = default);
+       public static ValueTask WriteAllTextAsync(string path, ReadOnlyMemory<char> contents, CancellationToken cancellationToken = default);
+       public static ValueTask WriteAllTextAsync(string path, ReadOnlyMemory<char> contents, Encoding encoding, CancellationToken cancellationToken = default);
    }
}

Question: Should I merge a description and change the issue title, or open another issue about APIs for ReadOnlyMemory<char>?

@carlossanlop
Copy link
Member

Good catch, @Tan90909090. I updated your main issue description with all the APIs from your last comment.
I'm also marking this as ready for review, @GrabYourPitchforks.

@carlossanlop carlossanlop added api-ready-for-review and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Apr 20, 2020
@carlossanlop carlossanlop added this to the Future milestone Apr 20, 2020
@Tan90909090
Copy link
Author

It seemed that a public static ValueTask WriteAllTextAsync(string path, ReadOnlyMemory<char> contents, CancellationToken cancellationToken = default) is missing so I added this.
Thank you for your quick action.

@Tan90909090
Copy link
Author

Tan90909090 commented Apr 26, 2020

I also forgot about synchronous APIs... I updated this issue description for consistency.
Since there are FileStream.Write(ReadOnlySpan<Byte>) and StreamWriter.Write(ReadOnlySpan<Char>), the File class should be able to implement these methods.
Note: IEnumerable<ReadOnlySpan<T>> is prohibited so I omited AppendAllLines and WriteAllLines overloads.

@Tan90909090 Tan90909090 changed the title API proposal: Add File.WriteAllBytesAsync overload for ReadOnlyMemory<byte> API proposal: Add File class method overloads for ReadOnlyMemory and ReadOnlySpan Apr 26, 2020
@carlossanlop carlossanlop added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-ready-for-review labels Jul 6, 2020
@jkotas
Copy link
Member

jkotas commented Jul 24, 2020

There is not much benefit in returning ValueTask from these APIs. They should be able to use Task just fine.

@GrabYourPitchforks
Copy link
Member

@carlossanlop Do you suppose any of our existing internal call sites would be able to use the new proposed overloads? I [very briefly] looked through source.dot.net and didn't immediately see any good candidates.

@terrajobst
Copy link
Member

terrajobst commented Aug 7, 2020

Video

  • It seems odd to combine a low-level concept like span/memory with very high-level APIs like "write all this data to file".
  • The goal is to make it very convenient to write some data to a file, not performance.
  • My concern is that these additions would "pollute" a core API that is frequently used by beginners. The API is designed to grow to expose users to more concepts, such as StreamReader and FileStream.
  • If the arugment is that creating a file stream is too hard, for example, opening it for async, I'd rather we add dedicated methods for that, such as OpenForAsync.
  • Unless there is a compelling reason for those APIs, we're inclined to say no.
namespace System.IO
{
    public static class File
    {
        public static ValueTask AppendAllLinesAsync(string path, IEnumerable<ReadOnlyMemory<char>> contents, CancellationToken cancellationToken = default);
        public static ValueTask AppendAllLinesAsync(string path, IEnumerable<ReadOnlyMemory<char>> contents, Encoding encoding, 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 ValueTask AppendAllTextAsync(string path, ReadOnlyMemory<char> contents, CancellationToken cancellationToken = default);
        public static ValueTask AppendAllTextAsync(string path, ReadOnlyMemory<char> contents, Encoding encoding, CancellationToken cancellationToken = default);
        public static ValueTask WriteAllLinesAsync(string path, IEnumerable<ReadOnlyMemory<char>> contents, CancellationToken cancellationToken = default);
        public static ValueTask WriteAllLinesAsync(string path, IEnumerable<ReadOnlyMemory<char>> contents, Encoding encoding, 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 ValueTask WriteAllTextAsync(string path, ReadOnlyMemory<char> contents, CancellationToken cancellationToken = default);
        public static ValueTask WriteAllTextAsync(string path, ReadOnlyMemory<char> contents, Encoding encoding, CancellationToken cancellationToken = default);
        public static void WriteAllBytes(string path, ReadOnlySpan<byte> bytes);
        public static ValueTask WriteAllBytesAsync(string path, ReadOnlyMemory<byte> bytes, CancellationToken cancellationToken = default);
    }
}

@ghost ghost locked as resolved and limited conversation to collaborators Dec 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-ready-for-review API is ready for review, it is NOT ready for implementation area-System.IO
Projects
None yet
Development

No branches or pull requests

6 participants