Save full state in nullable walker when visiting conditional operand

[jjonescz]

Fixes #67153.

In a conditional operator with a user conversion, there's usually not a conditional state inside the operands. But in an expression like cond ? false : obj with a bool constant and obj convertible to bool, the first operand suddenly has a conditional state. NullableWalker.ConvertConditionalOperandOrSwitchExpressionArmResult did not expect that, it was saving and restoring only non-conditional state. This PR fixes that by saving the full state.

Note that the error originally occurred in bootstrap build on this line (in the conditional operator):

roslyn/src/VisualStudio/Core/Test.Next/Options/VisualStudioStorageReadFallbackTests.cs

Line 32 in 3310a7b

fallback.TryRead(language, (storageKey, _, _) => storageKey == "TextEditor.CSharp.Specific.SpaceWithinExpressionParentheses" ? false : default(Optional<object?>)).Value,

[jaredpar]

@RikkiGibson, @jcouv PTAL

[AlekseyTs]

var savedState = PossiblyConditionalState.Create(this);

Are we simply trying to suppress the assert? This might be a wrong thing to do. Because sometimes we are failing to "unsplit" the state when we are supposed to, and asserts help us find these cases. If the consumer indeed handles conditional state properly, It might be better to adjust the consumer to deal with the fact rather than suppressing the assert across all possible consumers. #Closed

[jjonescz]

No, I'm trying to do what the method was doing previously - saving and restoring state - but handling the conditional state, too. Although I see what you're saying, it doesn't seem to me that unsplitting is needed here.

[AlekseyTs]

but handling the conditional state, too.

This is exactly what I am saying. This can be looked at like the method is not expected to be called on a conditional state. Perhaps we should assert the fact on entry and adjust the consumer to deal with conditional state appropriately. It is quite possible that saving and restoring the state there (for the specific consumer that, presumably, already deals with conditional state) is the right thing to do.

[jjonescz]

That makes sense, thanks, I've investigated it more. So, the method ConvertConditionalOperandOrSwitchExpressionArmResult is used only in two places as its name suggests - in conditional operator and switch expressions. The latter cannot result in conditional state, whereas the former can. Thanks to that, in D.M1 below, it's inferred that c won't be null in the second return statement. Switch expressions currently cannot do that (see D.M2), but I imagine this restriction could be lifted in the future.

class C
{
    public static implicit operator bool(C _) => throw null!;
}
class D
{
    public static C M1(bool b)
    {
        C? c = null;
        if (b ? true : c = new C())
        {
            return c; // warning
        }
        return c;
    }
    
    public static C M2(bool b)
    {
        C? c = null;
        if (b switch { true => true, false => c = new C() })
        {
            return c; // warning
        }
        return c; // warning - unnecessary
    }
}

Anyway, from that I think ConvertConditionalOperandOrSwitchExpressionArmResult should be able to save conditional state, then visit the operand, and then restore the same state (because switch expressions might want to support that in the future). Alternatively, I could handle that in the caller (VisitConditionalOperatorCore) as you suggest. But I think I would do that the same way (just one level up) - save the conditional state and set the unconditional state temporarily - which seems unnecessary then. But if you would still prefer that, I guess that's not a problem.

[AlekseyTs]

LGTM (commit 3)