Rules to help with conflicts between Primary Constructor parameters and field style rules

[swythan]

Describe the problem you are trying to solve

The new C# Primary Constructor behaviour creates "captured parameters" that are in scope in all instance members as if they were private fields, but they are not in fact fields so you can't prefix them with this.

In my opinion this leads to issues when you have chosen to enforce the existing rules on field naming (docs) and/or this prefixes on fields (docs).

• Your intent in enforcing those rules is presumably to make it clear in all code when a class-scoped variable is being used (either read or write).
• The PC parameters will presumably have to be named according to normal parameter naming conventions (not field conventions)
• You can't use this. to qualify access to them
• There is therefore no way to enforce a visual way of distinguishing between class-scoped and locally scoped data access in this case

It can also lead to potential problems, as even if you do create fields with your preferred naming convention the original PC parameters will still be in scope (and offered in Intellisense) throughout the class, making it easy to accidentally use them. This may be logically wrong, and will also add a new hidden field (for the capture) to the class the first time you do it.

In this example, we have created a field derived from a constructor parameter, but our naming conventions prevent us from having it shadow the parameter, so the parameter is still in scope for accidental use anywhere in the class. I don't think any current rules will fire,

public class LookupExample(IDictionary<string, User> users)
{
    // In this example I want the local lookup to be case-insensitive.
    // My style conventions enforce a different name for fields and parameters, so the field does NOT
    // shadow the PC parameter.
    private Dictionary<string, User> _users = new Dictionary<string, User>(users, StringComparer.OrdinalIgnoreCase);

    public User GetUserGood(string id) => _users[id];

    // There is nothing in the compiler/language to stop us we accidentally using the constructor 
    // parameter here (which may be case-sensitive, so incorrect logically). 
    // This is where I want the rule to fire
    public User GetUserBad(string id)  => users[id]; 
}

BTW: I know this class might be better as a record, it's hard to come up with short examples where that isn't true. I'm much more worried about this happening in much larger classes (i.e. where the whole thing doesn't fit on one screen).

Describe suggestions on how to achieve the rule

Two rules (or one with config) that would fire if:

• A captured PC parameter is used in an instance member (i.e. anywhere except an initializer)
• A captured PC parameter that has been assigned to a field/property is used in an instance member.

The first is more strict, and would require creating and initializing fields for all the PC parameters. This would be be useful if you wanted to be really strict about only having class-scoped data accessed where it followed naming/this conventions. The code fix would be to create and initialize the property

The second would initially allow you to use the PC parameters anywhere they were legal. If you did define a field or property and assigned it directly from a PC parameter it would then prevent you using that PC parameter directly anywhere else. The code-fix for this would be to switch access from the PC parameter to the field.

Now I write them down I guess the 2nd rule wouldn't actually help with my LookupExample, but it would help in the "accidental additional capture field" case.

Additional context

I originally suggested this in a language feedback thread here:
dotnet/csharplang#7109 (comment)

I made a bunch of earlier comments in that discussion, but perhaps the most relevant ones start here:
dotnet/csharplang#7109 (reply in thread)

[mavasani]

Tagging @CyrusNajmabadi - this feels like another code style/naming rule, and is also specific to a new language feature (PCs). I think this feature request belongs to dotnet/roslyn as an IDE code style recommendation that can be enforced on build.

Moving to Roslyn repo..

[CyrusNajmabadi]

I don't see the need for any special rules here. Specifically:

If you did define a field or property and assigned it directly from a PC parameter it would then prevent you using that PC parameter directly anywhere else.

The compiler just checked in a compiler warning if you do this. So any additional work would just be superfluous on the ide side.

[swythan]

Thanks, Cyrus.

To avoid scrolling, my suggestions were:

Two rules (or one with config) that would fire if:

A captured PC parameter is used in an instance member (i.e. anywhere except an initializer)
A captured PC parameter that has been assigned to a field/property is used in an instance member.

Judging from the unit tests added in #68662, I think the new compiler warnings cover my second suggested rule.

I always thought my first suggestion was rather strict (and against the design you've opted for) and would have to be opt-in, so I'm not surprised that's not in there! I guess I can write my own analyzer for that one if I decide I need it.

Two questions/notes:

1. There's a unit test that I think shows that this code will NOT trigger a warning:

class C1(int p1)
{
    long F = (long)p1;

    void M()
    {
        _ = p1; 
    }
}

Is that because of the type cast? That seems reasonable; The alternatives feel like rabbit holes full of nightmares,

2. I can see why (from a language/compiler PoV) but the warning message feels like it might be the "wrong way around" for an obvious way to accidently hit it.

I'm thinking that a common case will be that you've initialized a field (or property) from a PC parameter, then accidentally used the PC parameter in a method instead of the field. In this case the warning complaining about the field is technically reasonable, but perhaps unhelpful.

Could the docs for the warning please make it clear that one way to diagnose it is to look for other usages of the PC parameter that is being used on the line the warning is attached to?

Thanks again.

[CyrusNajmabadi]

I always thought my first suggestion was rather strict (and against the design you've opted for) and would have to be opt-in

You could trivially write and analyzer for this. It would be unlikely to be something we would do imo. And, personally, it seems like an anti-pattern. It would be extra craft and state at the type level that would serve no purpose.

If you would like help writing such an analyzer though, let us know and we can def advise on how to do it :-)

[sharwell]

I strongly agree with @swythan here. In its current form, I find primary constructors to be a nearly unusable feature and have finally settled on just disallowing their adoption in projects as a way to proactively prevent the issues described above. Customized analyzers do not address the fact that the built-in refactorings related to this feature fail to adhere to long-standing code style practices. Users end up needing to rewrite the entire UI layers of the feature in order to make it bearable.

tl;dr: On any project where I'm a maintainer on code style issues, allowing support for primary constructors would be conditional on coexistence with two things (the first is the rule stated above):

1. A [diagnostic is reported if a] captured PC parameter is used in an instance member (i.e. anywhere except an initializer)
2. The code fixes and refactorings related to primary constructors produce code which does not violate (1)

[CyrusNajmabadi]

Customized analyzers do not address the fact that the built-in refactorings related to this feature fail to adhere to long-standing code style practices.

I disagree. The current analyzers and fixers adhere to our naming conventions and best practices. They even supply two useful forms. One that continues the user of fields/properties, even when superfluous. This is handy for those that really want to look in the member section to understand the type. And the other which removes all unnecessary/redundant member affectations.

Additional rules here go directly against the exact patterns the design teams believe are bear practices for idiomatic c# for many patterns moving forward.

[CyrusNajmabadi]

The code fixes and refactorings related to primary constructors produce code which does not violate (1)

Picking the first fixer option will always give you this.

[sharwell]

I filed #68817 to further close the functional gap. I'll have to work on adding (1) to Roslyn's private analyzers package.

It sounds like part of my passionate dislike for this feature stems from users voluntarily (🤯) using the second refactoring which removes fields. It would still be nice to block certain refactorings that never produce code according to style, but there are other cases where we show incorrect refactorings and as long as we can immediately report the error with an analyzer people will stop using it.

[CyrusNajmabadi]

To add to the anti-pattern part: in my own usage here, I've found the "superfluous member" approach to PCs to be highly problematic. It requires an excess amount of investigation of the type to determine why members are being specified. And it is outright confusing to discover that there is no actual need for them. On several occasions I thought the members then implied something about the type that was not true at all. And it was only be careful examination that I was able to determine that.

Conversely, making those types into pure PCs (no members), it's became immediately obvious how the type was intended to be produced, consumed, and how instances interacted.

Having members only exist when they serve a necessary (i.e. 'non documentary') purpose, makes code super clear. Having unnecessary members is in the same category of having comments like // increments the value of x by 1 :-)

[CyrusNajmabadi]

Closing out. The style of hte feature in its default state is as the LDM expects. The features themselves that we provide opt for the form that won't cause concern for those that do prefer superfluous fields. And an option is provided for users who would prefer less redundancy. As such, there isn't anything necessary to do here as all the options we offer abide by all our code style guidelines, and match the LDM's expectations on idiomatic coding patterns for the ecosystem.