More colorful error messages

[beef331]

This PR uses ANSI colours to make the terminal output of errors quicker to read and spacing out the errors for overload errors. Highlighting the actual issues. Also adds a explict message about immutable variables being the cause of errors.

#Sample code used for this test.
let stringValue = "Hmm"
stringValue.add(10)

(EDIT) links

• refs https://github.com/nim-lang/RFCs/issues/87
• refs https://github.com/nim-lang/Nim/issues/2844#issuecomment-107060552

[juancarlospaco]

This needs a switch, can break Testament tests that check output.

[beef331]

Yea I've noticed that

[juancarlospaco]

I dont remember but investigate this, it should allow to customize stacktrace messages without breaking the compiler and all tests: https://nim-lang.github.io/Nim/stackframes.html#setFrameMsg.t%2Cstring%2Cstring Maybe you can improve it.

[beef331]

I'm uncertain how that module would help.

[timotheecour]

I support the idea of colorful error messages, but I'm not sure this is the right approach, because it pollutes compiler sources with formatting, in potentially lots of places.

D's approach seems better IMO, see how it was implemented here: https://github.com/dlang/dmd/pull/6777 and corresponding discussion here https://forum.dlang.org/post/[email protected]

in D's case, syntax highlighting is triggered when encountering backtick

`

in nim's case, we could trigger with single quote

'

instead, but the idea is the same. benefits:

• reuse existing logic for syntax highlight (and honors user customizations etc)
• works out of the box with compiler messages, without cluttering code with formatting logic; all that's needed is ensuring quotes are consistently, which can be done using an auxiliary function

example

corresponding source code in D: src/dmd/expressionsem.d:2656:24:

exp.error("undefined identifier `%s`, did you mean %s `%s`?", exp.ident.toChars(), s2.kind(), s2.toChars());

[beef331]

Yea I agree that the D approach is better, although I think something that lets you tell the style of the value would be better(immutable error being yellow and the immutable var in the error message being yellow). So I'm thinking of value, style tuples that use enums styles which means it can be switched off or customized by users rather easily.

[beef331]

Now on parity with my original PR, though I think I'm proceeding to still require changing a lot of the compiler to have the ' and '' in the error messages shoehorning in the feature instead of properly supporting it. At the very least it's easily toggable and can generate the same error as what's expected(minus the extra new line to make the overload error more readable).

[beef331]

Just want to update that I am continuing this in a different branch, but with manual highlighting as I and Clyybber feel it's more beneficial, although for more work. Using the quote method is quick, but it does not allow highlighting as much as possible in my view.

[timotheecour]

@beef331 now that your other https://github.com/nim-lang/Nim/pull/18384 was merged, any interest in continuing this?

I still prefer this approach to https://github.com/nim-lang/Nim/pull/16446, because it's simpler, and is also simpler for code that generates error messages which don't need to be aware of colors. It was also the option adopted in D.

I don't think we need advanced coloring options, simply highlighting any text inside delimiters (either single quotes or backticks TBD, easily changeable) plus maybe also highlighting known tokens like file(line,col) would be enough IMO.

[beef331]

I probably should :sweat_smile: the second method although more helpful is also much more to maintain and add to, so I sorta agree this method is the "better" one just due to the tedium otherwise required. Now I sorta agree we dont need hand coloured highlighting cause if the error messages need that, probably better to just clean them up.

[timotheecour]

great. Note also that for this PR, you don't need to cover all cases, IIRC what you had was already close to mergeable, it can always improve in future work; I'd recommend rebasing to fix bitrot conflicts, then i can take another look to see what should be done before merge vs what can be left for future work

[beef331]

Well hopefully all the tests pass now, but dont know with the colouring here due to hastily fixing the crashing, the required type probably shouldnt be quoted :smile:

[stale[bot]]

This pull request has been automatically marked as stale because it has not had recent activity. If you think it is still a valid PR, please rebase it on the latest devel; otherwise it will be closed. Thank you for your contributions.