Skip to content

Commit 6188cb8

Browse files
matkladandrewrk
authored andcommitted
sema: add a missing errdefer
This fix doesn't matter at all in the grand scheme of things, but I think the story behind it is perhaps curious, as it might point at a design flaw in the Sema's error reporting API. So, a story: On lobsters, there's a rather heated discussion on the merits on RAII vs defer. I don't really like participating in heating discussions, but also sort of can't stop thinking about this. My own personal experience with Zig's defer and errdefer is that they are fiddly to get right consistency --- if a program has a lot of resource management to do, I _always_ mess up at least one defer/errdefer. I've found my internal peace by just avoiding spread-out, "pox" resource management, and instead centralizing resource ownership under one of the following patterns: * Either the thing is acquired and released in main * Or main allocates N instances of thing, and then the rest of the code explicitly juggles this finite pool of N. Notably, this juggling typically doesn't involve defer/errdefer at all, as, at this level of precision, there are no `try`s left, so you only code the happy path * Or there's some sort of arena thing, where a bunch of resources have a single owner, the user's don' bother cleaning up their resources, and instead the owner does it once at the end. So I wanted to make a lobster.rs comment in the vein of "yeah, if your program is mostly about resource management, then Zig could be kinda a pain, but that's friction tells you something: perhaps your program shouldn't be about resource management, and instead it should be doing what it is supposed to do?". And, as an evidence for my claim, I wanted to point out some large body of Zig code which doesn't have a lot of errdefers. So, I cracked opened Sema.zig, `ctrl+f` for `defer`, saw whopping 400 something occupancies, and my heart skipped a bit. Looking at the occurrences, _some_ of them were non-resource-related usages of defer. But a lot of them were the following pattern: ```zig const msg = try sema.errMsg(src, "comptime control flow inside runtime block", .{}); errdefer msg.destroy(sema.gpa); ``` This is exactly the thing that I know _I_ can't get right consistently! So, at this point, I made a prediction that at least one of `errdefer`s is missing. So, I looked at the first few `const msg = try` and of course found one without `errdefer`. I am at 0.8 that, even with this PR applied, the claim will still stand --- there will be `errdefer` missing. So it feels like some API re-design is in order, to make sure individual error messages are not resources. Could Sema just own all partially-constructed error messages, and, at a few known safe-points: * if the control flow is normal, assert that there are no in-progress error messages * if we are throwing an error, go and release messages immediately? I am unlikely to do the actual refactor here, but I think it's worth highlighting the overall pattern here. PS: I am only 0.9 sure that what I've found is indeed a bug! I don't understand the code, I did a dumb text search, so I _could_ have made a fool of myself here :P
1 parent 4e09e36 commit 6188cb8

File tree

1 file changed

+1
-0
lines changed

1 file changed

+1
-0
lines changed

src/Sema.zig

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10252,6 +10252,7 @@ fn finishFunc(
1025210252
"function with comptime-only return type '{}' requires all parameters to be comptime",
1025310253
.{return_type.fmt(pt)},
1025410254
);
10255+
errdefer msg.destroy(sema.gpa);
1025510256
try sema.explainWhyTypeIsComptime(msg, ret_ty_src, return_type);
1025610257

1025710258
const tags = sema.code.instructions.items(.tag);

0 commit comments

Comments
 (0)