-
Notifications
You must be signed in to change notification settings - Fork 9
Add hook for testing JuliaLowering in core #32
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
Conversation
c42f
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks!
src/hooks.jl
Outdated
| # e.g. LineNumberNode, integer... | ||
| return Core.svec(code) | ||
| end | ||
| st0 = code isa Expr ? expr_to_syntaxtree(code) : code |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very cool that we can do this now, with expr_to_syntaxtree(). Do we get a benefit from the following?
| st0 = code isa Expr ? expr_to_syntaxtree(code) : code | |
| st0 = code isa Expr ? expr_to_syntaxtree(code, LineNumberNode(line, file)) : code |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I forgot to pass that in. That will be the source attribute for the root and all nodes until we traverse a linenode.
| ctx4, st4 = convert_closures(ctx3, st3) | ||
| ctx5, st5 = linearize_ir( ctx4, st4) | ||
| ex = to_lowered_expr(mod, st5) | ||
| return Core.svec(ex, st5, ctx5) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, so I see that in JuliaLang/julia#58207 you allowed for the trailing elements of this to be anything and we'll specify it later (seems reasonable). Are st5 and ctx5 used by JETLS somehow? (I see they're not used by the C code as expected ... who knows what the ctx5 API even is 😅 )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not used by JETLS yet, and even if it was, you're free to "break" this non-API. JETLS does use ctx objects outside of Core._lower, though, I think mostly for the binding table.
I see they're not used by the C code as expected
The C code unfortunately has no idea what a SyntaxTree is yet. I ran into this when trying to make Core._parse produce a syntax tree---toplevel.c had no way to positively check the thing it parsed was a SyntaxTree, and I had to smuggle it through by wrapping it in Expr in the parse hook, which is pretty goofy.
|
Oh, this also needs tests:
|
Co-authored-by: Claire Foster <[email protected]>
c42f
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks really nice now, please merge when ready :)
This change allows us to activate JuliaLowering as the core lowering implementation via
@activate JuliaLowering(InteractiveUtils) orJuliaLowering.activate!().This is a simplified version of the hook originally in #10. The previous version attempted to swap out the parser hook for one that produced
SyntaxTree, but that was broken in many cases, since parser output is expected to be Expr by consumers other than lowering. This change just uses theExpr->SyntaxTreeconversion before lowering, which doesn't solve our provenance problems, but is helpful for testing both the lowering implementation and the tree conversion itself.In the future, we can update
Core._parseto take an argument specifying the output type. This needs to happen after JuliaSyntax changes, and both JuliaLowering and base julia updating to use them, which will take some time.