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

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

Closed
rik-smeets opened this issue Oct 27, 2017 · 18 comments

Comments

@rik-smeets
Copy link
Contributor

@rik-smeets rik-smeets commented Oct 27, 2017

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

This comment has been minimized.

Copy link
Contributor Author

@rik-smeets rik-smeets commented Oct 27, 2017

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

@jcouv jcouv added the Area-IDE label Oct 31, 2017
@sharwell

This comment has been minimized.

Copy link
Member

@sharwell sharwell commented Oct 31, 2017

@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

This comment has been minimized.

Copy link
Contributor Author

@rik-smeets rik-smeets commented Oct 31, 2017

@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

This comment has been minimized.

Copy link
Contributor

@CyrusNajmabadi CyrusNajmabadi commented Nov 1, 2017

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

This comment has been minimized.

Copy link
Contributor Author

@rik-smeets rik-smeets commented Nov 2, 2017

@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!

@jinujoseph jinujoseph added the Bug label Nov 3, 2017
@sharwell

This comment has been minimized.

Copy link
Member

@sharwell sharwell commented Nov 5, 2017

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.

📝 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

This comment has been minimized.

Copy link
Contributor

@CyrusNajmabadi CyrusNajmabadi commented Nov 5, 2017

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!

@sharwell sharwell added Discussion and removed Bug labels Nov 5, 2017
@rik-smeets

This comment has been minimized.

Copy link
Contributor Author

@rik-smeets rik-smeets commented Nov 18, 2017

@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

This comment has been minimized.

Copy link
Contributor

@CyrusNajmabadi CyrusNajmabadi commented Nov 18, 2017

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

This comment has been minimized.

Copy link

@gbreen12 gbreen12 commented Jan 18, 2018

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

This comment has been minimized.

Copy link
Contributor Author

@rik-smeets rik-smeets commented Jan 18, 2018

@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 comment has been minimized.

Copy link
Contributor

@Neme12 Neme12 commented May 19, 2018

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

This comment has been minimized.

Copy link
Contributor Author

@rik-smeets rik-smeets commented May 20, 2018

@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.

wachulski added a commit to wachulski/roslyn that referenced this issue Jun 20, 2018
ConvertAutoPropertyToFullProperty had prefixed fields with '_' by default
when no user naming preferences had been set up (hardcode for that specific
refactoring).

According to dotnet#22884, it should be aligned to fallback defaults
of CSharpInitializeMemberFromParameterCodeRefactoringProvider.

Fixes dotnet#26992
wachulski added a commit to wachulski/roslyn that referenced this issue Jun 20, 2018
ConvertAutoPropertyToFullProperty had prefixed fields with '_' by default
when no user naming preferences had been set up (hardcode for that specific
refactoring).

According to dotnet#22884, it should be aligned to fallback defaults
of CSharpInitializeMemberFromParameterCodeRefactoringProvider.

Fixes dotnet#26992
wachulski added a commit to wachulski/roslyn that referenced this issue Jun 20, 2018
ConvertAutoPropertyToFullProperty had prefixed fields with '_' by default
when no user naming preferences had been set up (hardcode for that specific
refactoring).

According to dotnet#22884, it should be aligned to fallback defaults
of CSharpInitializeMemberFromParameterCodeRefactoringProvider.

Fixes dotnet#26992
@IanKemp

This comment has been minimized.

Copy link

@IanKemp IanKemp commented Dec 7, 2018

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

This comment has been minimized.

Copy link
Contributor

@CyrusNajmabadi CyrusNajmabadi commented Dec 7, 2018

@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

This comment has been minimized.

Copy link

@IanKemp IanKemp commented Dec 7, 2018

@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

This comment has been minimized.

Copy link
Contributor

@CyrusNajmabadi CyrusNajmabadi commented Dec 7, 2018

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

This comment has been minimized.

Copy link

@IanKemp IanKemp commented Dec 7, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.