Runtime: Need Span based path join API

[JeremyKuhne]

We need an API that does a simple join of ReadOnlySpan<char> segments, adding a directory separator only if needed.

Rationale and Usage

We don't have API for combining paths that works with Span<char>. It will become common that segments of existing strings will be combined into paths. A simple example will be getting a filename from an existing path and adding it to the directory of another.

``` C#
// entry.FileName being a ReadOnlySpan, directory being a string or span
string path = Path.Join(directory, entry.FileName);

We could add overloads for `Path.Combine()`, but the behavior of returning the last argument that passes `IsPathRooted()` isn't intuitive, and is not desired in the common case. If the intent was to "resolve" a path against another, getting the final path correct is difficult to do, particularly on Windows.  Take the following:

``` C#
string result = Path.Combine(@"C:\Foo", @"\Bar");

This returns \Bar, which can only be correctly resolved (via Path.GetFullPath()) if the current directory happens to be on the C: drive. This can be better handled by the dotnet/runtime#24262 proposal for Path.GetFullPath().

Additionally, it is common for people to use the existing Combine() APIs on paths that aren't file system paths (URIs, registry paths, etc.).

Proposed API

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

Implementation Notes

• We will add the default directory separator if either type of separator is not already present (we'll allow an existing AltDirectorySeparator to be valid)
• Only spans that have a length will be combined
• We will not care about rooting (unlike the existing Combine APIs)
• This will not check the validity of the given paths in any way (character validity, etc)

[stephentoub]

Can you share some examples of where this would be used and the allocations it would save?

[JeremyKuhne]

Can you share some examples of where this would be used and the allocations it would save?

See dotnet/corefx#25426 and CombineNoChecks() for one example of allocation reduction (that I would replace with this). With dotnet/corefx#25539 this will enable various msbuild evaluation scenarios that they are essentially writing their own "span-like" internal APIs for.

This is also useful for scenarios where you don't have span sources and want straight-forward join behavior.

[justinvp]

Will having both Path.Combine and Path.Join APIs be confusing to users? How will users know/remember which API to use?

Maybe we should just add new overloads of Combine that accept ReadOnlySpan<char> that behave the same as the existing string-based overloads, and if there’s a need to provide new behavior, come up with some other approach to specify it, like a bool or PathCombineOption enum parameter. Or a method name like CombineXxx where Xxx describes the behavior, or at least denotes that it behaves differently from the normal Combine (e.g. CombineRaw, CombineExact, etc.)?

[JeremyKuhne]

like a bool or PathCombineOption enum parameter.

Not fond of this option. Note that we currently have 4 Combine() overloads. We can't add it to the one that takes a params, so that puts us at 7.

Or a method name like CombineXxx where Xxx describes the behavior

Possible, but I don't like any names I could come up with. It isn't obvious or even understood that Path.Combine has the "return last rooted argument" behavior, which makes describing the "not" harder. Additionally this won't have invalid char checks. While we're in the process of removing those char checks in Core, I want the ability to back port this API without using any quirks on Desktop.

This is really CombineCorrectly() or CombineCompletelyDifferently()- what I _really_ want to do long-term is depreciate the existing Combine methods and direct towards the new Path.Join() methods, Path.GetFullPath(), and the proposed Path.GetFullPath(root, path). While mentally transitioning isn't fun or easy, it is easier to audit with a new name I think.

It's easy to find how easily people stumble with the current API. Here is one thread from StackOverflow: Why does Path.Combine not properly concatenate filenames that start with Path.DirectorySeparatorChar? Another where it shows up: What is the worst gotcha in C# or .NET? 😜

To further illustrate how convoluted the existing API is, look at this blurb from our docs:

If path1 is not a drive reference (that is, "C:" or "D:") and does not end with a valid separator character as defined in DirectorySeparatorChar, AltDirectorySeparatorChar, or VolumeSeparatorChar, DirectorySeparatorChar is appended to path1 before concatenation.
If path2 does not include a root (for example, if path2 does not start with a separator character or a drive specification), the result is a concatenation of the two paths, with an intervening separator character. If path2 includes a root, path2 is returned.
The parameters are not parsed if they have white space. Therefore, if path2 includes white space (for example, " \file.txt "), the Combine method appends path2 to path1 instead of returning only path2.
Not all invalid characters for directory and file names are interpreted as unacceptable by the Combine method, because you can use these characters for search wildcard characters. For example, while Path.Combine("c:\", "*.txt") might be invalid if you were to create a file from it, it is valid as a search string. It is therefore successfully interpreted by the Combine method.

[terrajobst]

Video

It seems weird to me to introduce a new API without deprecating the old API. However, deprecating the old API would be fairly invasive (high usage).

1. Could we fix the behavior of Combine? We could quirk the behavior. @JeremyKuhne will take a look.
2. It seems less ideal but more acceptable to me to introduce an overload of Combine that takes Span with the new behavior. People would opt-into the new behavior by converting to Span. Reason being we have a unified set of verbs.

[karelz]

FYI: The API review discussion was recorded - see https://youtu.be/9F_NsviGT0Y?t=957 (30 min duration)

[JeremyKuhne]

This is now the span overload for Path.Combine.

[JeremyKuhne]

Based on internal discussions the original plan of keeping Combine semantics the same and using Join() is the current POR.

[ahsonkhan]

In many of our other APIs that have destination Span, the parameter occurs after the input ROS.

So, how about:
C# public static bool TryJoin(ReadOnlySpan<char> path1, ReadOnlySpan<char> path2, Span<char> destination, out int charsWritten);

Also, should we provide a TryJoin with 3 paths specified?

[JeremyKuhne]

@ahsonkhan I don't care what order the arguments are. I was following another one I saw. Presumably it was in that order due to the need to do overloads- if we have path1 and path2, then add 1,2,3- easier to get the right behavior if in the other order.

[JeremyKuhne]

Note: Besides being good to have period, having our customers write equivalent wrappers is painful. To illustrate, here is our current internal implementation that Combine calls after the appropriate checks:

``` C#
private unsafe static string CombineNoChecksInternal(ReadOnlySpan first, ReadOnlySpan second, ReadOnlySpan third)
{
Debug.Assert(first.Length > 0 && second.Length > 0 && third.Length > 0, "should have dealt with empty paths");

        bool firstHasSeparator = PathInternal.IsDirectorySeparator(first[first.Length - 1])
            || PathInternal.IsDirectorySeparator(second[0]);
        bool thirdHasSeparator = PathInternal.IsDirectorySeparator(second[second.Length - 1])
            || PathInternal.IsDirectorySeparator(third[0]);

        fixed (char* f = &MemoryMarshal.GetReference(first), s = &MemoryMarshal.GetReference(second), t = &MemoryMarshal.GetReference(third))
        {
            return string.Create(
                first.Length + second.Length + third.Length + (firstHasSeparator ? 0 : 1) + (thirdHasSeparator ? 0 : 1),
                (First: (IntPtr)f, FirstLength: first.Length, Second: (IntPtr)s, SecondLength: second.Length,
                    Third: (IntPtr)t, ThirdLength: third.Length, FirstHasSeparator: firstHasSeparator, ThirdHasSeparator: thirdHasSeparator),
                (destination, state) =>
                {
                    new Span<char>((char*)state.First, state.FirstLength).CopyTo(destination);
                    if (!state.FirstHasSeparator)
                        destination[state.FirstLength] = PathInternal.DirectorySeparatorChar;
                    new Span<char>((char*)state.Second, state.SecondLength).CopyTo(destination.Slice(state.FirstLength + (state.FirstHasSeparator ? 0 : 1)));
                    if (!state.ThirdHasSeparator)
                        destination[destination.Length - state.ThirdLength - 1] = PathInternal.DirectorySeparatorChar;
                    new Span<char>((char*)state.Third, state.ThirdLength).CopyTo(destination.Slice(destination.Length - state.ThirdLength));
                });
        }
    }

```

https://github.com/dotnet/coreclr/blob/master/src/mscorlib/shared/System/IO/Path.cs#L518-L543

[terrajobst]

Looks good with minor adjustments:

namespace System.IO
{
    public static class Path
    {
        public static string Join(ReadOnlySpan<char> path1, ReadOnlySpan<char> path2);
        public static string Join(ReadOnlySpan<char> path1, ReadOnlySpan<char> path2, ReadOnlySpan<char> path3);
        public static bool TryJoin(ReadOnlySpan<char> path1, ReadOnlySpan<char> path2, Span<char> destination, out int charsWritten);
        public static bool TryJoin(ReadOnlySpan<char> path1, ReadOnlySpan<char> path2, ReadOnlySpan<char> path3, Span<char> destination, out int charsWritten);
    }
}

[JeremyKuhne]

Finished via dotnet/corefx#27522.

[dsyme]

Some comments on the introduction of this API https://github.com/fsharp/fslang-suggestions/issues/648#issuecomment-393728310:

As I've said elsewhere I think it is very, very wrong for .NET and C# (let alone F#) if basic, simple, clear entry-level APIs like System.IO are using Span or ReadOnlySpan in a way that is in-the-face of programmers. This appears to be just one example, but on a first glance it is, in my humble opinion, a clear case of a misuse of Span in API design.

IMHO Span should never, ever be used in API design as a catch-all replacement for the ubiquitous and simple string. I wonder if we will see a lot more of this, before people realise they have sacrificed simplicity (for, say, teenagers or data scientists or Python programmers or F# programmers...). You should never accidentally find yourself reading a tutorial on high-performance memory primitives just because you tried to join two paths.

It is very common to see this: people get a thing and they start hammering everything with that thing. If that goes ahead then the thing that ultimately gets hammered is usability and simplicity. For .NET, 99% of programmers should never need to know or think about Span. If we force that 99% to think about it, we will lose 50% of them, since the thing that the vast majority of programmers value above everything else is simplicity.

[akeeton]

Will having both Path.Combine and Path.Join APIs be confusing to users? How will users know/remember which API to use?