-
Notifications
You must be signed in to change notification settings - Fork 9
Expand import/using to newer Base runtime calls
#43
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
Calls to `import` and `using` are expanded by lowering as of the changes in JuliaLang/julia#57965 and no longer dealt with by the C function `jl_toplevel_eval_flex`. This implies we can't use `eval()` for these if we want to activate JuliaLowering in Core, or we'll hit a stack overflow. I've chosen to duplicate the flisp lowering here for consistency and import paths are thus lowered to a restricted kind of quoted `Expr`. (It's mildly annoying to rely on quoted `Expr` in the lowered paths than the previous use of `Core.svec` but deleting the svec representation allows us to use `Base._eval_import` and `Base._eval_using` directly so seems like a worthy simplification.)
Like expansion of import/using, this list can be computed at expansion time rather than emitting each element into the lowered code individually.
aviatesk
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.
Thanks for the update.
I know this might be a bit of a pain, but is it possible to maintain compatibility with v"1.12"? Looking ahead, when considering integration JL into JET, it would be helpful if JL could work with v1.12.
I also understand that maintaining compat code can be a hassle. If you're not up for it, I think we could special-case statements containing using/import on the JET side.
|
Adding compat code for this case is easy enough and I'm happy to do it and keep compatibility for now. Is your plan to target 1.12 (and dev) with your packages? Lowering is pretty strongly coupled to the runtime so I'm not sure how long we can keep compatibility - it really depends on the changes Base throws our way. For example supporting things like the const/global lowering changes both before and after sounds like a bad time. |
|
Yeah, for now we're targeting v1.12, but this is just temporary. JET has a very loose compatibility guarantee, and JET might drop support for v1.12 eventually. However, since JET hasn't been updated to 1.13 yet, and to test the integration between JET and JL, I want to support v1.12 for the time being (for the next few months or so?). This short-term compatibility issue between JET and JL will disappear when JET is updated to v1.13, or when JL is integrated into Base. I completely agree with you about future compat concerns. Honestly, I think this package should always aim to be compatible with nightly. I'd appreciate it if you could maintain compatibility for |
|
Ok, I've made it work with 1.12 now (hopefully - tests pass locally). Also fixed some bugs with the |
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.
Thanks so much!! Really appreciate your support on this.
Calls to `import` and `using` are expanded by lowering as of the changes in JuliaLang/julia#57965 and no longer dealt with by the C function `jl_toplevel_eval_flex`. This implies we can't use `eval()` for these if we want to activate JuliaLowering in Core, or we'll hit a stack overflow. I've chosen to duplicate the flisp lowering here for consistency and import paths are thus lowered to a restricted kind of quoted `Expr`. (It's mildly annoying to rely on quoted `Expr` in the lowered paths than the previous use of `Core.svec` but deleting the svec representation allows us to use `Base._eval_import` and `Base._eval_using` directly so seems like a worthy simplification.) Similarly, use a precomputed vector of names in public/export expansion - this list can be computed at expansion time rather than emitting each element into the lowered code individually. Includes minor test+CI fixes julia 1.12 in support of JETLS.
…ering.jl#43) Calls to `import` and `using` are expanded by lowering as of the changes in JuliaLang#57965 and no longer dealt with by the C function `jl_toplevel_eval_flex`. This implies we can't use `eval()` for these if we want to activate JuliaLowering in Core, or we'll hit a stack overflow. I've chosen to duplicate the flisp lowering here for consistency and import paths are thus lowered to a restricted kind of quoted `Expr`. (It's mildly annoying to rely on quoted `Expr` in the lowered paths than the previous use of `Core.svec` but deleting the svec representation allows us to use `Base._eval_import` and `Base._eval_using` directly so seems like a worthy simplification.) Similarly, use a precomputed vector of names in public/export expansion - this list can be computed at expansion time rather than emitting each element into the lowered code individually. Includes minor test+CI fixes julia 1.12 in support of JETLS.
Calls to
importandusingare expanded by lowering as of the changes in JuliaLang/julia#57965 and no longer dealt with by the C functionjl_toplevel_eval_flex.This implies we can't use
eval()for these if we want to activate JuliaLowering in Core, or we'll hit a stack overflow.I've chosen to duplicate the flisp lowering here for consistency and import paths are thus lowered to a restricted kind of quoted
Expr. (It's mildly annoying to rely on quotedExprin the lowered paths than the previous use ofCore.svecbut deleting the svec representation allows us to useBase._eval_importandBase._eval_usingdirectly so seems like a worthy simplification.)Fix #42