display overload type mismatches as highlighted comments

[adamdruppe]

One of the things that I dislike about D's error messages is not giving information the compiler knows in an easy-to-read way. For example, in a no overload matches message, lining up what you passed with what the compiler expects is a huge pain for non-trivial functions.

I was thinking of coloring the matches green and mismatches red, but I have an even better idea now: print the mismatched type as a comment in the overload. Then, when you look down the candidates, you can easily see "oh I passed a string[] when it was expecting a string" without needing try to count commas yourself. See the following picture:

With syntax highlighting, the rare comment in an error message can stand out making it easy to scan for them - helping mitigate my objection to syntax highlighting in the first place - and without syntax highlighting or color support in general, an inline comment is still readable by the user. Everybody wins. If lines become too long, we can even run this through something akin to dfmt... but that's a future idea.

If this works, I'd like to do something similar for template constraints.

[CyberShadow]

From a failing test case:

fail_compilation\ice14923.d(21): Error: function ice14923.parse `(/* A */ C a)` is not callable using argument types `(A)`

Not sure about this case, here the comment is displayed before explaining the error fully and mentioning what A is. Maybe the message could simply be reworded.

[adamdruppe]

Yeah, I was just editing the test to make it pass, though I'm not 100% happy with the message yet either.

The first time you see it, the comment might be weird, but I think users will be able to figure it out pretty quickly and find it as useful as I do once they do... in fact, the "using argument types %s" portion is redundant now, assuming I did this right, and perhaps we should just remove that part, but a more conservative change would be

Argument types %s cannot be used to call function %s(/* arg given */ arg expected)

then it is introduced early, just like overloads, I think that would be pretty good, and moving the args to the end of the line means we can format it more easily later into multiple lines if needed.

[CyberShadow]

but a more conservative change would be

Argument types %s cannot be used to call function %s(/* arg given */ arg expected)

That's an improvement, though wording is a little bit awkward.

I think we shouldn't be afraid of putting error messages across multiple lines. At least Rust and Haskell do it, and I don't think that's a frequent complaint for them.

[CyberShadow]

Using the error message for overloads as a base, it could look like this:

Error: the function `foo` is not callable using argument types `(int, string[])`, its signature is:
       test.foo(int _param_0, /* string[] */ string _param_1)

[adamdruppe]

On Wed, May 17, 2017 at 08:19:57PM -0700, Vladimir Panteleev wrote: I think we shouldn't be afraid of putting error messages across multiple lines. At least Rust and Haskell do it, and I don't think that's a frequent complaint for them.

Oh, absolutely, actually I'd like to open this up a lot later. Though Andrei and I have butted heads over my preference of using many lines in documentation... and for now, I just want to get the overload display so I'm being as conservative as I can for the first go. Nevertheless, I did just change it to the "the signature is:" idea, just like overloads.

[jacob-carlborg]

I'm not sure I really like this way of doing it. How would the programmer know that the comment is the mismatched type? The original declarations might actual look exactly like that, with the comment. That might be very confusing. I prefer the way Clang prints the error message:

main.cpp:13:3: error: no matching function for call to 'foo'
  foo(1, 1);
  ^~~
main.cpp:8:6: note: candidate function not viable: no known conversion from 'int' to 'Foo' for 2nd argument
void foo(int a, Foo b) {}
     ^
main.cpp:9:6: note: candidate function not viable: no known conversion from 'int' to 'Bar' for 2nd argument
void foo(int a, Bar b) {}
     ^
1 error generated.

It explicitly says which argument that failed (the position), the expected and the given types. Although I think that Clang's error message can be improved as well. To explicitly mention all types of the arguments and be more explicit which type that failed to match. Something like:

main.cpp:13:3: error: no matching function for call to 'foo' of type (int, int)
  foo(1, 1);
  ^~~
main.cpp:8:6: note: candidate function not viable: expected 'int', given 'Foo' for 2nd argument
void foo(int a, Foo b) {}
     ^
main.cpp:9:6: note: candidate function not viable: expected 'int', given 'Bar' for 2nd argument
void foo(int a, Bar b) {}
     ^
1 error generated.

[UplinkCoder]

that shouldn't be in here.

the hdrgen is supposed to do AST-printing only.

[adamdruppe]

I'm altering its purpose. Pray I don't alter it any further.

[UplinkCoder]

I know it's on purpose.
This has to go into a separate function.
maybe parameterErrorsToBuffer ?

[adamdruppe]

On Wed, May 17, 2017 at 11:37:35PM -0700, jacob-carlborg wrote: I'm not sure I really like this way of doing it. How would the programmer know that the comment is the mismatched type?

They can learn it pretty quickly. The first time you see it, it might look a bit weird, but then you either look it up and learn the shortcut, or ignore it and be no worse off than you are now and hopefully notice the pattern as you keep using dmd and eventually learn it that way.

I prefer the way Clang prints the error message: Although I think that Clang's error message can be improved as well.

Yeah, that's not bad either.

[adamdruppe]

One of the things I really like about the inline display is that it makes it clear if you forget something in the middle of a long call. Take a look here:

Take a look at that and you can tell pretty easily that things are off-by-one; the bool arg is just missing. (of course you could levenshteinDistance that too... but eyeball does just as well here)

This is what g++ gives:

a.cpp: In function ‘int main()’:
a.cpp:5:24: error: cannot convert ‘Color’ to ‘bool’ for argument ‘5’ to ‘void bar(int, int, int, int, bool, Color)’
  bar(0, 0, 0,0, Color());
                        ^

Not bad, being told it is argument 5 and printing the line points you to the right place, but it doesn't tell you at a glance that something is missing.

and clang:

a.cpp:5:2: error: no matching function for call to 'bar'
        bar(0, 0, 0,0, Color());
        ^~~
a.cpp:2:6: note: candidate function not viable: requires 6 arguments, but 5 were provided
void bar(int x, int y, int w, int h, bool cool, Color c);
      ^

That's the worst of the bunch, it tells you something is missing, but doesn't tell you what.

With the inline comment display plus the colors, your eyes can quickly scan the line and see what needs to be seen without reconstructing a mental image of what argument 5 is or painstakingly finding which argument was missing in the middle.

In this case, I wouldn't mind doing what g++ does.... but I really think the inline comment, once you learn it - I grant first time users might scratch their heads but insist they'll learn it quickly - gives the best presentation I've seen so far for all these cases.

[CyberShadow]

@adamdruppe I think you will need to update some tests in light of some other PRs that have been merged since.

@WalterBright Anything fundamental that's blocking this PR, or can it be merged once it's green?

[CyberShadow]

Not a fault of this PR, but the colours look kinda awful on a black-on-white terminal:

[CyberShadow]

I guess this doesn't touch templated functions, only regular ones. Tried against issue 7841's test case and there is no difference there.

test.d(3): Error: template test.foo cannot deduce function from argument types !()(const(int[])), candidates are:
test.d(1):        test.foo(T)(ref T[] a)

[adamdruppe]

On Sun, Jun 25, 2017 at 04:59:45AM -0700, Vladimir Panteleev wrote: @adamdruppe I think you will need to update some tests in light of some other PRs that have been merged since.

Yeah, at the time I opened it Walter was still changing tons of messages himself so I decided to wait a bit till he finished his pass to avoid inevitable merge conflicts. But I can come back to it this week and catch up now.

[adamdruppe]

On Sun, Jun 25, 2017 at 05:03:57AM -0700, Vladimir Panteleev wrote: Not a fault of this PR, but the colours look kinda awful on a black-on-white terminal:

oh get a better terminal! The problem is that this is a no win scenario for the application. You cannot detect light vs dark from inside, and the default palette has colors that are barely legible on one scheme or the other. Bright blue, yellow and green are impossible to see on white. Dark blue is particularly hard to see on dark. (I used to find it hard to see "Deprecation" on my preferred white terminals due to its coloration.) Many applications will use a setting or environment variable (e.g. vim's `set bg=dark` or elvis' `ELVIS_BG=dark`) to switch palettes on demand from the user, but short of that, the best an application can do is stick... probably to just regular red and purple, and maybe dark green since they are the best looking on both. But this bothered me so much that in my terminal emulator, it detects various low contrast colors as it displays them and adjusts its palette automatically. You can output any combination of the 16 bit palette to it and it remains legible (of course, you can still break it using 8 or 24 bit colors, eek). I'm a bit amazed more terminal emulators haven't done that; the exact colors aren't important in a terminal, just that you have a legible variety. Anyway, for dmd... well, I've said I think the syntax highlight is too much anyway, so I'd probably restrict the palette to those decent contrast on light or dark selections and use color judiciously.

[CyberShadow]

The problem is that this is a no win scenario for the application. You cannot detect light vs dark from inside, and the default palette has colors that are barely legible on one scheme or the other.

I think there's plenty of colors to choose from that will look fine on both dark and light backgrounds.

oh get a better terminal!

My terminal is awesome. Regardless of that, it doesn't look great on Windows either, and you don't really have a say there:

[adamdruppe]

On Mon, Jun 26, 2017 at 10:44:47AM -0700, Vladimir Panteleev wrote: I think there's plenty of colors to choose from that will look fine on both dark and light backgrounds.

Eh, I'd say about seven of them are passable (including bright versions), but then we need to use ones that are different enough from each other too. I'd use: * default, obviously * bright red * dark purple * dark teal (bright teal is totally unusable on white) * maybe bright blue * maybe bright purple Which is plenty for dmd. And we could use the others for things we don't actually want to read if we have to, but I think we're best keeping to these (probably in the order of preference I wrote them too) since they will give best results for all viewers.

My terminal is awesome. Regardless of that, it doesn't look great on Windows either, and you don't really have a say there:

Actually, you have plenty of say on Windows since you can change the palette there.... at least you the end user. I don't believe that is accessible through the API though.

[CyberShadow]

Actually, you have plenty of say on Windows since you can change the palette there.... at least you the end user. I don't believe that is accessible through the API though.

Err, yes, but we can't expect people to do that just to make the error messages readable. On Linux you could at least wave your arms and say "eeeh there are thousands of terminal emulators, it's going to look bad on some of them no matter what colors you choose", but you don't have that luxury on Windows.

Speaking of said variety of terminals, I think making it look OK on iTerm (OSX default terminal, has a white background by default) and Gnome Terminal (Ubuntu default terminal, dark background by default) should be enough.

[adamdruppe]

On Mon, Jun 26, 2017 at 11:24:16AM -0700, Vladimir Panteleev wrote: Err, yes, but we can't expect people to do that just to make the error messages readable.

Yeah, I know.

Speaking of said variety of terminals, I think making it look OK on iTerm (OSX default terminal, has a white background by default) and Gnome Terminal (Ubuntu default terminal, dark background by default) should be enough.

Sounds good to me. I just think those options I laid out are the best ones to use, but we can prove it with those tests.

[jacob-carlborg]

iTerm with solarized light, solarized dark (the two themes I'm using) and default:

[CyberShadow]

iTerm with solarized light, solarized dark (the two themes I'm using) and default:

Thanks. My bad though, iTerm is not the default terminal in OSX, I was thinking of the Terminal app, which has a white background by default.

[jacob-carlborg]

And here is Terminal, default theme:

[adamdruppe]

On Mon, Jun 26, 2017 at 11:44:58AM -0700, jacob-carlborg wrote: And here is Terminal, default theme:

Looks like the Mac terminal uses a nicer default palette than the xterm and Windows consoles since that looks ok.

[jacob-carlborg]

Yeah, the example CyberShadow shows above #6806 (comment) looks like the bright colors on Mac. The normal colors are not as bright, naturally.

[PetarKirov]

Why not simply allow specifying the colors in dmd.conf / sc.ini (or as a command-line argument / environment variable)? I'm sure that if we do that, soon people would start sharing syntax highlighting themes for dmd. That way dmd's output could look on everybody's terminal. And most importantly - no need to argue about the color of the bike shed :D

[adamdruppe]

On Tue, Jun 27, 2017 at 03:42:17AM -0700, Petar Kirov wrote: Why not simply allow specifying the colors in dmd.conf / sc.ini?

You'd still need legible defaults, since very, very few people are going to mess with that file anyway.

[CyberShadow]

but the colours look kinda awful

-> #6943

[MetaLang]

I like the sentiment but we should not be inserting stuff into people's code that's not really there. We should do it properly, as @jacob-carlborg suggested.

[CyberShadow]

@MetaLang @jacob-carlborg Can you put together a pull request implementing the idea? If not, then we might as well go with the approach here, as it's simpler to implement, and we already have an implementation, and it's still an improvement over status quo, and it's not a problem to change it at any time.

[adamdruppe]

This isn't people's code. It is a compiler-generated error message built from the AST. You'll learn how to read it very quickly and then the benefits of inline details shine through the spam. Especially consider the same pattern applying to template args and constraint conditions.

[MetaLang]

@CyberShadow I don't, but considering there is a dub library that already does this, why wouldn't we want to go for a more robust implementation instead of a quick fix that will need replacing down the line?

[MetaLang]

@adamdruppe I'm thinking from the POV of somebody new to D and/or programming. When they see an error message like this I can guarantee their response will be "WTF? I didn't write this!"

[adamdruppe]

On Mon, Jul 10, 2017 at 02:58:40PM +0000, MetaLang wrote: @adamdruppe I'm thinking from the POV of somebody new to D and/or programming. When they see an error message like this I can guarantee their response will be "WTF? I didn't write this!"

Then they realize "oh that's a tip from the compiler as part of a legible error message" and five seconds later, have learned a new job skill that will save them hours down the road. The compiler already reformats whitespace in error messages (and does a piss poor job of it, but I'd like to improve that later too) and surrounds it with other stuff.

[adamdruppe]

On Mon, Jul 10, 2017 at 02:56:57PM +0000, MetaLang wrote: @CyberShadow I don't, but considering there is a [dub library](http://code.dlang.org/packages/concepts) that already does this

I don't see how that's related at all. This PR handles standard function overloads, no templates. Templates are coming later, and the benefit of the compiler doing it is that it will be legible for any constraint of any combinations of functions.

[MetaLang]

@adamdruppe You're right, I misinterpreted the purpose of this PR. In that light I more strongly feel that this should not be merged, but I can't say that I care all that much. I wouldn't hate if it was merged but I would prefer if it wasn't.

[p0nce]

FWIW I spend a lot of time looking for what argument are wrong in function parameter list. The current message often is very long to parse and finding manually the difference between the call and function is the most time-consuming error to fix.

[JinShil]

#7554 and #7584 look like promising alternatives to this. OK to close?

[adamdruppe]

Yeah. I like mine better still, but I've given up on getting it upstream and just maintain a private fork for myself now anyway, so might as well just close this regardless.