Skip to content
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

Cleanup return type open syms #261

Merged
merged 4 commits into from
Mar 30, 2022
Merged

Cleanup return type open syms #261

merged 4 commits into from
Mar 30, 2022

Conversation

Menduist
Copy link
Contributor

Fixes status-im/nimbus-eth1#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
Copy link
Contributor

mratsim commented Feb 23, 2022

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
Copy link
Contributor Author

Menduist commented Feb 23, 2022

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
Copy link
Contributor Author

nim-lang/Nim#11091 (comment)
Apparently we could also put it in the stdlib

@Araq
Copy link
Contributor

Araq commented Feb 23, 2022

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
Copy link
Contributor Author

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
Copy link
Contributor

mratsim commented Feb 24, 2022

In my experience working with types with macros is fraught with peril nim-lang/RFCs#44 and worse, this might manipulate generics which tend to become a timesink status-im/nimbus-eth2#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.

@Menduist Menduist requested a review from arnetheduck March 4, 2022 10:38
@Menduist Menduist mentioned this pull request Mar 4, 2022
@Menduist Menduist requested a review from zah March 7, 2022 09:25
@zah zah merged commit fc65ff8 into master Mar 30, 2022
@zah zah deleted the cleanupopensyms branch March 30, 2022 13:14
Menduist added a commit that referenced this pull request Apr 1, 2022
And more tests. Follow up of #261
Menduist added a commit that referenced this pull request May 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot instantiate Future got: <T> but expected: <T> after bumping nim-chronos
4 participants