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

macros: make quote a proper quasi-quoting operator #1393

Merged
merged 11 commits into from
Aug 1, 2024

Conversation

zerbina
Copy link
Collaborator

@zerbina zerbina commented Jul 31, 2024

Summary

quote now keeps the quoted block as is, which means that:

  • no symbols are bound or mixed in automatically from the enclosing
    scopes
  • identifiers in definition positions aren't turned into gensyms; they
    stay as is
  • .gensym and .inject pragmas within the quoted block don't affect
    the AST

Symbols thus need to be bound or created explicitly, via bindSym and
genSym, respectively. This is a breaking change.

Details

Motivation For The Change

  • symbols from the quote's scope being bound is error-prone, leading
    to confusing compilation errors
  • the intention of documentation of quote already said it does quasi-
    quoting (even though it didn't)
  • the implementation relied on the intricacies of templates and
    template evaluation

New Behaviour

  • quoted AST is not modified. No symbols are bound and no identifiers
    are turned into gensyms

Implementation

  • semQuoteAst transforms the quote call into a call to the internal
    quoteImpl procedure
  • the pre-processed quoted AST is passed as the first arguments; the
    extracted unquoted expression are passed as the remaining arguments
  • the internal-only evalToAst magic procedure is used for evaluating
    the unquoted expressions. newLit cannot be used here, as the trees
    it produces for object values are only valid when all the type's
    fields are exported
  • placing the AST is of the evaluated unqouted expressions is handled
    in-VM, by quoteImpl

Standard Library And Test Changes

  • multiple modules from the standard library relied on the previous
    symbol binding and gensym behaviour; they're changed to use bindSym
    or genSym. Outside-visible behaviour doesn't change
  • the t7875.nim test relied on the gensym behaviour. The definition
    outside the macro is not relevant to the issue the test guards
    against, so it can just be removed
  • the quoted ASTs in tsizeof.nim are wrapped in blocks in order to
    prevent the identifiers from colliding

* no symbol binding takes placed for unquoted expressions
* identifiers in definition positions are not turned into gensyms

Internally, no templates are involved, nor `getAst`. `sem` extracts all
unquoted expressions from the quoted AST, wraps them in `newLit` calls,
and delegates the substitution work to the `quoteImpl` procedure, which
is run in the VM.
Include `nkNimNodeLit` in the set of nodes to ignore.
The problem is that it doesn't work with objects that have private
fields, which would be a regression. The internal `evalToAst` magic is
introduced, which translates to an `opcDataToAst` instruction in the
VM.
Symbols that should be bound early need to explicitly use `bindSym`,
and identifiers that need to be unique require usage of `genSym`.
It's used by a `quote`d AST, and since some of the type's fields are
already exported, the type itself is exported too, fixing the
"undeclared identifier" error.
The line information of the error now points to the correct expression.
The `mysym` definition outside the macro is unnecessary for the
reproducer of the original issue, so removing it is fine.
The usage of `quote` relied on identifiers in definitions being
gensym'ed. To prevent redefinitions, the macro outputs are now wrapped
in blocks.
@zerbina zerbina added compiler/sem Related to semantic-analysis system of the compiler lang/macro labels Jul 31, 2024
@alaviss
Copy link
Contributor

alaviss commented Jul 31, 2024

  • identifiers in definition positions aren't turned into gensyms; they stay as is

Is there some rationale for this? I expect that this will make using quote a lot harder.

@zerbina
Copy link
Collaborator Author

zerbina commented Jul 31, 2024

Is there some rationale for this? I expect that this will make using quote a lot harder.

That's just a consequence of quote now doing quasi-quoting. Previously, quote was just sugar for a template definition + expansion, which is where the symbol binding and gensym behaviour came from.

@alaviss
Copy link
Contributor

alaviss commented Jul 31, 2024

Are there plans to allow gensym to happen to unquoted identifiers in the future? quote was very handy in allowing for easy generation of helper statements without bloating my macro with a bunch of explicit genSyms

@zerbina
Copy link
Collaborator Author

zerbina commented Jul 31, 2024

Are there plans to allow gensym to happen to unquoted identifiers in the future?

No, at least not from my side. quote should only do quasi-quoting, nothing else. However, that doesn't preclude things such as allowing the .gensym pragma on definitions in macro output, or making macros open a new shadow scope by default.

Edit: For my answer, I've assumed that with "unquoted identifier" you mean "identifiers that are not accent-quoted".

In the meantime, you can either use genAst (which effectively works the same as the old quote, minus the unquoting behaviour), or use a custom quote operator, like so:

template gensymQuote(bl: untyped): untyped {.dirty.} =
  block:
    let name = genSym(nskTemplate, "templ")
    quote do:
      template `name`(): untyped =
        bl
      `name`()

Depending on the quoted AST, you can also just wrap the code in an if true: in order to confine the identifiers to their own scope.

@saem
Copy link
Collaborator

saem commented Aug 1, 2024

Per the discussion in matrix, concern around being able to get the old behaviour have been resolved per @zerbina's solution.

In the meantime, you can either use genAst (which effectively works the same as the old quote, minus the unquoting behaviour), or use a custom quote operator, like so:

template gensymQuote(bl: untyped): untyped {.dirty.} =
  block:
    let name = genSym(nskTemplate, "templ")
    quote do:
      template `name`(): untyped =
        bl
      `name`()

Depending on the quoted AST, you can also just wrap the code in an if true: in order to confine the identifiers to their own scope.

@zerbina zerbina added the enhancement New feature or request label Aug 1, 2024
The previous comment is no longer relevant.
Code was added to `macros.nim`, so the line where the offending node
originates from (the `newNimNode` call) changed.
@zerbina
Copy link
Collaborator Author

zerbina commented Aug 1, 2024

/merge

Copy link

github-actions bot commented Aug 1, 2024

Merge requested by: @zerbina

Contents after the first section break of the PR description has been removed and preserved below:


To-Do

  • write a proper commit message

Notes for Reviewers

  • the test and library changes provide a good overview of how users of quote are affected

@chore-runner chore-runner bot added this pull request to the merge queue Aug 1, 2024
Merged via the queue into nim-works:devel with commit ec9b4ac Aug 1, 2024
31 checks passed
@zerbina zerbina deleted the proper-quasi-quoting branch August 13, 2024 21:03
github-merge-queue bot pushed a commit that referenced this pull request Aug 21, 2024
## Summary

Add  `stamp`  to  `std/macros` , which takes a template body and applies
it immediately. This is the same behaviour as the previous  `quote` 
that was changed after #1393.


## Details

After #1393 we have a hole in
the stdlib for  `quote` -like interpolation but with automatic binding
of symbols to reduce boilerplate. Various libraries within the ecosystem
such as  `criterion`  and  `npeg`  have found the feature useful and
extensively employ it.

Reintroduces old  `quote`  behavior as  `stamp` , without support for
custom operators and explicitly document the template-like nature of 
`stamp` 's body as well as its pitfalls.

This macro was originally written and shared to Matrix by @zerbina.

---------

Co-authored-by: zerbina <[email protected]>
Co-authored-by: Saem Ghani <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler/sem Related to semantic-analysis system of the compiler enhancement New feature or request lang/macro
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants