dotnet / csharplang

The official repo for the design of the C# programming language
11.4k stars 1.02k forks source link

Discussion: `try` expression without `catch` for inline use #220

Closed lachbaer closed 7 years ago

lachbaer commented 7 years ago

Proposal: try expression without catch for inline use

Intent

Sometimes it is not necessary to catch an exception, because you can either check side conditions or proceed though the expression is failed. In these cases it would be nice to skip the exception checking.

Example

In current code there would be something like

try {
    var textStream = new StreamReader("C:\nonexistingfile.txt");
    ProcessFile(textStream);
}
catch { }
GoOnWithOtherThings();

or even wrap it additionally with

var fileName = @"C:\notexisting.txt";
if (File.Exists(fileName)) {
   // try block from above
}
GoOnWithOtherThings();

This could be abbrevated and streamlined drastically with

var textStream = try new StreamReader("C:\nonexistingfile.txt");
if (textStream != null) ProcessFile(textStream);
GoOnWithOtherThings();

The catching isn't necessary here, because a null-check (that should be done in ProcessFile() anyway) already cares about the failure. A complete try { } catch { } is just unnecessary boilerplate.

I guess that there are plenty of other useful scenarios where this would come in handy.

Other languages

PHP uses the @ operator before an expression to suppress errors and warnings

$fp = @fsockopen("www.example.com", 80, $errno, $errstr, 30);
if (!$fp) {
    echo "$errstr ($errno)<br />\n";

In PHP @ is used quite often to shorten things.

Now, open for discussion... 😄

orthoxerox commented 7 years ago

I'm sure this change will push people not into the pit of success, but into the cliffs of insanity. There's a reason why "On Error Resume Next" is the most offensive thing you can say to a VB programmer.

lachbaer commented 7 years ago

@orthoxerox In VB On Error Resume Next has been the only way to at least simulate something like a try ... catch, because the code would not have thrown a "Run time error" and one could examile the Err.Number by hand.

Nevertheless I understand your concerns, but I don't think that it will have too much impact. As with so many company conventions, there could be one that disallows usage of a try expression when not thorougly documented why it is used.

For all others this would much probably be an ease!

DavidArno commented 7 years ago

In PHP @ is used quite often to shorten things.

PHP, the good parts

lachbaer commented 7 years ago

And by the way, actually it wouldn't be much different than using TryParse() - a framework supported feature. Other question: haven't you too already seen used code with empty catch blocks anywhere? If not, your bad 😉

DavidArno commented 7 years ago
public static StreamReader TryNewStreamReader(string filePath)
{
    try 
    {
        return new StreamReader("C:\nonexistingfile.txt");
    }
    catch { }

    return null;
}
...
var textStream = TryNewStreamReader("C:\nonexistingfile.txt");
if (textStream != null) ProcessFile(textStream);
GoOnWithOtherThings();
lachbaer commented 7 years ago

@DavidArno are you kidding me....?!?!?!?! 😕

var textStream = TryNewStreamReader("C:\nonexistingfile.txt");
// compare to
// var textSTream = try new StreamReader("C:\nonexistingfile.txt");
if (textStream != null) ProcessFile(textStream);
GoOnWithOtherThings();

The complete "TryNewStreamReader()" is so something of spoiling overhead! And besides you used an empty catch block in it. Exactly what this proposal is going to avoid! ;confused; ;confused; ;confused; 😕

DavidArno commented 7 years ago

How do you propose avoiding the empty catch block? I simply copied your example code.

lachbaer commented 7 years ago

@DavidArno

How do you propose avoiding the empty catch block? I simply copied your example code.

That's the whole point of this proposal.

var textStream = try new StreamReader("C:\nonexistingfile.txt");

is all that is needed. It is semantically equivalent to

try {
    var textStream = new StreamReader("C:\nonexistingfile.txt");
 } 
catch { }

but without the boilerplate around it.

DavidArno commented 7 years ago

catch { } is evil. There are times when it might make sense to use it after careful analysis of the consequences. Having the compiler encourage its casual use through a syntactic shortcut though, would be a seriously bad thing to do.

lachbaer commented 7 years ago

@DavidArno I like to look at things without any prejudice or preassumptions and from different angles from time to time.

During my early days I have teached beginner programming classes. That was around the time of C# 1.0 (the classes were for Pascal though). That again teached me to "dive into the thread of thoughts" of others, to see where their problem of understanding is or why they wrote the code as it was when it came to debugging.

From a theoretical and operational point of view, I totally agree, that every exception should be caught and thought of wisely. From a practical and more humal like POV in my eyes it isn't that bad to omit exceptions.

It's due to the vast number of exceptions that could throw anytime. Non-experienced programmers could easily be discouraged, because it takes too much time to respect them here and there. And all this try and catch clauses with all the braces and indenting makes the code harder to follow, also working against people who already have a hard time to concentrate of the essential algorithms.

When you - without prejudice! - look at some code ( I don't mean enterprise class code, just everyday laying around code ) you will most certainly find quite a lot of catching that actually isn't really necessary, or only exists to catch the annoying occurences of exceptions. In my eyes a simple try expression as proposed here would extremely enhance that already existing code.

By the way, in this forum are mainly enthusiasts who might have a more professional look on things. Please respect that there are others on this planet who love C# and program for fun, but find this or that annoying and improvable. (And professionals will probably ignore features like this proposed one anyway - and if not, then there might be a use for it, indeed 😉 )

FredyFerrari commented 7 years ago

I just don't think that a language should help programmers to write bad code but guide them to write proper code. And, in my opinion, a catch all clause is always dangerous and therefore should not be something that the compiler should insert automatically.

lachbaer commented 7 years ago

@FredyFerrari Please look it at from a diffrent angle! You could also see it this way:

By now the language encourages to write catch all clauses, that also look ugly, leads to additional braced blocks and indention:

try {
    var textStream = new StreamReader("C:\nonexistingfile.txt");
 } 
catch { }

When do I write that kind of code? => When I'm too lazy to catch the exception right now, because I concentrate on the actual work of the module. Maybe I come back later to care about exceptions. ( 'I' is not ment personally )

So all I have done is to insert that try / catch intentionally - already.

Inlining that very try would not have me encouraged to write bad code - I would have written that bad catch-all-code anyway. It now just looks tighter and cleaner with respect to the reading flow.

The IDE can always transform it to a full try-catch-block (e.g. one-way only) and help me finding inlined try statements. Even better! Inlined try statements are syntactically easier to find for code enhancers and -reporters and thus can hint you where you should further inspect your exception handling, isn't it?

FredyFerrari commented 7 years ago

But allowing this would make people think, var textStream = try new StreamReader("C:\nonexistingfile.txt"); is a proper way to write code and not make them aware that this catches all exceptions.

lachbaer commented 7 years ago

@FredyFerrari But do you admit that

var textStream = try new StreamReader("C:\nonexistingfile.txt");

looks better than

var textStream = try { new StreamReader("C:\nonexistingfile.txt"); } catch { }

? I myself do not like the extensive use of curly braces, e.g. like here (besides it wouldn't compile).

iam3yal commented 7 years ago

@lachbaer

And by the way, actually it wouldn't be much different than using TryParse() - a framework supported feature.

Point of the Tester-Doer pattern is to avoid the exception not to swallow it, so it's very different.

lachbaer commented 7 years ago

Well, how about this:

    var textStream = (
        (Func<StreamReader>)
        delegate () {
            try { return new StreamReader("C:\nonexistingfile.txt"); } catch { return null; }
        })();

vs.

var textStream = try new StreamReader("C:\nonexistingfile.txt");

:joy: :joy: :joy: :joy:

DavidArno commented 7 years ago

@lachbaer,

By now the language encourages to write catch all clauses

You are right, and it's a historic flaw in the language. It's easier to write a bad catch-all than it is to eg write:

try
{
    var x = new StreamReader("C:\nonexistingfile.txt");
    ...
}
catch (Exception e) when (e is ArgumentException ||
                          e is FileNotFoundException ||
                          e is DirectoryNotFoundException ||
                          e is IOException) {}

There's nothing we can do about that historic problem. However, making it even easier to write catch-all's, as per this proposal, is just digging that "pit of failure" hole even deeper.

iam3yal commented 7 years ago

@lachbaer There! you even get the PHP syntax you always wanted.

public static class Utilities
{
    public static T @try<T>(Func<T> construct) where T : class
    {
        try
        {
            return construct();
        }
        catch
        {
            return default(T);
        }
    }
}

All you need to do is put using static Utilities at the top. 😆

lachbaer commented 7 years ago

Exception handling can be more performant than test-doing https://www.dotnetperls.com/tester-doer

And image the code on that page with the proposed try statement...

gulshan commented 7 years ago

I would rather support a try-catch expression, which returns a value if no exception, but on exception executes the catch block and do not proceed after that. In void returning methods, it just returns after executing catch block. For other methods, it's an error not returning within the catch block. Similar to proposed guard syntax. For example-

try {
    var textStream = new StreamReader("C:\nonexistingfile.txt");
    ProcessFile(textStream);
}
catch (Exception e) {  Logger.Log(e); }
GoOnWithOtherThings();

becomes

var textStream = try new StreamReader("C:\nonexistingfile.txt")
                   catch (Exception e) { Logger.Log(e); }
// textStream is definitely defined at this point
// and no exception thrown in the try expression evaluation
ProcessFile(textStream);

// In case of exceptions, this method will not be executed.
GoOnWithOtherThings();

But if you want to swallow, you have to do it explicitly-

var textStream = try new StreamReader("C:\nonexistingfile.txt") catch { return null; }
ProcessFile(textStream);
iam3yal commented 7 years ago

@gulshan You should make a new proposal about it as it is divergent.

lachbaer commented 7 years ago

However, making it even easier to write catch-all's, as per this proposal, is just digging that "pit of failure" hole even deeper.

I completely disagree! As you say, it is already there. I think I get you right if you would have it closed completely, but that will most likely never be the case.

Please think again about what I wrote concerning the help of code analyzers. When people start using a try statement instead of a catch-all block these occurences could be easily highlighted and complained by an analyzer. They will write those catch-all's anyway! So, in this view it even encourages to inspect those places and to refactor to meaningful catches.

It also enhances writability, because by using the try statement you can concentrate on the main issue, but also marking places that you gonna have to inspect later.

lachbaer commented 7 years ago
var textStream = try new StreamReader("C:\nonexistingfile.txt") catch { return null; }

Actually, I think the main purpose for my suggestion of a try-statement is for assignments. So, I could go with his. However, catch { return null; } again is too much boilerplate to me.

As this is for returning it could be

var textStream = try new StreamReader("C:\nonexistingfile.txt")
                 catch => null;

@gulshan If you make a new proposal, I will close this one in favor of yours.

-- Addendum -- I'd rather have the try being an expression body, because that implies a possible return value. The catch shouldn't return anything.

var textStream = try => new StreamReader("C:\nonexistingfile.txt");
                 catch { /* ordinary catch block */ };

Or if you prefer a no-catch one-liner

var textStream = try => new StreamReader("C:\nonexistingfile.txt"); catch {}

would translate to

StreamReader textStream = null;
try {
    textStream = new StreamReader("C:\nonexistingfile.txt");
}
catch { /* ordinary catch block */ };

In case you just need catch to return anything else:

var textStream = ( try => new StreamReader("C:\nonexistingfile.txt"); catch { } )
                       ?? new StreamReader("C:\existingfile.txt");
DavidArno commented 7 years ago

@lachbaer,

Since @eyalsk has provided a simple function that you can include in any of your code, that offers almost exactly the syntax you want:

var textStream = @try(() => new StreamReader("C:\nonexistingfile.txt"));

I don't really see the point of @gulshan creating a new proposal.

lachbaer commented 7 years ago

@DavidArno The point is to make that common pattern available in the language for everybody, not only for those who know the function. I thought that's obvious. 🙁

lachbaer commented 7 years ago

@DavidArno Besides a "try-catch-expression" goes far beyond that one written function.

jnm2 commented 7 years ago

-1. I have witnessed first-hand the horrors of catch-all blocks. Unless you are a message loop or threading framework, you have no business swallowing exceptions that don't belong to you. (And if you are, you re-throw them.) It's both dangerous- by far the safest action for any business is for the program to immediately crash on an unexpected exception rather than run in an unknown state- and frustrating because other code swallowing makes it impossible for you to handle the exception higher up the call stack, even if the exception is your responsibility to know about and handle!

When this happens in real life, you gain more perspective than simply the fallacy that it's okay to swallow all exceptions.

HaloFour commented 7 years ago
On Error Resume Next
lachbaer commented 7 years ago

@vbcodec The intent is to reduce boilerplate code, behind the scenes exceptions are still caught if they occur.

HaloFour commented 7 years ago

catch { } should be the very antithesis of boilerplate code.

JasonBock commented 7 years ago

My $0.02.

Don't. Do. This.

Having people write code with a try block around the entire method body with an empty catch block is a really, really, bad idea. Allowing people to just have try is even worse.

If this is implemented in C# I will yell from any soapbox I can find to tell C# devs to never, ever use this :)

lachbaer commented 7 years ago

@HaloFour @JasonBock @jnm2 The main issue for this proposal is not to seduce people to use On Error Resume Next.

Don't. Do. This. - okay, agreed! ; but it is done! Frequently!

Maybe not by you, you and doubleyou, but by others... And that's the point!

I want to quote again, what I have already written above:

Please think again about what I wrote concerning the help of code analyzers. When people start using a try statement instead of a catch-all block these occurences could be easily highlighted and complained by an analyzer. They will write those catch-all's anyway! So, in this view it even encourages to inspect those places and to refactor to meaningful catches.

It also enhances writability, because by using the try statement you can concentrate on the main issue, but also marking places that you gonna have to inspect later.

It is a completely different approach on things that do happen!

Do you know the TV series Mayday (aka Air Crash Investigation)? I'm a leisure pilot myself and a huge part of learning and teaching lies on human factors. Among other things accidents do happen because of the way human people behave, even if they learned not.to.do.this. The solution to this is to find ways that can help or lead them to the right decisions.

For this topic one could be to prohibit the use of catch { } or catch (Excpetion e) { }, but that doesn't seem to be possible. So an easy help could be to offer a syntax that does essentially the same, but gives helping hands from the IDE and compiler that the work isn't already finished. The proposed syntax would do this.

Again, please have a look from the other side of the coin and don't keep your opinionated POV for a second.

yaakov-h commented 7 years ago

So an easy help could be to offer a syntax that does essentially the same, but gives helping hands from the IDE and compiler that the work isn't already finished. The proposed syntax would do this.

An Roslyn Analyzer of custom FxCop rule can achieve this - in fact, CA1031 already does exactly this.

Also, catch { } does have it's legitimate uses, e.g.:

public IDisposableThing CreateDisposableThing()
{
    IDisposableThing x = new DisposableThing();
    try
    {
        x.SomeOperationThatMayThrowAnException();
    }
    catch
    {
        x.Dispose();
        throw;
    }
    return x;
}
lachbaer commented 7 years ago

@yaakov-h Why does nobody understand me 😭 (sobbing)...

I am not a beginner, just an enthusiast, and I didn't know about FxCop! That's the whole point!!!!!! This proposal is not for programming pros who would actually never, never, never do a catch { }. It is for John and Jane Doe Programmers...

yaakov-h commented 7 years ago

So you're suggesting that we add language syntax to make it easier for newbie programmers - who don't necessarily understand what they're doing and who definitely don't understand the full ramifications - to write the kind of code that experts agree is an awful idea to begin with and should never be done? 😕

lachbaer commented 7 years ago

@yaakov-h YES - and no...

Just for one second clean your mind and imagine yourself being a non-professional C# programmer who know all the do's and dont's:

  1. Are you going to write catch { } blocks, because all the exceptions annoy you? Most certainly yes!
  2. As long as you are fine with it, will you continue to do so? Again yes.
  3. Will there be quite a lot of code with catch-alls, because until now you're still fine? Oh, yes!
  4. Will you be annoyed of writing so much try { ... } catch { } "boilerplate" with all that indention making your code weary to read, when you already have a hard time to follow your logic? Probably yes.
  5. After your first experiences that catch-all is a bad idea, are you gonna inspect all empty catches that are sill there? Probably not all, but some.
  6. You're still an amateur and do not know about all other tools out there (like e.g. FxCop). Would it be of help to simply search for try and immediately see that this is an inline-try that you sould inspect further? It would for me...

Again, it is a different approch to have a view on these things. Please take a minute or two, put yourself in that situation and ask yourself those questions.

Be honest, isn't there quite a lot of truth in the points...?

yaakov-h commented 7 years ago

There are a lot of implicit assumptions in there to unpack, including that for some reason my code is throwing a lot of exceptions, but by ignoring them with empty catch blocks my program still works well enough to keep going... wtf? I've never seen that happen at the sort of scale you're describing.

lachbaer commented 7 years ago

Just talking about me - and it happens to be that I known I'm just (the) average - while learning .NET I had quite a lot of exceptions thrown. It went that far that I had quite a bad time and I couldn't concentrate on the essential things. Sometimes I simply "switched off" the exceptions ("On Error Resume Next") and went to the C-style checking, if my variables are assigned. Just to have it more comfortable to run-time trace the code and not being bothered by all of those exceptions. That was (partly is) during the learning phase. I would have loved to have something like a try statement! - just for those very moments!

For my little production code don't have that many unmeaningful try blocks, as I also know about the good rules of catching (maybe that wasn't clear already).

HaloFour commented 7 years ago

@lachbaer

Ignoring exceptions is bad practice, period. That's something novice programmers will have to learn. The language makes ignoring them difficult, on purpose. If the language were to add a feature like this it would be the C# team officially enshrining that ignoring exceptions is not only acceptable but also that it's a good practice. It's not, plain and simple.

Stating over and over again that bad/novice programmers write catch { } all the time doesn't sway me in the least. Such programmers do a lot of really horrific stuff and none of that should find its way built-in to any reasonable programming language.

gulshan commented 7 years ago

C# does not have checked exceptions. So, as a novice, you do not have to use try-catch everywhere at first. And when an exception do occur, rather than just putting the error generating code within a try-catch block, one should find out the cause of the exception. And then one realizes, in some cases, try-catch is necessary. And in those cases, exceptions do have to be handled properly. Empty catches are not a good answer anyway. An exception should never be just ignored.

I did not proceed with my proposal because I want to wait for Non-nullable reference types and method contract to roll out at first. These features are almost sure to come and will significantly change error handling concepts.

And if there is a case of "ignorable" errors/exceptions API provider should provide a Try... method, which will return a success/failure value. If something else should be returned, tuples are there.

iam3yal commented 7 years ago

@lachbaer

Again, it is a different approch to have a view on these things. Please take a minute or two, put yourself in that situation and ask yourself those questions.

I was a beginner once too, we all were at some point and spent much more than a min or two being there so it's not like we don't understand, in fact, because we understand the consequences and the impact we discourage it.

I don't think that C# needs to be designed and provide features that fits the experience of the programmer and say it does, then I'd rather have features that target professionals, aka, people that actually understand what they are doing.

Humor: In the favour of empty parenthesis I vote for the compiler to force you to write a comment, the comment would be evaluated and finally if the compiler would find it unreasonable it would throw an error. 😆

JasonBock commented 7 years ago

@lachbaer "Don't. Do. This. - okay, agreed! ; but it is done! Frequently!" And your point? Just because it's done doesn't make it right.

"Again, please have a look from the other side of the coin and don't keep your opinionated POV for a second." I have. I used to do catch { }. I learned how bad it is, and that it should be avoided if at all possible. I don't do it anymore, and I don't want C# to encourage such a bad practice with syntax.

lachbaer commented 7 years ago

If there is so much consent to never ever use catch { } wouldn't it be probably be okay to deprecate it in C# 7.1 (with warning) and finnaly remove it in C# 8 (with error)?

Even if basically C# should be backwards compatible to the max, wouldn't this issue reason for a breaking change? PHP has changed features from major version to major version - sometimes breaking too much IMO. But in essence it worked for PHP and we know that there are soooo many PHP based products on the market.

C# could introduce a compiler switch, that would allow a backwards compatibility for catch { }. And at least there is always catch (Exception ex) { } 😉

(PS: I know this could be another proposal, but it fits in this discussion at this point.)

svick commented 7 years ago

@lachbaer What I see is consensus that catch { } is a bad practice and should not be encouraged (which is what your proposal effectively does), not necessarily that it should never be used.

Also, the C# team is extremely averse to breaking changes, they're not even willing to add new warnings without making them disabled by default in existing projects (see https://github.com/dotnet/roslyn/issues/1580), so I seriously doubt they would consider deprecating catch { }.

JasonBock commented 7 years ago

@lachbaer I don't think anyone, including me, said to "never" use it. In fact I said, "it should be avoided if at all possible". I never use it as a default way to do exception handling. There are cases (as some have mentioned in this thread) where you have to do this. For example, exception filters for WebAPI - something has to catch the general exception type so you can get that exception in your filter (so you can log it, for example) and then remove all the try { } catch { } block from every API method in your controller. Also, as @svick mentioned, removing catch { } altogether would be a breaking change, and IIRC C# would still have to emit a catch block as IL requires you to have one for any try block.

I'd much rather see pattern matching get amped up in C# beyond what will be in version 7 so we can get toward handling errors in a cleaner way, rather than relying upon exceptions or getting this proposal implemented. Look at Rust's match for a good example of this. F# has a similar syntax.

jnm2 commented 7 years ago

catch { } and catch (Exception ex) { } are equally dangerous unless you unconditionally throw; within the catch block. The only exception to this is if you are matching very specific exception types; all exception types you don't own must be thrown.

A top-level catch-all logging handler is fine. A top-level exception handler is one you hook up to AppDomain.UnhandledException, Application.ThreadException/Application.DispatcherUnhandledException, or around the bare threadstart if you start your own thread via Thread.Start. This allows you to log the details and provide UI to the user warning them that the state of the application is corrupted to an unknown extent and that they should restart the program as soon as possible. For a website, you'd show a 500 and log the error and fire off some notifications.

But none of these crucial things can happen if you catch all exceptions (or any exceptions that aren't in the domain of the code you're writing at the moment). State corruption is a real thing I've seen happen. It can have permanent side effects on the real world, aka databases, aka customer's feelings. The safest action is to log and abort.

JasonBock commented 7 years ago

I just realized I said something wrong in my last post. You don't have to have a catch block for a try block. You need a catch or a finally block. (There's also a fault block in IL but that's not in C# anyway :)).

lachbaer commented 7 years ago

@gulshan

I did not proceed with my proposal because I want to wait for Non-nullable reference types and method contract to roll out at first. These features are almost sure to come and will significantly change error handling concepts.

I don't yet see how that would probably reduce many exception cases, except for NullReferenceException- or ArgumentOutOfRange-alike exceptions. I much like your idea of adding an expression syntax to try for assigning purposes.

@JasonBock

I'd much rather see pattern matching get amped up in C# beyond what will be in version 7 so we can get toward handling errors in a cleaner way,

Can you make an example, please, where this would help?

sharwell commented 7 years ago

This feature makes it easier to write code in a form which I could never recommend for multiple reasons:

  1. Using the feature implies the use of a catch-all block, something I generally discourage
  2. Using the feature implies the use of a catch-all block which discards the exception without tracking for diagnostic purposes, something I disallow
  3. Using the feature implies the use of exceptions for flow control, something I generally discourage

The suggestion by @DavidArno above to create a TryNewStreamReader method would be preferable, because it clearly reveals the bad code.

If there is so much consent to never ever use catch { } wouldn't it be probably be okay to deprecate it in C# 7.1 (with warning) and finnaly remove it in C# 8 (with error)?

@lachbaer Removing a feature is totally different from adding a feature. No one ever claimed it is impossible to write correct code containing catch { }, so it's entirely plausible that fully-working libraries exist that use this construct. Removing the feature would unnecessarily break these libraries. On the other hand, if the use of catch { } is generally known to make it more difficult to write and/or maintain a correct library, then it's difficult to justify a change that makes it easier to write code with this form.

:bulb: Analyzers provide developers with a way to disallow catch { } in new development without breaking other applications.

lachbaer commented 7 years ago

@sharwell

Removing the feature would unnecessarily break these libraries.

Not necessarily. Already compiles assemblies will of course keep working, because the working of the bytecode is not affected. In case there are code libraries that will need a new compilation, then this will fail - for a good reason. The programmer should overlook the code to see how s/he can approve it. If that also is not possible or adequate, then a compiler switch could again optionally switch on that "outdated" behavior.

Wouldn't this procedure be practical possible?