Nim icon indicating copy to clipboard operation
Nim copied to clipboard

Do not expand macros in most error messages

Open beef331 opened this issue 3 years ago • 0 comments

This PR is another attempt at making macros/templates less difficult to understand when something is semantically wrong with their usage. It uses a table to hold onto expanded macros for reference in case of an error, the usage of this code should be capable of being disabled with --nimMacrosExpandErrors.

beef331 avatar Aug 09 '22 05:08 beef331

I don't know. Error messages that teach you how Nim actually works are not bad and all this additional logic in the compiler eventually makes it harder and harder to evolve.

Araq avatar Aug 24 '22 12:08 Araq

@beef331 Can you give an example of output before and after this change?

Varriount avatar Aug 24 '22 20:08 Varriount

Error messages that teach you how Nim actually works are not bad

I find this sentiment a bit dubious as it's not teaching one how Nim works, it's more just irritating the user cause the error message is unintelligible. Unintentionally obfuscating the error makes it harder to reason the error. The error gets more and more difficult to find the cause the deeper the template/macro gets. I do not know if this is the best solution to showing the source, but I still think there has to be some improvement on the error messages of macros as it can be difficult even for a competent Nim programmer to figure out what the problem actually is.

@Varriount

import std/[asyncdispatch, macros]

proc doThing: Future[void] {.async.} = discard
proc doOtherThing {.async.} = discard await doThing()

used to error:

Error: expression '
var internalTmpFuture`gensym14: FutureBase = doThing()
yield internalTmpFuture`gensym14
read(cast[typeof(doThing())](internalTmpFuture`gensym14))' has no type (or is ambiguous)

But now would error

tunexpandedmacro.nim(25, 17) Error: expression 'waitFor doThing()' has no type (or is ambiguous)

Another example

template doThing(a: int): untyped =
  case a:
  of 100: 300i64
  of 200: 400i64
  else: a
var a: float = doThing(300)

used to error:

tmp/test.nim(6, 16) Error: type mismatch: got 'int64' for 'case 300
of 100:
  300'i64
of 200:
  400'i64
else:
  300' but expected 'float'

but now would error:

/tmp/test.nim(6, 16) Error: type mismatch: got 'int64' for 'doThing(300)' but expected 'float'

Presently this does not render each ast as it's source if it was expanded, it just does it at root level.

beef331 avatar Aug 24 '22 20:08 beef331

What about a compromise? Something like

/tmp/test.nim(6, 16) Error: type mismatch: got 'int64' for 'doThing(300)' but expected 'float'
Expanded expression:
  case 300
  of 100:
    300'i64
  of 200:
    400'i64
  else:
    300

That way, the user can see both the original expression and its expanded form, which (to my mind) makes it much easier to learn how one expression relates to the other.

Varriount avatar Aug 25 '22 00:08 Varriount