Skip to content

Conversation

@mlechu
Copy link
Member

@mlechu mlechu commented Sep 10, 2025

Addresses #39 on the Expr side with a slightly simpler implementation than before. We may need to store more information in the hygienic_scope node on the SyntaxTree side. This change is mainly to get @enum to work.

I've put an explanation of the behaviour in the docstring:

"""
Insert a hygienic-scope around each arg of K"toplevel" returned from a macro.

It isn't correct for macro expansion to recurse into a K"toplevel" expression
since one child may define a macro and the next may use it.  However, not
recursing now means we lose some important context: the module of the macro we
just expanded, which is necessary for resolving the identifiers in the
K"toplevel" AST.  The solution implemented in JuliaLang/julia#53515 was to save
our place and expand later using `Expr(:hygienic-scope toplevel_child mod)`.

Of course, these hygienic-scopes are also necessary because existing user code
contains the corresponding escaping, which would otherwise cause errors. We
already consumed the hygienic-scope that comes with every expansion, but won't
be looking for escapes under :toplevel, so push hygienic-scope under toplevel
"""

Copy link
Member

@c42f c42f left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this works for multiply nested esc() inside the toplevel expression? Or for any occurrence of identifiers inside the top level expression with scope_layer attached? Ie, not a general solution but these cases were already broken?

It would be nice to make @enum work so I think it's fine to merge with some comment (or @test_broken) about the cases which are still broken.

@mlechu mlechu force-pushed the ec/docsystem-edge-cases branch from 9b55a0d to 792a221 Compare September 15, 2025 16:58
@mlechu mlechu force-pushed the ec/fix-macro-toplevel branch from 1dc0d3a to f3dd751 Compare September 15, 2025 17:48
@mlechu
Copy link
Member Author

mlechu commented Sep 15, 2025

We may not need to deal with those cases, though I'll add a comment because I'm not certain.

  • Do we need to handle scope_layer when locals can't cross the :toplevel boundary?
  • I think double-escaping within :toplevel would have the same problems it has with flisp lowering. The only way to escape multiple times in a macro and have it work is to either be introducing the hygienic scopes you're escaping from, or be in control of all the macros that call you. In either case, you're in control of all hygienic-scopes around the :toplevel and double-escaping within it can't touch those.

@mlechu mlechu merged commit 437c067 into ec/docsystem-edge-cases Sep 15, 2025
2 checks passed
@mlechu mlechu deleted the ec/fix-macro-toplevel branch September 15, 2025 18:38
@mlechu mlechu restored the ec/fix-macro-toplevel branch September 15, 2025 18:40
@mlechu
Copy link
Member Author

mlechu commented Sep 15, 2025

Oops, forgot to change the base branch. I'll just wait for docsystem-edge-cases to be merged.

mlechu added a commit that referenced this pull request Sep 15, 2025
c42f pushed a commit that referenced this pull request Sep 16, 2025
@c42f c42f mentioned this pull request Oct 29, 2025
@mlechu mlechu deleted the ec/fix-macro-toplevel branch November 6, 2025 19:57
adienes pushed a commit to adienes/julia that referenced this pull request Nov 20, 2025
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.

2 participants