Skip to content

Catch and release semantics for errdefer |err| #22079

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

Closed
mnemnion opened this issue Nov 26, 2024 · 6 comments
Closed

Catch and release semantics for errdefer |err| #22079

mnemnion opened this issue Nov 26, 2024 · 6 comments
Labels
proposal This issue suggests modifications. If it also has the "accepted" label then it is planned.

Comments

@mnemnion
Copy link

Status quo Zig has an errdefer variation, errdefer |err| {...}, which intercepts the error on its way out of the function call. This can indeed be useful, to add logging of the error, say, or to perform cleanup in an error-specific way.

I believe the more useful semantic would be for errdefer |err| to catch the error, that is, capture it the way that catch does now. This would mean that, while defer and errdefer without a capture cannot return, an errdefer |err| must return any value compatible with the function signature.

I ran into this in designing a dispatch loop. It has several points of entry for client code, each of which has numerous try clauses. Within the dispatch, anything which fails and throws an error may be handled in one fashion or another. Sometimes the result of handling would mean there's no error at all, sometimes it would involve propagating the error, sometimes it might mean swapping the received error for another error in the error set (which is explicit, not inferred, in this case).

The natural thing to want here is to add an errdefer |err| at the top of each function block, delegating all of that handling to a single helper function. The return values are either bool or void, so there isn't a risk of poorly thought-through code returning partially constructed data.

I view this as avoiding any hidden control flow problems. If a defer could return a value, say with a try statement, that would invisibly swap the original return for something else, which is bad.

But with errdefer |err|, it's quite explicit: an error is in the process of being returned, and the errdefer block has captured it: it goes into |err|. It is now the responsibility of that block to return something, compatible with the original signature.

It can even be argued that the current errdefer |err| has a bit of hidden control flow! It's the only situation in which a value is captured, but then that same value invisibly propagates elsewhere regardless of the result of the capture-scoped block. With a catch block or an error-capturing if, once the error is caught, it's caught, unless code explicitly releases it. This change would mean that property holds for an errdefer |err| capture as well.

Currently, errdefer |err| is consistent with other defers, but inconsistent with other captures which it syntactically resembles. This way, errdefer |err| is semantically consistent with all other uses of its syntax, but behaves differently from the other two defer statements. Either way there's something to understand, and I claim that this moves complexity around as much as it adds complexity, with good results.

Ordering

This is the main consideration for the idea. I think it's good to have, or I wouldn't have suggested it, but it has the potential to complicate reasoning about code, and that should at least be addressed here.

The way I see it, there's only one possible interpretation. defer and errdefer blocks run in the reverse order in which they're introduced. This doesn't change that, with the following result:

  • Any defer or errdefer block introduced after the errdefer |err| will run before the errdefer |err| captures the error.
  • When errdefer |err| returns an error, all previous errdefer and defer blocks run in the accustomed order.
  • When errdefer |err| returns a non-erroneous result, any prior errdefers do not run, and defers run as usual.

The inside-out execution of defers is an essential complexity in learning Zig. Without understanding and respecting that, a user can get into real trouble trying to do complex things with defers.

I don't believe that adding this facility would make that learning harder. There are several places where Zig documentation points out code smells, and other usage risks, this can be one of those.

Because if code has an errdefer |err| which does anything other than catch-and-release the error, that code is almost certainly wrong unless the errdefer |err| comes before any try block and is the only errdefer statement in the scope. An errdefer |err| changes nothing if it returns an error (including the error it received) and changes one thing if it returns the normal return value: errdefers defined before it will not run. But it's also going to be usually a bad idea to run later errdefer cleanups, if the prior errdefer |err| is going to handle that same error. Maybe not always, but a word to the wise should be sufficient here.

There's already a whole chapter on common errdefer slipups, so there's a place to put this wisdom as well.

Alternatives

It's currently possible to add the general-purpose error handler as a catch block to each error-unioned function call, because the subset of errors which gets returned by those calls can be cast to the superset which is documented as the return value of the dispatch function.

In the project which motivated this, that results in twelve otherwise-identical catch blocks. That's the current number, and it may double before the MVP of this project is ready for release.

I would suggest that a result like that, where a somewhat extreme amount of code duplication could be replaced with this:

pub fn dispatch(self: *@This()) ErrorSet!bool {
    errdefer |err| {
        return self.handleError(err);
    }
    // ...
}

Suggests that a change like this would pull its weight. I claim that the snippet of code just illustrated is easy to understand, and is actually the optimal way to express this pattern.

@rohlem
Copy link
Contributor

rohlem commented Nov 26, 2024

// status-quo
fn f() !Result {
  return g() catch |e| {
    // produce alternative value
  };
}
fn g() !Result {
  // produce result
}
// proposed
fn f() !Result {
  errdefer |e| {
    // produce alternative value
  };
  // produce result
}

I'm not sure whether I have a clear favorite.

The first variant means you have to split up the function, which means
coming up with intermediate function names, and re-stating the function signature
(arguments and result type, which may include an explicit error set).
While these things can be incidentally helpful to documentation and modularizing code,
I can see how if they're used as a workaround for achieving these semantics,
they might just be annoying/unhelpful noise.

The second variant feels more like pushing things onto a mental stack.
To read the function you have to think it through from one capturing errdefer to the next.
Note that labeled blocks already provide some similar options, but they result in indentation/nesting.
They also currently lack deducible error sets.
(Relevant proposal for block-scoped error handling: #5610 (comment))

I think for experimenting with this change I wouldn't special-case only capturing errdefer,
but instead introduce two keywords for symmetry of defer and errdefer (f.e. defercapture and errdefercapture),
then see how their usage affects the readability of code.
(My hunch is that it would depend on style, and depending on the situation they might all be useful.)

@andrewrk
Copy link
Member

There's already a whole chapter on common errdefer slipups, so there's a place to put this wisdom as well.

what the...

reverted in 3ce6de8. I really need to do an entire audit of the langref.

@andrewrk andrewrk added the proposal This issue suggests modifications. If it also has the "accepted" label then it is planned. label Nov 26, 2024
@andrewrk
Copy link
Member

Please do not file language proposals.

@andrewrk andrewrk closed this as not planned Won't fix, can't repro, duplicate, stale Nov 26, 2024
@mnemnion
Copy link
Author

Sorry @andrewrk. You had said in a comment (about something specific) not to file a proposal without a public zig repo with at least a hundred commits. I've added more focused proposals successfully in the past.

I didn't intend to test your patience or waste your time.

@mnemnion
Copy link
Author

To read the function you have to think it through from one capturing errdefer to the next.

I had considered suggesting the additional stricture that an errdefer |err| can only be the first errdefer in a block, and therefore, there can only be one per block. It did occur to me that playing catch-and-release multiple times, and making whether a prior errdefer executes at all dependent on the result of an errdefer |err| creates at least puzzling control flow, which that added rule would solve.

But it seems now is not the time to work out any such details. Perhaps at some future point.

@andrewrk
Copy link
Member

andrewrk commented Nov 29, 2024

No hard feelings. I may even revisit this, but I just don't have the bandwidth to chat about (non-accepted, non-core-team-member-proposed) language stuff on the issue tracker, sorry.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
proposal This issue suggests modifications. If it also has the "accepted" label then it is planned.
Projects
None yet
Development

No branches or pull requests

3 participants