Pattern Matching should respect [DoesNotReturn] and [DoesNotReturnIf]

[dazerdude]

This (somewhat unexpectedly for me) produces a compiler error.

public void Read(ref Utf8JsonReader reader) {
    var node = JsonNode.Parse(ref reader);
    Assert(node is JsonObject obj);
    Console.WriteLine(obj.ToString());  // CS0165: Use of unassigned local variable `obj`
}

public void Read(ref Utf8JsonReader reader) {
    var node = JsonNode.Parse(ref reader);
    if (node is not JsonObject obj) { Fail(); }
    Console.WriteLine(obj.ToString());  // CS0165: Use of unassigned local variable `obj`
}

[DoesNotReturn]
private static void Fail() => throw new Exception();

private static void Assert([DoesNotReturnIf(false)] bool cond) {
    if (cond) { throw new Exception(); }
}

whereas using nearly-equivalent null checks works as expected

public void Read(ref Utf8JsonReader reader) {
    var obj = JsonNode.Parse(ref reader) as JsonObject;
    Assert(obj != null);
    Console.WriteLine(obj.ToString());
}

public void Read(ref Utf8JsonReader reader) {
    var obj = JsonNode.Parse(ref reader) as JsonObject;
    if (obj == null) { Fail(); }
    Console.WriteLine(obj.ToString());
}

I assume this is because [DoesNotReturn] and [DoesNotReturnIf] is only taken to account when analyzing nullability, but it seems like it should apply to certain other scenarios as well since it declares pretty general code reachability information. Shouldn't the compiler be able to use the attribute information to determine that the final line is only reachable if obj is populated, or am I missing some bit of subtlety here?

[KennethHoff]

Obj is only assigned in a debug context, so this wouldn't be possible regardless.

[dazerdude]

That's particular to Debug.Assert and isn't really relevant to my point here. I was just using it as convenient example, but the issue described is more general than that. I've updated to the sample to hopefully provide some clarity.

[dazerdude]

Weirdly this compiles for me in Release w/o nullable static analysis complaining:

public void Read(ref Utf8JsonReader reader)
{
    object? obj = null;
    Debug.Assert(obj != null);
    Console.WriteLine(obj.ToString()); // throws NullPointerException at runtime
}

whereas this produces the expected error in Release:

public void Read(ref Utf8JsonReader reader)
{
    object? obj = null;
#if DEBUG
    Debug.Assert(obj != null);
#endif
    Console.WriteLine(obj.ToString()); // CS8602: Dereference of a possibly null reference.
}

But I'm probably misunderstanding some detail of Debug.Assert. I don't use it much in my own code.

[TahirAhmadov]

The problem here is that "variable assignment" is a core language compilation error, and they don't want to tie it to certain attributes. There are proposals to make "does not return" a core language feature, which would allow your scenarios.

[CyrusNajmabadi]

Wouldn't both assignment analysis and NRT analysis be affected if/when "return type never" is implemented?

Likely yes?

[TahirAhmadov]

It's about it being enforced or not.

Correct me if I'm wrong, the enforcement would be done by a "core rule", not an analyzer, and stylistically it's much more likely that a "core rule" is driven by a keyword, and not an attribute, right?
Meaning, a "core rule" can be driven off of an attribute, but at that point it would be "weird", right?

[dazerdude]

(Oh my god, so many comments, I'm so slow. I wrote this immediately after my previous comment. I should've refreshed I guess.)

I don't think it's awkward. Indeed, I think asking for the language to be impacted by attributes is more awkward.

I merely meant that I find implementation details of the compiler to be an awkward justification for contract inconsistencies between the two implementations of flow analysis. (As an aside, I find this generally true of most software, implementation details influencing contracts is a reality of the world we live in, but a disappointing one). After @TahirAhmadov's comments, I don't disagree that using attributes would probably be the wrong approach for a more general extension to reachability, and totally understand why it is currently this way.

Flow analysis isn't suffering with the same issues that NRT did, so the solution doesn't need the same compromises

Well, yes and no. I agree that this wouldn't be something I'd complain about if NRT had never been implemented. But I'd argue that all other features that rely on flow analysis suffer by proxy. Because NRT's implementation of flow analysis has additional features in it's flow analysis, I don't think it's unreasonable for a programmer to expect those flow analysis extensions to be supported by other features that are built on top of flow analysis. Those extensions are more generally applicable than merely to NRT (if only for programmer comfort).

(I'm using flow analysis and reachability analysis interchangeably here. Please correct me if that's wrong.)

[CyrusNajmabadi]

@dazerdude The way to think about this is that these are different systems in the language design, with different goals. I understand your desire for them to be unified an to have uniform impact in the language feature-set.

All i can say is: that was considered, and explicitly rejected because the resultant language would actually have more negatives than positives over what we have today.

This is often a fundamental tension we have in lang design. Often times there is a preference for purity and uniformity. However, this butts heads with the need for pragmatic outcomes. NRT is fundamentally solving a different set of issues in a different fashion, due to many complex factors. Trying to do the same with definite assignment is a non-goal, and we feel would actually result in a worse situation.

[dazerdude]

Reachability analysis having the same problems of NRT where the system thinks the code is fine, but you can still blow up at runtime.

Having a method state it does not return, but still then return would not be acceptable as it would form a safety hole.

Would it blow up at runtime? I totally get the need for adequate enforcement of reachability is absolutely necessary and that NRT in general is full of holes. But I'm under the impression that [DoesNotReturn] or [DoesNotReturnIf] in specific were adequately enforced. They appear to have produced all the necessary errors in annotated methods in my experience at least. Is there a known issue w/ NRT's flow analysis w.r.t. these attributes?

Unless I'm mistaken the other NRT attributes don't really effect statement reachability the same way. I completely understand that those shouldn't be applied to other features.

[dazerdude]

@dazerdude The way to think about this is that these are different systems in the language design, with different goals. I understand your desire for them to be unified an to have uniform impact in the language feature-set.

All i can say is: that was considered, and explicitly rejected because the resultant language would actually have more negatives than positives over what we have today.

This is often a fundamental tension we have in lang design. Often times there is a preference for purity and uniformity. However, this butts heads with the need for pragmatic outcomes. NRT is fundamentally solving a different set of issues in a different fashion, due to many complex factors. Trying to do the same with definite assignment is a non-goal, and we feel would actually result in a worse situation.

The tension between purity and pragmatism is fundamental to software and I completely understand. Thanks for the discussion. I do really appreciate the insights you and @TahirAhmadov have provided me here.

[CyrusNajmabadi]

Would it blow up at runtime?

Yes. Consider something as simple as:

int i;
if (whatever)
{
    DoesNotReturn();
}
else
{
     i = ComputeActualValue();
}

Use(i);

Now imagine that the code that provides DoesNotReturn actually has the IL equivalent to return 0;. Now what? If we just blindly trusted that method, then we can literally get to the point that we're Useing i despite it not being initialized. This could lead to all sorts of blow-ups or security issues.

[CyrusNajmabadi]

But I'm under the impression that [DoesNotReturn] or [DoesNotReturnIf] in specific were adequately enforced.
They appear to have produced all the necessary errors in annotated methods in my experience at least.

THere is no actual enforcement. The following compiles successfully:

    [DoesNotReturn]
    public T DoesNotReturn<T>()
    {
        return default(T);
    }

[TahirAhmadov]

#538