Don't format regions inside inactive conditionals

[Youssef1313]

Fixes #62612

[CyrusNajmabadi]

this likely warrants docs :)

[CyrusNajmabadi]

nit, consider inverting.

[CyrusNajmabadi]

needs more explanation :)

[CyrusNajmabadi]

what about end-of-line trivia?

[Youssef1313]

what about end-of-line trivia?

The following test just passes:

        [Fact]
        public async Task TestRegion2()
        {
            var testCode = @"
class MyClass
{
#if true
    public void M()
    {
[||]#region ABC
        System.Console.WriteLine();
[||]#endregion
    }
#else
    public void M()
    {
#region ABC
        System.Console.WriteLine();
#endregion
    }
#endif
}
";
            var fixedCode = @"
class MyClass
{
#if true
    public void M()
    {
        #region ABC
        System.Console.WriteLine();
        #endregion
    }
#else
    public void M()
    {
#region ABC
        System.Console.WriteLine();
#endregion
    }
#endif
}
";
            await Verify.VerifyCodeFixAsync(testCode, fixedCode);
        }

(I'll definitely commit and push it)

'#region'.GetPreviousTrivia(..) is not an end of line trivia for this case though.

Do you have an interesting test case where the tree will have end of line trivia?

I found a niche case where my logic is incorrect apart from EOL trivia:

#region OUTER
        #region ABC
        System.Console.WriteLine();
        #endregion
#endregion

Here, after we go back consuming whitespace trivia, we don't find a disabled text, but we are in disabled text.

Is there a compiler API for the "Is this position conditionally excluded?" question?

[CyrusNajmabadi]

does IsInInactiveRegion work?

[Youssef1313]

@CyrusNajmabadi It doesn't 😕

roslyn/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/CSharp/Extensions/SyntaxTreeExtensions.cs

Lines 477 to 538 in 75bbc51

	public static bool IsInInactiveRegion(
	this SyntaxTree syntaxTree, int position, CancellationToken cancellationToken)
	{
	Contract.ThrowIfNull(syntaxTree);
	// cases:
	// $ is EOF
	// #if false
	// |
	// #if false
	// |$
	// #if false
	// |
	// #if false
	// |$
	if (syntaxTree.IsPreProcessorKeywordContext(position, cancellationToken))
	{
	return false;
	}
	// The latter two are the hard cases we don't actually have an
	// DisabledTextTrivia yet.
	var trivia = syntaxTree.GetRoot(cancellationToken).FindTrivia(position, findInsideTrivia: false);
	if (trivia.Kind() == SyntaxKind.DisabledTextTrivia)
	{
	return true;
	}
	var token = syntaxTree.FindTokenOrEndToken(position, cancellationToken);
	if (token.Kind() == SyntaxKind.EndOfFileToken)
	{
	var triviaList = token.LeadingTrivia;
	foreach (var triviaTok in triviaList.Reverse())
	{
	if (triviaTok.Span.Contains(position))
	{
	return false;
	}
	if (triviaTok.Span.End < position)
	{
	if (!triviaTok.HasStructure)
	{
	return false;
	}
	var structure = triviaTok.GetStructure();
	if (structure is BranchingDirectiveTriviaSyntax branch)
	{
	return !branch.IsActive || !branch.BranchTaken;
	}
	}
	}
	}
	return false;
	}

That seems a hard-to-answer question on the IDE side.

[Youssef1313]

That seems a hard-to-answer question on the IDE side.

Not really. I think I see how I can do it.

[Youssef1313]

@CyrusNajmabadi This is ready for another look now :)

[CyrusNajmabadi]

Currently failing test: Microsoft.CodeAnalysis.CSharp.UnitTests.Formatting.FormattingEngineTriviaTests.MixAll

[Youssef1313]

Currently failing test: Microsoft.CodeAnalysis.CSharp.UnitTests.Formatting.FormattingEngineTriviaTests.MixAll

Fixed. Not an ideal fix though, just falling back to the original behavior for edge cases that are harder to handle.

[Youssef1313]

@CyrusNajmabadi Can you take a look please? Thanks!

[CyrusNajmabadi]

tentative approval. but would like answer to question posed.