Roslyn: Initialize field from constructor parameter defaults to using "this.", where it should default to field with prefixed underscore

[rik-smeets]

Steps to Reproduce:

1. Ensure you don't have custom naming styles set in Visual Studio.
2. Add a constructor to some class with a parameter which is not initialized yet:

    public class TestClass
    {
        public TestClass(string test)
        {
        }
    }

1. Open Quick refactorings (Ctrl + .) at the parameter name and choose: Create and initialize field (..) .

Expected Behavior:
I expect the field to be created with a prefixed underscore _ and initialized like this:

    public class TestClass
    {
        private readonly string _test;

        public TestClass(string test)
        {
            _test = test;
        }
    }

I expect this because this is preferred in the C# coding style, which is also referenced from the Contributing to Roslyn-markdown in this repository.

Actual Behavior:

    public class TestClass
    {
        private readonly string test;

        public TestClass(string test)
        {
            this.test = test;
        }
    }

This conflicts with the aforementioned C# coding style, which explicitly states that using this. should be avoided unless absolutely necessary.

I would like to hear your opinion(s) about this! Also, I will look into this today and see if I can provide a fix for this if you agree with this issue.

[rik-smeets]

If you agree, I have a fix ready for this. It can already be viewed here.

[sharwell]

@rik-smeets This feature should use the code style of the current project, and not define its own separately. It looks like the bug is the feature currently ignores the current configuration and instead uses a hard-coded one.

[rik-smeets]

@sharwell It does use the user's own rules, but has a set of built in rules as a fallback. See lines 39-42 (the comment explaining this) in class AbstractInitializeMemberFromParameterCodeRefactoringProvider.cs. So if the user has rules set, they will be honoured, which is correct.

This issue is about the ordering of those built in rules, which, I think, is not the order in which they should be provided. I'd love to hear your opinion about this. If you agree, I will open a pull request fixing this order, based on the referenced code style.

[CyrusNajmabadi]

I do not see why "fieldWithUnderscore" would be the default:

I expect this because this is preferred in the C# coding style, which is also referenced from the Contributing to Roslyn-markdown in this repository.

These are they preferred coding style for these specific repos. They are not universal, nor should they necessarily be pushed on users by default.

[rik-smeets]

@CyrusNajmabadi I completely agree they should not be pushed on users. However, if you view it like that, right now the this. variant is "pushed" on users.

But I don't see it that way. I just see them as fallback options, so a fix is provided to introduce a field from a constructor even if the user did not explicitly create styles in Visual Studio. I think having a default in that specific case is just fine.

However, I do believe the this. variant is less popular than the _ variant, which is why I created this issue. In my experience, many people and projects tend to follow Microsoft standards, mostly just adhering to defaults too. Seeing a number of thumbs up, some people agree with this, but I'm all open for discussion, because I can't really measure what the most popular way is around the globe!

[sharwell]

The current defaults match the default behavior of this functionality going back to at least Visual Studio 2013. Considering we now have a way for individual projects to change the behavior as part of the .editorconfig settings for the project (a feature we are actively working to encourage adoption of), I am against changing the fallback behavior at this time.

:memo: This comment reflects my viewpoint, but I am not in a "deciding" position on this issue. Just because I would argue against making a change at this time doesn't necessarily mean the change will not occur.

[CyrusNajmabadi]

I completely agree they should not be pushed on users. However, if you view it like that, right now the this. variant is "pushed" on users.

True. But this is also how things have been since the first days we offered any of these features. I would prefer that we just stick with that as a default, and allow users to use our existing style features to opt out if they want.

--

For better or for worse, .net and C# have remained very un-opinionated on these topics. It's only very recently that we've even made some style guides. And those style guides are only followed by certain repos. The reality of the situation is that the .net ecosystem is allowed to do what it wants, and the goal of the tools is to support those decisions, and not force them in a certain direction.

--

So, i would keep things as is. It seems to be a fine default (and we risk aggravating many existing users by changing it), and we have appropriate escape hatches for people who prefer different things.

Thanks!

[rik-smeets]

@sharwell @CyrusNajmabadi Thank you for sharing your points of view. I understand where this default is coming from now. Also, in the meantime, I've heard that work is being done by one of your teams into analyzing C# projects on GitHub as part of creating a default .editorconfig file in Visual Studio, which is great.

Awaiting that, this discussion got me thinking: why not simply offer both field creation options to the user (only if said user has no custom naming styles set)? Best of both worlds! What is your opinion on that idea?

[CyrusNajmabadi]

I don't like that approach simply because it pushes so many choices in the user's face and clutters up an experience that we've actually worked super hard to streamline down. I don't have any pictures of this anymore, but you could routinely see tens of items in the list. It was horribly cluttered and overloading.

Instead, we strongly want to give the "right" fix and allow users to change thigns elsewhere if that fix is not for them. This keeps the list always lightweight, while giving users control.

[gbreen12]

I've looked at the .editorconfig documentation but I wasn't able to figure out how to accomplish this behavior (_ rather than this. by default) using that. Any suggestions on how one would accomplish that?

[rik-smeets]

@gbreen12 You have to define a custom naming style in your .editorconfig, like this:

[*.{cs,vb}]
dotnet_naming_rule.private_members_with_underscore.symbols  = private_fields
dotnet_naming_rule.private_members_with_underscore.style    = prefix_underscore
dotnet_naming_rule.private_members_with_underscore.severity = suggestion

dotnet_naming_symbols.private_fields.applicable_kinds           = field
dotnet_naming_symbols.private_fields.applicable_accessibilities = private

dotnet_naming_style.prefix_underscore.capitalization = camel_case
dotnet_naming_style.prefix_underscore.required_prefix = _

This should achieve the desired effect. Let me know if you have any questions.

[Neme12]

This conflicts with the aforementioned C# coding style, which explicitly states that using this. should be avoided unless absolutely necessary.

In case of this.test = test;, it is absolutely necessary.

[rik-smeets]

@Neme12 When naming the class variable test it is indeed necessary, but if you name it anything else (with the prefixed _ for instance), it is not.

That's what this issue was about - the default naming style when class variables are initialized from a constructor, which as of current defaults to the this.-style, but as said can be overridden by an .editorconfig file.

[IanKemp]

I've searched through the Roslyn codebase for the following rules as mentioned by @rik-smeets:

dotnet_naming_rule.private_members_with_underscore.symbols
dotnet_naming_rule.private_members_with_underscore.style
dotnet_naming_rule.private_members_with_underscore.severity

dotnet_naming_style.prefix_underscore.capitalization
dotnet_naming_style.prefix_underscore.required_prefix

but am unable to find anything along these lines.

Is the VS IDE handling these options itself, i.e. outside of Roslyn? If so, why?

[CyrusNajmabadi]

@IanKemp These are all handled inside roslyn. But you won't fine those specific strings anywhere. htat's because naming works by combining several different options together into a combined naming string. You can read more about that here:

https://docs.microsoft.com/en-us/visualstudio/ide/editorconfig-naming-conventions?view=vs-2017

[IanKemp]

@CyrusNajmabadi TY, I get it now: the actual name of the naming_rule doesn't matter, just what symbols and style rules it points to.

I guess then I should ask my actual question: assuming you're in an analyzer context, what is the API for getting the effective naming rules?

Basically what I've got is this:

    // context is Microsoft.CodeAnalysis.Diagnostics.SyntaxNodeAnalysisContext
    var optionSet = context.Options.GetDocumentOptionSetAsync(context.Node.SyntaxTree, context.CancellationToken).GetAwaiter().GetResult();
    var namingPreferencesOption = optionSet.GetOption(<relevant-option-name>, <language>);

and I don't know what <relevant-option-name> should be. I've found Microsoft.CodeAnalysis.Simplification.SimplificationOptions.NamingPreferences which seems to be the obvious candidate (and is used throughout Roslyn), but it's internal.

[CyrusNajmabadi]

I guess then I should ask my actual question: assuming you're in an analyzer context, what is the API for getting the effective naming rules?

... but it's internal.

If you're asking what public API is available, then i don't think there is anything currently. Maybe something can be made available once .editorconfig support is moved down the compiler layer.

[IanKemp]

A bit depressing that there is currently no public API, but thanks for the info!