flyx / NimYAML

YAML implementation for Nim
https://nimyaml.org
Other
191 stars 36 forks source link

Fixes #54 #56

Closed sthenic closed 6 years ago

sthenic commented 6 years ago

Hi,

I would like to preface this by say that I am by no means an expert in Nim. I just picked up the language a few weeks ago so macros are a bit beyond me at this point. However, I have a project where I would like to parse some YAML files so I took a stab at trying to resolve this issue anyway.

The offending line of code seems to be yaml/serialization.nim:1060 and the instantiation of the internalError() template. As has been pointed out in #54, the error message states:

'quit 1' is of type 'NimNode' and has to be discarded

I replaced the instantiation of internalError() on line 1060 with simply quit(1) and unexpectedly got the same error. Attempting to discard the return value with

discard quit(1)

instead yielded the error message

expression 'quit(1)' has no type

which seems peculiar since it openly contradicts the previous error message.

I restored the macro instantiation and added an internal quit procedure to yaml/private/internal.nim that looked like

proc quit(errorcode: int) {.discardable, inline.} = system.quit(errorcode)

and then replaced the two calls to quit() with unambiguous calls to internal.quit().

This allowed the code to compile. I thought that it was the discardable pragma that allowed the call to quit() to be correctly compiled. So just to be sure, I removed it. To my surprise the code still complied with the internal procedure defined as

proc quit(errorcode: int) = system.quit(errorcode)

which seemed odd.

At this point I reverted every change I had and instead tried

...
1060 else:
1061   block:
1062     internalError("Too many children: " & $recCase[i][1].recListlen)
...

in yaml/serialization.nim. This also allowed the code to compile without any issues.

My best guess would be that this is unintended behavior and caused by a regression in the Nim compiler when dealing with macros. There are many other places throughout the project where the internalError() template is used, some of which in similar contexts, i.e. in a macro. Somehow these don't cause any issues. Perhaps that should be taken as support for the theory that this is indeed unintended behavior.

I tried creating a MWE to provoke the error involving a macro and the quit() procedure but I could never get it to crash, the compilation was always successful. Granted, macros are a bit above my level at the moment so I might have stripped away some important detail and made it too simple.

Out of all the things I tried I hope you will agree that the block approach is the least invasive and 'easiest' to motivate. It's still a workaround but should have no impact on the overall behavior.