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

Projects
None yet
4 participants
@isaachier
Contributor

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

This comment has been minimized.

Member

bnoordhuis commented Jun 28, 2018

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"));

This comment has been minimized.

@andrewrk

andrewrk Jun 28, 2018

Member

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.

This comment has been minimized.

@isaachier

isaachier Jun 28, 2018

Contributor

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"));

This comment has been minimized.

@andrewrk

andrewrk Jun 28, 2018

Member

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.

This comment has been minimized.

@isaachier

isaachier Jun 28, 2018

Contributor

True I'll add that in.

This comment has been minimized.

@andrewrk

andrewrk Jun 28, 2018

Member

I suggest having the original error message, and then adding an error note with your extra text.

@isaachier

This comment has been minimized.

Contributor

isaachier commented Jun 29, 2018

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

@andrewrk andrewrk merged commit f1c56f7 into ziglang:master Jun 29, 2018

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@isaachier isaachier deleted the isaachier:shift-amt-error-improvement branch Jul 2, 2018

@travisstaloch

This comment has been minimized.

travisstaloch commented Nov 30, 2018

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

This comment has been minimized.

Member

andrewrk commented Nov 30, 2018

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

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

@andrewrk

This comment has been minimized.

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

This comment has been minimized.

travisstaloch commented Nov 30, 2018

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

This comment has been minimized.

Member

andrewrk commented Nov 30, 2018

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

This comment has been minimized.

Member

andrewrk commented Nov 30, 2018

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

@travisstaloch

This comment has been minimized.

travisstaloch commented Nov 30, 2018

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

@andrewrk

This comment has been minimized.

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

This comment has been minimized.

travisstaloch commented Nov 30, 2018

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