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

Prefer ValueTuple over Tuple #33791

Open
terrajobst opened this issue Mar 19, 2020 · 7 comments
Open

Prefer ValueTuple over Tuple #33791

terrajobst opened this issue Mar 19, 2020 · 7 comments
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime code-analyzer Marks an issue that suggests a Roslyn analyzer code-fixer Marks an issue that suggests a Roslyn code fixer help wanted [up-for-grabs] Good issue for external contributors partner-impact This issue impacts a partner who needs to be kept updated
Milestone

Comments

@terrajobst
Copy link
Member

terrajobst commented Mar 19, 2020

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

@terrajobst terrajobst added api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Runtime code-analyzer Marks an issue that suggests a Roslyn analyzer labels Mar 19, 2020
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label Mar 19, 2020
@jeffhandley jeffhandley added this to the Future milestone Mar 20, 2020
@jeffhandley jeffhandley added the code-fixer Marks an issue that suggests a Roslyn code fixer label Mar 21, 2020
@jeffhandley
Copy link
Member

Estimates:

  • Analyzer: Large
  • Fixer: Medium

@terrajobst terrajobst removed the untriaged New issue has not been triaged by the area owner label Mar 21, 2020
@carlossanlop carlossanlop added the needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration label Mar 2, 2021
@carlossanlop
Copy link
Member

@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
Copy link
Member Author

terrajobst commented Mar 30, 2021

@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
Copy link

dsyme commented Mar 31, 2021

  1. 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
Copy link
Member

@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
Copy link
Contributor

buyaa-n commented May 30, 2023

[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;
}

@buyaa-n buyaa-n added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration labels May 30, 2023
@bartonjs
Copy link
Member

bartonjs commented Jun 1, 2023

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

@bartonjs bartonjs added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Jun 1, 2023
@buyaa-n buyaa-n added the help wanted [up-for-grabs] Good issue for external contributors label Jun 1, 2023
@tannergooding tannergooding modified the milestones: 8.0.0, Future Jul 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime code-analyzer Marks an issue that suggests a Roslyn analyzer code-fixer Marks an issue that suggests a Roslyn code fixer help wanted [up-for-grabs] Good issue for external contributors partner-impact This issue impacts a partner who needs to be kept updated
Projects
None yet
Development

No branches or pull requests

8 participants