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

Shorten coercion error messages by eliding non-conflicting parts of types #20576

Closed
wants to merge 1 commit into from

Conversation

tau-dev
Copy link
Contributor

@tau-dev tau-dev commented Jul 10, 2024

Attempt to address #12401 by printing only the components of types that are mismatched. As far as I can tell, Rustc and Clang do similar things to prevent error message blowup.

So far, this only covers Type.print(), and results in messages like this:

test {
    var a: error{Boo}!u32 = 2;
    _ = &a;
    _ = @as(error{Baz}!u32, a);
}
fn funcA(_: u32) u32 { return 2; }
test { _ = @as(*fn(u32) u32, &funcA); }
test { _ = @as(fn(u32, u32) u32, funcA); }
test { _ = @as(fn(u32) u8, funcA); }
/tmp/d.zig:4:29: error: expected type 'error{Baz}!…', found 'error{Boo}!…'
    _ = @as(error{Baz}!u32, a);
                            ^
/tmp/d.zig:4:29: note: 'error.Boo' not a member of destination error set
/tmp/d.zig:7:30: error: expected type '*…', found '*const …'
test { _ = @as(*fn(u32) u32, &funcA); }
                             ^~~~~~
/tmp/d.zig:7:30: note: cast discards const qualifier
/tmp/d.zig:8:34: error: expected type 'fn (u32, u32) …', found 'fn (u32) …'
test { _ = @as(fn(u32, u32) u32, funcA); }
                                 ^~~~~
/tmp/d.zig:8:34: note: function with 1 parameters cannot cast into a function with 2 parameters
/tmp/d.zig:9:28: error: expected type 'fn (…) u8', found 'fn (…) u32'
test { _ = @as(fn(u32) u8, funcA); }
                           ^~~~~
/tmp/d.zig:9:28: note: return type 'u32' cannot cast into return type 'u8'
/tmp/d.zig:9:28: note: unsigned 8-bit int cannot represent all possible unsigned 32-bit values

I made this draft to check to check whether this sounds like a decent strategy for Zig error messages. If you think it is, I'll expand this to cover the actually really long type names mentioned in #12401, which are produced by createAnonymousDeclNamed for NameStrategy.func. To solve that, I will probably need to preserve the function name and argument values in the anonymous type's Decl and then implement similar diffing logic in Value.print().

@tau-dev tau-dev changed the title Elide non-conflicting parts of types when printing coercion errors Shorten coercion error messages by eliding non-conflicting parts of types Jul 10, 2024
@rohlem
Copy link
Contributor

rohlem commented Jul 10, 2024

I like the change, though for certain scenarios it might be helpful to see the elided part as well;
to not regress those scenarios we could add an additional note: … = u32 directly following the error.
I think that would still be easier to read and shorter in most cases.

@tau-dev
Copy link
Contributor Author

tau-dev commented Jul 10, 2024

Do you have an example at hand of where you would need the rest of the type?

@mlugg
Copy link
Member

mlugg commented Jul 10, 2024

I agree with @rohlem -- there should be a note: where '_' is 'u32'. Also, avoid using the Unicode eliipsis, so as to not introduce a dependency on correct Unicode printing for compiler error messages. I'm not quite sure what we use in its place -- _ feels too much like a discard, so perhaps just an ASCII ...? Lastly, there should certainly be some kind of length check before this kicks in; expected type '*u32', found '*const u32' is a better error message than expected type '*_', found '*const _'.

Do you have an example at hand of where you would need the rest of the type?

I can think of some esoteric cases, but they are besides the point. We should not omit key information about the error (the types in a type mismatch!) in the name of compactness.

@tau-dev
Copy link
Contributor Author

tau-dev commented Jul 10, 2024

Full agree on the length check and ASCII. The unicode ellipsis was provisional to distinguish from e.g. the ... in vararg function types. If we want to spell out the elided part later, we will need to introduce distinct identifiers anyways for the multiple elided parts in function or compound types.

We should not omit key information about the error (the types in a type mismatch!) in the name of compactness.

Is it actually key information though? If it isn't, we shouldn't be spamming the output with it.

@rohlem
Copy link
Contributor

rohlem commented Jul 10, 2024

We should not omit key information about the error (the types in a type mismatch!) in the name of compactness.

Is it actually key information though? If it isn't, we shouldn't be spamming the output with it.
Do you have an example at hand of where you would need the rest of the type?

The main scenario I was thinking of would be a generic function (accepting anytype) that calls a fn(type) type that only misbehaves for certain input types, say because it switches on @typeInfo internally.
Say it correctly maps *const u8 -> X(*const u8) but []u8 -> ?X([]u8) instead of X([]u8) for some reason, leading to a mismatch X([]u8) vs ?X([]u8) - the user would only see expected _, found ?_.
If it is called for 20 different input types and the compile error only triggers for one of those, it might be useful to know the exact type it is triggered with, to more quickly locate the corresponding logic / code path.

@tau-dev
Copy link
Contributor Author

tau-dev commented Jul 10, 2024

@rohlem excellent point, thanks

@tau-dev
Copy link
Contributor Author

tau-dev commented Jul 10, 2024

So something like this?

fn funcB(x: u32, y: u32, z: u8) u32 { return x + y + z; }
test { _ = @as(fn(u32, u64, u8) u32, funcB); }
x.zig:2:39: error: expected type 'fn ({a}, u64, {b}) {c}', found 'fn ({a}, u32, {b}) {c}'
x.zig:2:39: note: where {a} = u32
x.zig:2:39: note: where {b} = u8
x.zig:2:39: note: where {c} = u32

Imagine this only kicking in for much longer types names than u32.

@andrewrk
Copy link
Member

draft status, no updates in 30+ days

@andrewrk andrewrk closed this Oct 15, 2024
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.

4 participants