Fix line breaks between members when a property has an initializer

[DoctorKrolic]

Fixes: https://github.com/dotnet/roslyn/issues/42336

The issue is quite old, so the actual problem changed a bit. Before this PR the output for property with initializer was

class A
{
  public string Prop { get; } = "xyz";
  public void Foo()
  {
  }
}

This is not expected, we want to have an empty line between members.

In this PR I added this missing line break, so the behaviour is the same as in the case without property initializer. Added test cases for both. Put them in the same place, where all other declaration tests currently are

[Youssef1313]

Fixes: https://github.com/dotnet/roslyn/issues/42336

Per your description and issue description, the behavior described in the issue isn't the current behavior. Can you confirm?

So basically the complain in #42336 is already fixed, but this PR wants to handle another scenario.

[DoctorKrolic]

the behavior described in the issue isn't the current behavior. Can you confirm?

Yes

So basically the complain in https://github.com/dotnet/roslyn/issues/42336 is already fixed, but this PR wants to handle another scenario.

I mean, I took code example from the issue, and it is still problematic, but in a bit other way. Nevertheless, even if to treat this case as a separate one, the issue should be closed as completed anyway)

[Youssef1313]

I mean, I took code example from the issue, and it is still problematic,

I just wanted to confirm I understand the situation correctly.

[DoctorKrolic]

Can I get a review here please?

[CyrusNajmabadi]

This is not expected, we want to have an empty line between members.

it's unclear to me that we want this. this feels like this coudl change a lot of code that currently normalizes.

[DoctorKrolic]

@CyrusNajmabadi Added a bunch of tests to better represent the change. Prior to this PR all test cases with property initializers would be just crumpled together, but now they behave the same as those without initializers and emit a nice additional line break between themselves and the next member

[DoctorKrolic]

@jcouv I would also suggest to split declaration test into multiple once (1 test for usings, 1 test for methods, 1 test for properties etc.). Current single test for all declarations is getting really enormous and hard to navigate in. But this smells for me like a "style-only" change, so asking you the permission to do it (probably in a separate PR after this one is done)

[jcouv]

Current single test for all declarations is getting really enormous and hard to navigate in

Agreed. Adding method/test boundaries won't cause that much churn, so should be fine especially if some grouping remains (property initializers, indexers, ...).

[DoctorKrolic]

Can I get a review and failed CI restart here pls?

[333fred]

@CyrusNajmabadi did you still have concerns about the baseline changes here?

[DoctorKrolic]

@jcouv @CyrusNajmabadi @333fred PTAL. Added small tweak to group properties and added tests to verify both properties and fields

[DoctorKrolic]

@Youssef1313 Well, this is what the CI is for)

[Youssef1313]

@Youssef1313 Well, this is what the CI is for)

I'm referring to external source generators that are in different codebases.

Generally, any change to syntax normalizer can affect them, but thinking this one is quite common.

[DoctorKrolic]

any change to syntax normalizer can affect them

Well, the syntax normalizer output isn't a public API, so it may change between even minor versions, right? It isn't a big deal to add and remove several line breaks from your test in the end

[333fred]

We generally don't treat syntax normalizer's output as a public API. This may be a common change, but a generator author isn't going to observe it until they update their roslyn dependency version in their own tests, and it shouldn't have meaningful impact at generator runtime.

[333fred]

What impact does this PR have on non-auto properties? Will they be grouped in the same way? Consider adding some tests with expression-bodied and full-bodied properties.

[DoctorKrolic]

Is this PR forgotten? @jcouv Can you please take a look, since you've already signed this off before. Also probably want a look from @CyrusNajmabadi to verify that all behaviour expectations match

[jcouv]

@DoctorKrolic Indeed, that had fallen off my radar (we have many open PRs at the moment). Thanks for the ping. Looks good, thanks!

[jcouv]

/azp run

[azure-pipelines[bot]]

Azure Pipelines successfully started running 2 pipeline(s).

[jcouv]

@DoctorKrolic We've had some CI issues. Could you merge latest bits from the main branch into this PR? (I do git fetch origin main then git merge origin/main then push the commit here)

[DoctorKrolic]

I preffer to use GitHub UI since I am already in the browser and it doesn't disable auto-merge

[jcouv]

Thanks for the contribution @DoctorKrolic!