dotnet/roslyn

"IDE0036: Modifiers are not ordered" - inconsistent ordering

SetTrend opened this issue · 13 comments

The "IDE0036: Modifiers are not ordered" rule evaluates modifier sequence inconsistently.

Brief description:

It looks like this rule prefers abstract to prefix the internal access modifier while it's preferring the same to postfix the protected access modifier.

Languages applicable:

I checked with C# analyzer only.

Code example that the analyzer should report:

Modifierts are not ordered

This piece of code was entered:

protected abstract void BeforePropertyChange();
protected abstract void Undo();
internal abstract void Save();

The final line was flagged by Code Analyzer. It suggested the following modifier sequence:

protected abstract void BeforePropertyChange();
protected abstract void Undo();
abstract internal void Save();

Based on the following code:

private static readonly SyntaxKind[] s_preferredModifierOrderDefault =
{
SyntaxKind.PublicKeyword, SyntaxKind.PrivateKeyword, SyntaxKind.ProtectedKeyword, SyntaxKind.InternalKeyword,
SyntaxKind.StaticKeyword,
SyntaxKind.ExternKeyword,
SyntaxKind.NewKeyword,
SyntaxKind.VirtualKeyword, SyntaxKind.AbstractKeyword, SyntaxKind.SealedKeyword, SyntaxKind.OverrideKeyword,
SyntaxKind.ReadOnlyKeyword,
SyntaxKind.UnsafeKeyword,
SyntaxKind.VolatileKeyword,
SyntaxKind.AsyncKeyword
};

Both protected and internal have the same level, and they both come before abstract.

I also couldn't reproduce using Visual Studio 16.8.3:

image

Note that this rule is configurable, can you make sure that your .editorconfig doesn't set csharp_preferred_modifier_order option?

Thanks, @Youssef1313, for replying.

Yes, you are right. I had Visual Studio 2019 create an .editorconfig file in order to add a custom rule.

The .editorconfig file created by Visual Studio 2019 contains the rule you mentioned:

#when this rule is set to a list of modifiers, prefer the specified ordering.
csharp_preferred_modifier_order = public,private,protected,readonly,abstract:suggestion

How come the generated .editorconfig file differs from the originally configured Visual Studio 2019 behaviour? Shouldn't it rather fully reflect the original Visual Studio behaviour when being generated?

@SetTrend When I tried to let VS generate the .editorconfig on its own, I found:

# Modifier preferences
csharp_prefer_static_local_function = true:suggestion
csharp_preferred_modifier_order = public,private,protected,internal,static,extern,new,virtual,abstract,sealed,override,readonly,unsafe,volatile,async:silent

Could you provide the steps that results in the missing internal modifier?

I finished my sample project now and uploaded it to Github. Please find the corresponding repository here.

Later on, I will provide further steps to reproduce ...

This is so strange:

Today I deleted the existing .editorconfig file from my solution and had it re-created by Visual Studio 2019 ... and the new .editorconfig file now looks completely different from the .editorconfig file created 4 days ago by the same Visual Studio.

I just checked in the latest file.

Does anybody have an explanation for this disturbing behaviour?

@sharwell Is there any kind of Machine Learning that generates .editorconfig based on the codebase?

There are several different commands that can be used to create a .editorconfig file, and they all do very different things.

Create .editorconfig to match current Tools → Options...

image

Create (mostly) empty .editorconfig

With root = true

image

Without root = true

image

Create .editorconfig with partial content that doesn't seem to match anything specific

image

Create .editorconfig using IntelliCode (machine learning)

image

I've been using the latter approach.

If the outcome of the "New EditorConfig" file isn't deterministic, that's not a good idea.

It feels highly uncomfortable to report failure to the team with no-one being able to reproduce, not even I myself.

It may be good to rename the menu item to "New EditoConfig (using Maching Learning)" or something that makes it clear. @sharwell What do you think?

Obviously, the ML generated .editorConfig file appears to be only a snapshot, valid at a certain point in time; probably not even valid at all.

If the ML generated .editorConfig varies over time, it would be a necessity to have Visual Studio automatically regenerate the file to match the forth growing VS project situation.

Is this really wanted?

It may be good to rename the menu item ...

I thought the name used to be "New EditorConfig (IntelliCode)" but perhaps I'm mistaken. It's provided by the IntelliCode extension (different team), and I normally have that extension disabled so I'm not sure when it changed.

I would suggest filing feedback through Developer Community if you have suggestions regarding that command, since those developers wouldn't be watching this repository.

Thanks to all of you for your valuable contribution.

I'm going to switch over then to Developer Community ...

Actually, there is already a thread existing at Developer Community, dealing with a very similar situation:

https://developercommunity.visualstudio.com/content/problem/689621/unable-to-disable-ai-assisted-intellisense.html