Why does ReSharper recommend the removal of its own fix?

Question

ReSharper recommends rethrowing an exception and then, when I do that, it says the entire catch clause is redundant anyway, and suggests it be removed.

I am using this code (from MethodMan here):

public static DataTable ExecuteDataSet(string sql, CommandType cmdType, params SqlParameter[] parameters)
{
    using (DataSet ds = new DataSet())
    using (SqlConnection connStr = new SqlConnection(UsageRptConstsAndUtils.CPSConnStr))
    using (SqlCommand cmd = new SqlCommand(sql, connStr))
    {
        cmd.CommandType = cmdType;
        foreach (var item in parameters)
        {
            cmd.Parameters.Add(item);
        }

        try
        {
            cmd.Connection.Open();
            new SqlDataAdapter(cmd).Fill(ds);
        }
        catch (SqlException ex)
        {
            throw;
        }
        return ds.Tables[0];
    }
}

When I have ReSharper Inspect > Code Issues in Solution, it wonders whether "exception rethrow possibly intended" here:

catch (SqlException ex)
{
    throw ex;
}

If I accept ReSharper's suggested fix ("rethrow exception"), Resharper removes the "ex":

catch (SqlException ex)
{
    throw;
}

...but then, on the next Inspection, it says, "catch clause is redundant" and suggests it be removed altogether.

But, of course, if I remove the catch block, it won't compile ("Expected catch or finally"). I could remove the try...but... if I change it to this:

                catch (SqlException sqlex)
                {
                    for (int i = 0; i < sqlex.Errors.Count; i++)
                    {
                        var sqlexDetail = String.Format("From
ExecuteDataSet(), SQL Exception #{0}{1}Source: {2}{1}   
Number: {3}{1}State: {4}{1}Class: {5}{1}Server: {6}{1}Message: {7}
{1}Procedure: {8}{1}LineNumber: {9}",
                            i + 1, // Users would get the fantods if they
saw #0
                            Environment.NewLine,
                            sqlex.Errors[i].Source,
                            sqlex.Errors[i].Number,
                            sqlex.Errors[i].State,
                            sqlex.Errors[i].Class,
                            sqlex.Errors[i].Server,
                            sqlex.Errors[i].Message,
                            sqlex.Errors[i].Procedure,
                            sqlex.Errors[i].LineNumber);
                        MessageBox.Show(sqlexDetail);
                    }
                }
                catch (Exception ex)
                {
                    String exDetail
String.Format(UsageRptConstsAndUtils.ExceptionFormatString, ex.Message, 

Environment.NewLine, ex.Source, ex.StackTrace);
                    MessageBox.Show(exDetail);
                }

...ReSharper's inspection doesn't complain about it.

The original `throw` and the `throw ex` are both pointless. Your final example (with code in the `catch` block) is clearly doing something with `ex` so I guess Resharper realizes that and doesn't complain. — xxbbcc, Nov 23 '15 at 19:42
http://stackoverflow.com/questions/730250/is-there-a-difference-between-throw-and-throw-ex — xxbbcc, Nov 23 '15 at 19:43
Just a `throw;` (or release as I like to call it) is pointless, and `throw ex;` (which I call rethrow) is actually bad because it resets the stack trace. So basically Resharper is first having you fix the bad practice of `throw ex;` then fixing the pointlessness of just a `throw;`. — juharr, Nov 23 '15 at 19:45
@B.ClayShannon do you have this code placed in a separate static Class..? regardless the throw pare you can actually replace it with the `Console.WriteLine(ex.Message)` or you can log the actual ex.Message what I have shown you in my previous example was a way to implement. sometime you have to think outside the box in regards to using examples do not have to use them literally word for word you can alter the original method I have giving you to make it work for your use case.. — MethodMan, Nov 23 '15 at 19:46
@B.ClayShannon please be careful with editing peoples answers you should only be editing your own question in regards to formatting and or fixing typo's etc unless what you are editing really does fix an issue in regards to you question.. if so then post if as a comment under that posters answer.. so that you do not confuse the issue. — MethodMan, Nov 23 '15 at 19:49
@xxbbcc - if the `throw` and `throw ex` are pointless, why is Visual Studio giving me an error when I get rid of them? (`Expected catch or finally`) — jbyrd, Oct 18 '17 at 18:29
@xxbbcc - ohhh, I think I answered my own question - is it because if I can say just `catch { throw }`, then I can remove the try/catch pair altogether? — jbyrd, Oct 18 '17 at 18:42

score 6 · Accepted Answer · answered Nov 23 '15 at 19:44

When you do this, you're resetting the call stack, which loses valuable information about what threw the exception in the first place.

catch (SqlException ex)
{
    throw ex;
}

If ReSharper were being smarter, it would've told you to just remove that part in the first place, saving you the time it took to rewrite that portion of code.

The following code is better since it doesn't lose the stack trace info, but it's unnecessary.

catch (SqlException ex)
{
    throw;
}

Even if you omit the above, the exception is going to "bubble up" and can just be caught somewhere further up the stack in your program, wherever you're ready to actually deal with it (log it, or display a message, or take some alternative action, etc).

score 1 · Answer 2 · edited Nov 23 '15 at 19:44

1

catch (SqlException ex)
{
    throw ex;
}

Is a bad programming practice. You lose the original stack trace from the thrown exception.

catch (SqlException ex)
{
   throw;
}

Does absolutely nothing for you except waste time re-throwing the same identical exception. Re-sharper is expecting you to do something with the variable ex like log it.

edited Nov 23 '15 at 19:44

B. Clay Shannon-B. Crow Raven

8,547
144
472
862

answered Nov 23 '15 at 19:44

user957902

3,010
14
18

Why does ReSharper recommend the removal of its own fix?

2 Answers2