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

Clarify reason implicit cast does not work for large RHS #1168

Merged
merged 4 commits into from Jun 29, 2018

Conversation

isaachier
Copy link
Contributor

@isaachier isaachier commented Jun 28, 2018

Took me a while to figure out that I had to cast LHS of bit shift for this case:

var x : u8 = 5;
fn make16Bits() u16 {
    return (x << 8) & 0xff00;
}

Error message:

 error: integer value 8 cannot be implicitly casted to type 'u3'

Fix:

var x : u8 = 5;
fn make16Bits() u16 {
    return (u16(x) << 8) & 0xff00;
}

So I improved the error message to help out here.

@bnoordhuis
Copy link
Contributor

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"));
Copy link
Member

@andrewrk andrewrk Jun 28, 2018

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.

Copy link
Contributor Author

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"));
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

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.

@isaachier
Copy link
Contributor Author

I think rerunning the build would resolve the issue in Travis.

@andrewrk andrewrk merged commit f1c56f7 into ziglang:master Jun 29, 2018
@isaachier isaachier deleted the shift-amt-error-improvement branch July 2, 2018 21:27
@travisstaloch
Copy link
Sponsor Contributor

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;
    }
}
... error: LHS of shift must be an integer type, or RHS must be compile-time known
        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;
    }
}
... error: expected type 'u6', found 'u64'
        result |= u64(1) << u6(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?

@andrewrk
Copy link
Member

result |= u64(1) << u6(idx);

u6(x) no longer forces an integer cast. You'll have to use @intCast for that.

@andrewrk
Copy link
Member

andrewrk commented Nov 30, 2018

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 ;)
var result: u64 = @import("std").math.maxInt(u64);

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;
    };
}

@travisstaloch
Copy link
Sponsor Contributor

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;
    }
}

@andrewrk
Copy link
Member

your example does this for me in debug mode:

Test 1/1 works...integer overflow
/home/andy/downloads/zig/build/test.zig:4:30: 0x2050b0 in ??? (test)
    while (idx <= 63) : (idx += 1) {
                             ^
/home/andy/downloads/zig/build/lib/zig/std/special/test_runner.zig:13:25: 0x2231fa in ??? (test)
        if (test_fn.func()) |_| {
                        ^
/home/andy/downloads/zig/build/lib/zig/std/special/bootstrap.zig:112:22: 0x222eb3 in ??? (test)
            root.main() catch |err| {
                     ^
/home/andy/downloads/zig/build/lib/zig/std/special/bootstrap.zig:49:5: 0x222c70 in ??? (test)
    @noInlineCall(posixCallMainAndExit);
    ^

Tests failed. Use the following command to reproduce the failure:
/home/andy/downloads/zig/build/zig-cache/test

@andrewrk
Copy link
Member

A u6 is always less than or equal to 63, so that's equivalent to while (true)

@travisstaloch
Copy link
Sponsor Contributor

Oops off by one. Strange that I didn't see an error on windows. Anyway. Thank you.

@andrewrk
Copy link
Member

andrewrk commented Nov 30, 2018

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 @intCast the RHS to the correct type, which inserts a safety check in debug mode to make sure the value is in range.

@travisstaloch
Copy link
Sponsor Contributor

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.

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.

None yet

4 participants