Fix the dotted line for multiline condition in else if

[Youssef1313]

Fixes #45682
Replaces #48533

Screenshot after the fix:

---

I don't like the way I fixed this tbh.

From reading the existing code, the bug should exist even for single-line conditions, but that's not the case.
Actually, the following existing test shows that:

roslyn/src/EditorFeatures/CSharpTest/Structure/BlockSyntaxStructureTests.cs

Lines 287 to 302 in cf55f3a

	[Fact, Trait(Traits.Feature, Traits.Features.Outlining)]
	public async Task TestIfElse1()
	{
	const string code = @"
	class C
	{
	void M()
	{
	if (true)
	{
	}
	else {|hint:if (false){|textspan:
	{$$
	}|}|}
	}
	}";

the start of the hint is at the "else" part.
There should be some modifications on the hint span somewhere, and that unknown place should be where the fix is included.

[CyrusNajmabadi]

an we have test for:

if (foo)
{
}
else
    if (bar)
    {
    }

?

[Youssef1313]

I don't know to unit test this.

I can run with F5 and see what's the behavior and add screenshot though. But surely we would need to find a way to unit test.

For the case of:

if (foo)
{
}
else
    if (bar)
    {
    }

I haven't yet tested what is the behavior, but I think the dotted line would be in the "if", is that the expected behavior?

[CyrusNajmabadi]

but I think the dotted line would be in the "if", is that the expected behavior?

Yes.

[CyrusNajmabadi]

needs test (esp. showing the difference for single-line conditionals vs multi-line conditionals.

[Youssef1313]

needs test (esp. showing the difference for single-line conditionals vs multi-line conditionals.

Need some guidance on how to test this.

[CyrusNajmabadi]

Need some guidance on how to test this.

We should have some existing block structure tests that validate the ranges for these.

[Youssef1313]

Need some guidance on how to test this.

We should have some existing block structure tests that validate the ranges for these.

The tests I found are referenced in the PR description, but the existing expected result in it implies that the bug should exist for single-line else if.
So, that's why I mentioned 2 things in the PR description:

1. The spans created in GetHintSpan aren't directly used to determine the placement of the dotted line.
2. Wherever the spans get modifier (or new ones are created), this should be a better place for this fix.

I can simply update BlockSyntaxStructureTests, but based on the above, this would be kind of liar unit tests.

[Youssef1313]

To be more clear:

roslyn/src/EditorFeatures/CSharpTest/Structure/BlockSyntaxStructureTests.cs

Lines 287 to 302 in cf55f3a

	[Fact, Trait(Traits.Feature, Traits.Features.Outlining)]
	public async Task TestIfElse1()
	{
	const string code = @"
	class C
	{
	void M()
	{
	if (true)
	{
	}
	else {|hint:if (false){|textspan:
	{$$
	}|}|}
	}
	}";

The previous test shows the hint at the start of the "if" not the "else". How the indent guide shows in the "else"? Where the results of GetHintSpan are altered?

[CyrusNajmabadi]

I can simply update BlockSyntaxStructureTests, but based on the above, this would be kind of liar unit tests.

We are responsible for the data on our side. VS is responsible for the presentation. if things are fixed onour end, that's sufficient. if it regresses again, we can move to VS side. Can you please add a test here?

[Youssef1313]

@CyrusNajmabadi Thanks for clarifying. I added tests, but I'd still recommend that you give a quick look to the internal implementation in VS side.
The presentation side in VS seems to be already doing manipulation on the data given from Roslyn. So either:

1. the manipulation is now unnecessary as Roslyn reports now the correct spans; or
2. the manipulation part could be fixed and leave the data side in roslyn as-is

[Youssef1313]

Ping @CyrusNajmabadi