Added support for .editorconfig #1526

filipw · 2019-06-15T17:17:18Z

This PR adds support for .editorconfig.
It spans across multiple features:

ability to control formatting options via .editorconfig (i.e. indentation, spacing, new lines etc)
ability to control code style options via .editorconfig (i.e. prefer var over concrete types etc)
ability to control naming conventions via .editorconfig (i.e. camel casing or pascal casing of members, prefixes etc)
ability to control analyzers via .editorconfig

.editorconfig rules are also respected by the invoked analyzers, the invoked refactoring and code fixes. In other words, they manifest themselves through analyzers and in code generation.

At the moment, changes to .editorconfig related to formatting are recomputed in real time, for other features they require omnisharp restart.
The implementation is reflection based since the APIs are not public in Roslyn, however this is a very important feature, that we have waited years for, and is one of the most upvoted feature requests, so as much as we try to avoid it, I think reflection is acceptable here.

The approach is adapted from how https://github.com/dotnet/format works (although it only deals with formatting, not with code style/naming conventions/analyzers).

Fixes #950
Fixes #1242
Fixes #31
Fixes dotnet/vscode-csharp#1726

kpko · 2019-06-15T18:21:39Z

This is awesome, thank you so much! I think this will enable a lot of people to switch from Visual Studio to VSCode or even VIM via Omnisharp.

Is there an existing issue to open up the APIs on Roslyns side?

Hoffs · 2019-06-16T19:12:55Z

Are there additional changes required in omnisharp-vscode extension? I've looked through and I saw that you've added additional option "EnableEditorConfigSupport" which I suppose needs to be passed from mentioned extension as well, so I guess it does indeed need an update. Is this planned by you or someone else?

bjorkstromm · 2019-06-16T19:18:11Z

@Hoffs quickly checked, and it should be enough to enable it via omnisharp.json in the FormattingOptions section. See https://github.com/OmniSharp/omnisharp-roslyn/wiki/Configuration-Options#formatting-options for more info.

Hoffs · 2019-06-16T19:48:18Z

I see, well I tested another way by adding an option to the extension (similar to enableRoslynAnalyzers), but what I've found out is that code actions don't work correctly. It seems OmniSharp server is returning what I believe incorrect identifier:

{
        "Identifier": "NamingStyleCodeFixProvider",
        "Name": "Fix Name Violation: _mapper"
}

Where as in tests comitted here the request to fix it is sent in same fashion as the message:

var runRequest = new RunCodeActionRequest
                {
                    Line = 2,
                    Column = 31,
                    FileName = testFile.FileName,
                    Identifier = "Create and initialize field 'xxx_something'",
                    WantsTextChanges = false,
                    WantsAllCodeActionOperations = true,
                    Buffer = testFile.Content.Code
                };

where other test suggest that identifier is identical to text

Assert.Contains(getResponse.CodeActions, f => f.Name == "Create and initialize field 'xxx_something'");

Similarly other actions return identifier in same style.

{
        "Identifier": "Encapsulate field: 'mapper' (and use property)",
        "Name": "Encapsulate field: 'mapper' (and use property)"
}

So my guess is that the NamingStyleCodeFixProvider code action identifiers are returned incorrectly for them to work properly.

filipw · 2019-06-16T20:13:28Z

I see, well I tested another way by adding an option to the extension (similar to enableRoslynAnalyzers)

yes the intent is the feature is opt-in in the beginning (until we gather more feedback and polish it off)

So my guess is that the NamingStyleCodeFixProvider code action identifiers are returned incorrectly for them to work properly.

That's a good catch, however this is not really caused by this PR. The code fixes that use EquivalenceKey have actually never worked in OmniSharp 😀
That said I can have a look at fixing it in this PR too, but we may also handle this separately.

filipw · 2019-06-16T20:20:01Z

That's a good catch, however this is not really caused by this PR. ~~The code fixes that use EquivalenceKey have actually never worked in OmniSharp 😀~~
That said I can have a look at fixing it in this PR too, but we may also handle this separately.

Actually I had a quick look and this is caused by a sigh null reference in the Roslyn code, will need to dig a bit deeper 😅

Hoffs · 2019-06-16T20:47:34Z

Yeah, code action does fail due to NullReference, just was guessing that identifier bit was the cause

System.NullReferenceException: Object reference not set to an instance of an object.
  at Microsoft.CodeAnalysis.CodeFixes.NamingStyles.NamingStyleCodeFixProvider.FixNameCodeAction.<ComputeOperationsAsync>d__8.MoveNext()

filipw · 2019-06-16T21:26:30Z

@Hoffs can you check again after the latest commits? it should be fixed now

Hoffs · 2019-06-16T21:33:48Z

Just did, it worked this time

filipw · 2019-06-16T21:50:16Z

....Roslyn/WorkspaceServices/ExportWorkspaceServiceFactoryWithAssemblyQualifiedNameAttribute.cs

+{
+    [MetadataAttribute]
+    [AttributeUsage(AttributeTargets.Class)]
+    public class ExportWorkspaceServiceFactoryWithAssemblyQualifiedNameAttribute : ExportAttribute


cc @savpek needed to pull this in 😀
used the same name so that there are no conflicts

david-driscoll · 2019-06-16T23:41:50Z

src/OmniSharp.Roslyn/Options/IWorkspaceOptionsProvider.cs

-        OptionSet Process(OptionSet currentOptionSet, OmniSharpOptions omniSharpOptions);
+        OptionSet Process(OptionSet currentOptionSet, OmniSharpOptions omniSharpOptions, IOmniSharpEnvironment omnisharpEnvironment);
+
+        int Order { get; }


Public api change... but I guess we don't publish packages yet... so we're good.

indeed. this will be one of the key extensibility points of OmniSharp so we have to get it right, hopefully now it's good, it's changed 3 times already to facilitate various features 😀we do have 3 feature implementations on top of it already too

filipw · 2019-06-17T06:52:06Z

...WorkspaceServices/OmniSharpSymbolRenamedCodeActionOperationFactoryWorkspaceServiceFactory.cs

+{
+    [Shared]
+    [ExportWorkspaceServiceFactoryWithAssemblyQualifiedName("Microsoft.CodeAnalysis.Features", "Microsoft.CodeAnalysis.CodeActions.WorkspaceServices.ISymbolRenamedCodeActionOperationFactoryWorkspaceService")]
+    public class OmniSharpSymbolRenamedCodeActionOperationFactoryWorkspaceServiceFactory : IWorkspaceServiceFactory


Just to explain what is going on here - we need to do this because NamingStyleCodeFixProvider internally tries to resolve ISymbolRenamedCodeActionOperationFactoryWorkspaceService. That doesn't ship together with Roslyn but instead is only implemented inside Microsoft.VisualStudio.LanguageServices, which we can't really reference (it has the whole tree of VS specific GUI dependencies).

On the other hand, ISymbolRenamedCodeActionOperationFactoryWorkspaceService is internal, so implementing is by hand requires a bit twisting

jasonmalinowski · 2019-06-17T18:19:00Z

The approach is adapted from how https://github.com/dotnet/format works (although it only deals with formatting, not with code style/naming conventions/analyzers).

Do note that we're currently in the progress of changing how dotnet-format works. We've been working to update Roslyn to support .editorconfig parsing natively in the compiler and thus won't be reliant on the VS parsing library. I'm not sure what that means for PR, as much as be aware we consider dotnet-format to be an example of how not to do it. 😄

filipw · 2019-06-17T18:54:10Z

@jasonmalinowski unfortunately OmniSharp has to flex quite a lot to make things work in different places (this is just another case) - we've been chasing various internal APIs of Roslyn for a long time, so I think we'll just keep adapting as things change, as always 😀

bjorkstromm

Code and tests look good! Will take it for a spin tomorrow, great work! 💪

jasonmalinowski · 2019-06-18T04:07:41Z

@filipw The good news is this time we actually thought about you all and are happy to make changes accordingly. 😄

bjorkstromm · 2019-06-18T19:44:10Z

Had a chance to play with this. Works really great and it's super powerful when analyzers support is enabled! Thank you!

rchande · 2019-06-18T20:34:57Z

@filipw Note that @jasonmalinowski and I have some time scheduled tomorrow to go over the new APIs and talk about how OmniSharp should use them. Might be nice if you could hold off merging until then... :)

filipw · 2019-06-18T20:47:17Z

we can of course wait for the APIs but personally I'd like to be able to already use this from the "omnisharp": "latest" just so that we can dogfood this and gather some feedback (note that it's all behind an opt-in flag), at the end of the day we can always adapt and change when needed. Not to mention there are plenty other "proper" APIs we are not consuming yet (completion API, quick info etc) too

…he providers

…r formatting to crash when editor config options application doesn't work

filipw · 2019-06-22T13:04:30Z

rebased on top of latest master

Hoffs · 2019-06-23T15:40:09Z

When is this expected to release or rather when is the next planned release of ombisharp-roslyn?

ExceptionGit · 2019-07-12T06:46:16Z

Thanks, but how it's work?

I installed the latest build omnisharp-vscode v1.21.0-beta2 and enabled these flags:
"omnisharp.enableRoslynAnalyzers": true
"omnisharp.enableEditorConfigSupport": true dotnet/vscode-csharp#3136
"omnisharp.path": "latest"

I can't get warning:

-root
--.editorconfig
--proj1
---proj1.csproj
---Program.cs


Peogram.cs
...
FileInfo fi = new FileInfo(path);
Console.WriteLine(fi.FullName);
...


.editorconfig
...
root = true
[*.cs]
# https://github.com/MicrosoftDocs/visualstudio-docs/blob/master/docs/ide/editorconfig-language-conventions.md#csharp_style_var_when_type_is_apparent
csharp_style_var_when_type_is_apparent = true:warning
...

filipw · 2019-07-12T07:08:03Z

can you please provide your omnisharp log and a repro project?

This is what I see with your configuration using omnisharp-vscode v1.21.0-beta2

ExceptionGit · 2019-07-12T09:20:41Z

My root folder ${workspaceFolder} name [Foo] bar contains the characters []. If I rename to foobar, everything will work.

create folder [Foo] bar & open
dotnet new console -n Proj1
put .editorconfig
open via VSCode [Foo] bar folder
no warning

filipw · 2019-07-12T10:58:22Z

@ExceptionGit thanks - I have created a new issue based on your comment #1551
you are correct, when the folder contains [] characters, the editorconfig settings are not discovered.

We actually use an external component - Microsoft.VisualStudio.CodingConventions to do this discovery, and it's really a bug there.
In fact, I just checked, it doesn't work in Visual Studio too. Nevertheless we will use #1551 for tracking this.

nickspoons · 2019-07-15T00:10:46Z

This is really great, thanks @filipw!

After some experimentation however, it seems like .editorconfig settings aren't being passed through to analyzers, is that right? Using roslynator, omnisharp.json settings appear to be passed through to the analyzer and affect diagnostics and code actions, but not .editorconfig settings.

Example:

// Program.cs
public class Program
{
   public string Something { get; set; }
}

# .editorconfig
root = true
[*.cs]
indent_size = 3

// omnisharp.json
{
   "FormattingOptions": {
      "EnableEditorConfigSupport": true,
      "IndentationSize": 2
   },
   "RoslynExtensionsOptions": {
      "EnableAnalyzersSupport": true,
      "LocationPaths": [
         "C:/Users/Nick/.omnisharp/roslynator"
      ]
   }
}

If I now Format Document, OmniSharp-roslyn correctly sets the indent to 3 (as specified by .editorconfig). However, if I move the cursor to the beginning of line 2 there is a "Fix formatting" code action available from roslynator, and running this sets the indent back to 2 (as specified by omnisharp.json).

Removing the "IndentationSize" line entirely from omnisharp.json results in roslynator using the default of 4.

Similar things occur with newlines, where roslynator applies omnisharp.json preferences and O#-roslyn applies .editorconfig preferences.

Enable support for EditorConfig in OmniSharp. Since omnisharp-roslyn version 1.33.0, OmniSharp has had this support. * https://github.com/OmniSharp/omnisharp-roslyn/blob/master/CHANGELOG.md#1330---2019-07-01 * PowerShell#7357 * OmniSharp/omnisharp-roslyn#1526

ramosisw · 2020-09-26T17:02:50Z

can someone help me ?, I have added the file .editorconfig with following lines

root = true

[*.cs]
dotnet_naming_style.instance_field_style.required_prefix = _

also enabled the option

{
    "omnisharp.enableEditorConfigSupport": true
}

in settings.json of vs code, but omnisharp does not consider it.

I wait for the following result:

omnisharp-roslyn/tests/OmniSharp.Roslyn.CSharp.Tests/EditorConfigFacts.cs

Line 285 in f7c6f88

    
           Assert.Contains(getResponse.CodeActions, f => f.Name == "Create and assign field 'xxx_something'");

filipw requested review from david-driscoll, rchande and bjorkstromm June 15, 2019 17:17

filipw commented Jun 16, 2019

View reviewed changes

david-driscoll approved these changes Jun 16, 2019

View reviewed changes

filipw commented Jun 17, 2019

View reviewed changes

filipw force-pushed the feature/editorconfig branch from 16bed31 to 9f14f15 Compare June 17, 2019 14:06

bjorkstromm approved these changes Jun 17, 2019

View reviewed changes

This was referenced Jun 19, 2019

Make it possible to "auto-fix" documents #1519

Closed

Allow to add a global ignore for code diagonistics messages #1518

Closed

Support for IgnoredCodeIssues in config #301

Closed

filipw added 4 commits June 22, 2019 08:46

added ability to read and apply editorconfig options to the optionset

381b5c0

added EditorConfigWorkspaceOptionsProvider and support for ordering t…

c0dc99b

…he providers

fixed tests

221949a

respect editor config when invoking formatting operations

1e7c9ab

filipw added 14 commits June 22, 2019 08:46

enabled feature toggle for EditorConfig

a4f0955

added EditorConfigFormattingFacts

d36d31c

added respecting of codestyle options

b525160

moved initialization of EditorConfigOptionsApplier to static constructor

1edd9e5

added some EditorConfig tests

7d6b716

more tests

2728fdc

added support for naming conventions

24026ee

added tests for code generation

2591fac

added vs editor feed

7de1a79

log provider order

1d6bf9e

added support for running NamingStyleCodeFixProvider code fix

29a3326

added test for NamingStyleCodeFixProvider

d7441b3

added and improved tests

ed9402f

added more logging around editorconfig, also do not allow omnisharp o…

7842db3

…r formatting to crash when editor config options application doesn't work

filipw force-pushed the feature/editorconfig branch from 9f14f15 to 7842db3 Compare June 22, 2019 06:47

david-driscoll merged commit 100e73f into OmniSharp:master Jun 23, 2019

Hoffs mentioned this pull request Jun 24, 2019

Add support for EnableEditorConfigSupport flag dotnet/vscode-csharp#3136

Merged

savpek mentioned this pull request Jul 7, 2019

Enhancements for roslyn analyzers after initial version. #1301

Open

8 tasks

filipw mentioned this pull request Jul 12, 2019

Editorconfig support doesn't work when folder contains [] characters #1551

Closed

filipw mentioned this pull request Jul 15, 2019

Code fixes don't respect formatting settings from EditorConfig #1558

Closed

xtqqczze mentioned this pull request Jan 19, 2020

vscode: Enable EditorConfig support in OmniSharp PowerShell/PowerShell#11627

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added support for .editorconfig #1526

Added support for .editorconfig #1526

filipw commented Jun 15, 2019 •

edited

Loading

kpko commented Jun 15, 2019

Hoffs commented Jun 16, 2019

bjorkstromm commented Jun 16, 2019

Hoffs commented Jun 16, 2019 •

edited

Loading

filipw commented Jun 16, 2019

filipw commented Jun 16, 2019

Hoffs commented Jun 16, 2019 •

edited

Loading

filipw commented Jun 16, 2019

Hoffs commented Jun 16, 2019

filipw Jun 16, 2019

david-driscoll Jun 16, 2019

filipw Jun 17, 2019

filipw Jun 17, 2019

jasonmalinowski commented Jun 17, 2019

filipw commented Jun 17, 2019

bjorkstromm left a comment

jasonmalinowski commented Jun 18, 2019

bjorkstromm commented Jun 18, 2019

rchande commented Jun 18, 2019

filipw commented Jun 18, 2019

filipw commented Jun 22, 2019

Hoffs commented Jun 23, 2019

ExceptionGit commented Jul 12, 2019

filipw commented Jul 12, 2019

ExceptionGit commented Jul 12, 2019

filipw commented Jul 12, 2019

nickspoons commented Jul 15, 2019

ramosisw commented Sep 26, 2020

Added support for .editorconfig #1526

Added support for .editorconfig #1526

Conversation

filipw commented Jun 15, 2019 • edited Loading

kpko commented Jun 15, 2019

Hoffs commented Jun 16, 2019

bjorkstromm commented Jun 16, 2019

Hoffs commented Jun 16, 2019 • edited Loading

filipw commented Jun 16, 2019

filipw commented Jun 16, 2019

Hoffs commented Jun 16, 2019 • edited Loading

filipw commented Jun 16, 2019

Hoffs commented Jun 16, 2019

filipw Jun 16, 2019

Choose a reason for hiding this comment

david-driscoll Jun 16, 2019

Choose a reason for hiding this comment

filipw Jun 17, 2019

Choose a reason for hiding this comment

filipw Jun 17, 2019

Choose a reason for hiding this comment

jasonmalinowski commented Jun 17, 2019

filipw commented Jun 17, 2019

bjorkstromm left a comment

Choose a reason for hiding this comment

jasonmalinowski commented Jun 18, 2019

bjorkstromm commented Jun 18, 2019

rchande commented Jun 18, 2019

filipw commented Jun 18, 2019

filipw commented Jun 22, 2019

Hoffs commented Jun 23, 2019

ExceptionGit commented Jul 12, 2019

filipw commented Jul 12, 2019

ExceptionGit commented Jul 12, 2019

filipw commented Jul 12, 2019

nickspoons commented Jul 15, 2019

ramosisw commented Sep 26, 2020

filipw commented Jun 15, 2019 •

edited

Loading

Hoffs commented Jun 16, 2019 •

edited

Loading

Hoffs commented Jun 16, 2019 •

edited

Loading