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

Yet another try expression proposal #980

Closed
gulshan opened this issue Oct 7, 2017 · 36 comments
Closed

Yet another try expression proposal #980

gulshan opened this issue Oct 7, 2017 · 36 comments

Comments

@gulshan
Copy link

gulshan commented Oct 7, 2017

It seems recently try-... expression issue came into discussion again. I have some new ideas regarding this. So, this "yet another" try expression proposal. Please bear with me.

This is actually a two(or three) layered proposal, where each layer is built on the previous one.

First layer: Support catch blocks with try expression if it returns or throws.
The idea is, with try expression, support catch blocks which eventually ends the current method with return or throw. It will look something like-

// This current code
public int GetResult(int input)
{
    try
    {
        var x = DoSomethingExceptional(input);
        var result = DoFurtherProcessing(x);
        return result;
    }
    catch (Exception e)
    {
        Logger.log("GetValue input : " + input + " Exception: " + e);
        return 0;
    }
}


// can be translated to-
public int GetResult(int input)
{
    var x = try DoSomethingExceptional(input)
            catch (Exception e)
            {
                Logger.log("GetValue input : " + input + " Exception: " + e);
                return 0;
            }
    var result = DoFurtherProcessing(x);
    return result;
}


// a more accurate representation of this code in currently supported
// form which is not normally used will be-
public int GetResult(int input)
{
    int x = null;  // type has to be known and a default value assigned
    try
    {
        x = DoSomethingExceptional(input);
    }
    catch (Exception e)
    {
        Logger.log("GetValue input : " + input + " Exception: " + e);
        return 0;
    }
    var result = DoFurtherProcessing(x);
    return result;
}

And same goes for throws. The first layer ends here.

Second layer: Support non-returning and non-throwing catch blocks with certain regulations.
Supporting non-ending catch blocks comes with two problems. First is, what will be the value of the expression in the case of exception. Second is, wouldn't this enable exception-swallowing anti patterns?

I think there can be three values an expression will evaluated to in case of exceptions-

  • null
  • default
  • let the programmer decide with some new syntax

I tend to like the null option most as it denotes absence of a value and also enables ?? operator if someone likes to put a default value for fail cases.

Now let's address the big question. This will enable to swallow exceptions, which is an anti-pattern. I think, simply swallowing an specific type of exception is not as bad as swallowing ALL exception. So, lets make (non-method ending)all-swallowing catch blocks like
catch {...} or catch (Exception e) {...}
illegal in try expression. One cannot simply swallow all. The code will show exactly what it is swallowing, which is better and enough IMHO. Is this restriction enough for you?

Third layer: Simplify catches
If all I want to do is to catch and swallow some specific types of exception without doing anything else and get nulls, why not make it easier? Make the parens and "block" part of a catch clause optional and let multiple types of exceptions be combined with | or ,. This will enable to code like-

int? result = try Int32.Parse(str) catch ArgumentNullException|FormatException
// or with commas
int? result = try Int32.Parse(str) catch ArgumentNullException, FormatException

So, a sample try-catch expression demonstrating different parts of this proposal is-

var result = try SomeOperation()
                catch FooException ?? 0
                catch BarException|BazException ?? -1;

Parent issues: dotnet/roslyn#16072 #220
Recent discussion: https://github.com/dotnet/corefx/issues/19928
cc: @jnm2 @HaloFour @KrzysztofCwalina @jaredpar

@iam3yal
Copy link
Contributor

iam3yal commented Oct 7, 2017

@gulshan The following code:

// This current code
public int GetResult(int input)
{
    try
    {
        var x = DoSomethingExceptional(input);
        var result = DoFurtherProcessing(x);
        return result;
    }
    catch (Exception e)
    {
        Logger.log("GetValue input : " + input + " Exception: " + e);
        return 0;
    }
}

is in my opinion more readable than the following:

// can be translated to-
public int GetResult(int input)
{
    var x = try DoSomethingExceptional(input) catch (Exception e)
                {
                    Logger.log("GetValue input : " + input + " Exception: " + e);
                    return 0;
                }
    var result = DoFurtherProcessing(x);
    return result;
}

I don't understand all these exception handling proposals, especially, the ones that propose to make it easier to swallow exceptions, what's the real benefit here? besides make the syntax terser and "cuter" which conveys the opposite of what many experienced C# devs would consider an anti-pattern.

Now let's address the big question. This will enable to swallow exceptions, which is an anti-pattern. I think, simply swallowing an specific type of exception is not as bad as swallowing ALL exception. So, lets make (non-method ending)all-swallowing catch blocks like
catch {...} or catch (Exception e) {...}
illegal in try expression. One cannot simply swallow all.

The anti-pattern is about swallowing exceptions not about what you swallow so I'm not sure why swallowing ApplicationException, SystemException would be fine whereas swallowing Exception wouldn't.

In some circumstances it's fine to swallow exceptions and I don't think that educating developers by preventing them what's available through another syntax is the way to go, especially when this is a syntactic sugar and nothing more.

Finally, if we're going to have a terser syntax I'd rather have what @CyrusNajmabadi suggested and incorporate it into match:

var v = DoSomething() match (
    case SomeType t: Something(),
    case OtherType x: Whatever(),
    catch (Exception e): AndSoOn());

For anything more complex, you can always fall back to the "awesome" syntax we have today.

Again, just my opinion about it.

@bondsbw
Copy link

bondsbw commented Oct 8, 2017

I get wanting terser syntax. But I'd prefer to have language updates that support functional error handling, similar to F#'s Result<'TSuccess,'TError> discriminated union type.

@gulshan
Copy link
Author

gulshan commented Oct 8, 2017

@eyalsk I would not create this issue unless the discussion happened in this thread- https://github.com/dotnet/corefx/issues/19928
The try* methods are a common pattern in C#. And to make the pattern part of language while keeping it safe is something many people want. Especially this comment https://github.com/dotnet/corefx/issues/19928#issuecomment-331920196 says people in the dotnet core team still considering something like try-else expression found in swift. Then after seeing this comment by @jnm2 https://github.com/dotnet/corefx/issues/19928#issuecomment-331920196 the idea of conditional swallowing of specific exceptions came into my head. And I wanted to share, because I think it is something new. I want to know more about @CyrusNajmabadi 's proposal. May be with some demonstration how it make the code easier. Any link? I have tried to elaborate my example with another portion. This style will lit up when there are nested try blocks now. I want to disallow Exception because all and any exception is subclass of it. So, when people use it, they want to catch all/any exception that occurred. This is not the case for other exception classes.

@bondsbw C# is so much so into the try-catch style error-handling, it will be very difficult to introduce another way of doing IMHO.

@orthoxerox
Copy link

@bondsbw you can use a library for that. I know language-ext supports that feature out of the box, not sure about SuccincT.

@iam3yal
Copy link
Contributor

iam3yal commented Oct 8, 2017

@gulshan

I would not create this issue unless the discussion happened in this thread- dotnet/corefx#19928
The try* methods are a common pattern in C#. And to make the pattern part of language while keeping it safe is something many people want.

Fair enough, the issue though is the proposal on CoreFX is an API and there's already existing code that using the Tester-Doer Pattern all over the framework, do you expect them to change these APIs and mark them obsolete or do you expect consumer to change their code? in my opinion you should address that in your proposal too.

I want to know more about @CyrusNajmabadi 's proposal.

It wasn't a proposal, it was comment. :D

I want to disallow Exception because all and any exception is subclass of it. So, when people use it, they want to catch all/any exception that occurred. This is not the case for other exception classes.

It is and I already provided at least two more, ApplicationException, SystemException and these probably aren't the only ones.

Now, I've seen codebases where people swallowing exceptions for the wrong reasons plenty of times and a developer once told me that he hates handling exceptions so he swallows them so I offered different options but the answers I got were completely detached from reality such as "It's too much code where I can just swallow it" or "it's just easier to swallow it than deal with it" even when I pointed out that it's even shorter not to swallow it and then you get answers like "yes but the application will crash" so you stop there and realize that the developer is well, unfortunately, clueless and so when reason doesn't matter to some developers I wouldn't want the language to make this any easier and at the same time I wouldn't want the language to educate me about what I swallow when I need it and there's a good reason to do it.

This being said swallow Exception is probably not a good idea as you pointed out but I don't think the language is the place to educate developers about it.

Just a silly example but what if someone relies on some 3rd-party library where this library throw instances of Exception and some consumer relies on it? you wouldn't think it exists in the wild but it does.

@jnm2
Copy link
Contributor

jnm2 commented Oct 9, 2017

@eyalsk

This being said swallow Exception is probably not a good idea as you pointed out but I don't think the language is the place to educate developers about it.

Just a silly example but what if someone relies on some 3rd-party library where this library throw instances of Exception and some consumer relies on it? you wouldn't think it exists in the wild but it does.

Good points. We definitely need code quality analyzers though to warn on catch and catch (Exception) without throw or when. Wish stuff like this shipped with the SDK. It's a huge education issue.

@yaakov-h
Copy link
Member

yaakov-h commented Oct 9, 2017

Warning Waves? 😄

@jnm2
Copy link
Contributor

jnm2 commented Oct 9, 2017

Overall, I want something like this. try is one of those statements that seems right to have an expression form and people will continue asking for it.

I'd prefer that you have to be explicit about returning a default value, but this can be pretty nice because we can allow null without an explicit cast just like the proposed condition ? 42 : null would allow inferring int? for the ternary.

Adding catch Type t when to the proposed match syntax, alongside case Type t when is almost too cute not to use, but it could easy and more clearly just be composed together via try match so long as the catches all come at the end.

var x = try GetInt() catch FooException: null, catch BarException ex: ex.Number;

var x = try match GetObj()
    case int n: n,
    default: 0,
    catch FooException: null,
    catch BarException ex: ex.Number;

Also what about finally to execute a void expression or statement block?

BeginX();
return try DoOperation() finally EndX();

Meh?

@jnm2
Copy link
Contributor

jnm2 commented Oct 9, 2017

Of course, the problem with composing try match is that the try applies to every branch of the entire match expression. Presumably we'd usually want this:

var x = match try GetObj()
    catch FooException: null,
    catch BarException ex: ex.Number,
    case int n: n,
    default: 0;

Which is starting to feel awkward and yet is quite expressive in its way of what's going on.

Or different whitespace:

var x = match
    try GetObj()
        catch FooException: null,
        catch BarException ex: ex.Number,
    case int n: n,
    default: 0;

@jnm2
Copy link
Contributor

jnm2 commented Oct 9, 2017

Composing either way also has the issue that either the catches are applied to the cases or the cases are applied to the catches. You can't have everything apply only to the expression unless you include catch in the single non-try match branching as a peer of case, with no composing of nested expressions.

I guess I do find that language mechanic the most appealing right now.

@gulshan
Copy link
Author

gulshan commented Oct 10, 2017

@jnm2 Using when will be an welcome addition to this proposed syntax. And came into my mind again, evaluating to null in case of exceptions let us use ?? operator for explicitly assigning a default value, although not separately for each catch. Altogether this can look something like-

var result = try SomeOperation()
                catch FooException
                catch (BarException e) when (e.ErrorCode == 0x1234)
                ?? 0;

As it was a totally new syntax, I took the liberty of making blanket catch-alls (catch {...} or catch (Exception e ) {...}) illegal/error. On a second thought after the mention of warning waves by @yaakov-h , I think the behavior can be consistent with the normal try-catch syntax in default case. But when a stricter case is enabled by warning waves, both current and new catch blocks can show warnings(not error) in case of non-method-ending (non-throwing and non-returning) blanket catch-all clauses. And as SystemException is mentioned with Exception in this official docs for not catching-
https://docs.microsoft.com/en-us/dotnet/standard/design-guidelines/using-standard-exception-types#exception-and-systemexception
SystemException can also be included in the warning with base Exception class.

I don't see match yet in the .net horizon. So, depending on it for a feature proposal is not a good idea in my opinion.

@HaloFour
Copy link
Contributor

@gulshan

I don't see match yet in the .net horizon. So, depending on it for a feature proposal is not a good idea in my opinion.

An expression version of switch has been mentioned quite a few times and is in the pattern matching proposal, which is something that the team has mentioned returning to in the C# 7.3 timeframe. Even if try expressions were championed I'd expect match to happen first.

@gulshan
Copy link
Author

gulshan commented Oct 10, 2017

Actually using ?? operator to assign separate default values for different cases should work with catch-to-null conversion-

var result = try SomeOperation()
                catch FooException ?? 0
                catch BarException ?? 1
                catch BazException ?? 2;

// This should be evaluated like- 

var result = (
                (
                    (try SomeOperation()
                    catch FooException) ?? 0
                catch BarException) ?? 1  // Exception occurred, escaped FooException catch
            catch BazException) ?? 2;     // Exception occurred, escaped previous catches

With this even fall-through will be ok-

var result = try SomeOperation()
                catch FooException 
                catch BarException ?? 0   // value for both FooException and BarException
                catch BazException ?? -1;

@HaloFour I've probably seen the match proposal embedded in pattern matching proposal. I think it should be a separate proposal now. Still try expression can be independent of that.

@jnm2
Copy link
Contributor

jnm2 commented Oct 10, 2017

@gulshan

var result = (
                (
                    (try SomeOperation()
                    catch FooException) ?? 0
                catch BarException) ?? 1  // Exception occurred, escaped FooException catch
            catch BazException) ?? 2;     // Exception occurred, escaped previous catches

I don't like this because if the 0 was an expression that throws, the exception would be caught by catch BarException and catch BazException. And if 1 was an expression that throws, the exception would be caught by catch BazException. Contrast this with the try...catch statement where exceptions are only caught in the try section; no catch block catches exceptions out of a previous catch block.

@DavesApps
Copy link

@gulshan take a look at #965, more terse and different syntax but also a proposal to be able to avoid Try type (i.e. TryParse) and improve performance in cases of exceptions for methods you don't want to throw exceptions.

@HaloFour
Copy link
Contributor

@DavesApps

Any syntax such as this will be inherently much slower than specialized Try methods in that the latter can often avoid exceptions entirely. A well-written Try method does not just wrap whatever logic in a try/catch and return false whenever an exception occurs, it bails as early as it can determine that whatever action cannot be successful.

@jnm2
Copy link
Contributor

jnm2 commented Oct 10, 2017

Yes! Please do not confuse the Try pattern with catching exceptions.

@gulshan
Copy link
Author

gulshan commented Oct 10, 2017

@jnm2 I think that depends on the implementation. I don't think exception thrown by an expression in place of 0 should be caught by sub-sequent catches as it is outside of the try scope.

@DavesApps
Copy link

@HaloFour the proposal #965 offers a suggestion that the implementation could be instead of throwing exceptions when a function is wrapped by trycatch it would be recompiled to RETURN the exception rather than throwing it, to avoid the performance overhead of the exception. So basically it does rewrite a method to some extent. It won't be as good as a custom one but in theory during recompile of a method it could disable finalization options for the exception which should significantly improve the performance of the create and clean up.

@HaloFour
Copy link
Contributor

@DavesApps

You're suggesting that by wrapping an arbitrary method that it will be decompiled, reinterpretted and rewritten to not throw? And this will happen with library methods? How do you propose that work? The compiler certainly has no capacity to affect the nature or code of referenced assemblies which means that you're expecting the JIT to somehow completely rewrite methods at runtime. I can't see that remotely getting off of the ground.

@HaloFour
Copy link
Contributor

@DavesApps

And to expand on that further, many of those Try methods aren't handling the logic themselves. They pass the validation/execution logic elsewhere, often several layers deep. The methods themselves are often not trivial implementations either. The method int.Parse goes about 6 levels deep and at the core is a quite complicated method which relies on unsafe byte buffers and further recursive calls. Exceptions can and are thrown at multiple points in and around that method depending on the options chosen. There's no way that the JIT is going to rewrite that at runtime, and there's nothing that the C# compiler can do to change how that method behaves at compile time.

@DavesApps
Copy link

DavesApps commented Oct 10, 2017

@HaloFour when it's jitted it could just replace return values with the tuple and replace throw with return the tuple. Jit happens on all methods at runtime. It should be a relatively simple replacement/addition of the IL to Jit during generation. (It was just a suggestion at solving this ubiquitous problem that lots of people are asking for all different sorts of methods with Try on it to avoid exceptions... Any other ideas that could solve having to write regular methods and Try methods all the time?) Seems like enough folks run into this once in a while would be nice if we could find a general solution.

@HaloFour
Copy link
Contributor

@DavesApps

Given that literally any method can throw this is asking the JIT to be on standby for rewriting literally every single piece of IL that passes its way. That includes constructors which cannot modify their return values. I don't think that's practical.

It's also not a proper replacement for Try methods which not only specialize their parsing/validation logic but also don't just squelch potential exceptions. int.TryParse can and does still throw exceptions when you pass it invalid arguments, such as an illegal combination of NumberStyles enum values.

@Pxtl
Copy link

Pxtl commented Jul 17, 2019

Obviously, we need a try/catch expression.

The above proposal failed to handle the ambiguity of multiple nested try/catches - in such a case it's impossible to tell which catch corresponds to which try.

Instead, I'm suggesting a syntax that should be pretty idiomatic to C# 8.0 and later. C# 3.0 introduced lambda syntax, C# 7 introduced pattern-matching, and C# 8 introduced switch expressions. Fundametnally a Catch is a pattern-matching operation on the Exception object, so we should reuse the pattern-matching-expression syntax here.

proposal

By toy example first:

var myValue = try => dict[key] catch {KeyNotFoundException ex => null};

as you can see, slightly noisier than a normal Try-Catch, but still terse and clear, and leverages familiar Lambda, Switch expression, and pattern-matching syntaxes.

More elaborate example.

var responseCode = try => doStuff().ReponseCode catch{
    NotFoundException => 404, //don't need ex variable, use immediate value.
    HttpException ex when (ex.Code < 500) => ex.Code, //just pass-through the response-code
    HttpException ex => { //for 500 and up errors, log the error before returning
           logger.Error(ex);
           return ex.Code;
    },
    Exception ex => {  //treat all other errors as 500s.
         logger.Error(ex);
         return 500;
    }
};

so you can see, the pattern is

try => try_expression catch{
    exception_type_1 [exception_var_1_name] [when exception_when_expression_1] => exception_expression_1, 
    exception_type_2 [exception_var_2_name] [when exception_when_expression_2] => exception_expression_2, 
     ...
    exception_type_N [exception_var_N_name] [when exception_when_expression_N] => exception_expression_N
}

where all of exception_expression_N must return a type that is or can be implicit cast into the return type of try_expression, or they must throw.

If an exception expression needs to be broken out into a multi-statement body, then the syntax already established for doing the same for lambdas is used.

Finally is not a thing, because it's really non-expressiony.

This also avoids the ambiguity problems - the lambda-arrows make it unambiguous compared to a normal try...catch (an existing try will never be followed by a lambda arrow). Because all the catch blocks are in a single catch{ ... } braces there is no ambiguity if tehre are multiple nested try => blocks about which catch corresponds to which try.

Using braces to contain multiple (type varname) => expression, delimited with commas, is already idiomatic C# because it's commonly done in pattern-matched switch expressions.

No existing try...catch features such as multiple catch-blocks, when-clauses, etc. are sacrificed other than "finally", and the empty catch{...} which can be emulated with catch{Exception => null} and should never be used any time ever anyways.

@bondsbw
Copy link

bondsbw commented Jul 17, 2019

@Pxtl That aligns pretty well with switch expressions. I don't think the first => would be necessary.

I'm not sure I like it more than adding it to switch expression syntax.

@Pxtl
Copy link

Pxtl commented Jul 17, 2019

@bondsbw yeah, I'm not sure the first => would be necessary, but I know there are others who are dreaming of single-line-try that corresponds to single-line-if, and without that I think it's not sufficiently different from the vanilla "try" syntax so it would be confusing if you got the inline try when you were expecting the block try.

What do you mean by "than adding it to switch expression syntax"? After all, that ship has sailed - switch expression syntax, including arrows, is in C# 8.

@orthoxerox
Copy link

@Pxtl Why do you need try => for this?

@bondsbw
Copy link

bondsbw commented Jul 17, 2019

@Pxtl

After all, that ship has sailed - switch expression syntax, including arrows, is in C# 8.

I don't think it would be a breaking change to add try into switch expressions in a later version.

@Pxtl
Copy link

Pxtl commented Jul 18, 2019

@bondsbw I'm still not following - add try into switch? What would that look like?

var x = switch try (doSomethingThatCouldFail()) {
    Exception ex => logAndReturnNull(ex)
};

?

@orthoxerox
Copy link

@Pxtl more like add catch into switch, with catch working like case:

var x = doSomethingThatCouldFail() switch {
    catch ex => logAndReturnNull(ex),
    case var _x => _x,
};

@gulshan
Copy link
Author

gulshan commented Jul 18, 2019

@orthoxerox Switch expression does not use case.

@bondsbw
Copy link

bondsbw commented Jul 18, 2019

@Pxtl It was covered above in the thread.

It needs to be updated slightly for the decided switch expression syntax... probably what @orthoxerox has plus try minus case.

@gulshan
Copy link
Author

gulshan commented Jul 22, 2019

After finalization of the switch expression, I don't support my proposal in this issue now and want to close it, when discussion here is wound down. I think the proposal by @Pxtl is a good one because of its syntactic alignment with the switch expression and deserves its own proposal/discussion issue. Some minor points from my side regarding it-

  • I think => is not needed after try.
  • The right hand side in the catch patterns should just be expressions and not value returning blocks, in alignment with switch expression. There can be helpers to log an exception and return a value to support this.
  • To prevent swallowing, unlike switch expression, default cases should not be supported in the catch block.

With these the example code given by @Pxtl will look like-

var responseCode = try doStuff().ReponseCode catch
{
    NotFoundException => 404, //don't need ex variable, use immediate value.
    HttpException ex when (ex.Code < 500) => ex.Code, //just pass-through the response-code
    HttpException ex => logger.LogErrorAndReturn(ex, e => e.Code), //for 500 and up errors, log the error before returning
    Exception ex => logger.LogErrorAndReturn(ex, _ => 500),  //treat all other errors as 500s.
};

Regarding putting catches in the switch expression, I think putting a try on a function call with return type T can return a Result<T> type (or more preferably a T | Exception union type according my #399 proposal). Then switch expression can just have another match line like NotFoundException e => default without the phrase catch. But it will be a competing proposal to @Pxtl 's proposal (or my modification of it). It would look something like-

var value = try int.Parse(stringValue) switch
{
    int x => x,
    ArgumentException e => 0,
    FormatException e => 0,
}

@spydacarnage
Copy link

In my current project, I've got an Outcome<T> struct, which is essentially the same as Result<T> in all but name (I prefer the name).

In that, I have a static method:

public static Outcome<T> Try(Func<Outcome<T>> tryFunc, Func<Exception, Outcome<T>> catchFunc)
{
    try
    {
        return tryFunc.Invoke();
    }
    catch (Exception ex)
    {
        return catchFunc(ex);
    }
}

Which is then used as:

Outcome<int>.Try( () => DoSomething(), ex => Outcome<int>.Error() );

It's a little more syntax heavy that just try DoSomething(), but it does work now. You could use switch and pattern matching on the ex parameter of the catchFunc to run different code based on the type of error if needs be, but I normally just log the base Exception.

@Pxtl
Copy link

Pxtl commented Jul 22, 2019

I'd agree with that. So @gulshan , I'm not familiar with the chsarplang process here - do we create a new proposal issue and close this one?

@gulshan
Copy link
Author

gulshan commented Jul 22, 2019

@Pxtl Just create a new issue with your proposal, like you did in the roslyn repo. I'll close this issue later.

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

No branches or pull requests