Indicate which sections of a method do not return, if possible

[Pilchard123]

Analyzer

Diagnostic ID: CS0161

Describe the improvement

It may be useful if the message for CS0161 could show which branches in a method do not return. In some cases it is not immediately obvious why the method does not return, either because the method has convoluted branching or because the writer has misunderstood or mistyped a condition and so mistakenly believes that all cases are covered. This proposed change would indicate which particular section is likely to be incorrect.

Describe suggestions on how to achieve the rule

Perhaps given the code in the context section below the message could say something similar to "not all code paths return a value - lines A-B, Z-end", with A-B being a section within a method but not at the end and Z being the start of a section that ends a method.

This may be better done as a new analyzer, with one report for each section that if entered will never return or throw, with messages like "Section from lines X-Y will never return" or "Method will never return if line Z is reached".

Additional context

int Foo()
{
    if(condition)
    {
        return 1;
    }
    else
    { // A
        
    } // B
}

int Bar()
{
    if(condition)
    {
        return 1;
    }
    // Z
}

[mavasani]

I believe this is already covered by https://docs.microsoft.com/en-us/dotnet/fundamentals/code-analysis/style-rules/ide0035. When you open a source with unreachable code blocks in Visual studio, you should see an IDE0035 for each such section and the section is also faded out. Additionally, there is a code fix to remove unreachable block.

[Pilchard123]

Not quite - this is for code that exists and can be reached, but that will never return. Consider the toy function Bar, as above:

int Bar()
{
    if (condition)
    {
        return 1;
    }
    // we get to here if condition is false
    // but if condition *is* false, then we get to a path that has no return
}

We already have something similar in CS8509. If you have something like

var bool1 = true;
var bool2 = false;

var foo = (bool1, bool2) switch
{
    (true, true) => true,
    (true, false) => false,
    (false, true) => false
};

then you will get "The switch expression does not handle all possible values of its input type (it is not exhaustive). For example, the pattern '(false, false)' is not covered." I'm proposing a similar analyzer but for methods.

[mavasani]

The compiler should already generate an error when a code path has a missing return. Can you please clarify the request?

[Pilchard123]

The compiler will currently generate an error if there is a path that does not return. I'm asking if it's possible for that error to identify which path(s) don't return.

[Pilchard123]

As an example, the code below gives the error "'Program.Main()': not all code paths return a value". Could it instead (or as well) say something like "'Program.Main()' will not return a value if line 16 is reached"?

using System;

namespace IssueDemo
{
    class Program
    {
        static int Main()
        {
            var rng = new Random();
            if (rng.Next() % 2 == 0)
            {
                return 1;
            }
            else
            {

            }
        }
    }
}

[Youssef1313]

@mavasani Looks like this is a suggestion for compiler diagnostic message. Can you move to dotnet/roslyn?

[Pilchard123]

Ooh, I thought this was dead. Nice to see it's been picked up again - is there any more info that you want from me?

[jcouv]

Took a quick look at how this diagnostic is reported. If the end of the method body is reachable, then we reported. To determine whether it is reachable, we run control flow analysis. The main state that this flow analysis maintains is a bool indicating whether it is reachable.

So in the above example, we start in reachable state, then the branch with the return gets set to unreachable. The state after the conditional is the "merge" of an unreachable and a reachable states, which is reachable. So the end of the method is reachable.

To report the improved diagnostic, we'd have to store more state (something like list of bound nodes that result in a reachable chain). But even that would not be enough, we'd have to remember the specific branch of the conditional. Then we'd have to decide how to describe that path to the user in a diagnostic, which isn't trivial.

This all seems pretty involved for a rather limited gain in user experience, so I don't think we'll ever get to it. Moving to backlog.