Skip to content

Make each "parameter" of a catch clause optional #3815

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

Open
srawlins opened this issue May 16, 2024 · 10 comments
Open

Make each "parameter" of a catch clause optional #3815

srawlins opened this issue May 16, 2024 · 10 comments
Labels
request Requests to resolve a particular developer problem

Comments

@srawlins
Copy link
Member

Today catch "parameters" are kind of special creatures. You can specify one, catch (e) <body>, and the catch body will be "called" with one argument. Or you can specify two, catch (e, st) <body>, and the catch body will be "called" with two arguments. The Dart backend is happy to oblige. But you cannot specify zero parameters. 🤷

It seems to be an inconsistent syntax (I might be missing some justification). Firstly, it is inconsistent with how functions work. I think the simplest example is callbacks: The Iterable.forEach method takes a void Function(E). If you don't want to use the parameter, you don't get to pass a void Function(). You might imagine that you could pass () => print('yay'), and that Dart would just call your callback with no arguments, no problem. But no, you just get a compile-time error. You might similarly think that an update to Iterable.forEach could start to accept functions of the shape void Function(E, {int index}), like (_, index) => print('yay $index'), and that such an update would be non-breaking because Dart could, similar to Future.catchError or catch, either pass one argument or two. But no, you just get a compile-time error.

OK, no problem, that's how functions work in Dart. But catch (and some others like Future.catchError) allow you some wiggle room, presumably for some purpose. Maybe it's a big burden to make people write catch (e, st) <body> every time they oh-so-frequently write catch clauses, so we let them write catch (e) <body>. (Maybe we save some cycles by avoiding instantiating a StackTrace 🤷 ) OK, no problem. So if I don't want to accept a StackTrace, I don't have to accept a StackTrace. What if I don't want to accept the error?

In current Dart, I'm out of luck. I'm stuck writing such lengthy or verbose code like catch (e) <body> or catch (_) <body>. Por que? It seems quite inconsistent to go to lengths to step away from function calling conventions and allow optional "parameters," but only for this one "parameter," and not that one "parameter."

I think this could be tied to the Wildcards feature. It would be non-breaking, so safe to sneak in. And it's related because of what analyzer can start reporting with this feature. With wildcards + no change to catch, analyzer could report warnings that seems to only point out the language inconsistency. When a user writes catch (e, st) {}, they could get two messages that are head-scratching seen togother:

Hey you don't use st, get rid of it!
Hey you don't use e, keep it and rename it _!

@srawlins
Copy link
Member Author

CC @kallentu @dart-lang/analyzer-team

@Reprevise
Copy link

For what it's worth, you can do this if you don't care about the error:

try {
  // ...
} on Object {
  // ...
}

@natebosch
Copy link
Member

Yeah on Object { is the way to write catch () {. I think that's the most awkward case to read or write - on SpecificException { doesn't look as strange. It's very unusual to have a catch-all handler that also doesn't read the thrown value. It also relies on the reader being aware that null can never be thrown (it gets replaced with a NullThrownError) to understand that on Object covers every possible error.

@natebosch
Copy link
Member

Hmm, } catch (_) { is way more common than I expected (and so is } catch (_) {} 😨 ). I wonder if we should either write a hint that this could be written as } on Object {, or consider allowing } catch () {.

@pq
Copy link
Member

pq commented May 16, 2024

} catch (_) { is way more common than I expected

Also quite common are catch (e) {} and catch (exception) {}, which are equally ignored by the analyzer. (Open question about changing that in dart-lang/sdk#55723 (comment).)

@srawlins
Copy link
Member Author

ignored by the analyzer.

Sort of. As far as the empty body goes, we have a lint rule, empty_catches, which is in the core lint set.

@pq
Copy link
Member

pq commented May 16, 2024

Sort of. As far as the empty body goes, we have a lint rule, empty_catches, which is in the core lint set.

Good point. It's worth noting that it just takes a comment to silence the lint:

try {
  ...
} catch(e) {
  // ignored, really.
}

With wildcards, I'd expect the comment to be unnecessary and would hope that

try {
  ...
} catch(_) { }

wouldn't trigger it... But that's up for discussion! (dart-lang/sdk#57132)

@stereotype441
Copy link
Member

FWIW, even though in principle I know that on Object is the canonical way to catch everything when you don't care to inspect the error, for some reason I find the grammar of it really hard to remember in the moment, so I wind up writing catch (e) anyhow.

I would be tempted to take things one step further and allow not just catch () as a synonym for on Object, but also catch (T e) as a synonym for on T catch (e) and catch (T e, st) as a synonym for on T catch (e, st). I might even allow catch (T e, StackTrace st). Basically, let the users do what's intuitive to them based on function parameter syntax.

@lrhn
Copy link
Member

lrhn commented May 17, 2024

it is inconsistent with how functions work.

It's not a function, so that one doesn't bother me. It's consistent with onError functions mainly because catch came first, and onError functions tried to match catch clauses.

The underlying idea was that if a catch clause doesn't need a stack trace, and exception handling generally shouldn't, then the runtime system doesn't have to create a stack trace object at all.
(In practice that had far less effect than hoped, because since we added async and futures capturing throws, you can't know if you need the stack trace until the Future object containing the throw is GC'ed.)

I'm not sure allowing catch () is winning me over. It looks like it's catching an empty record. I actually find catch (_) more readable.

What I would really want is to allow patterns in catch clauses:

} catch (StateError e, s) when e.toString().contains("banana") {
  ...
}

which would be able to inspect the thrown object more than just by its type, before deciding whether to catch or not, instead of catching and maybe rethrowing.
Meaning also that } catch _ { should work, and } catch StateError _ { what } on StateError { is today.

Each such catch clause is checked against (error, stackTrace), and just error (the static expected type of the pattern should allow trying only one), and then the pattern and when clause runs, and if that succeeds, the error is caught, possibly already destructured into what the handler needs.

Another thing I'd like to do is to remove that stack trace from the catch entirely. That gets rid of having to choose between matching (Object, StackTrace) or Object (and handling it in onError functions).
If the object thrown is an error, you can get the stack trace from Error.stackTrace. If it's an exception, there is no stack trace, and there shouldn't be any need for a stack trace, because the exception itself should contain all necessary information to handle the exception at runtime. If it wants to contain a stack trace, it can embed StackTrace.current deliberately.

@lrhn lrhn added the request Requests to resolve a particular developer problem label May 17, 2024
@escamoteur
Copy link

What I would really want is to allow patterns in catch clauses:

I would l love that. Also I think the on Type is not intuitive and I never liked it. The type T that you use in the catch(T error) should be enough.
I don't agree on removing the optional stacktrace because it's often used to send logging to Sentry and the like. As it is an optional second parameter, it doesn't really hurt to have it.

I also regularly use the catch(_) to just undo a recent action when the reason the action failed isn't important. IMHO using _ as a placeholder that is ignored and not used is a well known practice it communicates exactly what is going on so we don't need an extra catch().

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
request Requests to resolve a particular developer problem
Projects
None yet
Development

No branches or pull requests

7 participants