Roslyn: Format document should not add braces by default

[davkean]

Formatting a document by default now has started adding braces around single line blocks:

``` C#
using System;

namespace ConsoleApp75
{
class Program
{
static void Main(string[] args)
{
if (args.Length == 0)
throw new Exception();
}
}
}

To:

``` C#
using System;

namespace ConsoleApp75
{
    internal class Program
    {
        private static void Main(string[] args)
        {
            if (args.Length == 0)
            {
                throw new Exception();
            }
        }
    }
}

However there is no way to enforce whether or not braces are optional for single line statements in editorconfig. This is going to lead to unneeded changes to my tree without a way to enforce it. This option should be turned off by default unless there is a way to enforce it.

[sharwell]

I believe the setting is controlled by csharp_prefer_braces.

[davkean]

Yes that is correct, this works like a charm exactly as I expected. The only question is, should this be the default?

[davkean]

I'll change the description.

[sharwell]

Marked as Need Design Review, but it will almost certainly change to By Design. The first time this feature is used, it prompts users to configure the items that get formatted, of which braces is one. The setting can be disabled for a team via .editorconfig or a user via Tools → Options (it only works if both are enabled). This aligns with our design by intent (as opposed to by omission), and thus is unlikely to change unless users are unable to locate the settings which control the feature.

[sharwell]

Removed [Regression] from the title as this was an intentional design change intended to impact the behavior in question.

[davkean]

@sharwell I was not prompted. When does the prompt show?

[sharwell]

The prompt should show as a gold bar the first time you use Format Document without previously visiting the C# Formatting page in Tools → Options.

[davkean]

I never got prompted, maybe I visited the formatting page looking for other settings? I was surprised by the defaults.

[sharwell]

This went to a design review today. We ended up deciding to not take action on the original design, and instead focus on ensuring the current design is presented in the manner intended. This specially means making sure features like the gold bar work where intended (https://github.com/dotnet/roslyn/issues/27821#issuecomment-397780098) and the options related to braces get applied in the expected manner following a user change.

In the event we revisit this in the future, we will focus on ensuring the default severity of the "use braces" diagnostic matches user expectations. Given the reliability/predictability of the code fix for this diagnostic, we believe it is a reasonable one to include by default in the Code Cleanup feature (candidates for this were identified primarily by their low probability of producing incorrect changes).

:memo: We believe some of the early negative impressions of this feature were influenced by prominent bugs affecting adoption which have since been resolved but have not yet shipped. We are especially interested in ongoing customer discussions about initial and ongoing impressions that didn't suffer from these initial drawbacks. In other words, 15.8 will present a better overall adoption story than we have in the past, and that story may (hopefully) positively impact the overall acceptance of the current design.

[davkean]

@sharwell Please point me to the bugs around the gold bar.

[sharwell]

@sharwell Please point me to the bugs around the gold bar.

@JieCarolHu @heejaechang @kuhlenh Can you check internal telemetry to verify users who run Format Document in the latest builds are seeing this bar?

[davkean]

Can we please not close this bug until we've got bugs on the gold bar? In my case this was a surprising change of behavior and want to make sure we've captured this in a bug.

[sharwell]

@davkean Filed #27974

[JieCarolHu]

The gold bar only shows when user click Ctrl+K,D, and the code cleanup is not configured yet

And code cleanup is considered Configured if user goes to the Formatting setting page and click "Ok" to exit.

One of the scenario that user may never see the gold bar is:
User open the formatting setting page via tools->options without Ctrl+K,D, and click OK.

I didnot explicitly write any code telemetry for code cleanup, so unless we have some "built-in" tememetry with the gold bar, we probably don't the telemetry...

[CyrusNajmabadi]

And code cleanup is considered Configured if user goes to the Formatting setting page and click "Ok" to exit.

This feels wrong to me. There is not explicit consent being given when the user just navigates somewhere, then hits 'ok' to close a page. What would probably make sense would be to change this list:

Into something more like:

This "perform additional cleanup during formatting" would be unchecked by default. It would only be checked if the user explicitly clicked it, or if they accepted through the gold-bar.

Thoughts @kuhlenh @sharwell @jinujoseph @JieCarolHu ?

[CyrusNajmabadi]

@davkean As well. I'm curious if you would have preferred the sort of approach i outline above ^

[kuhlenh]

Yes, I agree with you Cyrus. Carol, can we still make this change for 15.8?

---

From: CyrusNajmabadi [e-mail]
Sent: Wednesday, June 20, 2018 1:07:33 PM
To: dotnet/roslyn
Cc: Kasey Uhlenhuth; Mention
Subject: Re: [dotnet/roslyn] Format document should not add braces by default (#27821)

@davkeanhttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fdavkean&data=02%7C01%7Ckaseyu%40microsoft.com%7C457b04e8a804477e462008d5d6e975d8%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636651220550440495&sdata=1SKvyvaxKjq0EW5wVBUKNBz6bUESWJmJE0XfgAiAdlE%3D&reserved=0 As well. I'm curious if you would have preferred the sort of approach i outline above ^

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHubhttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fdotnet%2Froslyn%2Fissues%2F27821%23issuecomment-398879664&data=02%7C01%7Ckaseyu%40microsoft.com%7C457b04e8a804477e462008d5d6e975d8%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636651220550450503&sdata=SgiPGw9AYvgeKueM267UywMlqm%2B5guOnAnt2Wya6vH4%3D&reserved=0, or mute the threadhttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FABVnRsdq6IFBVAFpd8yeGloBUQO8Ak36ks5t-quFgaJpZM4UnWqr&data=02%7C01%7Ckaseyu%40microsoft.com%7C457b04e8a804477e462008d5d6e975d8%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636651220550450503&sdata=KHZeaMJ%2FwTBNnyt6qwhvUDUuOWvFqmlbuEEr%2BkQfYGc%3D&reserved=0.

[heejaechang]

didn't this opinion show up in design meeting and we said we do this way so that it is more on by default?

I am +1 on off by default for all new features. so I like it being off and require very explicit users opt-in (not just this feature but all new features), but I thought our general team policy is we like things to be on by default. and we have gold bar for now since it is experimental? but otherwise, on by default without the goldbar?

[Neme12]

I'd personally want this not only off by default for formatting, but as a separate command from formatting. There are many reasons why I'd like to format the document but not cleanup (yet), for example:

1. "make field readonly" I do like to keep my fields readonly, but maybe I just haven't written the code that mutates them yet. I'll do cleanup later (when more of the code is written), but for now, I may want to format whitespace because it is completely broken after some edit.
2. "remove unused variable" same logic - maybe I just haven't written the line that uses this variable yet
3. "remove usings" - again, I could imagine a scenario where I know I will need to use System.Linq, but haven't gotten to that point yet
4. "expression/block body preferences" - same idea, I have written one statement inside a method and I'm about to write another one, but I'd like to do formatting before that.

All of this makes me worry about whether coupling format and cleanup into the same command is a good idea. (I haven't used this feature yet so I don't know if/how often this will be a problem in practice, but it concerns me that if I want to use code cleanup, it needs to always happen together with formatting).

[CyrusNajmabadi]

I think 'on by default' and 'off by default' aren't really positions that can be taken without enough context. it really needs to be considered on a case-by-case basis.

When you have something very large, potential invasive, and which takes over an existing key binding people already use (like code-cleanup), then we may have to have a different policy versus a very minor feature that doesn't really impact hte user experience.

I would say that it's more of a sliding scale. With the more potentially disruptive and contentious features leaning more and more to 'opt in', and the simpler and less invasive stuff being opt-out.

--

in the case of code-cleaners, it seemed like we wanted to make it opt-in (hence the goldbar), however, we somewhat over-zealously also coded up another form of 'opt-in' that doesn't really match user expectations. i.e. just opening a dialog, not interacting with it, and then hitting "ok" is not an 'opt in' from the mind of most users. In this scenario (esp. as there was no change), 'ok' is on the same level of 'close' or 'cancel', and shoudl thus not be inferred to be opting in.

--

In the proposal i listed above, we far more clearly can understand the user is opting-in. This is because they have to explicitly check the "Perform additional code cleanup..." checkbox. So we can actually understand that this is something they intentionally enabled.

[CyrusNajmabadi]

I'd personally want this not only off by default for formatting, but as a separate command from formatting. There are many reasons why I'd like to format the document but not cleanup (yet), for example:

I agree with @Neme12 . I think it's totally fine to tie these two things together. However, i also think it would make sense to have a dedicated keystroke for this purpose that someone could use instead.

So, the changes i would have would be:

1. introduce a specific command for cleanup-document.
2. expose that command in Keybindings.
3. expose that command on edit|advanced next to format document.
4. have a way to configure what that command does in tools options.
5. have an option under the formatting page to say: run code cleanup when i format document.
6. still have the gold-bar

This seems to address nearly all the use cases i can consider. People who want to keep the commands separate can do so. People who want a single command can have it (albeit opt-in). People can control what runs when the command executes. And it's easy to discover the commands through the gold-bar and the edit menu.

[heejaechang]

this is just first step for us to have underlying subsystem and get feedback and incremental design going.

what we probably at the end wants is, not having separate code cleanup feature but giving existing diagnostics, refactoring system more trigger than LB.

right now, only way to apply code fix is LB.

we want to expand it so that users can define multiple trigger such as format document, edited text span (think like VB line commit), save files, commit, build and etc.

so when people configure analyzer/fixer and its severity, options, they can also configure when fix can be run. right now, it is manual through LB. but user can say analyzer/fixer/fix A should run when I do format document, analyzer/fixer/fix B should run when I commit and etc. and that should include all analyzer/fixer included not just one we picked from builtin analyzer/fixer/fixes.

and also provide a new batch fix feature which users can select all analyzer/fixer/fixes they want to apply to a scope (projects, solution and etc) with custom options and bulk apply those changes.

we might also provide code fix from error list.

anyway, we welcome any feedbacks!

[CyrusNajmabadi]

anyway, we welcome any feedbacks!

Definitely!

I think we're def at a decent starting position, and i'm glad that we've been able to ship something for people to be able to use and respond to. I also think there's a wealth of improvements we can make here (most of which you touched on yourself in your post). So i def hope we'll continue to improve this area greatly in the coming months!

[Neme12]

we want to expand it so that users can define multiple trigger such as format document, edited text span (think like VB line commit), save files, commit, build and etc.

That sounds interesting and powerful but will there ever be a way to run code cleanup without having to bind it to any of these other commands? Thanks

but user can say analyzer/fixer/fix A should run when I do format document, analyzer/fixer/fix B should run when I commit and etc.

Can you help me understand what's the use case for this? What would be an example of A and B such that someone would like to run A on format but B on save. Is that much flexibility actually useful or would it be overwhelming to the user? Do we expect most users to tinker with these individually? (Maybe there is a compelling use case for that, that's why I'm asking for an example so that I can see this better and rethink my opinion. Thanks)

What I had in mind is exactly what @CyrusNajmabadi said - Code cleanup would be a separate command and there would be a page in settings to configure what that entails - which features get run during code cleanup. Then there would be on/off options to run code cleanup 1) during formatting 2) during save 3) during build? etc. But this would of course remove the ability to run a different set of cleanup features with different triggers.

[heejaechang]

we would like to think it as a code transformation which are provided by diagnostic/fixer or refactoring. and has options around that.

we do not want to have 2 features that basically do same thing. then we get into confusion where options for 1 feature (diagnostic analyzer) applying to another feature (code cleanup).

for example, if add brace diagnostic analyzer is disabled by ruleset editor, should it run on code clean up feature?

that is why we want to consolidate all those in 1 place where users set all options including when code tranformation runs. and not think these as separate features.

…

and sure we can add new command if user want rather than merging it to format document.

…

the reason we have multiple triggers is that, if this is generic mechanism to run multiple code transformation including third party analyzer, it might take long time to do everything.

so, you might want to do only faster and quick stuff while format document but when you commit or save, you are willing to spend some more time to clean more things up.

at least, that was one of feedback from diagnostic/fixer side. such as I have bunch of errors/suggestion from analyzers, I want all those to be automaticallly fixed up when I commit/save.

that is why we have save/commit as one of trigger.

[heejaechang]

tagging @kuhlenh @jinujoseph since all these are design question.

[sharwell]

The check box requested in https://github.com/dotnet/roslyn/issues/27821#issuecomment-398878523 already exists in the UI. It just has visibility set to collapsed (not visible).

Edit: Nope, it was removed in c26eef10833e7c8474b46764d27a28a4188ea854.

[Neme12]

it might take long time to do everything

This is also another reason why I think I'd prefer a separate command for cleanup and not bind this to format, save or build.

[davkean]

it might take long time to do everything

Not might, it does take a long time: https://github.com/dotnet/roslyn/issues/28012.

[CyrusNajmabadi]

it might take long time to do everything

This is also another reason why I think I'd prefer a separate command for cleanup and not bind this to format, save or build.

Note: i've collected data on this, and it's not pretty: https://github.com/dotnet/roslyn/pull/28029#issuecomment-398925871

TLDR: currently (without a PR i'm working on), it can take >2.5 minutes to just format-document on a file in Roslyn. Even with the PR, we're still at around 100 seconds to format. This is super brutal and is likely never going to be acceptable in perf-sensitive scenarios like 'save'.

Now, we can def try to make all the cleaners faster (and i'm trying to figure out how i can capture that information myself to help speed these up). But it's likely that we're always going to have some amount of fixed cost here that makes these times very unpalatable for some users.

[kuhlenh]

We have a "north star" vision that includes something like an additional "Apply EditorConfig" command and a more flexible code cleanup (not to mention a command-line 'batch-fix'). For 15.8, we are putting together this MVP to get feedback and verify developer scenarios.

I do agree with Cyrus that we should probably have that top-level check box that is "Perform additional code cleanup during formatting". To my knowledge, (@JieCarolHu can confirm), all "additional" cleanup options are off-by-default unless you hit "Configure" from the gold bar. Then we will turn on that set of default options.

Thank you for all this feedback--would love to hear about your experience with 15.8 p3's prototype so we can gather all the right scenarios to perfect our north star design. Feel free to email me your feedback from this experience at [email protected]

[davkean]

all "additional" cleanup options are off-by-default unless you hit "Configure" from the gold bar. T

That is not my experience, hence the above bug.

[heejaechang]

@davkean we enabled the only quick one by default and left expansive one there but off by default. if you enabled them all, then sure it will take some time to run. especially if there are a lot of changes to make.

[davkean]

Which comment are you referring to? The speed? Or the defaults?

[heejaechang]

options in format document

only cheap one is on by default. and all expansive ones are off by default. if they got all on by default, that is some other bug.

[heejaechang]

by the way, these are roaming options. meaning, if you set those in one VS, all your other VS including different hives will all get that.

[davkean]

@heejaechang Please read the title of the bug, this bug is about that add/remove braces is on by default and results in a behavior change.

[heejaechang]

@davkean sorry. I am mixing this https://github.com/dotnet/roslyn/issues/28012 with this.

I think for this particular one, we decided to turn off "add brace" by default, @kuhlenh

[davkean]

I'm getting conflicting information; this bug as closed as "by design" by @sharwell, @kuhlenh mentioned that they are off by default and only if you choose Config do we turn them on (which is not happens per https://github.com/dotnet/roslyn/issues/27974). It sounds like the IDE team needs to get agreement amongst themselves before I can provide anymore input.

[heejaechang]

alright, I might misunderstood. leaving it to @kuhlenh whether "add brace" is on by default or not.

I think there are 3 different things here.

1. whether code clean up gets enabled without user explicit opt-in through gold bar + format option page. (it should off until explicitly opted in, otherwise, it is a bug)
2. whether "add brace" feature should be on by default in code clean or not. (@kuhlenh )
3. perf of code clean up. (right now, we only turned on ones that we know fast by default but still put expansive one there in case users want those)

[kuhlenh]

This bug is closed as "by design" per what @sharwell indicated we decided in a design meeting above.

@davkean I'm going to address the default config issue you have in #27974. As @heejaechang mentioned, the config is off unless you opt-in. I will discuss with you on this other issue.

[heejaechang]

actually, there are more things like

whether we want an explicit command for code clean or not. or we want separate code clean up features altogether or not. but all those are something part of longer design decisions we need to make.

[davkean]

I think the disconnect is the act "opting-in" which we can discuss under: https://github.com/dotnet/roslyn/issues/27974.