Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proposal: Support catch and finally for using #2018

Open
AyrA opened this issue Nov 20, 2018 · 18 comments
Open

Proposal: Support catch and finally for using #2018

AyrA opened this issue Nov 20, 2018 · 18 comments

Comments

@AyrA
Copy link

AyrA commented Nov 20, 2018

Proposal

Please allow us to append a catch and/or finally block to the end of a using, similar to try.

Example

using(var FS=File.OpenRead(@"C:\SomeFile"))
{
	//Work with FS here. Exceptions here will not be caught by the catch statement
}
catch(Exception ex)
{
	//This handles Errors regarding the File.OpenRead statement only,
	//not the encapsulated code of the using block.
	//The FS variable is not accessible here
}

Improvements

This change would be compatible with existing code and would put a stop to this pattern, which is not recommended according to the documentation of the using statement (see example right above the linked chapter):

FileStream FS;
try
{
	FS=File.OpenRead(@"C:\SomeFile");
}
catch(Exception ex)
{
	//Error handling code. File.OpenRead related
}
using(FS)
{
	//Work with FS here
}
//FS is still in scope but not usable because it has been disposed

Or even worse, this, which utilizes the using properly but stacks try blocks:

try
{
	using(var FS=File.OpenRead(@"C:\SomeFile"))
	{
		try
		{
			//Work with FS here.
			//Use additional try block to catch an Exception not related to File.OpenRead
		}
		catch(Exception ex)
		{
			//Error handling code. using encapsulated code related
		}
	}
}
catch(Exception ex)
{
	//Error handling code. File.OpenRead related
}

Other considerations

Another possibility would be that the using(...){...}catch(...){...} statement is set to catch all exceptions, including those that happen inside of the using encapsulated code. In that case, a new UsingException type would probably be necessary that encapsulates the using(...) related exception to allow the developer to distinguish between locations of exceptions.

@DavidArno
Copy link

Related: #1874

@Mafii
Copy link

Mafii commented Nov 21, 2018

The using statement is basically a try - finally, where the finally contains disposal of the object in the using statement. What moment in execution would the catch block represent? Would it be executed before or after object disposal?

This is a obviously an important question, but otherwise, I'm very much for this idea. It tightens up code and makes it more concise.

@HaloFour
Copy link
Contributor

HaloFour commented Nov 21, 2018

This is already legal code:

try {
    // ...
    using (var foo = new DisposableFoo()) {
        // ...
    }
    catch {
        // ...
    }
}
finally {
    // ...
}

This creates an ambiguity and breaking changes, particularly if there are multiple catch clauses as this proposal would affect the nesting of those clauses.

Update: I need to count my braces better, it's not legal code.

@Logerfo
Copy link
Contributor

Logerfo commented Nov 21, 2018

To follow the current implementation, I think the best approach would be to expand the following:

using (var foo = new Bar())
{
   //do something
}
catch
{
   //catch something
}

... to the following:

var foo = new Bar();
try
{
   //do something
}
catch
{
   //catch something
}
finally
{
   if (foo != null)
      ((IDisposable)foo).Dispose();
}

But there's an ambiguity problem: the using statement does not require braces. See it for yourself:

using (var foo = new Bar())
   try
   {
      //do something
   }
   catch (ArgumentException) { }
catch
{
   //catch what???
}

Because of this problem, every proposal to make try, catch and finally blocks braces optional has been mostly rejected.
A possible alternative would be to require using-catch blocks to always have braces.

@HaloFour
Copy link
Contributor

the using statement does not require braces.

This makes it trivial to create this behavior using existing syntax:

using (var foo = new Bar()) try
{
    // do something
}
catch (Exception exception)
{
    // oops, something went wrong
}

The formatter in VS might fight you but I think that it would be easier to fix the formatter in this case rather than modify the language.

@AyrA
Copy link
Author

AyrA commented Nov 21, 2018

@HaloFour your proposal would not catch an exception in the var foo = new Bar() statement, which this proposal is about.

I'm all in for making braces mandatory for a using statement that you want a catch for. We would not break anything existing either because you can't use catch for using at all right now.

@Logerfo
Copy link
Contributor

Logerfo commented Nov 21, 2018

@AyrA

@HaloFour your proposal would not catch an exception in the var foo = new Bar() statement, which this proposal is about.

Mine neither. A change would be required:

Bar foo = default;
try
{
   foo = new Bar();
   //do something
}
catch
{
   //catch something
}
finally
{
   if (foo != null)
      ((IDisposable)foo).Dispose();
}

But I disagree. I believe that if using blocks had catch blocks, they should not be hit if the using expression fail, just like the expanded finally block isn't.

@theunrepentantgeek
Copy link

an exception in the var foo = new Bar() statement, which this proposal is about.

Intuitively, I would not expect an exception in the expression part of the statement to be caught, violating the principle of least surprise.

I'm all in for making braces mandatory for a using statement that you want a catch for.

This would be really confusing - it would be akin to having braces optional for if unless you wanted to have an else as well, in which case the braces are mandatory .

@DarthVella
Copy link

I'd say it's closer to having braces optional for a while loop, but making braces and semicolon mandatory for do-while.

@Joe4evr
Copy link
Contributor

Joe4evr commented Nov 22, 2018

but making braces and semicolon mandatory for do-while.

Braces are still optional for do-while, though.

@DarthVella
Copy link

I know :P It was an analogy.

@Joe4evr
Copy link
Contributor

Joe4evr commented Nov 22, 2018

Both analogies come down to making perfectly legal code today illegal, which is the biggest No-No when it comes to evolving C#.

@DarthVella
Copy link

We were discussing syntax for the proposed feature, which would not invalidate currently correct code. Let's just say we got dragged off topic and leave it at that.

So, since using expands into a try/finally block, what would happen when someone wants to append a finally block to the using statement? The usual disposal gets inserted at the end of the existing finally block, or a new try/finally surrounds the user's changes?

using(var disposable = ...)
{
    //using disposable
}
catch(Exception ex)
{
    //omitted
}
finally
{
    //omitted
}

//Expands into
IDisposable disposable = ...
try
{
    //using disposable
}
catch
{
    //omitted
}
finally
{
    //omitted
    disposable.Dispose();
}

//or
IDisposable disposable = ...
try
{
    try
    {
        //using disposable
    }
    catch
    {
        //omitted
    }
    finally
    {
        //omitted
    }
}
finally
{
    disposable.Dispose();
}

@juliusfriedman
Copy link

juliusfriedman commented Dec 2, 2018

Just like with lock how it expands to try enter now we should ask the user to be as explicit as possible hence why we allow and perform special null checking on type right?

This is already valid syntax when written in another properly encapsulated block.

I'm against this as it promotes bad design.

Make the class not require the external try catch or document it to need one.

This notion of additional preservation for using beyond the scope of what's already there seems like bandaid for lack of good design paradigm for users to adopt and utilize.

E.g. a BaseDisposable implementation

@VanKrock
Copy link

VanKrock commented May 8, 2019

@Logerfo

using (var foo = new Bar())
   try
   {
      //do something
   }
   catch (ArgumentException) { }
catch
{
   //catch what???
}

to the following

var foo = new Bar();
try
{
   try
   {
      //do something
   }
   catch (ArgumentException) { }
}
catch
{
   //catch what???
}
finally
{
   if (foo != null)
      ((IDisposable)foo).Dispose();
}

So, "using" is "try" with default finally. But "try" has optional catch and "using" hasn't.

@rovercoder
Copy link

rovercoder commented Jun 4, 2019

Just an idea, maybe we could use the => operator to denote that instantiation is to be performed later inside try block. Would be nice to be able to write something like this.

DateTime startTime = DateTime.UtcNow;

using (var scope => new TransactionScope())
{
    using var db = new DBContext();
	
    var johnList = db.Persons.Where(x => x.Name == "John");
    johnList.ForEach(j => { j.Surname = "Doe"; });
    db.SaveChanges();
	
    scope.Complete();
	
    Log.Info("Johns updated successfully.");
}
catch (Exception ex)
{
    Log.Error("Johns updating failed.", ex);
    throw ex;
}
finally
{
    DateTime endTime = DateTime.UtcNow;
    Log.Info($"Johns update method took { (endTime - startTime).TotalMilliseconds }ms to complete.");
}

to

DateTime startTime = DateTime.UtcNow;

TransactionScope scope = default(TransactionScope);

try
{
    scope = new TransactionScope();

    DBContext db = new DBContext();

    try
    {
        List<Person> johnList = db.Persons.Where(x => x.Name == "John");
        johnList.ForEach(j => { j.Surname = "Doe"; });
        db.SaveChanges();

        scope.Complete();

        Log.Info("Johns updated successfully.");
    }
    catch
    {
        throw;
    }
    finally
    {
        if (db != null)
            db.Dispose();
    }
}
catch (Exception ex)
{
    Log.Error("Johns updating failed.", ex);
    throw ex;
}
finally
{
    if (scope != null)
        scope.Dispose();

    DateTime endTime = DateTime.UtcNow;
    Log.Info($"Johns update method took { (endTime - startTime).TotalMilliseconds }ms to complete.");
}

@jnsn
Copy link

jnsn commented Sep 21, 2019

I personally have no immediate case for the catch clause, but there have been a few times where a finally would have been useful for logging pursposes.

If you are doing multiple return branches within a using statement, or throwing exceptions, it would be extremely useful if there would be some kind of finally strategy were you could handle log messages.

One of the things I do regularly is throwing HTTP status code exceptions which are handled by my middleware to return the correct status code to the user. Granted this could be done in a different way, but it just makes for a nice clean piece of code.

_logger.LogInformation("starting block");

using (var a = new A())
{
    var result = a.DoSomething();
    if (result == 404)
    {
        throw new HttpNotFoundException();
    }
    else if (result == 400)
    {
        throw new HttpBadRequestException(result.ErrorMessage);
    }

    // do something else.
}
finally
{
    _logger.LogInformation("completed block");
}

@CyrusNajmabadi
Copy link
Member

but it just makes for a nice clean piece of code.

That seems to only be saving a single token. i.e. i could write that today as:

using (var a = new A())
try
{
   // stuff
    // do something else.
}
finally
{
    _logger.LogInformation("completed block");
}

being able to trim off the try for this super specialized case just seems like far too little gain to warrant doing it...

I also far prefer this approach because i totally understand the finally semantics. i.e. it will happen before Dispose is called on a. When it's just using () {} finally {} it's totally unclear to me what that means wrt the Dispose. does it happen before/after? Are the finally's merged? How does throwing in one affect the other, etc. etc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests