Skip to content
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.

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

Nullable analysis should understand XOR null tests #36927

Closed
drewnoakes opened this issue Jul 2, 2019 · 6 comments
Closed

Nullable analysis should understand XOR null tests #36927

drewnoakes opened this issue Jul 2, 2019 · 6 comments

Comments

@drewnoakes
Copy link
Member

Version Used: 3.3.0-beta1-19327-04

Steps to Reproduce:

string M1(object? o1, object? o2)
{
    if (o1 == null && o2 == null)
        return "both null";
    if ((o1 == null) ^ (o2 == null))
        return "one null";

    // at this point neither o1 nor o2 are null

    return o1.ToString()  // CS8602 possible null dereference
         + o2.ToString(); // CS8602 possible null dereference
}

string M2(object? o1, object? o2)
{
    if (o1 == null && o2 == null)
        return "both null";
    if (o1 == null)
        return "o1 null";
    if (o2 == null)
        return "o2 null";

    return o1.ToString()  // no diagnostics in this case
         + o2.ToString();
}

Expected Behavior:

No diagnostics.

Actual Behavior:

Diagnostics.

@canton7
Copy link
Contributor

canton7 commented Jul 2, 2019

Use != instead of ^? That's arguably more idiomatic for bools anyway. Although || has the same effect in your sample.

@drewnoakes
Copy link
Member Author

Thanks. Several workarounds exist. This issue is tracking a gap in the product. It's not uncommon to see bitwise operators used on bools.

@RikkiGibson
Copy link
Contributor

Our nullable analysis just doesn't learn anything in the WhenFalse case of o1 == null && o2 == null. I don't think it's realistic to start learning the "dependent" null relationship between variables (i.e. "at least one" of these variables was non-null, and "not exactly one" of these variables was non-null, therefore they're both non-null.)

I feel that in a scenario like this we should ask the user to change ^ to ||.

@agocke agocke added this to the Backlog milestone Jul 5, 2019
@agocke
Copy link
Member

agocke commented Jul 5, 2019

To provide a little background, the compiler does what is usually referred to as a "path-independent flow analysis" for these situations. What this means practically is that conditional states are intersected at the end of the condition.

So

    if (o1 == null && o2 == null) // cond 1
        return "both null";
    if ((o1 == null) ^ (o2 == null)) // cond 2
        return "one null";

Is processed like

if (o1 == null &&
     ^ split o1 is null and not null
   1) o1 is null
        o2 == null
         ^ split o2 is null and not null
           1) o2 is null
                  return
           2) goto cond 2
   2) goto cond 2
// cond 2
state = (o1 NotNull && o2 MaybeNull) /\ (o1 MaybeNull && o2 NotNull) = o1 MaybeNull && o2 MaybeNull

So because the states are intersected at cond 2, and in each state there is an o1 is MaybeNull and an o2 is MaybeNull, the resulting state is o1 is MaybeNull and o2 is MaybeNull. In other words, the condition doesn't actually tell the flow analysis anything new about the states of the variables.

One way to learn about the states would be to do what's called a "path-dependent" flow analysis, but that can be significantly more expensive and is not in the design of C# 8.

@agocke
Copy link
Member

agocke commented Dec 15, 2019

Closing as out of scope of the design.

@agocke agocke closed this as completed Dec 15, 2019
@jcouv
Copy link
Member

jcouv commented Dec 16, 2019

Duplicate of #37344 (nullability analysis for single and-operator)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants