Resharper refactorings introduce bad indentation

[Mpdreamz]

In the current version on master, doing any refactorings with resharper often introduces mixed or bad indentation. This is due to the more aggressive unloading of settings.

[robertcoltheart]

Not sure how solvable this is. Settings are unloaded when the window loses focus, which happens when Resharper pops up a refactor window. I haven't tested extensively, but I suspect Resharper is altering text before the window has a chance to grab focus and apply settings again?

[citizenmatt]

I might be able to help from the ReSharper side of things. I'm not sure what's going on with the "unloading of settings" thing, though - could you provide an overview of what the process is, please?

[robertcoltheart]

@citizenmatt I'll try to give a brief summary, but this is just from my understanding of the code.

EditorConfig only sets up the editor environment for spaces/tabs etc and does not do any conversion between spaces and tabs upon saving (see here). To that end, the global tab/spaces settings must be set on opening a file, as it doesn't seem sufficient to modify the local settings (see here and here).

However, modifying global settings means that another open instance of Visual Studio is also affected. To get around this, the original global settings are restored when the text window loses focus and then re-applied when the window gains focus. ReSharper comes into play here because when a ReSharper refactor window opens (like renaming a class) the text window focus is lost, and the tab/spaces settings rules are not followed.

@Mpdreamz do jump in if I've misinterpreted anything.

[citizenmatt]

Thanks! Could that logic be loosened a little so that settings are (re)applied only when the text control gets the focus, and nothing happens when it loses focus? This way, changing focus to a dialog in the same app wouldn't change anything, and ReSharper's refactoring dialogs wouldn't cause settings to change. And then when you swapped to another instance of VS, the new instance would get focus and apply its own settings. Swapping back to the original instance would just reapply the correct settings again. Would that help?

[citizenmatt]

Just for a bit of context, I've just had a look at how we get tab settings, and we cache them per language. Whenever IVsTextManagerEvents.OnUserPreferencesChanged is fired, we invalidate and refresh the cache. I don't know how expensive this is, but I guess we can't do it on demand due to threading requirements - we have to be on the main thread to get this data in the first place.

[citizenmatt]

And, because I'm enjoying replying to myself, how is editorconfig core loaded into VS? As in, if ReSharper had its own copy of editorconfig core, and took its settings direct, without using VS at all, could that work? Would it involve loading 2 editorconfig dlls, potentially of different versions? Is it strong named? (I guess we'd also need a way of checking to see if the VS extension was installed, so that we didn't do refactorings based on editorconfig, while VS ignored it).

The reason I'm asking is that we can also refactor code without opening the files (e.g. renaming a symbol across a hundred files). In this case, we wouldn't get the right settings. Mind you, we also treat the tab settings as global, and get them per file type, rather than per file, so we wouldn't work with more "interesting" configurations that are different in one folder to another.

[robertcoltheart]

With regards to only applying settings when you get focus, the problem is you won't know what the previous global settings were when you open up a new VS instance. This is the reason the original settings are applied on losing focus.

[robertcoltheart]

Not sure sure about working with ReSharper, I think you'd be more of an expert at it than me. EditorConfig just loads like a regular VS MEF package. The .editorconfig file is loaded every time it goes to save or apply settings like tab/space size, which can happen multiple times with a text window open.

[citizenmatt]

How is editorconfig core loaded? Is that a separate assembly that's owned by the VS addin? I'm just thinking if ReSharper added direct support, and the assembly was strong named, we could both load it without version clashes.

This problem might not be easily solved, especially the bit where ReSharper refactors code without opening the file. We wouldn't have that file's settings then, and it could easily be wrong. There's also the problem with Visual Studio's settings synchronisation, which could unexpectedly overwrite file settings at any time - although this affects both ReSharper and editorconfig.

[Mpdreamz]

Hey @citizenmatt thanks for exploring this space :smile:

Editorconfig visual studio plugin uses the C# port of editorconfig core https://github.com/editorconfig/editorconfig-core-net which is strong named.

What are some scenarios where you do refactorings without a file?

Never thought of the visual studio settings sync as a wreak havoc factor before! That will be very very tough to solve!

[robertcoltheart]

Maybe similar to https://github.com/editorconfig/editorconfig-vscode/pull/46#issuecomment-210569328 we're going to have to bend the Editorconfig rules a little bit? Seems like the only way around this is to either (1) find a way of doing this without modifying global settings or (2) stick to simply modifying the text in-place at save time. (1) is preferred, but after tinkering with it for a few days, I didn't get anywhere with it.

[citizenmatt]

@Mpdreamz If the refactoring is going to change and therefore open a large number of files, we'll pop up a message box warning that this will cause issues with the IDE and allow the refactoring to happen with the files closed. I forget the threshold, but let's say it's about 100 files. Similarly, other refactorings have a checkbox to allow the changes to happen without opening files, again for perf reasons, although we also mention about undo not being available if the files aren't open (usually an easy fix - run another refactor 😄)

[citizenmatt]

@robertcoltheart I'd much prefer (1) over (2) as well. Reformatting on save sounds like a recipe for surprises. Trouble is, I don't know how to do this without modifying global settings. If it's necessary to work with various VS features, then it has to be done. But I don't see how to work around the fact that they're global, and they will always affect a new instance of VS.

Perhaps bending the rules is the way to go. Perhaps you should always ignore the "real" VS global settings. Whenever a text editor gets focus, apply the settings for that file (local and global). If the solution isn't under editorconfig control, always apply known defaults - Visual Studio's own default values, hard coded, not looked up. This would allow getting rid of the un-setting, and ReSharper would at least work better. So you no longer care about keeping the global defaults in sync between instances, but just use a known set of defaults. That known set could be actually hardcoded to VS defaults (if you have editorconfig installed, you're most likely to set it in a file, or use the standard defaults), or even provide your own global settings in an EC options page.

Another option would be for ReSharper to implement EC directly. This looks feasible, and would solve most issues, including the refactoring without a text window open. I think it would even mean that EC would just work without any other plugin, in languages that ReSharper supports (we already manage formatting and indenting, so if we just ignore VS and use the EC file, it seems like it would just work). In languages that we don't support (e.g. F#), standard VS settings or EC plugin settings would apply. This looks like the best option for ReSharper users. I've chatted with the dev who kinda owns this area, and it looks like it will work, but I can't promise if/when we'll actually do this.

Settings sync can still cause problems. Looks like you can disable synching text editor settings - I don't know if you can do this programmatically, or even if you should, as there will be more than just indenting settings here. The other option is to look to see if VS will at least notify you that settings have changed. There are some interfaces but I haven't looked into them to see what's on offer. I suspect there's a notification, so you could probably subscribe to be told when they change, and then quickly change them back :) Or perhaps there's a way of taking the indent settings out of the sync list...

[citizenmatt]

Looks like the SVsSettingsPersistenceManager service and related interface provides a way of registering for events, but it's completely undocumented, and based on strings that also aren't documented, so it'd be a bit tricky to figure out quite how it works...

[Mpdreamz]

Perhaps bending the rules is the way to go. Perhaps you should always ignore the "real" VS global settings. Whenever a text editor gets focus, apply the settings for that file (local and global). If the solution isn't under editorconfig control, always apply known defaults

I like this route the best as its very pragmatic, we can ship with an EditorConfig settings dialog thats basically a big text box where folks can sculp their own default fallback editorconfig file. Defaulting to VS default settings.

This would make editorconfig work the same way as before which played alot nicer with resharper but still improve the story around opening a 2nd VS with an editorconfigless solution.

Another option would be for ReSharper to implement EC directly.

That would be huge too, at the same level of webstorm shipping with the plugin by default. If you guys end up doing that we should find a way so both plugins can detect the existence of one another so we can back off.

I'd much prefer (1) over (2) as well. Reformatting on save sounds like a recipe for surprises.

This is partly what we do now, and on open file. I've gone back and forth on these (e.g disabling it here but adding it back later).

I'm +1 on removing it again from on open thats evil. Getting rid of the on save will also go a long way with #38, but it is a really nice feature. Not sure where I stand there, you guys? In any case it could be an option in the settings dialog.

Is there a way we can hook into Format Document that you guys know off?

[citizenmatt]

Are you talking about ReSharper's code reformatter? If so, the answer is "not really". You might be able to do something with hooking VS commands, but otherwise you'd need a ReSharper extension, which is far from ideal here.

[clairernovotny]

Jumping in the fray here, but it almost seems like what's needed is an EditorConfig plugin to ReSharper where R# can get the right per-file formatting rules? That may work better than trying to co-ordinate between two addins, esp when files aren't open.

[citizenmatt]

Yeah, that's what we're suggesting - ReSharper knows about EditorConfig directly, rather than relying on the VS plugin. It requires changes to ReSharper though, so can't be implemented as a ReSharper plugin. There's no coordination necessary, since EC.Core is strong named, we can both ship a copy and the right versions will get loaded.

[robertcoltheart]

The core problem (setting the tab/spaces default settings for an editor) is not strictly to do with R# however. This is a problem that would exist with or without R#. You see this in VS2015 with the Roslyn formatter (for instance, when you enter a closing brace or ; char) when it does smart indentation and other formatting fixes.

To put it another way, local settings are used for typing normal characters and tab key etc, but the Roslyn formatter uses global settings to do smart indenting and formatting when you hit ;, which obviously messes things up.

[citizenmatt]

BTW, direct EditorConfig support is currently in the plan for the next ReSharper version (2016.3).

[robertcoltheart]

@citizenmatt @Mpdreamz What's our current thinking with this? Obviously this problem will go away with R# 2016.3 if plans go ahead, but presumably we'll want to continue supporting previous versions of R#. The problem may also exist for any other VS extensions that do popup windows that steal focus.

Personally I feel we're at a bit of a stalemate and that there won't be a "perfect" way of getting around this issue. Also given that there is talk of VS supporting EditorConfig natively (https://groups.google.com/d/msg/editorconfig/M7sl8ZWU4yo/8YLu1LLtCAAJ), do we need to spend a lot of time on this?

[Mpdreamz]

Yeah I'm personally all for having a global/default editorconfig (file/in memory) we ship with (and is editable) for solutions with no editorconfig file.

That way we can apply settings more aggressively (as per the currently released version) and it would play nicely with resharper (as the currently latest released version does too).

VS shipping with editorconfig will be HUGE, but until then and for older vs versions it would be nice if we can get another known good release out.

[citizenmatt]

Sounds good to me!

[zvirja]

@citizenmatt Are that plans for R# 2016.3 still actual (given that it will be released soon)? Do you have a ticket in youtrack which we could track? :)

[citizenmatt]

Hmm. Unfortunately not. It completely slipped through the cracks. It was in the planning docs, but didn't get any further. I've belatedly raised an issue - RSRP-461746.

[zvirja]

@citizenmatt Thank you for reply! Looking forward to this feature in the next updates! 😉 Hope it will be prioritized, because now it's like a hell - you need to change VS settings globally to ensure R# picks them up 😢

Update: Noticed that the bug has been scheduled to be a part of 2017.1 release.