-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
More colorful error messages #16356
More colorful error messages #16356
Conversation
This needs a switch, can break Testament tests that check output. |
Yea I've noticed that |
I dont remember but investigate this, it should allow to customize stacktrace messages without breaking the compiler and all tests: |
I'm uncertain how that module would help. |
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: dlang/dmd#6777 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.
examplecorresponding source code in D: exp.error("undefined identifier `%s`, did you mean %s `%s`?", exp.ident.toChars(), s2.kind(), s2.toChars()); |
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 |
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 |
compiler/msgs.nim
Outdated
@@ -486,6 +486,31 @@ proc formatMsg*(conf: ConfigRef; info: TLineInfo, msg: TMsgKind, arg: string): s | |||
else: ErrorTitle | |||
conf.toFileLineCol(info) & " " & title & getMessageStr(msg, arg) | |||
|
|||
proc colorError(s: string, color: ForegroundColor): string = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add a tests/compiler/tmsgs.nim test file with:
block: # colorError
# add tests here for `colorError`
(make some procs public if needed)
=> will make it easier to debug / improve it once we have tests (and would also avoid having to recompile nim just to test/improve this proc)
compiler/semcall.nim
Outdated
errUndeclaredField = "undeclared field: '$1'" | ||
errUndeclaredRoutine = "attempting to call undeclared routine: '$1'" | ||
errBadRoutine = "attempting to call routine: '$1'$2" | ||
errUndeclaredField = "undeclared field: $1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried your PR again, it's starting to look good!
- bad:
when true:
var a: string
a = 1+2
XDG_CONFIG_HOME= nim r main # ok
XDG_CONFIG_HOME= nim r --hint:Source main # bad
t11525.nim(12, 8) Error: type mismatch: got int literal(3) for 3 but expected string
a = 1+2
^
a = 1+2
isn't colorized
- bad:
echo 'proc fn(a = 0) = discard\nproc fn() = discard\nfn()' | nim r -
stdinfile.nim(3, 3) Error: ambiguous call; both stdinfile.fn(a: int) [proc declared in stdinfile.nim(1, 6)] and stdinfile.fn() [proc declared in stdinfile.nim(2, 6)] match for: ()
=> stdinfile.fn(a: int)
should be colorized but not the whole stdinfile.fn(a: int) [proc declared in stdinfile.nim(1, 6)]
(or at least only stdinfile.nim(1, 6)
should)
- bad
`nim --eval:'let a = "foo"; a.add 2'`
cmdfile.nims(1, 17) Error: type mismatch: got <string, int literal(2)>
but expected one of:
proc add(x: var string; y: char)
first type mismatch at position: 1
required type for x: var string
but expression a is immutable, not var
some improvements can be done there too:
<string, int literal(2)>
=>
'string, int literal(2)' so it colorizes
would be very cool to colorize only the mismatching argument in proc add(x: var string; y: char)
but that can be done in future work
- some quotes use backtick and should be changed to single quote
'
so they colorize properly:
when true:
proc fn[T]()
fn()
t11525.nim(59, 5) template/generic instantiation of `fn` from here
t11525.nim(58, 11) Error: cannot instantiate: T
- etc
there are other cases not properly colorized (eg hintCC); of course, can be done in future work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didnt even realize that --hint:source
was an option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try this with some Regex and PEG and see if it crashes.
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. |
@beef331 now that your other #18384 was merged, any interest in continuing this? I still prefer this approach to #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 |
I probably should 😅 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. |
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 |
@@ -5,29 +5,29 @@ t3330.nim(70, 4) Error: type mismatch: got <Bar[system.int]> | |||
but expected one of: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be done in the declared locs using the colors
as a control to toggle it.
@@ -5,29 +5,29 @@ t3330.nim(70, 4) Error: type mismatch: got <Bar[system.int]> | |||
but expected one of: | |||
proc test(foo: Foo[int]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea this could be done, though I personally dont like the boldness in the D message as i can barely notice a difference(though it may just be your font), ideally we could have a user defined string with a $color
as a marker for if they want colour, so then styling could be user definable using colour.
@@ -5,29 +5,29 @@ t3330.nim(70, 4) Error: type mismatch: got <Bar[system.int]> | |||
but expected one of: | |||
proc test(foo: Foo[int]) | |||
first type mismatch at position: 1 | |||
required type for foo: Foo[int] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO we shouldn't remove the quotes when coloring an expression, so that copy pasting a msg on terminal gives the same message with --colors:on vs --colors:off (avoids un-necessary complications)
as can be seen in https://github.com/nim-lang/Nim/pull/16356/files#r670864680, the quotes are currently removed when colored
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. |
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.
(EDIT) links