Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jjonescz merged 4 commits into from
Mar 6, 2023
+25 −5
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8902c23
Revert "Fix bootstrap build (#67156)"
jjonescz Mar 2, 2023
e5fae75
Add a test
jjonescz Mar 2, 2023
65acc6f
Save full state in nullable walker when visiting conditional operand
jjonescz Mar 2, 2023
a5bbc38
Merge branch 'main' into 67153-NullableWalker-ConditionalOperand-Cons…
jjonescz Mar 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 4 additions & 5 deletions src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5614,8 +5614,8 @@ private TypeWithState ConvertConditionalOperandOrSwitchExpressionArmResult(
LocalState state,
bool isReachable)
{
var savedState = this.State;
this.State = state;
var savedState = PossiblyConditionalState.Create(this);
Copy link
Contributor

@AlekseyTs AlekseyTs Mar 2, 2023 •
edited
Loading

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 •
edited
Loading

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

@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.

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 •
edited
Loading

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.

this.SetState(state);

bool previousDisabledDiagnostics = _disableDiagnostics;
// If the node is not reachable, then we're only visiting to get
Expand Down Expand Up @@ -5644,7 +5644,7 @@ private TypeWithState ConvertConditionalOperandOrSwitchExpressionArmResult(
resultType = default;
_disableDiagnostics = previousDisabledDiagnostics;
}
this.State = savedState;
this.SetPossiblyConditionalState(in savedState);

return resultType;
}
Expand Down Expand Up @@ -8457,8 +8457,7 @@ private TypeWithState VisitUserDefinedConversion(
bool reportRemainingWarnings,
Location diagnosticLocation)
{
// Uncomment when https://github.com/dotnet/roslyn/issues/67153 is fixed
// Debug.Assert(!IsConditionalState);
Debug.Assert(!IsConditionalState);
Debug.Assert(conversionOperand != null);
Debug.Assert(targetTypeWithNullability.HasType);
Debug.Assert(diagnosticLocation != null);
Expand Down
21 changes: 21 additions & 0 deletions src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53054,6 +53054,27 @@ static void M1(bool b, C c, D d)
comp.VerifyDiagnostics();
}

[Fact, WorkItem(67153, "https://github.com/dotnet/roslyn/issues/67153")]
public void ConditionalOperator_WithUserDefinedConversion_BoolOperand()
{
var source = """
struct C
{
public static implicit operator C(bool b) => throw null!;
}
class D
{
static void M(bool b)
{
_ = (b ? false : default(C)) /*T:C*/;
}
}
""";
var comp = CreateCompilation(source, options: WithNullableEnable());
comp.VerifyTypes();
comp.VerifyDiagnostics();
}

[Fact]
[WorkItem(33664, "https://github.com/dotnet/roslyn/issues/33664")]
public void ConditionalOperator_Ref_NestedNullabilityMismatch()
Expand Down