Code generates an error about not returning a value even though every possible path returns a value

Question

I feel like I'm missing something obvious in my code, but I just don't see it. Is the compiler really not able to detect that the function will never really reach the end? I think the compiler is correct, and I'm just not seeing something really obvious, so asking if anyone can take a look at this and let me know. Thanks in advance.

public async Task<string> MyReadAllTextAsync(string file, FileShare fileShare = FileShare.None, Encoding encoding = null, int retries = 0)
{
    for (int i = 0; i <= retries; i++)
    {
        try
        {
            // This code is not important for this question
            // ... only that it will either return from the function or throw an exception.
            using var fileStream = new FileStream(file, FileMode.Open, FileAccess.Read, fileShare);
            using var textReader = new StreamReader(fileStream, encoding ?? Encoding.UTF8);
            return await textReader.ReadToEndAsync();
        }
        catch (IOException ex) when (ex.Message.Contains("it is being used by another process"))
        {
            if (i < retries)
            {
                await Task.Delay(DelayMs);
                continue;
            }
            throw;
        }
    }

    // Can this code be reached?
    // Compiler generates error "not all code paths return a value"
    // Compiler is forcing me to add this throw even though I don't think it can ever reach here.
    throw new InvalidOperationException("MyReadAllTextAsync failure");
}

Update after question is answered

I got the correction to my code (below), but it still seems to me that the compiler should be able to catch this case now with the update, but it doesn't. Posting it here in case anyone sees another way for the function to reach the end. If you do, comment, and I'll create a separate answer to give you the points for answering.

if (retries < 0) throw new ArgumentException(nameof(retries));

public async Task<string> MyReadAllTextAsync(string file, FileShare fileShare = FileShare.None, Encoding encoding = null, int retries = 0)
{
    for (int i = 0; i <= retries; i++)
    {
        try
        {
            // This code is not important for this question
            // ... only that it will either return from the function or throw an exception.
            using var fileStream = new FileStream(file, FileMode.Open, FileAccess.Read, fileShare);
            using var textReader = new StreamReader(fileStream, encoding ?? Encoding.UTF8);
            return await textReader.ReadToEndAsync();
        }
        catch (IOException ex) when (ex.Message.Contains("it is being used by another process"))
        {
            if (i < retries)
            {
                await Task.Delay(DelayMs);
                continue;
            }
            throw;
        }
    }

    // Can this code be reached?
    // Compiler generates error "not all code paths return a value"
    // Compiler is forcing me to add this throw even though I don't think it can ever reach here.
    throw new InvalidOperationException("MyReadAllTextAsync failure");
}

If you comment out the last line in the function, you get a compiler error even though that line can never be reached (I don't think).

Answer 1

Let me first simply this code further

    public async Task<string> MyReadAllTextAsync(string file)
    {
        for(int i=0;i<5;i++)
        {
            try
            {
                return ... // return something
            }
            catch (IOException ex) when (ex.Message.Contains("it is being used by another process"))
            {
                if (i < retries)
                {
                    continue;
                }
                throw;
            }
        }
        // Can this code be reached?
    }

Now, the position where your comment // Can this block be reached is, can be reached by the following ways:

1. The condition of the for loop can be false to begin with: While you may use common sense to see that would not be the case, the compiler cannot do that, and it keeps two paths (whether the logic enters the loop or not). You might think of replacing this for loop with a do while loop (which has a compile time guarantee of iterating at least once), but there are more cases to consider.

2. Not handling all exceptions ... EDIT: I was wrong in this one. I said earlier that you should check for all exceptions, but that only means other exceptions are not handled. Those unhandled exceptions would simply be thrown, but no path to the code underneath is created because of this.

Here, I simplify the code, just to show the compiler behaviour. The following code has no errors issued by the compiler.

        do
        {
            try 
            { 
                ....
                ....
                return ...; // return statement after some block of code 
            }
            catch(IOException ex) { throw; } // Any other exception is also thrown
        }
        while( <condition> ); // Some condition

However, this still doesn't solve your issue, because you want to have another conditional statement in the catch block, and this now introduces the error again, because of the last reason:

2. Your logic is dependent on two different conditional statements: It may be obvious to you that after a certain amount of retries, the condition of the if statement in the catch block becomes false, and thereafter your code will always throw; but the compiler does not have that guarantee. To the compiler, there is one conditional statement for loop, and another conditional statement within the catch block that can determine the path taken by the code. The compiler cannot make any inference among these two conditions.

Let me illustrate this with the code below

        int i=0;
        do
        {
            try
            { 
                ....
                ....
                return ...; // return something
            }
            catch(IOException e)
            { 
                if(<condition_1>) // line A
                    continue;     // line B 
                else throw;
            }
        }
        while(<condition_2>);    // line C

The compiler has no guarantee that the <condition_1> in Line A will be false only at the last iteration of the loop. So (in the compiler's perspective), there is always a path within the catch block that can go to continue; statement (line B), which then leads to the while's conditional statement (line C), and if the <condition_2> happens to be false here (Since the compiler cannot know this beforehand), the logic can go below, exiting the loop. Thus, a path exists outside the loop.

This is also the reason you're still getting the same error in your updated code. The condition within the catch block in your code is i < 5, and the condition within the for loop in your code is i <= 5. Any human can reason on this code and infer when the loop conditions are true or false, but the compiler can only do such kinds of inference if the said statements are compile time constants. So if the for loop condition is <condition_2>, and the if block condition (within the catch block) is <condition_1>, the compiler can assign a path outside the loop as discussed above.

So taking the points into account you must make the loop condition a compile time constant. Now, if you keep the loop condition as true it does not matter whether you use a for loop, a while loop or a do-while loop. The compiler now has a compile time guarantee that the loop is always executed, so the issue in point #1 is also resolved.

So my suggestion is to make your code as shown

    public async Task<string> MyReadAllTextAsync(...)
    {
        for(int i=0;true;i++) // Loop condition is now a compile time constant, giving a compile-time guarantee that the loop is always executed
        {
            try 
            {
                // This code is not important for this question
                // ... only that it will either return from the function or throw an exception.
                using var fileStream = new FileStream(file, FileMode.Open, FileAccess.Read, fileShare);
                using var textReader = new StreamReader(fileStream, encoding ?? Encoding.UTF8);
                return await textReader.ReadToEndAsync();
            }
            catch (IOException ex) when (ex.Message.Contains("it is being used by another process"))
            {
                await Task.Delay(1000);
                if(i<5) continue; // Flow of code only depends on this condition evaluated during runtime.
                else throw;
            }
        }
    }

Answer 2

Yes the code throwing the exception at the bottom can be reached. What if the code calling this function passes in -1 for the parameter 'retries'? That would mean the for loop would not do any loops since 0 is not <= -1 and therefore progress to the bottom code.

I would recommend adding some validation to the retries parameter above the for loop to make that sure that if the parameter 'retries' is less than 0, to then set the loop counter to 0.

...
var loopCounter = retries;

if (loopCounter < 0)
{
   // If you want to make it run once.
   loopCounter = 0;

   // Or if you want to break the code flow if they don't provide any retries.
   throw new Exception();
}

for (int i = 0; i < loopCounter; i++)
...

You could also change the 'retries' parameter to be an unsigned int so that way it only allows positive numbers (including 0)

uint retries = 0