Skip to content
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

Improve error message when using 'try' #1650

Closed
wants to merge 4 commits into from
Closed

Improve error message when using 'try' #1650

wants to merge 4 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Oct 11, 2018

As described here: #1587 (comment) when using 'try' in a function with that expects a non-error return type you get a confusing error message.

As described here:
#1587 (comment) when
using 'try' in a function with that expects a non-error return type you
get a confusing error message.
@ghost
Copy link
Author

ghost commented Oct 11, 2018

This is my first time looking at zig source code, please tell me if this is the wrong way/place to handle this

@andrewrk
Copy link
Member

I think this is a huge improvement. There's a better place to put the ir_add_error_node call though, which I think is inside ir_analyze_instruction_return just before the call to ir_implicit_cast (which is what eventually is calling ir_add_error_node).

So you would check if value->value.type is an error, and that it could not be implicitly casted to ira->explicit_return_type (I think you can just check if it is an error union or an error set type), and in this case emit the error directly, without calling ir_implicit_cast.

There's a better way to get access to the function too, which is

        ZigFn *fn_entry = exec_fn_entry(ira->new_irb.exec);
        assert(fn_entry); // this assertion will be OK since we check earlier for `return` outside a function.

@andrewrk
Copy link
Member

Note that this will change the compile error even if the user does return error.Foo; in a function that does not accept a possible error. I suggest either

  • Check if instruction->value->id == IrInstructionIdUnwrapErrCode before doing this special error message, or
  • (my preference) adjust the compile error message to be meaningful and correct for both cases - try when the return type cannot handle an error, and return error.Foo when the return type cannot handle the error.

dec05eba added 2 commits October 13, 2018 01:51
As described here:
#1587 (comment) when
using 'try' in a function with that expects a non-error return type you
get a confusing error message.
As described here:
#1587 (comment) when
using 'try' in a function with that expects a non-error return type you
get a confusing error message.
@ghost
Copy link
Author

ghost commented Oct 12, 2018

I noticed that my changes do not apply to async functions and they require a different type of check

@andrewrk
Copy link
Member

I wouldn't focus too much on the async function aspect of it yet. I'm in the middle of rewriting async functions in another branch. Maybe you can revisit it once that's merged.

As described here:
#1587 (comment) when
using 'try' in a function with that expects a non-error return type you
get a confusing error message.
@@ -11217,11 +11217,45 @@ static IrInstruction *ir_analyze_instruction_add_implicit_return_type(IrAnalyze
return ir_const_void(ira, &instruction->base);
}

static bool type_is_error(ZigType *type) {
if (!type)
return false;
Copy link
Member

Choose a reason for hiding this comment

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

This could hide a bug. Better to assert that your parameter is non-null.

case ZigTypeIdErrorSet:
return true;
case ZigTypeIdOptional:
return type_is_error(type->data.maybe.child_type);
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure this case should be here? What would the zig source look like that uses this?

@andrewrk andrewrk added this to the 0.4.0 milestone Oct 31, 2018
@andrewrk andrewrk closed this in 2f5d1ec Nov 19, 2018
@andrewrk
Copy link
Member

I solved this in a more generic way:

export fn foo() void {
    return true;
}

export fn bar() void {
    try baz();
}

fn baz() anyerror!void {}
/home/andy/downloads/zig/build/test.zig:2:12: error: expected type 'void', found 'bool'
    return true;
           ^
/home/andy/downloads/zig/build/test.zig:1:17: note: return type declared here
export fn foo() void {
                ^
/home/andy/downloads/zig/build/test.zig:6:5: error: expected type 'void', found 'anyerror'
    try baz();
    ^
/home/andy/downloads/zig/build/test.zig:5:17: note: return type declared here
export fn bar() void {
                ^

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.

1 participant