ErrorBundle: fix integer overflow printing caret

[wooster0]

Do zig init-exe and put something like

    const x = [1]u8{
        .{
            `
        },
    };

in it and run zig build. You'll see:

zig build-exe x Debug native: error: the following command failed with 1 compilation errors:
/home/wooster/Desktop/zig-linux-x86_64/zig build-exe /home/wooster/Desktop/os/x/src/main.zig --cache-dir /home/wooster/Desktop/os/x/zig-cache --global-cache-dir /home/wooster/.cache/zig --name x --listen=-
Build Summary: 0/3 steps succeeded; 1 failed (disable with -fno-summary)
install transitive failure
└─ install x transitive failure
   └─ zig build-exe x Debug native 1 errors
src/main.zig:2:11: error: expected expression, found 'invalid bytes'
        .{
          ^
src/main.zig:3:13: note: invalid byte: '`'
            `
thread 431336 panic: integer overflow
/home/wooster/Desktop/zig-linux-x86_64/lib/std/zig/ErrorBundle.zig:227:51: 0x30384f in renderErrorMessageToWriter__anon_12356 (build)
            const after_caret = src.data.span_end - src.data.span_main -| 1;
                                                  ^

After applying this patch locally:

zig build-exe x Debug native: error: the following command failed with 1 compilation errors:
/home/wooster/Desktop/zig-linux-x86_64/zig build-exe /home/wooster/Desktop/os/x/src/main.zig --cache-dir /home/wooster/Desktop/os/x/zig-cache --global-cache-dir /home/wooster/.cache/zig --name x --listen=-
Build Summary: 0/3 steps succeeded; 1 failed (disable with -fno-summary)
install transitive failure
└─ install x transitive failure
   └─ zig build-exe x Debug native 1 errors
src/main.zig:2:11: error: expected expression, found 'invalid bytes'
        .{
          ^
src/main.zig:3:13: note: invalid byte: '`'
            `
            ^

Fixes #15596

[wooster0]

Sorry, I think that was my intention originally but I somehow thought it wasn't really possible to add a test for this but now thanks to your reminder I realize I can just add a regular test/cases/compile_errors test case!

[Vexu]

Sorry, I think that was my intention originally but I somehow thought it wasn't really possible to add a test for this but now thanks to your reminder I realize I can just add a regular test/cases/compile_errors test case!

I don't think that'll test this change since the case runner sets include_source_line to false. I think you'd need a standalone test or something.

[andrewrk]

mm that's a good point. if it's too much of a hassle I'm fine with merging the fix only.

another option would be updating the test harness to support an option that also shows the caret, and testing for it.

[wooster0]

Oh, yeah, that was why. The caret isn't included at all.

I'm not sure if we wanna add a special feature to the test harness for a single test. If we do it we should probably look for other things we could test with that too, but I can't think of much.

Maybe I can try a standalone test.

[wooster0]

While working on a standalone test I found something else that that seems like a regression:

$ zig build-exe test/cases/compile_errors/invalid_byte.zig --color off
test/cases/compile_errors/invalid_byte.zig:2:7: error: expected expression, found 'invalid bytes'
test/cases/compile_errors/invalid_byte.zig:3:9: note: invalid byte: '`'
$

--color should turn color off, not the source lines.

I'm pretty sure I remember before ErrorBundle we had this problem too and there was an issue and it got fixed but I can't find it.

So while I was at it I fixed that too and took the opportunity to write a standalone test that tests in error output:

1. Color (--color on)
2. No color (--color off)
3. The caret
4. Source lines
5. Reference trace
6. @compileLog output

In the future once @compileLog gains color (I think it should #12297) I would like to add a test for that too but that's for the future.

[wooster0]

zig build test-standalone is definitely passing for me locally on x86_64-linux but even that CI is failing here. I'll try to continue on error.FileBusy (like in test/src/Cases.zig) but other than that I'm not sure what's going on. Edit: nope, not working.

Maybe it is better though to just add a test manifest option in normal test cases to do this...

[andrewrk]

--color should turn color off, not the source lines.

This is working as intended by me, the idea is that --color off means you want machine-friendly output, not terminal output for humans. In such case, a source line and caret is just noise. If you want to argue against this, let's do that separately than this PR.

[wooster0]

Oh, those changes is probably also why the CI is failing because normal test cases also use --color off but now source lines etc. are included. Makes sense.

Hmm, this PR the way I wanted to do it is really hard to do if most output (the reference trace too) is just not there when color is off. I only really wanted one test with colors because they're harder to update. Sorry, I think I'll wait till there's a way to turn off just the color. Maybe there could be a separate flag for machine readable output. I'm also still not sure if it wouldn't be better to just add manifest options and use test/cases/compile_errors/ cases.

So, either way, I just don't see a way to add a test for this without including ANSI escape codes.

[andrewrk]

Thanks for looking into it though. I agree it's a bit more effort than it's worth at the moment.