Roslyn: How to determine if an expression has operators with equal/lower precedence than a certain operator?

Question

I'm writing a Roslyn analyzer to replace usages of boolean literals inside a ternary operator with the && and || operators. Here is what the code fix provider should do:

expr1 ? true : expr2 -> expr1 || expr2
expr1 ? false : expr2 -> !expr1 && expr2
expr1 ? expr2 : true -> !expr1 || expr2
expr1 ? expr2 : false -> expr1 && expr2

Here is the relevant portion of my code:

// In MA0002Analyzer.cs
internal static bool? ValueOfBoolLiteral(ExpressionSyntax expr)
{
    switch (expr.Kind())
    {
        case SyntaxKind.TrueLiteralExpression:
            return true;
        case SyntaxKind.FalseLiteralExpression:
            return false;
    }

    if (expr is ParenthesizedExpressionSyntax parenExpr)
    {
        return ValueOfBoolLiteral(parenExpr.Expression);
    }

    return null;
}

// In MA0002CodeFixProvider.cs
using static Microsoft.CodeAnalysis.CSharp.SyntaxFactory;

private static Task<Document> UseLogicalOperatorAsync(Document document, ConditionalExpressionSyntax ternary, CancellationToken ct)
{
    // expr1 ? true : expr2 -> expr1 || expr2
    // expr1 ? false : expr2 -> !expr1 && expr2
    // expr1 ? expr2 : true -> !expr1 || expr2
    // expr1 ? expr2 : false -> expr1 && expr2

    async Task<Document> UseBinaryOperatorAsync(SyntaxKind operatorKind, ExpressionSyntax left, ExpressionSyntax right)
    {
        var logicalExpr = BinaryExpression(operatorKind, left, right);
        var syntaxRoot = await document.GetSyntaxRootAsync(ct);
        return document.WithSyntaxRoot(syntaxRoot.ReplaceNode(ternary, logicalExpr));
    }

    bool? literalValue = MA0002Analyzer.ValueOfBoolLiteral(ternary.WhenTrue);
    if (literalValue != null)
    {
        // ? has lower precedence than ||, so it's possible we could run into a situation where stuff in expr1 binds more
        // tightly to stuff in expr2 than before. For example, b1 ? true : b2 && b3 equals b1 || (b2 && b3), but not b1 || b2 && b3.
        // We want to prevent this by wrapping expr2 in parentheses when necessary, but only when expr2 contains operators that
        // have equal/less precendence than ||.
        ExpressionSyntax right = MaybeParenthesize(ternary.WhenFalse);

        return literalValue == true ?
            // It's never necessary to parenthesize expr1 here because boolean operators are left-associative.
            UseBinaryOperatorAsync(SyntaxKind.LogicalOrExpression, ternary.Condition, right) :
            // However, it might be necessary to parenthesize expr1 here because it is being negated.
            UseBinaryOperatorAsync(SyntaxKind.LogicalAndExpression, Negate(ternary.Condition), right);
    }

    literalValue = MA0002Analyzer.ValueOfBoolLiteral(ternary.WhenFalse);
    if (literalValue != null)
    {
        // In "b1 ? b2 : true;", calling ToFullString() on b2's node will give "b2 ". This is bc the space to the right of b2 is counted as part
        // of its trivia. Therefore, we must remove trailing trivia before moving b2 to the end of a new expression, or we'll get "!b1 || b2 ;".
        // This is not an issue for the node on the false branch of the conditional.
        ExpressionSyntax right = MaybeParenthesize(ternary.WhenTrue.WithoutTrailingTrivia());

        return literalValue == true ?
            UseBinaryOperatorAsync(SyntaxKind.LogicalOrExpression, Negate(ternary.Condition), right) :
            UseBinaryOperatorAsync(SyntaxKind.LogicalAndExpression, ternary.Condition, right);
    }

    return Task.FromResult(document);
}

private static ExpressionSyntax MaybeParenthesize(ExpressionSyntax expr)
{
    // What goes here?
}

private static ExpressionSyntax Negate(ExpressionSyntax expr)
{
    if (expr.IsKind(SyntaxKind.LogicalNotExpression))
    {
        var pue = (PrefixUnaryExpressionSyntax)expr;
        return pue.Operand;
    }

    if (expr is ParenthesizedExpressionSyntax parenExpr)
    {
        return parenExpr.WithExpression(Negate(parenExpr.Expression));
    }

    return PrefixUnaryExpression(SyntaxKind.LogicalNotExpression, expr);
}

As you can see, I'm stuck on how to implement MaybeParenthesize. The problem is that since the ternary operator has lower precedence than ||, changing from expr1 ? true : expr2 to expr1 || expr2 will not always yield an equivalent expression. For example, b1 ? true : b2 && b3 equals b1 || (b2 && b3) but not b1 || b2 && b3. Specifically, this happens whenever expr2 contains any operators that are equal/lower precedence than ||.

How can I detect if expr2 contains such operators, so I know when it's necessary to parenthesize it and when it's not? Is there a special API/elegant way to achieve this in Roslyn?

Answer 1

If the problem you are trying to solve is deciding whether a generated expression in a code fix provider should have parentheses or not, then the standard solution is to let Roslyn decide it for you.

What that means is that you always generate the parentheses, but ask Roslyn to remove them again if it is safe to do so.

I have done this in the past using this extension method:

public static ExpressionSyntax AddParentheses(this ExpressionSyntax expression)
{
    switch (expression.RawKind)
    {
        case (int)SyntaxKind.ParenthesizedExpression:
        case (int)SyntaxKind.IdentifierName:
        case (int)SyntaxKind.QualifiedName:
        case (int)SyntaxKind.SimpleMemberAccessExpression:
        case (int)SyntaxKind.InterpolatedStringExpression:
        case (int)SyntaxKind.NumericLiteralExpression:
        case (int)SyntaxKind.StringLiteralExpression:
        case (int)SyntaxKind.CharacterLiteralExpression:
        case (int)SyntaxKind.TrueLiteralExpression:
        case (int)SyntaxKind.FalseLiteralExpression:
        case (int)SyntaxKind.NullLiteralExpression:
            return expression;

        default:
            return SyntaxFactory
                .ParenthesizedExpression(expression)
                .WithAdditionalAnnotations(Simplifier.Annotation);
    }
}

The switchand the first set of cases are not required. They have some cost, but they can eliminate some unnecessary allocations in some scenarios.

The real magic is in the default clause. The .WithAdditionalAnnotations(Simplifier.Annotation) tells the code fix infrastructure that the parentheses can be simplified, i.e. removed, if doing so does not alter the meaning of the code (in context).

The main advantage of this approach is of course that it is easy, and can reduce code complexity significantly. Since you are reusing existing, tested logic, there is also a fairly high probability that it is correct without you having to write extensive tests for it. And furthermore, it is likely to stay correct if and when future versions of C# add additional operators or other syntax.