Prefer ValueTuple over Tuple

[terrajobst]

The rule should find and flag cases where a Tuple<...> is being used but where a ValueTuple<...> would suffice, ideally with the C# language syntax employed. There are some cases where a Tuple<...> is beneficial, however, so the patterns identified here would be constrained.

Category: Performance
Severity: Info

[jeffhandley]

Estimates:

• Analyzer: Large
• Fixer: Medium

[carlossanlop]

@terrajobst when would it be beneficial to use a Tuple over a ValueTuple?

At first glance, I think the estimates are both Small (Analyzer and Fixer).

The Docs specify the differences between a Tuple and a ValueTuple. The biggest difference is that ValueTuple being a struct can avoid an allocation. It's not clear what are the disadvantages of using the ValueTuple though, which would be helpful to determine the limitations of a potential analyzer/fixer and confirm if their estimated complexity is indeed Large/Medium, respectively.

[terrajobst]

@terrajobst when would it be beneficial to use a Tuple over a ValueTuple?

Two reasons:

1. You've shipped the API with Tuple before.
2. You need to support tuples with a large number of arguments (which can happen in F# interop AFAIK, @dsyme or @cartermp would know more.)

[dsyme]

3. Copying around large structs can be a performance problem (especially generic ones that in turn get instantiated with structs). So you have to estimate the amount of copying before the tuple is pulled apart at its eventual consumption point, the number of long-term referenes to the tuple, and the distribution of sizes, via.a.vis copying+allocation costs.

[carlossanlop]

@terrajobst @dsyme thanks for the input.

Are there any other restrictions we should know about when using ValueTuple, like the restrictions we have with Task vs ValueTask (for example, don't use ValueTask more than once)? We'd like to make sure we limit the work of the analyzer and the fixer to only a clear set of cases.

[buyaa-n]

[Triage note]: The analyzer should suggest using ValueTuple for any Tuple.

• Exclude public APIs or make it configurable
• Should we exclude Tuples with large number of arguments (with how many arguments)

I wonder how the fixer should work and if we should suggest a code fix:

• If it should keep track of all references and update if needed (expensive, code fixes might interfere each other)
• Even if the fixer fixed all references, it still could introduce a build error for a null check for example

public class Test
{
    private Tuple<int, string> _field;
    public void M()
    {
        Tuple<int, string> t = GetTuple();
        if (t == null) 
        {
            Console.WriteLine(t.Item1);
        }

        if (t == _field)
        {
            Console.WriteLine(GetString(f));
        }
    }

    internal Tuple<int, string> GetTuple() => new Tuple<int, string>(1, "id");

    internal string GetString(Tuple<int, string> args) => args.Item2;
}

[bartonjs]

Video

Generally seems like a good idea.

A concern was raised about potentially limiting this to 5-tuples or smaller (or any other arbitrary number), but we don't feel that's important.

One diagnostic ID should be used for member bodies and for member signatures. By default the analyzer should not report on "exposed" API (e.g. public, protected), but it would be appropriate to have a configuration parameter to allow opting in to exposed API.

For the fixer, we identified a concern around type coercion (e.g. new Tuple<long, string>(42, "hi") involves a cast of the int 42 to the long 42). It may make sense to start the fixer with only Tuple.Create callsites, since type coercion is less of a concern with that pattern.

Category: Performance
Severity: Info