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 Action<Exception> exception handlers with catch clause #831

Closed
gulshan opened this issue Aug 19, 2017 · 12 comments
Closed

Comments

@gulshan
Copy link

gulshan commented Aug 19, 2017

A catch block in takes in an exception and performs some action on it. This pattern is very much similar to a lambda of the signature Action<Exception>. My proposal is, support lambda exception handler directly with the catch clause. This will look like-

try
{
    throw new Exception();
}
catch ( e => {
    Console.WriteLine(e);
});

But lambdas are not my actual focus here. The intent is to define an exception handling method and reuse it. This brings some convenience of code reuse-

static void HandleException(Exception e) =>
    Console.WriteLine(e);

static void Main(string[] args)
{
    try
    {
        throw new Exception();
    }
    catch (HandleException);
}

The next step will be using overloads to handle different kind of exceptions with separate methods-

static void HandleException(Exception e) =>
    Console.WriteLine(e);

static void HandleException(NullReferenceException e) =>
    Console.WriteLine(e);

static void Main(string[] args)
{
    try
    {
        throw new NullReferenceException();
    }
    catch (HandleException); // Appropriate method overload will be called
}

Previous discussion: dotnet/roslyn#9539

@iam3yal
Copy link
Contributor

iam3yal commented Aug 19, 2017

@gulshan We already discussed something similar back then on the Roslyn repo Declarative approach to exception handling so you may want to see some of the problems with it that @HaloFour and others are pointing out to me.

@gulshan
Copy link
Author

gulshan commented Aug 19, 2017

@eyalsk Thanks for pointing. Did not know about it. For me, the overloading was the sweet-spot in this approach, which was not discussed that much in the previous thread. With this approach, error handlers can even be injected.

Regarding other disadvantages like not having access to the locals and inability to use return, break or continue, I would suggest to just use usual exception handling for those cases. This is not going to replace the current exception handling constructs. And if lambdas are used (which will not be that frequent I guess), access to locals is available.

Another direction this can take, which I have not mentioned in the proposal as it is out of the current scope. It's about try-catch expressions. Which means both try and catch block will be returning some value. Meaning besides Action<Exception>, Func<Exception, T> will also be supported with catch. Which may look like-

var result = try MethodReturningValue()
            catch ( e => { Logger.Log(e); return null; });

In this case, value of result will be null from catch block if MethodReturningValue throws exception. And if someone wants to just swallow the exception (caution: everyone will not be happy...)-

var result = try MethodReturningValue() catch ( e => null );

This may require other things like null having it's own type null and further type inference etc.

@gulshan
Copy link
Author

gulshan commented Aug 19, 2017

And the previous issue was added to backlog by @gafter

@svick
Copy link
Contributor

svick commented Aug 20, 2017

If the goal is reusing exception-handling code, what's wrong with using a method that takes a lambda for the try block? That is, instead of:

static void HandleException(Exception e) =>
    Console.WriteLine(e);

static void HandleException(NullReferenceException e) =>
    Console.WriteLine(e);

static void Main(string[] args)
{
    try
    {
        throw new NullReferenceException();
    }
    catch (HandleException); // Appropriate method overload will be called
}

You would write:

static void Try(Action action)
{
    try
    {
        action();
    }
    catch (NullReferenceException e)
    {
        Console.WriteLine(e);
    }
    catch (Exception e)
    {
        Console.WriteLine(e);
    }
}

static void Main(string[] args)
{
    Try(() => throw new NullReferenceException());
}

No new syntax required.

@gulshan
Copy link
Author

gulshan commented Aug 20, 2017

The workaround seems to provide similar functionalities. But they do have differences-

  1. The proposal didn't touch try block. It remains same. But the workaround uses Action block around code, which means-
    • I loose the ability to use return, break, continue in the try block. (Not the catch block) Async method calls also have issues in this case.
    • There will be two more entry into callstack for each Try call.
    • Memory allocations for Action.
  2. There is no separate method handling each type of exception. So, exception handling parts cannot call each other if needed. Even if user wants to do something extra with an exception in an usual catch block (which is currently supported) and then call the method to do further routine work, it's not possible.
  3. If I do make separate methods for handling each type of exception and call them from the workaround Try method in separate catch blocks, firstly there will be repetitive codes of just calling the same method for each type of exceptions. And then there will be dual point of code insertion point for each new type of exception handling- a new method overload and a new catch block with that specific type. So, the convenience of just adding another overload of the error-handler method is lost.

So, it will just be an workaround and not as useful as the proposal.

@jnm2
Copy link
Contributor

jnm2 commented Aug 20, 2017

If HandleException returns bool it could also act as an exception filter.

Overload resolution can't happen at runtime, only compile time, so I'm not sure how you were thinking the exception would be dispatched by a single catch block. Since I don't see any way for overload resolution to be helpful1, we already have all the other functionality:

bool HandleException(Exception ex) { ... }
void HandleException(IOException ex) { ... }

try { ... }
catch (Exception ex) when (HandleException(ex)) { }
catch (IOException ex) { HandleException(ex); }

1If multiple catch blocks were autogenerated, one per method in the method group, that could make the overload resolution idea work but would also be a significant departure from the current straightforward behavior of C#.

@gulshan
Copy link
Author

gulshan commented Aug 20, 2017

@jnm2 Good Catch! And thanks for the solution to the catch also. 😃
It's the conciseness, ability to reuse code and possibility of enabling new features expanding upon this basic proposed construct makes it look good to me.

@jveselka
Copy link

@jnm2 Well you can always make use of DLR :-) But it is never a good idea to hide it.

static void HandleException(Exception e) => Console.WriteLine(e);
static void HandleException(NullReferenceException e) => Console.WriteLine(e);

static void Main(string[] args)
{
    try
    {
        throw new NullReferenceException();
    }
    catch (HandleException); // Appropriate method overload will be called
}

would mean this:

    catch (Exception ex)
    {
        dynamic dex = ex;
        HandleException(dex);
    }

Considering exception filters - unfortunately they have a great deal of problems as proposed:

  • You can not specify more handlers with different filter conditions for one type as their signatures would clash.
  • With exception filters, ordering matters much more and there is no way for the compiler to infer it for you. So it would probably need an attribute or something.

@jveselka
Copy link

jveselka commented Aug 21, 2017

Another possible approach is by extending using statement instead.

  • It would accept any expression, not just one returning IDisposable. If the expression does NOT return IDisposable it would use following behavior:
  • It would look for public void Dispose() method or extension method and if found call it from finally block (duck typing for Dispose method as it should have done anyway)
  • It would look for any public void Catch(AnyException ex) methods or extension methods and if found generate catch blocks for each of them.
    • This could be breaking change if some existing IDisposable object contains method name Catch, thats why we can have this behavior only for classes NOT implementing IDisposable
public class MyExceptionHandler
{
	public void Catch(Exception e) => Console.WriteLine(e);
	public void Catch(NullReferenceException e) => Console.WriteLine(e);
	public void Dispose() => Debug.WriteLine("Exiting using block");
}
...
static void Main(string[] args)
{
    using (new MyExceptionHandler())
    {
        throw new NullReferenceException();
    }
}

Other benefits are that you can easily carry context into exception handlers and compiler has no problem with staticaly resolving overloads.

What do you think?

@jnm2
Copy link
Contributor

jnm2 commented Aug 21, 2017

@zippec I don't think anyone thinks the DLR is a good idea here :D Your points against the overload resolution are quite good, thankfully, because I don't like that approach. I still don't see why catch (ExceptionType ex) { HandleException(ex); } is unclear or too much typing, but if any C# langauge construct is too much typing, we start by solving the problem by refactoring it into a helper method.

Your exception handler idea is interesting. I'll have to chew on it. I don't dislike it, yet I still don't really see the need.

@iam3yal
Copy link
Contributor

iam3yal commented Aug 21, 2017

@zippec, @jnm2 I think that something like that or similar was proposed back then in the Roslyn repo, could be wrong though hmm what do I think? I don't know how it improves the situation. :)

To be honest, I want to have better ways to do exception handling but then when I think about the amount of times I do it I don't have any desire investing thoughts on it, it's a weird area.

@HaloFour
Copy link
Contributor

We've had a couple of issues opened up for this idea. I just don't see what these "reusable handlers" provide that benefits the language and the developer. They seem to largely encourage two anti-patterns: log-and-throw and log-and-swallow. Anything beyond that is likely to require additional logic that can't be directly encoded in these handlers. And any useful logging handler is going to require information from the current context anyway which these handlers couldn't access.

@gulshan gulshan closed this as completed Oct 8, 2017
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

6 participants