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
Clarify reason implicit cast does not work for large RHS #1168
Clarify reason implicit cast does not work for large RHS #1168
Conversation
Can you add a regression test to test/compile_errors.zig? The CI failure is interesting... #1159 must be causing it but the run for that PR was green. |
src/ir.cpp
Outdated
@@ -11434,8 +11434,10 @@ static TypeTableEntry *ir_analyze_bit_shift(IrAnalyze *ira, IrInstructionBinOp * | |||
op1->value.type->data.integral.bit_count - 1); | |||
|
|||
casted_op2 = ir_implicit_cast(ira, op2, shift_amt_type); | |||
if (casted_op2 == ira->codegen->invalid_instruction) | |||
if (casted_op2 == ira->codegen->invalid_instruction) { | |||
ir_add_error(ira, &bin_op_instruction->base, buf_sprintf("RHS of shift is too large for LHS 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.
To follow the pattern of the code around, this if check should be doing if (type_is_invalid(casted_op2->value.type))
In general, when you get an invalid type from an ir function, it means that an error has already been reported, and it would be inappropriate to report another error.
Consider the example:
const Foo = struct {};
var x: u8 = 1;
var y = x << Foo;
I haven't tried it but I believe this would emit RHS of shift is too large for LHS type
which is not correct.
If we want to have this improved error message - which I agree with you that we do - then I think it would be better to look at the type of shift_amt_type
before calling ir_implicit_cast
. If it's an integer that is too big, then we can emit the specialized error message and return early. Otherwise, proceed with implicitly casting like normal.
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.
Right I was wondering about other cases that are not left shifts. I'll try that approach when I have a chance.
src/ir.cpp
Outdated
op2->value.data.x_bigint.is_negative)) { | ||
ir_add_error(ira, | ||
&bin_op_instruction->base, | ||
buf_sprintf("RHS of shift is too large for LHS 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.
this is still missing some of the useful tidbits of the original error message:
error: integer value 8 cannot be implicitly casted to type 'u3'
the fact that the value is 8 is helpful, and that it must be casted to u3
is helpful.
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.
True I'll add that in.
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.
I suggest having the original error message, and then adding an error note with your extra text.
I think rerunning the build would resolve the issue in Travis. |
I know this issue has been closed, but its the closest to this error I'm getting: test "error: LHS of shift..." {
var idx: u64 = 0;
var result: u64 = 0;
while (idx < 64) : (idx += 1) {
result |= 1 << idx;
}
}
Can anyone spot the problem? If I cast the 1 as u64, I get a different error. test "error: expected type 'u6', found 'u64'" {
var idx: u64 = 0;
var result: u64 = 0;
while (idx < 64) : (idx += 1) {
result |= u64(1) << idx;
}
}
Should I create a new issue for this? Am I missing something? I just want to do a simple shift. I take it the compiler is checking to make sure I can't overflow. What conditions must the operand types satisfy? Or do I need to use @shlWithOverflow and check for overflow? |
|
This is how I would write it: test "error: expected type 'u6', found 'u64'" {
var idx: u6 = 0;
var result: u64 = 0;
while (true) : (idx += 1) {
result |= u64(1) << idx;
if (idx == 63) break;
}
} Edit: or like this ;) Another one test "error: expected type 'u6', found 'u64'" {
const result = comptime x: {
var idx = 0;
var result = 0;
while (idx < 64) : (idx += 1) {
result |= 1 << idx;
}
break :x result;
};
} |
Thank you. That works! Here is what I came up with, maybe slightly more concise: test "works" {
var idx: u6 = 0;
var result: u64 = 0;
while (idx <= 63) : (idx += 1) {
result |= u64(1) << idx;
}
} |
your example does this for me in debug mode:
|
A |
Oops off by one. Strange that I didn't see an error on windows. Anyway. Thank you. |
By the way, the rules about integer types with regards to shifting are not arbitrary. Zig is protecting against undefined behavior here. In C it's actually pretty easy to accidentally invoke undefined behavior by shifting by a number of bits equal to the LHS integer size. The simplest way to shift is probably to just |
Oic, nevermind, I was ignoring some other compiler errors and the test was never running. I like that behavior. I really like where the language is going. |
Took me a while to figure out that I had to cast LHS of bit shift for this case:
Error message:
Fix:
So I improved the error message to help out here.