Runtime: API Proposal: Path Span APIs that write into a specified buffer

[JeremyKuhne]

We've added System.IO.Path overloads that take spans and output strings. To facilitate additional scenarios we should create overloads that allow you to specify the buffer to write to, rather than creating a string. Path.GetFullPath and Path.Combine are the key APIs here.

We've added the following for 2.1:

``` C#
namespace System.IO
{
public static class Path
{
public static bool TryJoin(ReadOnlySpan path1, ReadOnlySpan path2, Span destination, out int charsWritten);
public static bool TryJoin(ReadOnlySpan path1, ReadOnlySpan path2, ReadOnlySpan path3, Span destination, out int charsWritten)
}
}

What we'd like to consider adding:

``` C#
namespace System.IO
{
    public static class Path
    {
        public static bool TryGetTempPath(Span<char> destination, out int charsWritten);
        public static bool TryGetRandomFileName(Span<char> destination, out int charsWritten);
        public static bool TryGetRelativePath(ReadOnlySpan<char> relativeTo, ReadOnlySpan<char> path, Span<char> destination, out int charsWritten);
        public static bool TryGetFullPath(ReadOnlySpan<char> path, Span<char> destination, out int charsWritten);
        public static bool TryGetFullPath(ReadOnlySpan<char> path, ReadOnlySpan<char> basePath, Span<char> destination, out int charsWritten);
    }
}

Notes

• Often we know what the needed buffer size will likely be, how do / should we express that from the API?
  
  
  
  • We don't know positively as the current working directory can be changed, so the char count _can_ change
  
  

[MarcoRossignoli]

when you say buffer you mean Memory<char>?

[ViktorHofer]

I believe Jeremy is talking about Span<char> destination overloads, i.e. public static int GetFullPath(ReadOnlySpan<char> path, ReadOnlySpan<char> basePath, Span<char> destination).

[MarcoRossignoli]

@ViktorHofer you are right, no async or need of reference here

[JeremyKuhne]

Note that we did add public static bool TryJoin(ReadOnlySpan<char> path1, ReadOnlySpan<char> path2, Span<char> destination, out int charsWritten) in 2.1. TryGetFullPath() is still pending. I'll update the description.

[MarcoRossignoli]

@JeremyKuhne do you need an official api proposal like this?I've never written one i could try to help. One question, is there already some docs on Tryxxx new api?Or i need to read code to understand?

[JeremyKuhne]

@MarcoRossignoli this one is mostly together. As we're wrapping up 2.1 right now and I wanted to give a chance for some feedback I've left it at "api needs work". If you have any feedback it would be very much appreciated. If you're interested in trying to implement some part of this I can push this forward to a formal review.

[Anipik]

@JeremyKuhne i will like to implement them if @MarcoRossignoli is not ineterested in implementing them.

[MarcoRossignoli]

@JeremyKuhne @Anipik i'd like to help on this, i did some check and if i understand well there are some overloads to add to internal helpers api to use span to avoid ToString() is it correct?I tried to do some simple tests using existing api and pass span.ToString() but when i test on corefx i get MembersMustExist in compile time, even if i added new
method to ref

MembersMustExist : Member 'System.IO.Path.TryGetTempPath(System.Span<System.Char>, System.Int32)' does not exist in the implementation but it does exist in the contract. System.Runtime.Extensions (src\System.Runtime.Extensions)         

Finally, i'd like to try, but i know we are near 2.1 release so i don't want to be a bottleneck, let me know if i can help! TYVM

[JeremyKuhne]

@MarcoRossignoli, @Anipik whatever we do here it would have to go in after we fork for release, so it will be a awhile before we can get things checked in. I'll try and get this through API review in the next few weeks. I'll open up issues for the API implementation when they are approved.

Anirudh, can you help Marco understand how the Path class flows through the repos?

[Anipik]

@MarcoRossignoli The steps involved for adding these apis will be

1. Adding the implementation of these in apis in path.cs class in mscorlib in coreclr repo.
2. Adding the method to the ref file in system.Runtime.Extensions Project.
3. Adding the tests for these new apis in System.Runtime.Extensions Project.

We will need to test these new apis by using a local build of coreclr. The steps to do testing are

1. Build the coreclr in release or debug mode.
2. While building the corefx include a switch
   -- /p:CoreCLROverridePath=C:\CodeHub\coreclr\bin\Product\Windows_NT.x64.Release\ 
3. Run the tests

@JeremyKuhne let me know if i missed anything

[ViktorHofer]

For more infos see https://github.com/dotnet/corefx/blob/master/Documentation/project-docs/developer-guide.md

[MarcoRossignoli]

I apologize @ViktorHofer @Anipik i did it. But i get MembersMustExist at compile time, as the methods added to ref project aren't found on "implementation"(i think on private lib). Is there some way to understand if i'm using my local clr build?Maybe so i could understand what i'm missing, I'm using debug build. TYVM for your time.

[ViktorHofer]

From the link I posted:

Once you have updated your CoreCLR you can run tests however you usually do (via build-tests.cmd, individual test project, in VS, etc) and it should be using your copy of CoreCLR. If you want to verify that your bits are being used have a look in \corefx\bin\testhost\netcoreapp-Windows_NT-Debug-x64\shared\Microsoft.NETCore.App\9.9.9 which is the shared framework directory used by corefx tests.

[MarcoRossignoli]

@ViktorHofer thanks, i'll try again!

[MarcoRossignoli]

@Anipik @ViktorHofer i understood the problem and i don't know if it's so by design(i don't find anything on this behaviour on docs, maybe i'm missing something). After build corefx with my clr bits it's all ok, the correct bins are in place and with reflector i can see my new methods on System.Private.CoreLib.dll in 9.9.9 folder.
But if i do a build:

...\corefx\src\System.Runtime.Extensions>..\..\build.cmd .

compile phase download remote clr package and override my local clr bits(date 14/03/2018), and after i get MemberMustExist error.
By the way error say that the methods should be in System.Runtime.Extensions.csproj, but here i don't find for instance new Path.TryJoin methods. I'm missing something but i don't understand what, the procedure on docs are clear and easy :rage:, for completeness my test code, here new added api on clr, here on corefx ref. Do you have any advice?

[ViktorHofer]

.\build.cmd builds the entire repo with the coreclr package from myget. if you just want to build a specific project use msbuild /t:RebuildAndTest or msbuild /t:Rebuild. Of course you've overridden your local build you need to invoke build.cmd once more with the CoreCLROverridePath property as described by @Anipik.

These steps are also described in the link but don't bother to reach out for more questions. We are happy to help.

[MarcoRossignoli]

ugh...i usually do this https://github.com/dotnet/corefx/blob/master/Documentation/project-docs/developer-guide.md#building-individual-libraries , now i try with msbuild. Can i attach with VS for tests or i have to use only msbuild commands?

but don't bother to reach out for more questions

too kind i don't want to waste your time.

[MarcoRossignoli]

@ViktorHofer step forward

msbuild /v:m /t:RebuildAndTest "/p:XunitOptions=-trait MyTrait=MyTrait"  System.Runtime.Extensions.Tests.csproj

this works well!
But if try to use F5 in VS(to debug) i get

Error       MembersMustExist : Member 'System.IO.Path.TryGetTempPath(System.Span<System.Char>, System.Int32)' does not exist in the implementation but it does exist in the contract.   System.Runtime.Extensions (src\System.Runtime.Extensions)

ApiCompat failed for ...\corefx\bin\Windows_NT.AnyCPU.Debug\System.Runtime.Extensions\netcoreapp\System.Runtime.Extensions.dll' System.Runtime.Extensions (src\System.Runtime.Extensions)           

Is it possible debug with VS also?

[danmosemsft]

@MarcoRossignoli did you do 2. Adding the method to the ref file in system.Runtime.Extensions Project. ? Perhaps point to a branch we could see what you have.

[JeremyKuhne]

@MarcoRossignoli the API has been approved. I've created issues for each of the APIs. I'd suggest you try and tackle TryGetTempPath() to start. If that sounds good I can assign that one to you. We can help you work through the process on that issue.

[MarcoRossignoli]

@danmosemsft here is what i've done on core fx https://github.com/dotnet/corefx/compare/master...MarcoRossignoli:27418_path the strange thing is that with msbuild command all works, but if i try to use VS debug i get the error.

[MarcoRossignoli]

@JeremyKuhne ok i can try with TryGetTempPath() TYVM

[danmosemsft]

Ypu are missing method definitions in the src folder. Even empty stubs will be fine. The error is saying that you have ref but not src.

[danmosemsft]

I would expect MSBuild to error the same if you build in the src folder.

[MarcoRossignoli]

i thought the same...but on src i don't find TryJoin() for instance https://github.com/dotnet/corefx/search?utf8=%E2%9C%93&q=TryJoin&type= , is on ref https://github.com/dotnet/corefx/blob/f8df80f76769aa842ecf2c79eb3e3250d473c7e3/src/System.Runtime.Extensions/ref/System.Runtime.Extensions.cs#L1230

[ViktorHofer]

TryJoin is implemented in coreclr not corefx here: https://source.dot.net/#System.Private.CoreLib/shared/System/IO/Path.cs,b222a5ed8b7295f5

[MarcoRossignoli]

yes @ViktorHofer i see...but i don't understand why if TryJoin is on corefx ref and not in src(like my others TryGetXXX methods i'm writing) it doesn't raise same error at compile time, if i understand well @danmosemsft expect that all ref methods sould be also on src folder(corefx/src/System.Runtime.Extensions/src/...) look at my branch:

https://github.com/dotnet/coreclr/compare/master...MarcoRossignoli:27418_path here i implemented methods(like TryJoin)

here i added empty stub(like TryJoin) https://github.com/dotnet/corefx/compare/master...MarcoRossignoli:27418_path

[ViktorHofer]

it doesn't raise same error at compile time

because the src assembly only acts as a façade and type forwards to System.Private.Corelib (the "mscorlib" from coreclr) i.e. when calling TryJoin.

if i understand well @danmosemsft expect that all ref methods sould be also on src folder(corefx/src/System.Runtime.Extensions/src/...) look at my branch:

It seems you misunderstood. Dan suggested that you can verify that error goes away when adding the method to the src assembly just for testing purposes. The implementation of these methods should take place in System.Private.Corelib and not in the src assembly of System.Runtime.Extensions. The error should go away as soon as you add the method in System.Private.Corelib.

[danmosemsft]

I wasn't clear at all 😃

[MarcoRossignoli]

Sorry i misspoke, now with msbuild command all works well no more compile error. Now i want to try to debug test with VS to explore variables etc...but when i press F5 on test project i get ApiCompat failed(https://github.com/dotnet/corefx/issues/27418#issuecomment-374652389).
Can you confirm that use "msbuild" or "F5 on VS" the behaviour should be the same?

[MarcoRossignoli]

@danmosemsft it's my fault, this is my first time on writing api cross repo, thank you so much for precious help, i hope to be less clumsy in future.

[danmosemsft]

@MarcoRossignoli where do we stand on this one?

[MarcoRossignoli]

@danmosemsft waiting for apis reviews.

[ViktorHofer]

Right, this hasn't yet gone through API review. Ping @karelz

[karelz]

@ViktorHofer it is marked as api-approved. What am I missing?

[MarcoRossignoli]

@karelz @ViktorHofer
i try to clarify, during api dev some discussion raised, after that @JeremyKuhne decided to review api again:
https://github.com/dotnet/coreclr/pull/17528#issuecomment-389356715
https://github.com/dotnet/coreclr/pull/17097#issuecomment-379839570

[karelz]

In that case @JeremyKuhne please provide details what needs to be discussed here and flip it back to api-ready-for-review

[JeremyKuhne]

Sorry, spoke with @terrajobst about this one. @terrajobst, can you confirm that we should go ahead with this as originally approved? And can you give guidance on follow-up for those that have issues with new approved API designs?

[karelz]

@JeremyKuhne Immo is often not responsive on GH issues - can you please close on it offline, or flip the issue to api-ready-for-review to force the discussion in API review? Thanks!

[terrajobst]

Let's flip it back and discuss it.

@JeremyKuhne

Sorry, spoke with @terrajobst about this one. @terrajobst, can you confirm that we should go ahead with this as originally approved?

Apologies but I really don't remember this discussion 😞 I know, I literally have had one job and I failed. Let's cover it next Tuesday. Tomorrow we're booked for UTF8 string.

[JeremyKuhne]

I spoke with @jkotas offline. The idea is that we put this on the back burner until either https://github.com/dotnet/corefxlab/issues/2177 or possibly dotnet/corefx#28379 are approved. The issue is without another type we can't really write a clean API here (outside of TryGetRandomFileName). I'll update the issue showing usage examples.

If we find that missing one of these APIs is blocking key user scenarios we can revisit adding these in the "less-than-ideal" shape if the two linked issues aren't coming online in time (i.e. these are critical for 2.2 and the other types don't make 2.2).

@MarcoRossignoli is this course of action ok? I really appreciate the effort you've put into this so far. As stated above, I'll update this issue with example usage code (with the shape as is and how they might be with ValueStringBuilder, etc.).

[MarcoRossignoli]

@JeremyKuhne no problem, i'm here to try to help...feel free to close stale PR, when things will be "mature" you can rely on me if you want!Thanks for your consideration!

[JeremyKuhne]

Here is a look at usage for TryGetRandomFileName() with an alternative.

Original proposal

``` C#
namespace System.IO
{
public static class Path
{
public static bool TryGetRandomFileName(Span destination, out int charsWritten);
}
}

public static class TempDirectoryGetter
{
private const string repo = nameof(repo);

// As in other discussions this is mostly safe to cache
private static string s_tempPath = Path.GetTempPath();

public unsafe static string GetTempDirectory(string subdirectory = repo)
{
    Span<char> buffer = stackalloc char[12];
    Path.TryGetRandomFileName(buffer, out _);
    return Path.Join(s_tempPath, subdirectory, buffer);
}

}

## Alternative proposal

``` C#
namespace System.IO
{
    public static class Path
    {
        // Span sliced to length (always 12) or 0 if failed
        public static Span<char> TryGetRandomFileName(Span<char> destination);
    }
}

public static class TempDirectoryGetter
{
    private const string repo = nameof(repo);
    private static string s_tempPath = Path.GetTempPath();

    public unsafe static string GetTempDirectory(string subdirectory = repo)
    {
        Span<char> buffer = stackalloc char[12];
        return Path.Join(s_tempPath, subdirectory, Path.TryGetRandomFileName(buffer));
    }
}

[JeremyKuhne]

GetTempPath() is something we may want to consider not adding. While the temp path can technically change at any time, the odds of it happening are pretty slim.

That said, internally we need to always look this up to be strictly correct (as this was never cached internally in the past). Other users may wish to always look this up as well.

[terrajobst]

Video

Let's remove TryGetTempPath but otherwise it looks good.

[fredrikhr]

For the discussion in the triage, could we get a public const int field on the Path class which returns the constant length of the random filename?

[JeremyKuhne]

Triage: We need to take existing feeback back into the design and resubmit. See the linked issues above for full discussions.