search

status-im / nim-chronos

Chronos - An efficient library for asynchronous programming
https://status-im.github.io/nim-chronos/
Apache License 2.0
362 stars 51 forks source link

Cleanup return type open syms #261

Closed Menduist closed 2 years ago

Menduist commented 2 years ago

Fixes https://github.com/status-im/nimbus-eth1/issues/963 For some reason, the code was generating this kind of AST for the return type:

    Call
      OpenSymChoice
        Sym "[]"
        Sym "[]"
        ...
      Sym "Future"
      Call
        OpenSymChoice
          Sym "[]"
          Sym "[]"
          ..
        Ident "seq"
        Ident "OpenObject"

And this "recursive" OpenSymChoice wasn't handled properly by the macro

mratsim commented 2 years ago

I'm unsure how often we stumble upon this but maybe we need a toUntypedAST proc in stew to cleanup all the situation where we end up receiving typed AST in an untyped macros?

For example, I'm sure we hit this in serialization libraries macros. I didn't test using taskpools in a generic proc or a template yet but it's very possible as well.

Menduist commented 2 years ago

It would be a bit overkill for this fix, since we can only have OpenSyms AFAICT, but sure would be good to have as a general helper and this kind of bugs are hard to find

Menduist commented 2 years ago

https://github.com/nim-lang/Nim/issues/11091#issuecomment-486171129 Apparently we could also put it in the stdlib

Araq commented 2 years ago

The fix is acceptable but I would do it differently. I would use an extractReturnType helper instead. Your patch is conceptually quite brutal -- you change the AST to something else and then you extract what you're interested in.

Menduist commented 2 years ago

Not sure I understand @Araq, is it because I modify the AST in place instead of copying it? If so, last commit fixes that

mratsim commented 2 years ago

In my experience working with types with macros is fraught with peril https://github.com/nim-lang/RFCs/issues/44 and worse, this might manipulate generics which tend to become a timesink https://github.com/status-im/nimbus-eth2/issues/2219

If we can avoid manipulating types in macros and just modify AST to make it canonical untyped, it will likely save hours of debugging afterwards at the cost of forcing the compiler to redo semcheck/sigmatch.