dotnet / roslyn

The Roslyn .NET compiler provides C# and Visual Basic languages with rich code analysis APIs.
https://docs.microsoft.com/dotnet/csharp/roslyn-sdk/
MIT License
18.91k stars 4.01k forks source link

Connect: Entering a # character forces all preprocessor entries to be reindented #2487

Open dotnet-bot opened 9 years ago

dotnet-bot commented 9 years ago

Ported from TFS WorkItem: 1160552


Repro Steps:

This item was filed by a customer. Please communicate and close the loop with your customer via the customer tab inside TFS.


Problem Description


Hi,

Entering a # character forces all preprocessor entries to be indented again, even when all formatting options have been disabled. It also displaces some "}"s

My setup is: automatical formatting fully disabled , tabs size=4, keep tabs, indenting=none

I'm supplying two images to ease the explanation.

image1: https://dl.dropboxusercontent.com/u/49509038/img/vs2015_ctp6/img1.png

image2: https://dl.dropboxusercontent.com/u/49509038/img/vs2015_ctp6/img2.png


Revisions:

1) Created By Microsoft Connect (4/21/2015 4:46:12 AM)

------------------------
Customer Information
------------------------
User ID: 10125130
Handle: KakCAT

---------------------------
Connect Site Information
---------------------------
Site Name: Visual Studio and .NET Framework
Site ID: 210
Feedback ID: 1266613
Feedback Form: 6049

----------------------
Problem Description
-----------------------
Hi,

Entering a # character forces all preprocessor entries to be indented again, even when all formatting options have been disabled. It also displaces some "}"s

My setup is: automatical formatting fully disabled , tabs size=4, keep tabs, indenting=none

I'm supplying two images to ease the explanation.

image1: https://dl.dropboxusercontent.com/u/49509038/img/vs2015_ctp6/img1.png image2: https://dl.dropboxusercontent.com/u/49509038/img/vs2015_ctp6/img2.png


2) Edited By Macy Qu (Shang Hai Wei Chuang Ruan Jian) (4/21/2015 8:11:12 PM)

repro on Win8.1 with VS2015 CTP6


KakCAT commented 9 years ago

Hello,

I'm the original bug reporter. I decided to create a self explaining CS file to ease the bug hunt:

http://online.ru-zero.com/files/test.cs

Thanks!

basoundr commented 9 years ago

Thanks @KakCAT. Thank you for the feedback. We will be addressing the issue soon.

mattwar commented 9 years ago

The # key is a trigger character for formatting. It automatically formats the region before and after the #. This entails all trivia between the previous token and the next token after the #. All of the directives in the sample are part of the trivia between the # and the next token, so they all get formatted.

The sample has text that is not currently at the proper indentation level. All of the text in the format range gets indented to the proper level, this includes the directives, shifting all the way to the left and the method call, getting shifted to the right, but none of the surrounding type or method declarations get adjusted because they are outside of the auto formatted region.

AFAICT this is the expected and intended behavior of the formatter.

@pilchie any thoughts?

KakCAT commented 9 years ago

Hello,

I don't know if this is correct or not, but my opinion is that in case it is correct, there should be a way to disable it, as well as automatical format can be disabled on ; , on } and on paste.

Reformatting shouldn't be enforced on any case if the user doesn't want to use it.

mattwar commented 9 years ago

This one doesn't look like its going to be easy to fix, not without a lot of rewrite of the command handlers or the formatter. The formatter only operates between tokens and since the # is part of trivia it doesn't count as a token. All special action keys other than ENTER are handled by the format command handler, which just formats. The ENTER key has its own command handler that possibly uses the smart indenter (but in some circumstances uses the formatter). It looks like this scenario would be better served by the smart indenter and not the formatter, but I'm not certain of all the ramifications of that change. Possibly the formatter needs an overhaul.

As for options, there currently only options to control formatting when ; or } are typed. All other keys that trigger formatting have no option, so they default to on.

Pilchie commented 9 years ago

Moving out to 1.1 based on Matt's assesement. Note that this only an issue around comments - if there are real tokens inside the preprocessor directives, this isn't as bad.

KakCAT commented 9 years ago

Hi,

shouldn't there be a switch where all autoformatting is disabled?

I mean, this may or not may be important, but I'd really prefer to completely disable autoformatting if typing a '#' in my code is going to mess all my format. Right now my only option is keep using VS2013 and switch to VS2015 to compile when required.

Pilchie commented 9 years ago

@KakCAT You are right - this behavior should NOT occur when the Indent Style option is set to none or block. I filed #2907 to track that.

Pilchie commented 9 years ago

Note that connect bug http://connect.microsoft.com/VisualStudio/feedback/details/1247309/gets-wrong-indentation-for-region-and-endregion is related to this.

Once I wrap some text with #if false, we lose the indent of all the #regions when the #endif is typed.

weltkante commented 7 years ago

This issue makes it sounds like the impact isn't so bad. Actually it's really bad. So I don't quite see why this keeps getting pushed back:

When I type (or paste) preprocessor directives like #if conditions the IDE unindents all #region directives while the preprocessor directive I'm typing is not yet complete. The damage to the document is never repaired, so after I'm done writing the preprocessor directive the document remains edited in lines I never touched.

Just typing text shouldn't trigger global edits over the whole document! The suggested workaround of doing a "format document" often cannot be used in practice because they still result in whitespace damage of the document if your VS autoformat settings don't match the project exactly. Repairing the damage manually is a pain.

Also it often happens that the modified regions aren't even on screen so the developer may not notice the whitespace damage done when checking in to source control. (Made worse by default VS settings ignoring whitespace changes when comparing.)

ygoe commented 7 years ago

@weltkante It is actually so bad that it may eventually be fixed in whatever version will be released in a decade or so. A whole industry will have developed to work around that issue by then. Facing that, it will never be fixed not to hurt that industry. So it will forever remain on my public list of VS 2015 and 2017 bugs.

Trisibo commented 7 years ago

The reindentation also happens when some some code is autoformated after the preprocessor directives, see https://developercommunity.visualstudio.com/content/problem/50425/c-editor-removes-indentation-of-preprocessor-direc.html

CyrusNajmabadi commented 7 years ago

@ygoe @Trisibo PRs welcome :)

ygoe commented 7 years ago

@CyrusNajmabadi Tell me where the offending code is and I can try removing it for you. As soon as I find the time setting up a development environment for the code and getting sufficiently familiar with it. More advanced solutions should probably be made by more experienced people in this project.

CyrusNajmabadi commented 7 years ago

Based on what Matt says, it sounds like it could be in "FormatCommandHandler.cs". I don't think this is a case of there being "offending code". It's not like tehre's a section of code that says "if # is typed, go and do bad stuff". Rather, there is a confluence of many different components of roslyn interacting with your text and producing a result that is undesirable.

Of note: no effort has been made in roslyn to support coding in the following pattern:

namespace N1
{
class C1
{
void myfunc ()
{
...

That's just not a scenario the existing systems were designed for. They make be able to be updated to work for that style of coding, but it's not necessarily a matter of just going and removing some code.

Good luck!

ygoe commented 7 years ago

With the above pattern, do you mean writing code while not having closing braces? Which may be the case after an incomplete # line?

I imagine the offending code is the part that triggers reformatting when not appropriate. It might be appropriate if the process would actually produce a desirable result. But since it does not, it really shouldn't be called at those times.

I very much prefer too little correct automatic behaviour over too much wrong automatic behaviour. Especially in places I don't notice it. You can always use a hotkey to trigger formatting of local code if you feel it necessary. But I don't want to use that on other parts of the code only to clean up what the editor has messed up. This is frustrating. More than an editor not being as smart as it might get.

CyrusNajmabadi commented 7 years ago

No: i meant the pattern of having code not-indented. i.e. the style of code referenced in this file: http://online.ru-zero.com/files/test.cs

ygoe commented 7 years ago

I don't write such code either, and yet the formatting bug happens.

mattwar commented 7 years ago

There are 3 things going on: 1: Unrelated code near the # is being formatted when the line with the # is being formatted. 2: The code that is being formatted is not formatted matching the layout of the rest of the code. 3: There is no way to turn off the formatting when # is typed.

1 happens because the code in question is all considered trivia by the formatter and exists between actual tokens. The formatter does not have a concept of places between tokens, and so must choose the tokens before and after the # to define the area it is working on. One way to fix this problem is to change the implementation of the formatter to understand boundaries for trivia that is not whitespace.

2 happens because the formatter is based on settings and rules that don't generally account for the layout of existing code that it is not currently formatting.

3 happens because there is no setting for turning it off. Probably the easiest one to change.

Bunderant commented 6 years ago

It's inconvenient, but as a workaround, just get into the habit of triggering an "undo" after each time you type a # character. The reformatting gets applied as a separate event in the undo/redo stack, so your formatting will return to normal, keeping the # you typed in. Works for me on Visual Studio Community for Mac, at least. Hopefully this saves some of you some irritation.

ygoe commented 6 years ago

@Bunderant You can even reuse that habit to work around the broken forced auto-formatting of .cshtml Razor view files. I see a pattern here in Visual Studio: Always undo everything that's automatically changed for you to prevent damage. And it makes me sad.

jbienzms commented 6 years ago

I was recently searching to see if employees have access to ReSharper specifically because of this issue.

Our team writes code for Windows Mixed Reality, AR and VR. We frequently write C# code in Unity that needs to compile differently for iOS, Android and UWP. This code results in statements that look like:

#if UNITY_IOS
// iOS code
#elif UNITY_UWP
// UWP code
#endif

We want to read those #if statements as though they're regular if statements (though they're not since the missing platform-specific types would not compile). In addition, every time a # is typed, all of the regions in the file are reformatted. Undo has to be used every time. To make matters worse, if at least one region isn't on the screen when reformatting occurs you might not even realize that it's happened. This ends up leading to a lot more typing that has to be undone in order to restore formatting to the document. It's quite frustrating, interrupts mental flow, and has a very negative impact on productivity.

Interestingly, Visual Studio allows control over this feature for C++ but does not for C#. You can change it for C++ in Tools -> Options -> Text Editor -> C / C++ -> Formatting -> Indention -> Position of preprocessor directives.

The lack of this feature (and the frustration it can lead to) has been brought up many times. Here are just a few examples:

It currently has 29 votes on UserVoice:

https://visualstudio.uservoice.com/forums/121579-visual-studio-ide/suggestions/16825309-add-position-of-preprocessor-directives-to-c

ReSharper has the feature here:

https://www.jetbrains.com/help/resharper/EditorConfig_CSHARP_CSharpIndentStylePageSchema.html#Preprocessor_Directives

I notice that this issue was opened back in May of 2014 and is currently scheduled for milestone 16 ("The version after the next major version"). Can this be prioritized any sooner?

CyrusNajmabadi commented 6 years ago

@sharwell I think you mentioned in a previous issue that the current thinking is that there would not be features to control indentation here. Am i misremembering? I'm personally ok with this just be the standard behavior across all C# code given that this indentation has worked this way since V1 of the IDE.

jbienzms commented 6 years ago

I do sincerely hope that's not a vote not to fix the issue because "that's the way it's always been"...

CyrusNajmabadi commented 6 years ago

Not exactly. I'm definitely amenable to things like: we should not do any indentation/formatting when you type #.

My general position though is that we should not specifically add options to control formatting here. I'd prefer only a single way to format here. So, if you explicitly format, this gets unilaterally formatted (like we do with many other language features).

That said, i would not be opposed if someone from the community came and implemented this feature. But i would personally not deprioritize any IDE work, or otherwise adjust any sort of scheduling to make this happen.

jbienzms commented 6 years ago

I understand. However, I don't feel that one persons personal preferences should be hard-coded into the way Visual Studio enforces code formatting. Especially since Visual Studio already offers this formatting as a choice in other languages (C++). If this is something I could help implement I'd be happy to. Though I'd need some pointers on where to get started. It sounds like this issue is nested pretty deep in the way C# code is parsed and handled as "trivia".

CyrusNajmabadi commented 6 years ago

I understand. However, I don't feel that one persons personal preferences should be hard-coded into the way Visual Studio enforces code formatting.

This is how the IDE has worked for the vast majority of the language. There is simply no way to configure formatting/indentation for most of it. The behavior is baked in and non-overridable. At certain important points, we've exposed some formatting knobs. But, by and large, they're the vast exception to the rule.

specially since Visual Studio already offers this formatting as a choice in other languages (C++)

Different languages will feel differently about hte level of control and flexibility they feel is important for their ecosystem. For example, TypeScript has different choices in this arena. VB doesn't even expose any control over formatting :)

If this is something I could help implement I'd be happy to. Though I'd need some pointers on where to get started.

The most likely candidates here are the FormattingEngine: https://github.com/dotnet/roslyn/blob/master/src/Workspaces/CSharp/Portable/Formatting/Engine/CSharpFormatEngine.cs

As well as the indentation system: https://github.com/dotnet/roslyn/blob/master/src/EditorFeatures/CSharp/Formatting/Indentation/SmartTokenFormatter.cs

@heejaechang Knows these systems the best and could likely guide you along.

It sounds like this issue is nested pretty deep in the way C# code is parsed and handled as "trivia".

That's quite likely true. But not at all impossible to deal with :) The way Roslyn deals with "trivia" is actually pretty sane. I recommend starting over at https://docs.microsoft.com/en-us/dotnet/csharp/roslyn-sdk/get-started/syntax-analysis to get a sense of how the Syntactic model works as it's likely going to be important here.

It sounds like this issue is nested pretty deep in the way C# code is parsed and handled as "trivia".

sharwell commented 6 years ago

@jbienzms The issue you're talking about is a duplicate of #12306. The issue in this thread involves the incorrect alteration to existing indentation of unrelated preprocessor directives throughout a file during typing scenarios.

When I first wrote this comment, I was getting this issue mixed up with #25973. However, the request to provide an option for indentation of preprocessor directives is the other issue I linked to.

CyrusNajmabadi commented 6 years ago

Ah. The issue was closed. That's why i couldn't find it. Thanks @sharwell

heejaechang commented 6 years ago

Hi. sorry example code image has been removed. can someone write some examples on what the problem is? I might at least try not to do damage when we can detect such cases.

jbienzms commented 6 years ago

Thanks @sharwell. I truly feel it's a mistake to close #12306. I do understand that #12306 is about providing an option for controlling indention and this issue (#2487) is about the '#' character causing preprocessors to be re-indented.

It seems based on #12306 that there is an unwillingness to provide a configurable option. While I sincerely wish this stance would be re-evaluated, could the tool at least be updated to stop unindenting code that's been manually indented?

CyrusNajmabadi commented 6 years ago

could the tool at least be updated to stop unindenting code that's been manually indented?

I think the problem is that htere's no way to easily know if code was 'manually indented', or was intented for other reasons (i.e. bad copy/paste). In general, when we format, we format it all to match the coding style that we've baked in since V1. Having that code now try to understand what is intentionally manually indented, vs unintentionally bogusly indented is not something that's clear to me how we would code up.

CyrusNajmabadi commented 6 years ago

and this issue (#2487) is about the '#' character causing preprocessors to be re-indented.

I thought this was not supposed to happen if you had smart-indent off. If you change this:

image

To 'None' or 'Block' do you still see # automatically updating the indentation?

jbienzms commented 6 years ago

Maybe. But wouldn't turn off other smart indenting features too? Like inside of curly braces, while loops, etc.?

CyrusNajmabadi commented 6 years ago

Maybe. But wouldn't turn off other smart indenting features too? Like inside of curly braces, while loops, etc.?

Yes. I think so...

In general we don't give any control over indentation except for block contents, labels and switch-contents. Everything else is uncontrollable. I'd be wary about adding one-offs for each language construct that is currently indented today. For example, some people have requested there be a one-off construct to control if the contents of a class/struct/interface should be indented. i.e. they want:

class C
{
int i;
int j;
}

But we've felt that adding these options for all these cases just isn't a very good idea. It's healthier (IMO at least) to have consistency here across all codebases to uniformly indent in the same manner.

--

That said, that's why turning it off 'Smart' may be a workaround here. It gives you full control over this, allowing you to decide what sort of indentation you want. If you want classes to look like the above, but you also want preprocessor directives indented, then you can get that (and we shouldn't ever touch it**).

--

** Note: i can't 100% guarantee that we won't ever touch things. We do have features that format the code they affect (like Lightbulbs and whatnot). I'm not 100% sure that when they do their work that they won't end up causing this code to be formatted like all the rest of the code out there.

heejaechang commented 6 years ago

there are so many conversations so not sure what this bug is about.

if it is about supporting this - https://github.com/dotnet/roslyn/issues/12306, I don't think there is any people against it, just it seems it gets lower priority so not getting any traction from Roslyn people but open to any contribution.

otherwise, can't figure out what behavior people are talking about except it is related to "#" character.

heejaechang commented 6 years ago

ultimately, I think opening up formatting/indenting infrastructure of C# editor will be the only answer to it I think. since formatting, everyone has a different preference and there is no way we can provide every possible option for those preferences. the only way will be let them plug in their formatter that format code whatever way they want.

to make it easier, we might provide base or default implementation of formatter so that one can just override ones they don't like.

weltkante commented 6 years ago

@heejaechang see mattwars comment on May 3, 2017 for a summary of a few issues

my own another bug report #25973 which was resolved as duplicate of this one, was about interactively typing (or pasting) individual preprocessor directives which are part of a pair of directives. Typing or pasting a preprocessor directive was triggering auto-format of the whole document when seeing the initial # symbol, even though the document is in an incomplete state (I'm not done typing), so the auto-format gets indentation of e.g. #region directives wrong. This causes whitespace modifications on other (unrelated) preprocessor directives which are never repaired when you continue typing and the document eventually gets back into a valid state.

Workaround is to type ctrl+z (undo) whenever you are typing/pasting preprocessor directives.

[edit] actually this was not my bug report, got things mixed up, but great if it helps anyways

heejaechang commented 6 years ago

@weltkante ah, thank you for the screencast. now I see exactly what the issue is. this looks like a bug. not sure what is the actual cause. but let me assign the issue to me and see whether I can fix it.

sharwell commented 6 years ago

:memo: I'm going to collapse all the comments not related to the original issue so the person interested in working on this has a good starting place. I might get some of the reasons wrong, but there is only a very limited selection to choose from.

heejaechang commented 6 years ago

@sharwell thank you! didn't know one can do that.

sharwell commented 5 years ago

🔗 Also reported here: https://developercommunity.visualstudio.com/content/problem/336974/auto-formatting-too-aggressive-for-the-common-bloc.html

rjmholt commented 4 years ago

Brought over from https://developercommunity.visualstudio.com/content/problem/1094319/region-autoindent-dedents-herestring-lines-startin.html.

I've experienced this, plus erroneous formatting of #s inside a verbatim string:

string s = @"
    # A PowerShell comment
    Get-ChildItem
";

becomes:

string s = @"
# A PowerShell command
    Get-ChildItem
";
CyrusNajmabadi commented 4 years ago

@rjmholt I don't repro this at all. Can you supply your full file that demonstrates the issue? Thanks!

rjmholt commented 4 years ago

The full file I experienced this with was this one

CyrusNajmabadi commented 4 years ago

Still can't repro the issue. Which line were you on?

rjmholt commented 4 years ago

So I was making edits here.

Based on the guidance in the original issue I opened, I just experimented with editing and it occurs when new ifdefs are being added. When #else is added from the wrong indentation level and the auto-indenter kicks in, it then proceeds to indent everything after it (I assume due to the lack of a closing token), including lines starting with # in verbatim strings.

Here's a gif:

regionindent

CyrusNajmabadi commented 4 years ago

Ah thanks! that will def help narrow what to investigate. so it's not that editing in the string caused reindentation. Rather, editing of pp-directives outside of hte string affected indentation of code inside of it. Thanks! :)

mattwar commented 4 years ago

Once a section of code is momentarily #if’d out, it is no longer parsed as c# tokens, and is no longer thought of as containing things like keywords or verbatim strings. The one thing it does continue to recognize is other # directives, which is now no longer inside a string.

mattwar commented 4 years ago

It might be interesting to explore changing the c# parser to keep parsing c# tokens inside of #if'd out blocks. This way we could also colorized them (albeit dimmed).