Stylecopanalyzers: New rules for tuple types

[sharwell]

Normally I would propose the new rules individually, but in this case I believe the individual rules are closely related and likely need to be implemented as a group.

:memo: All of the following rules are automatically disabled if the project does not support C# 7 or newer.

SA1141: Use tuple syntax

Category: Readability

This rule catches three cases:

1. Prefer (T1, T2) to ValueTuple<T1, T2>
2. Prefer (value1, value2) to ValueTuple.Create(value1, value2)
3. Prefer (value1, value2) to new ValueTuple<T1, T2>(value1, value2)

:memo: As an aside, it appears the new tuple type syntax is not allowed inside pattern matching so it needs to be tested.

SA1414: Tuple types in signatures should have element names

Category: Maintainability

Tuple types appearing in element signatures should have explicitly named tuple elements.

SA1142: Refer to tuple elements by name

Category: Readability

This rule reports diagnostics when a tuple field is referenced by its metadata name (Item1, etc.) when a name has been provided.

SA1316: Tuple element names should start with a lowercase uppercase lowercase letter

Category: Naming

Tuple element names should be in camelCase PascalCase camelCase.

This rule will have a configuration option to analyze implicit names (C# 7.1). In the default configuration, only explicitly-provided names would be analyzed.

SA1317: Tuple element names in signatures should match

Category: Naming

See dotnet/roslyn#20850
See #1949

[vweijsters]

I totally agree with those rules. 👍

[Jericho]

I agree with those rules with the exception of tuple elements names starting with a lower case. I prefer names to start with upper case. Would you consider having a rule for "start with lower case" enabled by default and another rule for "start with upper case" disabled by default which would allow us to chose which rule we prefer?

[sharwell]

@Jericho a configuration option might be added in the future which controls the rule

[pdelvo]

Those rules sound good to me.

Other exceptions for the Use Tuple Syntax rule I can think of are typeof and nameof expressions because the tuple syntax is not legal there.
Tuple element names should start with a lowercase letter maybe should get a different name so it does not look weird if someone configures it to force uppercase letters.
Tuple element names in signatures should match: I would slightly prefer Maintainability as the category.

[sharwell]

Tuple element names should start with a lowercase letter

This should probably change to uppercase letter due to a recent framework design decision in dotnet/corefx. No need to diverge from the pattern they will be using.

[Neme12]

@sharwell Can you please provide a link for this decision? Thanks.

[sharwell]

@Neme12 https://github.com/dotnet/corefx/pull/26582#discussion_r229865636

[CyrusNajmabadi]

This should probably change to uppercase letter due to a recent framework design decision in dotnet/corefx. No need to diverge from the pattern they will be using.

i personally disagree. It make the consumption side worse as natural tooling will do things like this:

```c#
var tuple = GetSomething();
Console.WriteLine(tuple.First, tuple.Second);

IDE will suggest to deconstruct here, giving you:

```c#
var (First, Second) = GetSomething();
Console.WriteLine(First, Second);

But locals shouldn't be capitalized like that. So the options are keep things like this, or have the tooling camel-case these locals. Which then is a problem because now your local name is out of sync with the tuple name.

--

IIRC, C# LDM gave guidance that Tuple names should be camelcased (and this is what Roslyn itself does in the majority of cases). Perhaps CoreFx should reconsider this naming?

[Neme12]

IIRC, C# LDM gave guidance that Tuple names should be camelcased (and this is what Roslyn itself does in the majority of cases). Perhaps CoreFx should reconsider this naming?

This was my impression too.

[appel1]

@CyrusNajmabadi That's a tooling issue imo. Most tools can suggest properly cased variable names when you access properties and public fields like ValueTuple.Item1. Since both properties and public fields are pascal cased it makes sense to use the same casing when accessing the tuple elements as public fields.

If pascal case isn't the default I hope this is made configurable.

[sharwell]

i personally disagree.

A change to the default rule configuration would require a change to the corresponding framework design guideline. If you disagree, I would recommend bringing this up in the linked API review ASAP because once that decision ships it will never change.

IDE will suggest to deconstruct here, giving you:

It should not do this once naming styles for both tuple elements and local variables are configured. If it does, a bug could be filed on dotnet/roslyn so this can be corrected.

[CyrusNajmabadi]

It should not do this once naming styles for both tuple elements and local variables are configured. If it does, a bug could be filed on dotnet/roslyn so this can be corrected.

As i mentioned:

or have the tooling camel-case these locals. Which then is a problem because now your local name is out of sync with the tuple name.

This is not a great situation to be in. For example, the strong binding between names is being lost here.

that said, the IDE side of things was not the main point i was making. The point i made what the language itself has been pushing heavily (and was designed int he fashion) that these are like a bundle of parameters. The naming commonly used follows that belief which is why we did things in 7.2 (or 7.3) to infer tuple names. The expected use pattern was that more often than not you would infer the names from the locals you'd been working with, and you'd also deconstruct directly with no strangeness in names.

This also matches how we think about deconstruct where 'out-parameters' are used to indicate positionally the data being extracted. In all these cases, the idea is to make a strong correspondence (including syntactically) between parameters and tuples.

I've asked @stephentoub for next steps here. IMO, this is a big decision, and a stance on tuples should not be taken lightly as part of a single PR in corefx. Eitehr that PR should not use tuples, or there should be a full discussion on appropriate naming as it will affect things so far into the future. I personally think LDM should be conferred on this since there were definitely beliefs and expectations on how we wanted people to think about tuples, and the naming definitely ties into that.

[stephentoub]

tuples should not be taken lightly as part of a single PR in corefx

It wasn't taken lightly. This particular API was just the first time tuples were considered for API, which forced the larger discussion on whether tuples should be condoned in public .NET APIs, and if yes, what the naming should be. The answers were a) when appropriate on a case-by-case basis and b) PascalCased.

[CyrusNajmabadi]

It wasn't taken lightly.

Fair enough. Apologies for being flippant like that. I'll open an issue to track this.

[sharwell]

Based on the updated conclusion in dotnet/corefx, I've updated the rules above to again suggest camelCase naming. This is consistent will all cases of tuple usage I've observed to date.

[appel1]

@sharwell I hope you can still make this configurable. For us it makes sense to keep the naming convention the same for all public fields and properties. Very odd to make an exception for just named value tuple fields, the ItemN fields are still pascal cased even.

[sharwell]

@vweijsters I'm moving these rules to the 2.0 release

[danielloganking]

Will this also correct needless violations of SA1008? The following errors when it should not:

public override int GetHashCode() => (Property1, Property2).GetHashCode();
                                    ^^^ SA1008 Opening parenthesis must not be preceded by a space.

[sharwell]

@danielloganking That should already be fixed in the 1.1.1-rc releases. I see tests here:

https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/09aa5be73750d41e3bebfdf40caefacd04e1c1ea/StyleCop.Analyzers/StyleCop.Analyzers.Test.CSharp7/SpacingRules/SA1008CSharp7UnitTests.cs#L525-L527

[danielloganking]

Confirmed this works. Thanks so much I didn't realize that had been incorporated in the RC.