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

stage2: Add "tilde pointing" to errors for multi-character offenses #9201

Closed
wants to merge 1 commit into from

Conversation

greenfork
Copy link
Contributor

@greenfork greenfork commented Jun 22, 2021

Error refactoring in #9117 removed this style from main.zig file. This commit adds it back and in addition to all other possible compiler errors.

I added the lexeme_length to Compilation.AllErrors.Message in order to get more reliable data where it can be done. As an alternative we can use a similar approach as for Compilation.AllErrors.add for all errors since Message has both source_line and column present. But it doesn't look very good to me.

Sketchy implementation in Compilation.AllErrors.add is inspired by
https://github.com/ziglang/zig/blob/master/lib/std/zig/ast.zig#L97-L116

I tried to test all paths with the following snippets, zig run:

const std = @import("std");

pub fn main() void {
    hello();
}
Compilation_add.zig:4:5: error: use of undeclared identifier 'hello'
    hello();
    ~~~~~

const std = @import("std");

pub fn main() void {
    h();
}
single_caret.zig:4:5: error: use of undeclared identifier 'h'
    h();
    ^

const std = @import("std");

pub fn main() !void {
    const hello = true || false;
}
addZir_with_token_present.zig:4:11: error: unused local constant
    const hello = true || false;
          ~~~~~

const std = @import("std");

pub fn main() !void {
    suspend {
        suspend {}
    }
    // const hello = true || false;
}
addZir_with_node_present.zig:5:9: error: cannot suspend inside suspend block
        suspend {}
        ~~~~~~~
addZir_with_node_present.zig:4:5: note: other suspend block here
    suspend {
    ~~~~~~~

Error refactoring in ziglang#9117 removed this style from main.zig file.
This commit adds it back and in addition to all other possible
compiler errors.
@andrewrk
Copy link
Member

andrewrk commented Jun 22, 2021

Here's a fun test case, how does this look?

test {
    const x: i32 = 1;
    const y: i32 = 1;
    foo(x + y);
}

fn foo(x: bool) void {
    _ = x;
}

Related:

test {
    const x: i32 = 1;
    const y: i32 = 1;
    foo(x + // x is the first operand
        y // y is the second operand
    );
}

fn foo(x: bool) void {
    _ = x;
}

I have a feeling tilde pointing is going to be a bit more complicated than this implementation. We might want to enable it on a per-error-message basis.

@greenfork
Copy link
Contributor Author

Thank you for these counter-examples. Funnily enough they produce same error which also doesn't work as expected:

start.zig:85:9: error: struct 'counter_example1.counter_example1' has no member named 'main'
    root.main();
        ^

and if I change it to use pub fn main then these errors:

counter_example1.zig:4:11: error: expected bool, found i32
    foo(x + y);
          ^
counter_example2.zig:4:11: error: expected bool, found i32
    foo(x + // x is the first operand
          ^

So yes, this looks like a lot more work to do. I think I don't have enough understanding to implement it. I will try to come up with a list of questions and maybe attend a weekly stage 2 meeting.

@andrewrk
Copy link
Member

Thank you for these counter-examples. Funnily enough they produce same error which also doesn't work as expected:

Ah the instructions for running the example would be: zig test foo.zig

@greenfork
Copy link
Contributor Author

More on formatting:

Examples from gcc and clang for multiline errors:

$ cat me.c
int main() {
    return "a" /* comment */
    +          /* comment */
    1;
}

$ gcc me.c
me.c: In function ‘main’:
me.c:3:5: warning: returning ‘char *’ from a function with return type ‘int’ makes integer from pointer without a cast [-Wint-conversion]
    2 |     return "a" /* comment */
      |            ~~~~~~~~~~~~~~~~~
    3 |     +          /* comment */
      |     ^~~~~~~~~~~~~~~~~~~~~~~~
    4 |     1;
      |     ~

$ clang me.c
me.c:3:5: warning: adding 'int' to a string does not append to the string [-Wstring-plus-int]
    +          /* comment */
    ^~~~~~~~~~~~~~~~~~~~~~~~
me.c:3:5: note: use array indexing to silence this warning
    +          /* comment */
    ^
    [
me.c:2:12: warning: incompatible pointer to integer conversion returning 'char *' from a function with result type 'int' [-Wint-conversion]
    return "a" /* comment */
           ^~~~~~~~~~~~~~~~~
2 warnings generated.

@greenfork
Copy link
Contributor Author

I will close this PR for now as it appears quite complex. I don't know whether there should be an issue created or there will be a more broad RFC-like issue such as "friendlier errors" so I will leave it as is.

@greenfork greenfork closed this Jun 25, 2021
@greenfork greenfork deleted the tilde-error-pointing branch June 25, 2021 17:10
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.

2 participants