[RFC] type mismatch error message improvement

[timotheecour]

• The following would be clearer and more concise; especially useful when lots of overloads are shown:

test.nim(883, 12) Error: type mismatch: got <Stream, TaintedString>
but expected one of:
proc readLine(f: File; line: var TaintedString): bool
  first type mismatch at position: 1
  required type: File
  but expression 'outp' is of type: Stream
proc readLine(f: File): TaintedString
  first type mismatch at position: 1
  required type: File
  but expression 'outp' is of type: Stream

expression: readLine(outp, line)
      if outp.readLine(line):

=>

test.nim(883, 12) {if outp.readLine(line):} Error: type mismatch: got <Stream, TaintedString> in expression: readLine(outp, line); candidates:
  proc readLine(f: File {Stream}; line: var TaintedString): bool
  proc readLine(f: File {Stream}): TaintedString

EDIT

• furthermore, we should show fully qualified types instead of types, to make it easier to find where symbols are defined and remove ambiguities, eg in case Stream is defined in multiple modules:

  proc readLine(f: File {Stream}; line: var TaintedString): bool
=>
  proc readLine(f: mod1.File {mod2.Stream}; line: var mod3.TaintedString): bool

EDIT the overloads should also show the module + file where they came from (not obvious in complex cases)

[dom96]

The compiler should use colours and arrows for this:

if outp.readLine(line):
     ^----- `outp` is of type 'Stream' which doesn't match any candidates
test.nim(883, 12) Error: type mismatch: got <Stream, TaintedString>
but expected one of:
proc readLine(*r*f: File*r*; *g*line: var TaintedString*g*): bool
proc readLine(*r*f: File*r*): TaintedString

Where text delimited by *r* is red and by *g* is green.

[cheatfate]

@dom96 variant is much better, because it actually shows and highlights position of wrong argument, but your variant @timotheecour is hiding position at all.

[andreaferretti]

I actually like the existing version. It is a little verbose, but for a good reason: for each candidate it shows exactly why the matching didn't work. All other versions are not only shorter, but less explicit. There were many issues requesting better error messages along the lines of those of - say - Elm, and this one requests to go back to a less explicit form

[Vindaar]

I agree with @andreaferretti. Although adding a little bit of color would be a great addition!

[data-man]

It is a little verbose, but for a good reason: for each candidate it shows exactly why the matching didn't work.

What if the verbosed version will shown only with --verbosity:2 (or 3) switch?

[andreaferretti]

@data-man I think the verbose version is most useful for beginners, which probably do not even know about the verbosity switch. Maybe --shortErrorMessages?

[data-man]

@andreaferretti

which probably do not even know about the verbosity switch.

Yes. We can add defaultVerbosityLevel to cfg (now 1 is default).

--shortErrorMessages

Alternative version: --errorsverbosity:0|1|2|3 :-)

[dom96]

Some inspiration:

(Source)

[timotheecour]

@cheatfate

@dom96 variant is much better, because it actually shows and highlights position of wrong argument, but your variant @timotheecour is hiding position at all.

how is it hiding it?

proc readLine(f: File {Stream}; line: var TaintedString): bool
proc readLine(f: File {Stream}): TaintedString

shows the first argument is wrong, showing the mismatch between expected (File) and actual (Stream).

Obviously, colors can be added.

[cheatfate]

@timotheecour, your example is using just 2 arguments, try to use like 4-6 arguments and like 10-20 variants. Original version shows number of argument, while your version exactly hides this information and if function has much more arguments, your error output become much less valuable to quickly find a problem.

[timotheecour]

@cheatfate here's an updated version which shows number of arguments when there's a number of argument mismatch, and shows A {B} when there's no number of argument mismatch but a type mismatch:

test.nim(883, 12) {if outp.readLine(line, 1, 1+1):} Error: type mismatch: got <Stream, TaintedString, int, int> in expression: readLine(outp, line, 1, 1+1); candidates:
  proc readLine(f: File {Stream}; line: var TaintedString, a: A, b: B): bool
  proc readLine(f: Stream; line: var string {TaintedString}, a: A, b: B): bool
  proc readLine(f: Stream; line: var string, a: A): bool (3 arguments expected, got 4)
  proc readLine(f: Stream; line: var string): bool (2 arguments expected, got 4)

[andreaferretti]

@timotheecour The existing version pinpoints exactly, for each variant, which arguments do not match. For your version, once has to manually figure out by comparing the expected arguments with all the variants. This is why I find the current version more valuable

[alehander92]

What's the status here? That would be a very nice error improvement.
I really like @timotheecour 's format + @dom96 's color hints. The only thing I'd add would be maybe
not repeating proc everywhere like in nim-lang/Nim#8656

proc readLine:
  (args)
  (args)

@andreaferretti I absolutely don't agree, something like @dom96's ^ or color hints keeps the information there without taking a screen of space.

[andreaferretti]

I find the lines like

  first type mismatch at position: 1
  required type: File
  but expression 'outp' is of type: Stream

valuable, and they are lost in the other version, color or not

[alehander92]

but the info is not lost: with color or ^ you can see which argument doesn't match and then you can see the original argument there.

you can even have required: File, got: Stream in the end of the line

however having 3 lines for this , when it can be expressed in much less is a waste of space and not nice with long lists

[alehander92]

@andreaferretti e.g. compare the two code snippets in nim-lang/Nim#8656
isn't the second one way more clear? (disclaimer: the style there is different than the one this issue, but I think it looks similarly enough to demonstrate)

[alehander92]

some other benefits are that now when I see an error, it's very hard for me to analyze the other signatures: I have to read through all the additional lines. In a shorter view grouped by types it would be easier to find overloads for your usecase

[andreaferretti]

@alehander42 Yes, in the code snippet in #8656 the short version is a little more clear. But in the most common case, where you have few overloads and more arguments, I find it easier to pinpoint which argument does not match in each overload, at the cost of being a little repetitive

[Araq]

I'm sure we can come up with a solution that is just as clear but a little less verbose.

[alehander92]

@andreaferretti would e.g. a combination of https://github.com/nim-lang/Nim/issues/8200#issuecomment-402243841 (maybe with ^) and additional (expected: type) at the end of the line make it better ? I think we just need several more examples to compare side by side

[andreaferretti]

@alehander42 I am not sure I understand exactly what you describe, but I guess that would do in this case. The issue is that different overloads may have different combinations of parameters that match. To make a concrete example, say I write

let
  s = "hi there"
  a = @[1, 2, 3]
discard(a & s)

The current error reads

Error: type mismatch: got <seq[int], string>
but expected one of: 
proc `&`[T](x, y: seq[T]): seq[T]
  first type mismatch at position: 2
  required type: seq[T]
  but expression 's' is of type: string
proc `&`[T](x: T; y: seq[T]): seq[T]
  first type mismatch at position: 2
  required type: seq[T]
  but expression 's' is of type: string
proc `&`[T](x: seq[T]; y: T): seq[T]
  first type mismatch at position: 2
  required type: T
  but expression 's' is of type: string

expression: a & s

To me, this is missing a few other overloads (I think this is just a bug), so a complete error would also include

proc `&`(x: string; y: char): string
  first type mismatch at position: 1
  required type: string
  but expression 'a' is of type: seq[int]
proc `&`(x, y: string): string
  first type mismatch at position: 1
  required type: string
  but expression 'a' is of type: seq[int]

In total, we have 5 overloads, and sometimes the mismatch is at position 1, sometimes at position 2, depending on the overload.

Let me try to write this in your suggested style

discard(a & s):
            ^----- `s` is of type 'string' which doesn't match any candidates
test.nim(883, 12) Error: type mismatch: got <seq[int], string>
but expected one of:
proc `&`[T](x, y: seq[T]): seq[T]
proc `&`[T](x: T; y: seq[T]): seq[T]
proc `&`[T](x: seq[T]; y: T): seq[T]
proc `&`(x: string; y: char): string
proc `&`(x, y: string): string

This is (may be) misleading: possibly the error is actually in position 1, so s is correct but a is not. The compiler cannot show with an arrow where the error is, because we are not certain - both arguments could be the culprit.

Using colors to show matches/mismatches on the other hand may be a good way to go, because then we have a different information in each overload. The user still has to compare this with the provided arguments, but it is an ok solution

[Araq]

To me, this is missing a few other overloads (I think this is just a bug), so a complete error would also include

No, that was a deliberate change by me. The idea is that we treat the procs as attached to the first parameter's type and so cut these out. That's pretty much how C# does it and C# is doing fine.

[andreaferretti]

Then most of what I am saying makes little sense. My whole point was that in the current version you get the full list of overloads and you can check which one has mismatches and in which position.

Since in Nim proc are not actually attached to the first parameter - which has no special relevance in the language, except for a syntactic convenience - showing only some of the existing overloads is actively harmful. In C# I know how resolution works and I know that the compiler shows methods defined on an object. In Nim, this isn't so, and missing some of the overloads can easily be puzzling (shouldn't there be an overload that applies? I remember I saw it in the documentation... Let me check... Uhm, it's still there, am I even browsing the current version of the docs? Maybe on github docs are more up to date...)

[Araq]

Ok, will revert this change then. Have fun with the irrelevant overloads.

[alehander92]

@andreaferretti
no, the point is exactly that the errors will also show you the position in the overloads where the argument doesn't match: so you'll have exactly the same info

The compiler cannot show with an arrow where the error is : the arrow will be for each overload, not for the original type. it will be an alternative for the color.

[alehander92]

@andreaferretti basically what I meant was that I also prefer the color solution , but if one doesn't have colors, we can use arrows: otherwise I agree with you, we need the same info, visualized optimally

[timotheecour]

@Araq @andreaferretti

Ok, will revert this change then. Have fun with the irrelevant overloads.

I think we can get the best of both worlds, see proposal:
nim --verboselog:/tmp/log.txt ... to log more context when needed while not obfuscating stdout · Issue #8789

[alehander92]

Ok, so what is the consensus? @andreaferretti you thumbed up my last comment: does it mean you agree that #8200 (comment) is a good solution (sorry for the lack of clarity before that)?

[andreaferretti]

@alehander42 Yes, I agree that the information can be organized to take less visual space, using colors where available and arrows otherwise (I hope there is a library for that! :-D). My only requirement is not to lose the information that we have available now to free up space - I am ok with every rearranging that preserves the semantics, and I think that colors can be very useful for that

[krux02]

I like the error messages as they are. They are a bit verbose, but I am fine with it, and I got used to it. I would be ok to use the nimvm to register an error message callback that takes care to format error messages to arbitrarily. if somebody then develops objectively better error messages, I am ok to take them as the new default. But I don't like the discussions about opinions how error messages should look like.

The suggested error message is more compact, yes. But it is not clear what the {} means to any programmer. It is not a convention that I have seen anywhere.

[alehander92]

@krux02

I like the error messages as they are.

I also like a lot of the other error messages, obviously some of them can be even better with some love

and I got used to it

"Getting used" to something is usually a symptom of it being suboptimal :)
We all have different pain points.

I would be ok to use the nimvm to register an error message callback that takes care to format error messages to arbitrarily.

That's a very cool idea, but I am not sure it's top priority: the more important thing is to have a nice default and to be equally useful to pro users and beginners in the same time.

if somebody then develops objectively better error messages .. But I don't like the discussions about opinions how error messages should look like.

People proposed several good designs: there is no need to dismiss them.

Objectively some of the proposed solutions preserve all of the pro-s of the existing solution, + a better grouped output/way easier to read quickly output.

The {} doesn't seem optimal: but the discussion in the later comments is also about https://github.com/nim-lang/Nim/issues/8200#issuecomment-402243841 .

It doesn't use {}, it uses colors or ---^ markers which are both obvious. For no-color mode, one can use something different than {}: maybe some other details can be changed too, but the overall plan seems good

[alehander92]

@krux02

But I don't like the discussions about opinions how error messages should look like.

Well, error messages are the probably the main interface of the compiler: they're 90% of the whole interaction of the user with it. It's extremely important how they look

[krux02]

@alehander42 well that is a fair point.

[EriKWDev]

Made some Mockups in #323. Pasting one of them here with type mismatches for thoughts. Perhaps a bit verbose and I have no idea about feasibility, but arrows and colors ftw! And Rust has managed to do it similarly so Nim could too!

Many type mismatches

file pr.nim

let stringValue = "Hmm"
stringValue.add(10)

Real vs. Mockup

[Araq]

Colors can be hard to read if the contrast with the background isn't good enough. And you don't know the background color (though maybe you could query it?). Instead of colors, make the error messages less verbose and add a link to a HTML page that explains more.

Good error messages are subjective, not everybody appreciates the idea that the programmer uses a terminal from 1970 lacking a direct view into the source code so that the compiler has to output the source code side-by-side with the error message.

[haxscramper]

The point of annotations like these is not to provide help for a programmer that "uses a terminal from 1970 lacking a direct view into the source code" but to show important context in one place, so users won't have to reach into three separate locations (their editor, the HTML page with more explanation, and the original message of the error).

[Araq]

Well since the error message should "pop up" in the editor there is no point in repeating the source code line.

[Clonkk]

Good error messages are subjective, not everybody appreciates the idea that the programmer uses a terminal from 1970 lacking a direct view into the source code so that the compiler has to output the source code side-by-side with the error message.

Well since the error message should "pop up" in the editor there is no point in repeating the source code line.

I don't understand why the two should be in opposition. There is no reason why the output / formatting of either the LSP server or nimsuggest (which is what your IDE receive information from) should be identical to the formatting of the console stdout output.

Why can't we have good stdout error message in the console AND good formatting in your IDE for those who don't like CLI ?

Edit: okay, apparently this is not possible with the current implementation so disregard previous point

[Vindaar]

Edit: okay, apparently this is not possible with the current implementation so disregard previous point

Even if that may be the case, that is not a good reason to not provide said feature. It only means the implementation might be more work (pretty sure @haxscramper has invested huge amounts of time improving the error message handling in the compiler somewhere......... 😮‍💨 ).

Pure CLI based error messages are still extremely important, imo. First of all there's just plenty of people that use and will continue to use emacs / vim and likely compile their code in their terminal and second even if you're not that kind of person: what happens if you have to ssh into some machine and fix some piece of code there? May not happen to you, but it certainly happens a lot in academia when you're once again asked to fix someone's code running on some computer in some lab. Best programming practices are unfortunately not the norm everywhere.

[haxscramper]

Playground also does not have support for any advanced IDE features, but I assume a lot of people either do scratch tests there, or maybe even try out the language for the first time (in that case quality of error messages quite important, it might be a main deciding factor).

[SolitudeSF]

Colors can be hard to read if the contrast with the background isn't good enough. And you don't know the background color (though maybe you could query it?).

Thats why all important info that you will actively read is colorless and colored segments are just visual cues.
This is the most random argument i've seen here.

[haxscramper]

Also, --colors=off exists and can be used if someone doesn't like coloring

[Araq]

Thats why all important info that you will actively read is colorless and colored segments are just visual cues.

That's not what the example does, a significant part of the error was in yellow...

[haxscramper]

Visual cues - underline, error explanation is colored. Important part - the error itself (type mismatch) and procedure signatures are colorless. We can start debating what is the significant part here, or whether we can color cues or significant parts, and so on, but this is so besides the point, that I don't think it will bring any value to the discussion.

[Araq]

There is indeed no value in this discussion. I give up and accept I'm the only one left who doesn't like clang's / Rust's / Elm's error messages.

PRs are accepted.

[EriKWDev]

I'm trying out the red/green in overloads to show what matches and what doesn't. Don't know if it's as intuitive as one might first think..

a significant part of the error was in yellow...

Ye, not the best choice xP Should've probably just let it be white...

Good error messages are subjective [...]
I'm the only one left who doesn't like clang's / Rust's / Elm's error messages. [...]

Very true, and I beleive that it is important to acknowledge that not everyone wants these kinds of error messages. I feel like some of the variants discussed here should be tweakable and perhaps, as also discussed, only shown with certain verbosity levels or flags. I would still argue though that having more clear as a default would go a long way in terms of getting through what has gone wrong.

I've always felt that the first type mismatch at position: 1 [..] is very 'verbose' for what is being conveyed and having a red arrow gets across the exact same information without a bunch of text which I would consider less verbose.

PRs are accepted.

I've never contributed to the compiler before but I would really like to experiment a bit. Update: Checking out compiler/msgs.nim and read the compiler internals documentation. Exciting to see the internals of Nim xP Seems like liMessage and writeContext are of interest as well ass compiler/semcall.nim's presentFailedCandidates...