Roslyn: Editorconfig backwards compatibility dilemma

[jnm2]

We have a bit of a dilemma. If you don't fix this in 15.0–15.2, it will still get in the way and totally break the experience for anyone using 15.0–15.2 if we ever use type_parameter in an OSS project's .editorconfig.

Imagine that 15.3 is out with support for type_parameter and we want to enforce a T prefix followed by pascal case:

dotnet_naming_style.type_parameter.required_prefix = T
dotnet_naming_style.type_parameter.capitalization = pascal_case 
dotnet_naming_symbols.type_parameters.applicable_kinds = type_parameter
dotnet_naming_rule.type_parameters.severity = error
dotnet_naming_rule.type_parameters.symbols = type_parameters
dotnet_naming_rule.type_parameters.style = type_parameter

The problem is that this same block of settings in VS15.0–15.2 acts as though you had said applicable_kinds = *. This is a problem because suddenly every identifier ever is required by VS15.0–15.2 to begin with T.

This is a very similar problem to https://github.com/dotnet/roslyn/issues/20891.

The only fix I can see is to push a fix to VS15.0–15.2 that

1. splits the line by commas
2. removes the items it doesn't recognize
3. If the line exists and the list of recognized items is empty, apply the rule to nothing instead of applying the rule to everything.

For applicable_kinds, you have 1 and 2 correct but you can verify that you don't have 3 correct because type_parameter, event acts as event but type_parameter acts as *.

For dotnet_naming_rule.*.symbols, as far as I can tell, you have all three parts wrong for backwards compatibility.

Otherwise, as you version this very useful and attractive system, how is anyone supposed to adopt new features without forcing everyone in the codebase to upgrade to the very latest VS before continuing any work, or bear through a flood of erroneous squiggles?

Thanks for considering.

[sharwell]

💭 (thinking out loud, not meant to be a statement in favor of or against any action related to the issue) If someone isn't applying the updates to Visual Studio as they become available, how would this change help them? On the other hand, if they _are_ applying updates then it seems like they wouldn't hit this because they would be using a version of Visual Studio which supports the values.

[jnm2]

Hmm. Maybe a bug shows up in VS which is so bad for a few people that they stay on the previous minor version. I've seen people do this with .NET Core tooling.

With the more rapid release cadence, maybe Update upgrades aren't the big deal they used to be?
Still, I really dislike the fact that I can't use cool and helpful .editorconfig settings without making life difficult for people who just happen to not have upgraded yet for whatever reason. It makes .editorconfig, otherwise a no-brainer, a harder sell to OSS projects.

[jnm2]

In any case, at minimum, please don't ship 15.3 with the same lack of compatibility foresight.

[jnm2]

Ran into this with local too. Won't be able to use namespace, type_parameter or local until I'm sure no one is running 15.0–15.4, it looks like.

[jnm2]

Now if this whole naming feature had been behind a feature flag until the full compat story was ready to ship, I'd be able to start using namespace, type_parameter or local as soon as a version ships that recognizes them.

[sharwell]

💭 If we implement #19028 at the same time, you can place your settings there instead of in the "normal" location. Older releases of Visual Studio won't know how to find this file, so it would be like the setting wasn't specified at all.

[jnm2]

I'm not liking the overhead of maintaining a public NuGet package as a workaround. That's almost as hard a sell for an OSS project as causing a flood of incorrect errors for contributors using 15.0–15.4.

[sharwell]

Me either. The part I find partially redeeming is the fact that it's not currently possible to use .editorconfig in an analyzer that runs during the build, so even though the IDE might misbehave until it's updated, the build itself will not be negatively affected.

[brantburnett]

I think that this question is somewhat larger than just Visual Studio. My understanding is that .editorconfig currently or will in the near future also apply to VS Code. On top of that, the .editorconfig format is open source and not necessarily VS specific.

If there are multiple products using the file format, it seems like a fail-soft approach to new features included in the file makes the most sense. The current approach is somewhat fail-soft, in that it ignores the unknown values. However, I think the nature of the fail-soft is incorrect. An unknown value should be treated as match none instead of match all, which is far more intuitive.

[Neme12]

I have a proposal how this could be fixed and I would appreciate feedback from the team on whether they like this idea.

Both applicable_kinds and applicable_accessibilities are "match any" (as opposed to required_modifiers), which means in order to match, the symbol's kind must be any of the applicable kinds on this list. This implies that an empty list of kinds or accessibilities would match no symbol at all. However currently an empty list is treated in a special way to match everything:

dotnet_naming_symbols.my_symbols.applicable_kinds =

I hope others will agree that this is wrong and the correct interpretation of this should be that no symbols or accessibilities match this rule. If users want to match all symbols/accessibilities, they can either use a * or omit this entirely - meaning the default would be a *.

If users specify an unknown value, it is ignored (as expected) but the problem is that if there is only value, the result is an empty list which matches everything. Under my proposal, this resulting empty list would correctly match nothing.

This is a breaking change, but I think it's an acceptable one. For users that currently have an applicable_kinds = or applicable_accessibilities = (blank) in their editorconfig, this would no longer match everything. I think this is OK because it has always been documented that if you want to match all, the correct thing to do is add a *:

dotnet_naming_symbols.my_symbols.applicable_accessibilities = *

or not specify this line at all. I don't imagine many people would have an empty value after = right now (it looks wrong) and if they do, it's a really easy fix to add a *.

Would such a breaking change be acceptable? Can this be brought up for discussion? @sharwell @kuhlenh

[Neme12]

The only problem with this is that it wouldn't work for the Visual Studio UI:

where there is no way to specify * at all. To preserve compatibility here, an empty selection would be interpreted as a * rather than an empty list. Otherwise there would actually be no way to make a specification that works for things that don't have accessibility (parameters, locals).

In order to fix this properly as well (as opposed to a workaround to translate "empty" to *), I see two approaches:

1. The UI would have a new option called "all" or "any", which would be the equivalent of * in editorconfig. The only way to make a rule that works for parameters/locals would be to choose this option.
2. Another possible view is that we don't actually need anything like * in the UI. It is just a convenience so that users don't have to type everything manually. In the UI, there's already a "Select All" button that makes selecting everything easy (and it's also the default). The way to make accessibilities work for locals/parameters would be to add a new option to the accessibility list called "none" (this already exists, it is just not exposed in the UI). This would have to be selected for locals/parameters.

[sharwell]

If users want to match all symbols/accessibilities, they can either use a * or omit this entirely - meaning the default would be a *. ... Would such a breaking change be acceptable?

👍

... UI ...

I'll get back to you on this part.

[sharwell]

I see two primary options for dealing with items that don't have explicit accessibility:

1. Use the effective accessibility of the nearest containing member that does support it. This is an advantage for people who e.g. only want to set a naming convention for parameters exposed in the assembly API (effective accessibility public, protected, or protected internal).
2. Ignore the 'accessibilities' setting for these items. This is an advantage for people who e.g. want to use a single rule to define the naming convention for all parameters and private fields.

[sharwell]

After further discussion, I would propose the following to handle accessibility for items that do not support accessibility:

• Namespaces are public¹
• Add local as a supported accessibility in the UI and .editorconfig
• Local variables, constants, and anonymous/local functions have local accessibility
• Type parameters have the same accessibility as the code element that declares them (type, member, or anonymous/local function)
• Parameters have the same accessibility as the code element that declares them (delegate, member, or anonymous/local function)
• Named tuple elements have the same accessibility as the code element whose signature they appear in (type, member, local variable, or anonymous/local function)

¹ A future release would have the option of treating namespaces with no public types as internal, but this will not be implemented in the initial support for this proposal.

[sharwell]

@Neme12 Our joint design proposal has been approved by the IDE team. Thanks for all the work you put into figuring out the edge cases.

• Handling of empty lists: https://github.com/dotnet/roslyn/issues/20907#issuecomment-385058210
• Handling of items without accessibility: https://github.com/dotnet/roslyn/issues/20907#issuecomment-387071711

[Neme12]

I'm afraid there's another design problem here. While for applicable_kinds and applicable_accessibilities, extra values should be ignored, that doesn't make sense for required_modifiers.

As an example: field,property,method,unknown (for simplicity, let's ignore the case with 1 value now because this problem is unrelated to that) This applies to symbols that have any of the specified kinds and since we don't know about unknown, we can just ignore it and still apply this rule to all the other kinds in the list. Same for accessibilities.

On the other hand, let's say there's a new kind of modifier and the user creates a special rule for symbols that are both static and new_modifier. We cannot just remove new_modifier from this list and apply this rule to all symbols that are static. Since we don't know about new_modifier and it is one of the required ones for this rule to apply, we have to ignore the whole rule. Otherwise we'd be extending its applicability.

[sharwell]

We cannot just remove new_modifier from this list and apply this rule to all symbols that are static. Since we don't know about new_modifier and it is one of the required ones for this rule to apply, we have to ignore the whole rule.

Yes, this is a natural extension of the behavior from above.