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");
        }

CodePudding user response：

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

CodePudding user response：

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)
                {
                    await Task.Delay(1000);
                    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");
    }

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

1. The for condition 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, but there are more cases to consider.

2. Your catch block does might not work for all cases : You might think that in your case, the only exception that can be thrown is an IOException, but the compiler has no way of knowing that, and thus all paths are not handled. So you may think of using a catch (Exception) {...} block, or better yet, a simple catch {...} block for any exception in your code.

Here, I simplify the code, just to show the compiler behaviour

        do
        {
            try 
            { 
                return ...; // return statement after some block of code 
            }
            catch { throw; }
        }
        while( ... ); // Some condition

In this case, the compiler does not issue any error. However, this still doesn't solve your issue, because you want to have a condition in the catch block, and this now introduces the error again, because of the last reason:

3. You have a conditional block in the catch block: In the compiler's perspective, the code can continue; or throw depending on the condition. Again, it may be obvious to you that after a certain amount of retries, the condition becomes false and your code will always throw but the compiler does not have that guarantee. Therefore, the compiler can think of a path that after a certain continue; in the catch block, the loop ends and thus there is still a path outside the loop. So you get the same error back again when you do this:

        int i=0;
        do
        {
            try 
            { 
                return ...; // Large block ending with return statement
            }
            catch 
            { 
                if(i  <5) continue; 
                else throw; 
            }
        }
        while(i<=5);

So my suggestion is to make your code as shown

    public async Task<string> MyReadAllTextAsync(string file, ...)
    {
        for(int i=0;i<=5;i  )
        {
            try 
            { 
                return ...; // Large block ending with return statement
            }
            catch 
            { 
                continue; // Always continue
            }
        }
        throw new InvalidOperationException("MyReadAllTextAsync failure");
    }