Skip to content

Save full state in nullable walker when visiting conditional operand#67157

Merged
jjonescz merged 4 commits intodotnet:mainfrom
jjonescz:67153-NullableWalker-ConditionalOperand-ConstantBool
Mar 6, 2023
Merged

Save full state in nullable walker when visiting conditional operand#67157
jjonescz merged 4 commits intodotnet:mainfrom
jjonescz:67153-NullableWalker-ConditionalOperand-ConstantBool

Conversation

@jjonescz
Copy link
Member

@jjonescz jjonescz commented Mar 2, 2023

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):

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

@ghost ghost added the Area-Compilers label Mar 2, 2023
@jjonescz jjonescz marked this pull request as ready for review March 2, 2023 10:34
@jjonescz jjonescz requested a review from a team as a code owner March 2, 2023 10:34
@jaredpar
Copy link
Member

jaredpar commented Mar 2, 2023

@RikkiGibson, @jcouv PTAL

{
var savedState = this.State;
this.State = state;
var savedState = PossiblyConditionalState.Create(this);
Copy link
Contributor

@AlekseyTs AlekseyTs Mar 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@jjonescz jjonescz Mar 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@jjonescz jjonescz Mar 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (commit 3)

@jjonescz jjonescz enabled auto-merge (squash) March 6, 2023 08:12
@jjonescz jjonescz merged commit f64507a into dotnet:main Mar 6, 2023
@jjonescz jjonescz deleted the 67153-NullableWalker-ConditionalOperand-ConstantBool branch March 6, 2023 09:15
@ghost ghost added this to the Next milestone Mar 6, 2023
@allisonchou allisonchou modified the milestones: Next, 17.6 P3 Mar 28, 2023
jjonescz added a commit that referenced this pull request Dec 16, 2025
Closes #67153.

The assert was uncommented already in the original fix
(#67157),
but then it got commented again via automatic code flow
(#67507).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Nullable assert firing while building our Visual Studio code

5 participants