-
-
Notifications
You must be signed in to change notification settings - Fork 3k
ast: Add comptime reason for error set merge #25398
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
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Marek Maškarinec <[email protected]>
.clobber => "clobber must be comptime-known", | ||
|
||
.type => "types must be comptime-known", | ||
.merge_error_set => "operand to error set merge (||) must be an error set", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.merge_error_set => "operand to error set merge (||) must be an error set", | |
.merge_error_set => "operand to error set merge operator (||) must be a type", |
although, really, it would probably be better to use the types must be comptime-known
error message, with an additional note explaining why it expects a type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your efforts, however, this error message assumes too much. Consider for instance, if in your example, foo
was a function returning an error set. In this case your error message is incorrect. Compile errors must never state or imply incorrect things, even if doing so is usually helpful.
EDIT: Hmm, on the other hand, it may be possible to prove that the error message could never be displayed in the case that it is an error set. For instance if it is the return type of a function it will force the function to be compile-time. If that's the case, then the property is upheld that the compile error would never state or imply an incorrect thing, making this change good.
Yes, my assumption was comes from the fact that there isn't a way to create a non-comptime error set (but I might be wrong on this). That means a value being non-comptime implies it isn't an error set. A more conservative approach could be changing the note to what @silversquirl suggested. |
I think there could still be a problem if the comptime reason ends up being something else inside a function that returns an error set. Then the comptime reason trace would include the line "operand to error set merge (||) must be an error set" while pointing to an operand that is, indeed, an error set. |
Example: test "example" {
const A = error{A};
const B = A || foo();
_ = B;
}
fn foo() type {
bar();
return error{B};
}
extern fn bar() void;
|
the main reason i suggested changing "an error set" to "a type" is because "error set" is confusing - it somewhat implies a value of type @andrewrk i think this confusion is demonstrated quite well by the fact that your example actually makes exactly this mistake! |
Oops! Good catch. I think both of our points stand. |
This PR adds a better error message if a non-comptime operand is used with
||
. This would often happen when accidentally using||
instead ofor
. Previously the error looked like this:Now a new notes is added: