Improve NormalizeWhitespace for object initializers

[DoctorKrolic]

Fixes: #61204

Normilize whitespace for object/collection initializers by putting every new statement in a separate line:

new SomeClass
{
    A = 1,
    B = 2,
    C = new OtherClass()
    {
        D = "hello there"
    }
}

new int[]
{
    1,
    2,
    3,
}

new Dictionary<int, int>
{
    [0] = 1,
    [1] = 2,
    [2] = 3
}

When initializer is inside string interpolation (doesn't matter verbatim or not), the behaviour changes to nicely format initializer in a single line, e.g.:

$"{new SomeClass { A = 1, B = 2, C = new OtherClass() { D = "hello there" } }}"
$"{new int[] { 1, 2, 3 }}"
$"{new Dictionary<int, int> { [0] = 1, [1] = 2, [2] = 3 }}"

[CyrusNajmabadi]

When initializer is inside string interpolation (doesn't matter verbatim or not), the behaviour changes to nicely format initializer in a single line, e.g.:

note: i added support for multiline expressions in string interpolations to the language.

[DoctorKrolic]

note: i added support for multiline expressions in string interpolations to the language.

Yes, but of 2 available variants I think the single-line looks better. Let me show:

How it was before this PR:

$"{new SomeClass
{A = 1, B = 2}}"

How it could be if I didn't explicitly chenged it:

$"{new SomeClass
{
    A = 1,
    B = 2
}}"

How it is currently done:

$"{new SomeClass { A = 1, B = 2 }}"

Considering that it is very unlikely that an interpolated string would contain large object initializer, I think, the single-line approach looks better. Even if it can be not very obvious from the case I've shown above, just add some content and it will become a mess:

$"This is the first part of an interpolation string {new SomeClass
{
    A = 1,
    B = 2
}}. This is the second part if an interpolation string. And as this constant part grows the start of object declaration is more and more separated from its initializer: {new SomeOtherClass
{
    C = "whos this initializer is?)"
    D = 3.14m
}}"

The NormalizeWhitespace isn't a formatting engine after all, it just provides minimal functionality for code to be readable

[CyrusNajmabadi]

Considering that it is very unlikely that an interpolated string would contain large object initializer, I think, the single-line approach looks better.

Please doc the code to include this explanation. Thanks!

[CyrusNajmabadi]

Looked through, but i didn't see a test with nested initializers. e.g. new X { A = { ... } }. Did i miss it?

[DoctorKrolic]

In the latest commit there is a test with many nested initializers. Isn't it what you want to see?

[CyrusNajmabadi]

could you point me to it?

[DoctorKrolic]

https://github.com/DoctorKrolic/roslyn/blob/cfc1df001b9cfb7b21b78d111916265fd1857e53/src/Compilers/CSharp/Test/Syntax/Syntax/SyntaxNormalizerTests.cs#L2560-L2611

There are a bunch of nested different initializers and anonymous object declarations

[CyrusNajmabadi]

There are a bunch of nested different initializers and anonymous object declarations

I don't see the code construct I was calling out in my request though. I don't see simple nested initializers.

[DoctorKrolic]

Can I have a review here?

[jcouv]

LGTM Thanks (iteration 15)

[jcouv]

@dotnet/roslyn-compiler for second review. Thanks

[DoctorKrolic]

Still want a second review) And someone also restart failed CI pls

[RikkiGibson]

@dotnet/roslyn-ide for sign-off on workspaces tests change

[RikkiGibson]

LGTM just had some minor comments to address first.

[RikkiGibson]

In general, please try to keep style-only changes like these to a minimum in the compiler layer. No need to undo any such changes already in the PR.

[DoctorKrolic]

Originally I only removed accessorList as it was never used, but Cyrus then suggested to use pattern matching and I accepted it. Ok, will try to avoid such changes later.

Also I have an idea to convert all multiline strings is SyntaxNormalizerTests to raw form in order to make them actually readable in a separate PR after this one is merged. Would such change be considered as style-only and rejected?

[RikkiGibson]

Also I have an idea to convert all multiline strings is SyntaxNormalizerTests to raw form in order to make them actually readable in a separate PR after this one is merged

I personally would be fine with this. @jcouv as long as you're also fine with this, I'd say @DoctorKrolic can go ahead and open a subsequent PR to convert the @"" strings in that file to """ """.

[CyrusNajmabadi]

i would be fine with this. they're tests, so there's no risk. and test-strings were a primary use case for raw-strings :)

[RikkiGibson]

Just curious, where did all these baselines come from? Were they taken from somewhere or was there some method used which lead to producing all of them?

[DoctorKrolic]

I produced them manually myself. There is some logic behind almost every one. Here the pattern is the following:

1. Empty {}
2. Add simple properties
3. Add , after the last simple property (I actually caught several bugs when decided to add these tests)
4. Repeat this patter for new{...} -> new SomeClass{...} -> new SomeClass(){...}
5. Add nested complex property with its own initializer/anonymous object declaration
6. Repeat the whole pattern 1-4 for this nested property
7. Add another double-nested property and repeat 1-4 for it

Similar with with initializers tests

Collection/array initializers are not that much structured. But there are some interesting cases there as well, e.g. new[] { 2 + 2 * 2 }. Here the first parent of first 2is not an initializer, but a binary expression, while in a simpler case like new [] { 2 } the parent is an initializer, so I had to adopt code for that.

Probably the only test that is not actually structured is the last one in TestNormalizeNestedInitializers, where I just produced some weird pretty unrealistic scenario with a bunch on different nested initializers/anonymous object creations and I managed to find some bugs using it as well)

[jcouv]

Still LGTM Thanks (iteration 20)

[DoctorKrolic]

@RikkiGibson I resolved merge conflicts, just need an approval from you

[RikkiGibson]

Thanks @DoctorKrolic for the contribution!