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.73k stars 3.99k forks source link

An option controlling the indentation of code #if blocks should be provided. #12306

Open tannergooding opened 8 years ago

tannergooding commented 8 years ago

Currently #if blocks are always formatted to start at the beginning of the line:

public class ExampleClass
{
    public void ExampleMethod()
    {
#if DEBUG
#endif
    }
}

An option should be provided so that the #if block can be indented to the desired level (probably the same options as label has, which are left-most, indent one less, and indent normally):

public class ExampleClass
{
    public void ExampleMethod()
    {
#if DEBUG
        // Some code
#endif
    }
}
public class ExampleClass
{
    public void ExampleMethod()
    {
    #if DEBUG
        // Some code
    #endif
    }
}
public class ExampleClass
{
    public void ExampleMethod()
    {
        #if DEBUG
        // Some code
        #endif
    }
}

It would also be useful, in the last example, to be able to indent the contents one more level:

public class ExampleClass
{
    public void ExampleMethod()
    {
        #if DEBUG
            // Some code
        #endif
    }
}
gordanr commented 8 years ago

I agree. It would be useful.

daniel-keen commented 6 years ago

It's been a year already.. pls implement this feature :(

CyrusNajmabadi commented 6 years ago

@thexdd Would you be interested in contributing here? I can give you some pointers on how you could start!

sharwell commented 6 years ago

I'm adding this and #12305 to an upcoming design review to determine if this is a feature we want to start maintaining in the core formatting code.

daniel-keen commented 6 years ago

@CyrusNajmabadi I would love to participate in that, actually.

@sharwell Sounds great. Looking forward to hearing more information regarding this issue!

sharwell commented 5 years ago

The indentation behavior of #if has remained the same for many Visual Studio releases. During today's design review, the team generally felt that uniformity provided by this uniformity is an asset to the ecosystem, and we would not want to introduce a configuration item related to this outside of empirical evidence showing significant use of an alternative formatting.

@thexdd I know this isn't the news you wanted to hear. However, we would love to have you contribute to the project. We have a rather large selection of issues in our Small Fixes project, or you can look for issues labeled help wanted to see if something there catches your interest.

jbienzms commented 5 years ago

I would like to vote for this issue to be re-opened. Instead of copying and pasting all of my reasons why inline, I'll refer to the existing reasons I already gave here:

https://github.com/dotnet/roslyn/issues/2487#issuecomment-418837810

@sharwell you wrote above:

we would not want to introduce a configuration item related to this outside of empirical evidence showing significant use of an alternative formatting.

As I mentioned in my linked post above, there are many requests for this feature on Stack Overflow and this feature has 29 upvotes in Visual Studio UserVoice. This feature also exists in ReSharper, so there is at least some evidence that there is demand.

Any chance you could elaborate on the specific numbers or statistics that the team would accept as empirical evidence to necessitate this feature?

CyrusNajmabadi commented 5 years ago

@jbienzms I would be interested in the number of github projects, for example, that use this style for C# (i.e. the majority of their pp-directives are indented, and it's not just a one-off). I think there are some projects that have analyzed github and placed the results in queryable DBs. But i'm not personally familiar with them or how you'd go about efficiently querying all this data.

tannergooding commented 5 years ago

I would be interested in the number of github projects

@CyrusNajmabadi, one of the problems with this is the same as with the "switch case" indentation issue (which was eventually fixed). That is, the VS IDE, under its default settings, fights against you from formatting your code this way (and in many cases, there isn't an option to prevent the IDE from reformatting your code).

However, as @jbienzms indicates, there are a number of requests on various external sites and other editors or extensions do allow this. Other languages, even in VS, also provide options for these types of settings (such as C/C++).

CyrusNajmabadi commented 5 years ago

Other languages, even in VS, also provide options for these types of settings (such as C/C++).

Other languages do not though :) VB allows no formatting options for example (and there have definitely been requests for it :)). C/C++ has never been a guiding language to help motivate IDE scenarios. (And i believe i can say that that has been true since forever :)).

@CyrusNajmabadi, one of the problems with this is the same as with the "switch case" indentation issue (which was eventually fixed).

True. :)

Part of the reason there was because there was a regression involved. Our older formatter would not indent blocks in a case, but then we broke that (probably in the roslyn rewrite). We were then in the unpleasant position of potentially having two sets of users, those who had code with the original style, and code that was now being formatted into the new style. Fixing the regression would break thsi new group. So the undesirable position was taken to introduce the option to allow people to say which behavior they wanted.

CyrusNajmabadi commented 5 years ago

However, as @jbienzms indicates, there are a number of requests on various external sites and other editors or extensions do allow this.

Agreed. This may be enough information to warrant this. While i'm not necessarily of the belief that we should just do what another editor does, i do think there is significant enough feedback through regular channels to warrant the question being asked and decided on again.

As before, my personal opinion would be to not have it. But it's certainly up to the team, and they may feel differently.

jinujoseph commented 5 years ago

Reopening for reconsideration.

jbienzms commented 5 years ago

Since the comment I linked to above has been hidden as off-topic, I just wanted to paste in my supporting evidence for this issue here.

Thanks

Examples of users requesting this feature:

UserVoice Request for this feature:

Plugins for Visual Studio that add this feature:

sharwell commented 5 years ago

Thanks for gathering those links @jbienzms, it's very helpful. 😄

sharwell commented 5 years ago

We reviewed this proposal in light of the new options given above, and came to a conclusion that the proposal described in https://github.com/dotnet/roslyn/issues/28729#issuecomment-426247800 would apply to this feature request as well. I'll create a Story issue to design the new extension point and link it to the two open issues it covers.

sharwell commented 5 years ago

Marking as Blocked on #31691

jeroen-posch commented 3 years ago

L.S.

I would very much like to be able to specify the indentation level of the preprocessor directives as well. I am of the opinion that always moving the preprocessor directives to the left-most column does not much help with the readability of one's code. I think that indenting the preprocessor directives one level back from the code would be the most useful, but one should be able to specify this.

Regards, Jeroen Posch.

CyrusNajmabadi commented 3 years ago

@jeroen-posch This is currently blocked on https://github.com/dotnet/roslyn/issues/31691

auriou commented 2 years ago

same problem, which I described here https://stackoverflow.com/questions/70172837/roslyn-add-space-for-region-and-endregion

Chris-70 commented 1 year ago

The code exists to move the preprocessor directive to the first column so you'd think it would be easy to NOT move it. I can't believe that 6 1/2 years later and it still hasn't been fixed.

norrisgc commented 1 year ago

At the very minimum, there needs to be an option to prevent our own carefully crafted indentation being wrecked.

MV10 commented 1 year ago

Now a full seven years ... those indentations must be fiendishly tricky things!

Even margin-collapse can't get it right... image

CyrusNajmabadi commented 1 year ago

@MV10 can you file a separate issue on that?

those indentations must be fiendishly tricky things!

We'd happily accept a PR to fix that up if you're interested. Thanks! :)

sharwell commented 1 year ago

At the very minimum, there needs to be an option to prevent our own carefully crafted indentation being wrecked.

There does not need to be an option specifically for this. Features within the IDE today are generally expected to not reformat code which is outside the scope of the action being applied. There are a few cases where we know of problems occurring (e.g. #2487). Many other cases would be considered edge case bugs.

Note that in most cases, code does not need to include #if/#endif because other options are available (e.g. polyfills or ConditionalAttribute). The remaining cases should be sufficiently infrequent that the default indentation is irrelevant because users almost never see it anyway.

MV10 commented 1 year ago

I could make a shopping list of things that most code never uses ... unsafe, structs, probably half of the entire .NET API surface.

That doesn't make it irrelevant how the IDE treats it.

CyrusNajmabadi commented 1 year ago

@MV10 Would be happy to work with you on addressing the issue you ran into. It likely would not make it off our backlog for a while. But we could def spare the time/resources to help the community with fixing it and getting that fix into an upcoming release. Thanks! :)

TwoBitMachines commented 11 months ago

Why is thi still not fixed?

sharwell commented 11 months ago

@TwoBitMachines There is nothing to "fix" here. This is a feature request, not a bug. The relatively low priority of the request is explained in detail. A significant contributor is the fact that the formatting engine is extremely complex, poorly tested (due to combinatorial explosion), and was not designed to support this request. Alterations in this space are time consuming and high-risk.

mrnams commented 6 months ago

I surprise no solution even after 7 years

CyrusNajmabadi commented 6 months ago

@mrnams this is on the backlog, as there has been very little interest shown in it. We're not really positive on stylistic changes that will make the ecosystem less consistent based on the request of a small group of people. Our preference in that case is on uniformity.

norrisgc commented 6 months ago

We're not really positive on stylistic changes that will make the ecosystem less consistent based on the request of a small group of people. Our preference in that case is on uniformity.

What is requested is an OPTION. That does not affect any supposed uniformity when that OPTION is not selected. What does affect uniformity, is when someone's carefully indented code is re-arranged for them so it lacks any useful indentation. This means that code which, for uniformity's sake would remain indented, is blasted to smithereens instead. But, hey you know best what users actually need.

sharwell commented 6 months ago

@norrisgc Every added option does one of two things:

  1. Doubles (at minimum) the amount of testing required for coverage of behavior under the new option or
  2. Increases the risk of regression due to changes in behavior not being detected by the test suite

Item (1) has been demonstrated computationally impossible, and (2) has proven to be a significant problem for users who are relying on uncommon option configurations. The feature requested in this issue has a near-zero chance of being implemented directly as an option in Roslyn, but we are keeping it open as important validation of a future feature that allows third-parties to customize the behavior of the formatter through extension points. Under this feature, Roslyn itself would only provide one behavior, but the implementer of an extension could provide and be responsible for a different behavior.

jeroen-posch commented 6 months ago

I would also VERY much like to have an option in the C# editor of Visual Studio controlling the indentation of #if/#endif. I write most of my code in C++ and in the C++ version of the editor I am never bothered by the editor moving the indentation level #if/#endif. But that is actually a different story. I just find that moving the #if/#endif to the start of the line actually reduces readability of the code. Code should be easy to read and a #if at the start of a line where the current indentation level is somewhat large can easily be overlooked. Whereas a #if one indent back relative to the indentation of the code will be impossible to miss. I think cognitive scientists might agree with me here.

Chris-70 commented 5 months ago

It would just be nice to have control of the formatting. I've always found that #if/#endif at the start of a line within other code makes scanning the code much harder. I think in early C it was a compiler requirement, but that was more than 40 years ago and even C compilers don't need it to be the first character of a line anymore.

I'd accept it as an untested option in an advanced setting. It's a bit of a nit, but when the code is reformatted and all the alignment work is undone, it's frustrating. That being said, I'd rather have virtual space as an option if I could only have one.

Chris-70 commented 5 months ago

@norrisgc Every added option does one of two things:

  1. Doubles (at minimum) the amount of testing required for coverage of behavior under the new option or
  2. Increases the risk of regression due to changes in behavior not being detected by the test suite

That's a really outdated approach to testing that I haven't seen in 30 years.

Modern TDD and OO development should have the code isolated and fully testable within a class. Unit testing that is well written and through would mean only one additional test would be needed and the component/regression level testing would not need to be changed, just run as normal. If a small change such as changing the indent when a line starts with # impacts other code, there's an issue with the design.

Somewhere there is code that is triggered when you type a # for the first character that changes the indent. The change should be as simple as NOT changing the indent, thus not executing the code or just exiting the code when triggered and the option is selected. The additional test might be just altering the test for the indent to enable the option and validate the indent doesn't happen.

CyrusNajmabadi commented 5 months ago

If a small change such as changing the indent when a line starts with # impacts other code, there's an issue with the design.

Correct. Now you understand the state we're in and why this is such a challenge.

The change should be as simple

Unfortunately it didn't t with that way.

One reason for this is perf. Our original formatter worked the way you describe. But it was staggeringly slow due to isolation.

The next version was designed to scale, through the use of parallel algorithms that could share data. However, with that sharing, there is the risk of intended interactions of features.

So you see the predicament we're in.

If someone has a design for a formatter that can scale to the size of files we need, can work on small (in error) chunks of the file, and provides completely provably safe isolation of formatting features, we would be amenable to moving to that.

Chris-70 commented 5 months ago

@CyrusNajmabadi I understand the dilemma now, thanks for the insight. It is unfortunate that it was necessary to trade speed for maintainability as the latter will start an avalanche of compromised design decisions that will further reduce the maintainability of the code. I've lived this too many times myself.

Just how large are the files that are being targeted? I've worked with files that are ridiculously large from a software point of view, but they were only thousands of lines long.

CyrusNajmabadi commented 5 months ago

10k-100k files are normal enough here.

CyrusNajmabadi commented 5 months ago

. It is unfortunate that it was necessary to trade speed for maintainability as the latter will start an avalanche of compromised design decisions

Sure. But the alternative is also problematic. Having everything be isolated and clean to test and compose is problematic if things like typing speed are negatively impacted. As mentioned, we started with such a design, and it was very very problematic.

norrisgc commented 5 months ago

We have two opposing positions here. On one side we have user who are fed up with having their formatting screwed up - which can happen simply as a result of pasting something unrelated to that precise piece of code - and not because you actually asked for it. On the other we have developers who say that due to historic complexity of their code it would be vary hard to fix. Something, somewhere, is looking for "#" at the beginning of the non-whitespace on a line and slamming it to the left margin. Has anyone actually looked to see where that is happening, and if it would actually be hard to fix, or is that just a guess?

sharwell commented 5 months ago

@norrisgc Per https://github.com/dotnet/roslyn/issues/12306#issuecomment-412672491 and https://github.com/dotnet/roslyn/issues/12306#issuecomment-1890077858, even if we identified the location where this behavior is applied, it is unlikely that we would create a new option as part of this project. Microsoft tools for the C# language have always forced the indentation of #if to the left edge, so the overwhelming majority of C# code adheres to the "one true style" for these.

However, #2487 is a totally different situation, and we would absolutely take a fix for it. This would address the majority of situations where an unexpected indentation change occurs.

CyrusNajmabadi commented 5 months ago

Has anyone actually looked to see where that is happening

It can literally happen between any two tokens. That's the fun of preprocessor directives. :-)

And, in all those places, we have to share state to know where that pp-directive should go (since it will be affected by the tokens and other constructs around it).

That then means that info must be shared, which means that the tests must handle every possible interaction of pp directives with every possible pair of tokens in every possible construct, just to make sure it's working properly.

As @Chris-70 lamented, it would be nice if these were all isolated pieces, so you could do this without depending on, or impacting any other code. But that's not the nature of formatting (esp pp directives). And as such, the complexity (esp at the testing level) would be enormously high and very risky.

However, as Sam said, we'd be ok with another system coming in and making these changes. If it were done that way, it would be on that extension to validate all these permutations. Then, if anything went wrong with that extension, breaking any customer, out would be on that extension to fix, or on the customer to decide if they wanted to keep the extension or not.

tannergooding commented 5 months ago

I definitely understand the complexities that block this from being added today. That being said, I don't buy the following as a reason to not expose new options:

Microsoft tools for the C# language have always forced the indentation of #if to the left edge, so the overwhelming majority of C# code adheres to the "one true style" for these.

C# is not go and does not require a single format. It has always allowed the end user to pick and choose what works for them and there are literally millions of lines of code out there that have competing ideas on what is good/correct, even where the default tools did it another way.

Even within the Microsoft maintained dotnet/* repos and even within dotnet/roslyn itself, there isn't a single agreed upon style and different parts of the code base use different conventions for some syntax. Mono is notably a codebase that has historically had much different opinions (see https://www.mono-project.com/community/contributing/coding-guidelines/) on how spacing should exist around method bodies (typical is M(...), while Mono does M (...)) and has historically had millions of lines of code in this different format.

The general premise behind this issue is that some people dislike the currently forced indentation level because it reduces readability and maintainability to them. In many ways, the formatting of text can make or break the ability to process and understand the data for some users, especially if they suffer from dyslexia, have trouble mentally aligning the data, or have to shift focus across the screen to locate information/continue reading due to a significant break or inconsistency in the indentation. Thus, allowing users to pick the formatting that works for them is in many ways improving the accessibility of C# to many developers and allowing them to work on their own codebases in a way that allows them to be more productive.


I also notably don't really buy that this issue is "low interest". The issue has 27 upvotes, which is a considerable number when considering the typical amount that issues get. It is currently tied for the 26th most upvoted issue on the repo out of 8400 open issues. It is currently tied for the 71st most upvoted issue of all time on the repo out of the 32.1k total issues we've ever had.

People clearly care about this and have been asking for it for several years. If you factor in the people who don't comment, don't upvote, and who have asked for similar things (or complained about the behavior) historically on StackOverflow, VS Feedback, and other places the numbers go quite a bit higher.

That likely won't change the prioritization or feasibility of the issue compared to other things that have a higher direct impact, but I think it is better to be clear about the demand of the issue that is clearly present.

CyrusNajmabadi commented 5 months ago

It has always allowed the end user to pick and choose what works for them

That's true at the language level, but not at the IDE level. Since literally the first versions of the language services, there have been tons of possible personal-style choices in the formatting space that are simply not something we support, and which the tooling forces into a particular style.

We get there is desire for flexibility here. However, we've outlined the very significant cost that comes from that. In an ideal world, we'd just have an extensible formatting system that people could customize as they want. But even building that is an enormous undertaking, far more expensive than practically every other large-feature already on our plates today. So we cannot commit to such a thing in any sort of near term timeframe.

sharwell commented 5 months ago

Thus, allowing users to pick the formatting that works for them is in many ways improving the accessibility of C# to many developers ...

Then the right place to file an accessibility issue would be the core editor and not the C# language/IDE. Display-level formatting of text for the purpose of improving accessibility, e.g. high contrast modes, is handled independently of the source representation of text in order to ensure users with specific accessibility requirements are not penalized from working on a project based on decisions made by maintainers of that project.

norrisgc commented 5 months ago

Then the right place to file an accessibility issue would be the core editor and not the C# language/IDE. Display-level formatting of text for the purpose of improving accessibility, e.g. high contrast modes, is handled independently of the source representation of text in order to ensure users with specific accessibility requirements are not penalized from working on a project based on decisions made by maintainers of that project.

Are you suggesting that the editor should display the program in a different form to which it is stored and compiled? That makes sense when it comes to syntax colouring (which does not change the source layout,) but, really, for alignment too? If so, why not just shove it all over to the left margin and expect the editor to figure it out every time the file is opened, just like syntax colouration? (How's that going to work when it comes to error messages which show fragments of text with column numbers?)

tannergooding commented 5 months ago

We get there is desire for flexibility here.

Right, my intent above wasn't to try and say this isn't a hard problem nor that it's something that has to be prioritized. Rather, simply to cover that this is indeed an important and highly upvoted issues that many developers in the community care about and not simply some "niche" consideration.

I trust the IDE team is making the right decisions in balancing the overall direction and needs here in the vast space that needs to be supported and the transparency around why this is complicated is appreciated ❤️

Then the right place to file an accessibility issue would be the core editor and not the C# language/IDE

Accessibility is notably not the responsibility of only one tool. Nor would it be possible to handle something like this using only the core editor due to the actual lexical and semantic meaning of code and "trivia" in many scenarios. The official IDE services, even if done via an extensibility point like Cyrus mentioned, is the correct place for almost any/all formatting support for a programming language.

In an ideal world, there might be a general interface between the language service and editor such that relevant accessibility information can be done. For example, understanding that the user would like to see 0 or 1 space between a method name and its parameters or that a user may want to see tab vs spaces for leading indentation or that a user may want the start of comments to be aligned.

Such an interface would be complex/non-trivial to support and there are many considerations for where it can break down. But, it is most definitely not an issue to be pushed out to the core editor