Skip to content

Fix issues in @errorCast with error unions #17368

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

Merged
merged 1 commit into from
Oct 3, 2023
Merged

Conversation

Vexu
Copy link
Member

@Vexu Vexu commented Oct 2, 2023

Closes #17353
Closes #17355

#17354 passes analysis but crashes in codegen because error_set_has_value doesn't work with adhoc inferred error sets and I'm not quite sure how it should be fixed.

@andrewrk
Copy link
Member

andrewrk commented Oct 3, 2023

non-adhoc inferred error sets are intended to be resolved at the end of function body analysis:

zig/src/Module.zig

Lines 4896 to 4915 in 412d863

// Resolving inferred error sets is done *before* setting the function
// state to success, so that "unable to resolve inferred error set" errors
// can be emitted here.
if (sema.fn_ret_ty_ies) |ies| {
sema.resolveInferredErrorSetPtr(&inner_block, LazySrcLoc.nodeOffset(0), ies) catch |err| switch (err) {
error.NeededSourceLocation => unreachable,
error.GenericPoison => unreachable,
error.ComptimeReturn => unreachable,
error.ComptimeBreak => unreachable,
error.AnalysisFail => {
// In this case our function depends on a type that had a compile error.
// We should not try to lower this function.
decl.analysis = .dependency_failure;
return error.AnalysisFail;
},
else => |e| return e,
};
assert(ies.resolved != .none);
ip.funcIesResolved(func_index).* = ies.resolved;
}

adhoc inferred error sets are only used for inline/comptime calls and are resolved after the call:

https://github.com/ziglang/zig/blob/412d863ba5801c1376af7ab8f04a71b839a820a6/src/Sema.zig#L7542-L7553

In both cases, they should not be observable by the backend.

@@ -21771,10 +21771,10 @@ fn zirErrorCast(sema: *Sema, block: *Block, extended: Zir.Inst.Extended.InstData
// operand must be defined since it can be an invalid error value
const maybe_operand_val = try sema.resolveDefinedValue(block, operand_src, operand);

if (disjoint: {
const disjoint = disjoint: {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic in this block should be skipped for error unions.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I use it to avoid an error_set_has_value in the safety check but it could be removed too.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, my mistake

@andrewrk andrewrk merged commit 0bdbd3e into ziglang:master Oct 3, 2023
@Vexu Vexu deleted the error-cast branch January 29, 2024 09:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

@errorCast on error unions should allow disjoint sets @errorCast crashes on inferred error set
2 participants