Analyzer/fixer proposal: Collapse multiple `Path.Combine` or `Path.Join` in a row

[carlossanlop]

Suggested severity: Info
Suggested category: Reliability

When generating multiple combined/joined path segments in a row, where each depends on the previous one (excluding the first one of course), and only the last resulting segment is the one being properly consumed somewhere else, then they can be collapsed into a single invocation of Combine or Join.

The maximum number of segments in a row that can be fixed are 3, since the largest Combine/Join overloads take 4 parameters.

These are the APIs that can benefit from the analyzer:

static class Path
{
    // Unlimited segment collapse
    static string Combine (params string[] paths);
    static string Combine (string path1, string path2);
    static string Combine (string path1, string path2, string path3);
    static string Combine (string path1, string path2, string path3, string path4);

    static string Join (params string?[] paths);
    static string Join (string? path1, string? path2, string? path3, string? path4);
    static string Join (string? path1, string? path2, string? path3);
    static string Join (string? path1, string? path2);


    // Limited to up to 3 path segments
    static bool TryJoin (ReadOnlySpan<char> path1, ReadOnlySpan<char> path2, Span<char> destination, out int charsWritten);

    // Not flagged, but used by fixer for max allowed
    // static bool TryJoin (ReadOnlySpan<char> path1, ReadOnlySpan<char> path2, ReadOnlySpan<char> path3, Span<char> destination, out int charsWritten);


    // Limited to up to 4 path segments
    static string Join (ReadOnlySpan<char> path1, ReadOnlySpan<char> path2);
    static string Join (ReadOnlySpan<char> path1, ReadOnlySpan<char> path2, ReadOnlySpan<char> path3);

    // Not flagged, but used by fixer for max allowed
    // static string Join (ReadOnlySpan<char> path1, ReadOnlySpan<char> path2, ReadOnlySpan<char> path3, ReadOnlySpan<char> path4);
}

An additional improvement: whenever possible, switch to using the ReadOnlySpan<char> overload instead of the string overload, if the number of arguments allows it.

Flag

• Simplest case: Use a non-params overload

// Before
string first = Path.Join("folder", "another"); // first is consumed only as argument of second
string second = Path.Join(first, "onemore");
// After
string second = Path.Join("folder", "another", "onemore");

- Special case: Use the `params` overload
// Before
string first = Path.Join("folder", "another"); // first is consumed only as argument of second
string second = Path.Join(first, "onemore"); // second is consumed only as argument of third
string third = Path.Join(second, "anotherone"); // third is consumed only as argument of fourth
string fourth = Path.Join(third, "file.txt");
// After
string fourth= Path.Join("folder", "another", "onemore", "anotherone", "file.txt"); // passed as params

• For Join and TryJoin overloads that take ReadOnlySpan<char> parameters, Join can only collapse 4, TryJoin only 3

string one = "one";
string two = "two";
string three = "three";
string four = "four";
string file = "file.txt";

// Before
string first = Path.Join(one.AsSpan(), two.AsSpan());
string second = Path.Join(first.AsSpan(), three.AsSpan()); // first is consumed only as argument of second
string third = Path.Join(second.AsSpan(), four.AsSpan()); // second is consumed only as argument of third
string fourth = Path.Join(third.AsSpan(), file.AsSpan()); // third is consumed only as argument of fourth
// After: The first segment will have to be skipped if more than 4 parameters in total
string first = Path.Join(one.AsSpan(), two.AsSpan());
string fourth = Path.Join(first, three.AsSpan(), four.AsSpan(), file.AsSpan());

Do not flag

• If one of the joined/combined strings is being consumed somewhere else after the final collapse, do not flag

string first = Path.Join("one", "two");
string second = Path.Join(first, "three");
string third = Path.Join(second, "file.txt"); // Can't collapse: first is consumed in the WriteLine
Console.WriteLine(first);

• For Combine, the analyzer should not flag cases where there are potential null arguments.

string? MyNullStringMethod()
{
    // ...
}

string? one = null;
string two = "path";
Path.Combine(one, two, MyNullStringMethod());

cc @buyaa-n

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

Issue Details

---

When generating multiple combined/joined path segments in a row, where each depends on the previous one (excluding the first one of course), and only the last resulting segment is the one being properly consumed somewhere else, then they can be collapsed into a single invocation of Combine or Join.

The maximum number of segments in a row that can be fixed are 3, since the largest Combine/Join overloads take 4 parameters.

Flag

// Before
string first = Path.Join("folder", "another"); // first is used only as argument of second
string second = Path.Join(first, "onemore");
// After
string second = Path.Join("folder", "another", "onemore");


// Before
string first = Path.Join("folder", "another"); // first is used only as argument of second
string second = Path.Join(first, "onemore"); // second is used only as argument of third
string third = Path.Join(second, "final");
// After
string third = Path.Join("folder", "another", "onemore", "final"); // 4 strings is the largest overload available

// Before
string first = Path.Join("folder", "another");
string second = Path.Join(first, "onemore");
string third = Path.Join(second, "yetanother");
string fourth = Path.Join(third, "final");

// After: Can only collapse 3 strings, pick the last 3
string first = Path.Join("folder", "another");
string fourth = Path.Join(first, "onemore", "yetanother", "final");

Do not flag

string first = Path.Join("folder", "another");
string second = Path.Join(first, "final");
Console.WriteLine("{0}, {1}", first, second); // The last segment should be the only one being consumed

cc @buyaa-n

Author:	carlossanlop
Assignees:	-
Labels:	area-System.IO, untriaged, code-analyzer, code-fixer
Milestone:	-

[buyaa-n]

Thanks, @carlossanlop I have a few questions/comments:

• The maximum number of segments in a row that can be fixed are 3, since the largest Combine/Join overloads take 4 parameters.

  Looks like Path.Combine has an overload with a params argument, don't we want to support 4+ string arguments for Combine?

• Is the analyzer should track only local variables or any? I guess it would be quite expensive to track global variables, not sure that is worth it as I think this could happen very rarely

• Please add the suggested category and severity

[terrajobst]

Video

• This makes sense.
  • The category should be "Performance"
  • Severity should be "Info"
• We should special case using a single variable:

  string x = Path.Join("one", "two");
  x = Path.Join(x, "three");
  x = Path.Join(x, "file.txt");
  Console.WriteLine(x);

• Should this handle cases like this?

  _field = Path.Join("one", "two");
  _field = Path.Join(_field, "three");
  _field = Path.Join(_field, "file.txt");

• Do we have other examples of this pattern?
  • String.Concat