lowering: Don't resolve type bindings earlier than necessary #54999
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Keno
force-pushed
the
kf/isdefinedimplicit
branch
4 times, most recently
from
8d24794 to
9615722
Compare
This is a follow up to resolve a TODO left in #54773 as part of preparatory work for #54654. Currently, our lowering for type definition contains an early `isdefined` that forces a decision on binding resolution before the assignment of the actual binding. In the current implementation, this doesn't matter much, but with #54654, this would incur a binding invalidation we would like to avoid. To get around this, we extend the (internal) `isdefined` form to take an extra argument specifying whether or not to permit looking at imported bindings. If not, resolving the binding is not required semantically, but for the purposes of type definition (where assigning to an imported binding would error anyway), this is all we need.
Keno
force-pushed
the
kf/isdefinedimplicit
branch
from
9615722 to
2b11378
Compare
vtjnash
reviewed
Jul 7, 2024
| } | ||
|
|
||
| JL_DLLEXPORT int jl_boundp(jl_module_t *m, jl_sym_t *var) // unlike most queries here, this is currently seq_cst | ||
| JL_DLLEXPORT int jl_boundp(jl_module_t *m, jl_sym_t *var, int allow_import) // unlike most queries here, this is currently seq_cst |
There was a problem hiding this comment.
This comment appears to be inaccurate now:
Suggested change
| JL_DLLEXPORT int jl_boundp(jl_module_t *m, jl_sym_t *var, int allow_import) // unlike most queries here, this is currently seq_cst | |
| JL_DLLEXPORT int jl_boundp(jl_module_t *m, jl_sym_t *var, int allow_import) // unlike most queries here, this is currently seq_cst if `allow_import` is true. |
Keno
added a commit
that referenced
this pull request
Jan 8, 2025
In #54999 I extended `:isdefined` with the ability to specify whether or not to consider imported bindings defined. As a result, we now have two mechanisms for querying `isdefined` on globals (the other being `Core.isdefined`) with incompatible feature sets (`Core.isdefined` supports an atomic ordering argument, but not `allow_import`). Additionally, only one of them had proper codegen support. I also don't like to have IR forms for things that could be perfectly well handled by builtin calls (along the lines of #56713). So this tries to clean that all up by: 1. Adding a new builtin `isdefinedglobal` that has the full feature set 2. Dropping `:isdefined` on globals as an IR form (the frontend form gets lowered to the intrinsic if called on globals) 3. Wiring up codegen and correcting inference for that new builtin An additional motivation is that `isdefined` on globals needs support for partition edges (like other builtins), and having to have a special case for :isdefined was marginally annoying. Just using an intrinsic for this is much cleaner. Lastly, the reason for a new intrinsic over extending the existing `isdefined`, is that over time we've moved away from conflating fields and globals for Module (e.g. introducing `getglobal`/`setglobal!`), so this is a natural extension of that. Of course, the existing behavior is retained for ordinary `isdefined`.
Keno
added a commit
that referenced
this pull request
Jan 8, 2025
In #54999 I extended `:isdefined` with the ability to specify whether or not to consider imported bindings defined. As a result, we now have two mechanisms for querying `isdefined` on globals (the other being `Core.isdefined`) with incompatible feature sets (`Core.isdefined` supports an atomic ordering argument, but not `allow_import`). Additionally, only one of them had proper codegen support. I also don't like to have IR forms for things that could be perfectly well handled by builtin calls (along the lines of #56713). So this tries to clean that all up by: 1. Adding a new builtin `isdefinedglobal` that has the full feature set 2. Dropping `:isdefined` on globals as an IR form (the frontend form gets lowered to the intrinsic if called on globals) 3. Wiring up codegen and correcting inference for that new builtin An additional motivation is that `isdefined` on globals needs support for partition edges (like other builtins), and having to have a special case for :isdefined was marginally annoying. Just using an intrinsic for this is much cleaner. Lastly, the reason for a new intrinsic over extending the existing `isdefined`, is that over time we've moved away from conflating fields and globals for Module (e.g. introducing `getglobal`/`setglobal!`), so this is a natural extension of that. Of course, the existing behavior is retained for ordinary `isdefined`.
Keno
added a commit
that referenced
this pull request
Jan 8, 2025
In #54999 I extended `:isdefined` with the ability to specify whether or not to consider imported bindings defined. As a result, we now have two mechanisms for querying `isdefined` on globals (the other being `Core.isdefined`) with incompatible feature sets (`Core.isdefined` supports an atomic ordering argument, but not `allow_import`). Additionally, only one of them had proper codegen support. I also don't like to have IR forms for things that could be perfectly well handled by builtin calls (along the lines of #56713). So this tries to clean that all up by: 1. Adding a new builtin `isdefinedglobal` that has the full feature set 2. Dropping `:isdefined` on globals as an IR form (the frontend form gets lowered to the intrinsic if called on globals) 3. Wiring up codegen and correcting inference for that new builtin An additional motivation is that `isdefined` on globals needs support for partition edges (like other builtins), and having to have a special case for :isdefined was marginally annoying. Just using an intrinsic for this is much cleaner. Lastly, the reason for a new intrinsic over extending the existing `isdefined`, is that over time we've moved away from conflating fields and globals for Module (e.g. introducing `getglobal`/`setglobal!`), so this is a natural extension of that. Of course, the existing behavior is retained for ordinary `isdefined`.
Keno
added a commit
that referenced
this pull request
Jan 9, 2025
In #54999 I extended `:isdefined` with the ability to specify whether or not to consider imported bindings defined. As a result, we now have two mechanisms for querying `isdefined` on globals (the other being `Core.isdefined`) with incompatible feature sets (`Core.isdefined` supports an atomic ordering argument, but not `allow_import`). Additionally, only one of them had proper codegen support. I also don't like to have IR forms for things that could be perfectly well handled by builtin calls (along the lines of #56713). So this tries to clean that all up by: 1. Adding a new builtin `isdefinedglobal` that has the full feature set 2. Dropping `:isdefined` on globals as an IR form (the frontend form gets lowered to the intrinsic if called on globals) 3. Wiring up codegen and correcting inference for that new builtin An additional motivation is that `isdefined` on globals needs support for partition edges (like other builtins), and having to have a special case for :isdefined was marginally annoying. Just using an intrinsic for this is much cleaner. Lastly, the reason for a new intrinsic over extending the existing `isdefined`, is that over time we've moved away from conflating fields and globals for Module (e.g. introducing `getglobal`/`setglobal!`), so this is a natural extension of that. Of course, the existing behavior is retained for ordinary `isdefined`.
mlechu
added a commit
to mlechu/JuliaLowering.jl
that referenced
this pull request
Apr 1, 2025
mlechu
added a commit
to mlechu/JuliaLowering.jl
that referenced
this pull request
Apr 2, 2025
mlechu
added a commit
to mlechu/JuliaLowering.jl
that referenced
this pull request
Apr 2, 2025
mlechu
added a commit
to mlechu/JuliaLowering.jl
that referenced
this pull request
Apr 2, 2025
mlechu
added a commit
to mlechu/JuliaLowering.jl
that referenced
this pull request
Apr 2, 2025
mlechu
added a commit
to mlechu/JuliaLowering.jl
that referenced
this pull request
Apr 4, 2025
mlechu
added a commit
to mlechu/JuliaLowering.jl
that referenced
this pull request
Apr 18, 2025
serenity4
pushed a commit
to serenity4/julia
that referenced
this pull request
May 1, 2025
…Lang#56985) In JuliaLang#54999 I extended `:isdefined` with the ability to specify whether or not to consider imported bindings defined. As a result, we now have two mechanisms for querying `isdefined` on globals (the other being `Core.isdefined`) with incompatible feature sets (`Core.isdefined` supports an atomic ordering argument, but not `allow_import`). Additionally, only one of them had proper codegen support. I also don't like to have IR forms for things that could be perfectly well handled by builtin calls (along the lines of JuliaLang#56713). So this tries to clean that all up by: 1. Adding a new builtin `isdefinedglobal` that has the full feature set 2. Dropping `:isdefined` on globals as an IR form (the frontend form gets lowered to the intrinsic if called on globals) 3. Wiring up codegen and correcting inference for that new builtin An additional motivation is that `isdefined` on globals needs support for partition edges (like other builtins), and having to have a special case for :isdefined was marginally annoying. Just using an intrinsic for this is much cleaner. Lastly, the reason for a new intrinsic over extending the existing `isdefined`, is that over time we've moved away from conflating fields and globals for Module (e.g. introducing `getglobal`/`setglobal!`), so this is a natural extension of that. Of course, the existing behavior is retained for ordinary `isdefined`.
aviatesk
pushed a commit
to aviatesk/JuliaLowering.jl
that referenced
this pull request
Jun 14, 2025
aviatesk
pushed a commit
to aviatesk/JuliaLowering.jl
that referenced
this pull request
Jun 14, 2025
aviatesk
pushed a commit
to aviatesk/JuliaLowering.jl
that referenced
this pull request
Jul 2, 2025
aviatesk
pushed a commit
to aviatesk/JuliaLowering.jl
that referenced
this pull request
Jul 2, 2025
mlechu
added a commit
to mlechu/JuliaLowering.jl
that referenced
this pull request
Jul 29, 2025
mlechu
added a commit
to mlechu/JuliaLowering.jl
that referenced
this pull request
Jul 29, 2025
mlechu
added a commit
to mlechu/JuliaLowering.jl
that referenced
this pull request
Jul 30, 2025
mlechu
added a commit
to mlechu/JuliaLowering.jl
that referenced
this pull request
Aug 2, 2025
mlechu
added a commit
to c42f/JuliaLowering.jl
that referenced
this pull request
Aug 4, 2025
* Update CodeInfo struct and handling Co-authored-by: Claire Foster <aka.c42f@gmail.com> * Don't produce raw symbol from globalref This used to implicitly refer to a module-level name, but lowering is now expected to wrap it in a `globalref`. Part of JuliaLang/julia#54772 * Updates to const and global lowering; add K"constdecl"; omit `wrap` JuliaLang/julia#54773, JuliaLang/julia#56713, JuliaLang/julia#57470. Some changes omitted from `expand-decls` and `expand-assignment`. Note that the two-argument IR "const" is K"constdecl", whereas the one-argument K"const" only appears in the AST. Also note that the `wrap` parameter is omitted throughout assignment desugaring. As far as I'm aware, all this plumbing was just to support `const a,b,c = 1,2,3` having `b` and `c` inherit the `const`. TODO: find a better way of doing the same thing (a ScopedValue might be a clean solution; we currently throw an error). The check for `let; const x = 1; end`, (which should throw) is in scope analysis (lisp has it in `compile`). Co-authored-by: Claire Foster <aka.c42f@gmail.com> * Add `isdefinedglobal` builtin JuliaLang/julia#54999, JuliaLang/julia#56985 * :global no longer valid_ir_argument; rm `is_defined_nothrow_global` JuliaLang/julia#56746. Also call :slot and :static_parameter valid (for now) * Fix `is_defined_and_owned_global` (Core.Binding changes) Adapt to bpart changes in JuliaLang/julia#54788 * Struct desugaring: "Undo decision to publish incomplete types..." JuliaLang/julia#56497; Add self-referencing struct shim I have doubts about how long this solution will stay in the base repository, and how complete it is (doesn't seem to work with M1.M2.S), but we are testing for it here. Also change the expected value of a test changed in the same PR. * Emit `latestworld` world age increments For method defs, `latestworld` is produced in desugaring rather than closure conversion for now (our closure conversion doesn't seem to cover the same cases as lisp lowering yet). Covers JuliaLang/julia#56523, JuliaLang/julia#56509, JuliaLang/julia#57299. Also includes changes from JuliaLang/julia#57102 (bpart: Start enforcing minimum world age for const bparts) and JuliaLang/julia#57150 (bpart: Start enforcing min_world for global variable definitions) since the lowering changes from those appear to be amendments to the changes above (missing world age increments). Co-authored-by: Claire Foster <aka.c42f@gmail.com> * bpart changes: `Core._typebody!` signature `Core._typebody!` now takes a new "prev" argument, which we don't use yet here. Changes from JuliaLang/julia#57253 * bpart changes: struct desugaring Changes from JuliaLang/julia#57253 (bpart: Fully switch to partitioned semantics). This fixes one failing test and realigns struct desugaring to match lisp for now. Also changed: the expected result of redefining a primitive type (now allowed). * Additional argument in `new_opaque_closure` Fix segfaulting test. Thanks for the TODO * Adapt to different `GeneratedFunctionStub` signature Signature changed in JuliaLang/julia#57230. Thanks @aviatesk for the help! * Fix `public` and `export` As of JuliaLang/julia#57765, `jl_module_public` is no longer exported. Change our runtime to handle it like `public` and `export` like we handle `import` or `using` for now * Fix modules.jl test I believe this was a world age issue * Regenerate IR tests Too many to count. * Update README to known-good julia, JuliaSyntax versions Latest julia works. Changes are needed to work with the latest JuliaSyntax, but that isn't in base julia yet, and more changes are likely to come. * Fix small bug from #16 so tests pass The change lifted the scope of `note`, so it was being changed in the loop * Changes from code review: const/global lowering Ping me if you'd like this squashed into the original const/global commit! Co-authored-by: Claire Foster <aka.c42f@gmail.com> * Remove a special case No longer needed since we no longer put `global` or `local` forms back into the expand_forms machine. Some error messages change slightly as a result. * Changes from code review Co-authored-by: Claire Foster <aka.c42f@gmail.com> * Fix + test for assignment in value but not tail position * Disallow `static_parameter` as `valid_ir_argument` See added comment, and discussion at #10 (comment) Co-authored-by: Claire Foster <aka.c42f@gmail.com> * Change printing of `K"latestworld"` Parens are nice, but it wasn't consistent. Also make it a leaf (remaining non-leaves are deleted in the next commit.) * Move most `latestworld`s to linearization From the docs: ``` The following statements raise the current world age: 1. An explicit invocation of Core.@latestworld 2. The start of every top-level statement 3. The start of every REPL prompt 4. Any type or struct definition 5. Any method definition 6. Any constant declaration 7. Any global variable declaration (but not a global variable assignment) 8. Any using, import, export or public statement 9. Certain other macros like eval (depends on the macro implementation) ``` This commit handles each case as follows: ``` 1. = 9 2. I'm not sure this actually happens (or needs to happen, unless we're being defensive? Doing it after each world-changing operation should suffice). But if we need it, this would just be emitting once at the beginning of every lowered output. 3. = 2 4. = 6 5. Emit seeing `method` in linearize 6. Emit seeing `constdecl` in linearize 7. Emit seeing `global` or `globaldecl` in linearize 8. We just defer to `eval`, but should probably go in desugaring later - using/import recently became builtin calls, and I haven't updated JL to use them yet. Base._import_using has an expr-based API that may change, and our importpath destructuring is worth keeping. - export and public (special forms) are handled in toplevel.c 9. Done for us ``` Other quirks: - `JuliaLowering.eval_closure_type` calls eval to assign a const, so we still need to deal with that in closure conversion. - The `include` hack isn't mentioned in the docs, but can stay in desugaring. I'm not certain why we don't do the same for non-macro `eval`. --------- Co-authored-by: Claire Foster <aka.c42f@gmail.com>
c42f
added a commit
that referenced
this pull request
Oct 17, 2025
* Update CodeInfo struct and handling Co-authored-by: Claire Foster <aka.c42f@gmail.com> * Don't produce raw symbol from globalref This used to implicitly refer to a module-level name, but lowering is now expected to wrap it in a `globalref`. Part of #54772 * Updates to const and global lowering; add K"constdecl"; omit `wrap` #54773, #56713, #57470. Some changes omitted from `expand-decls` and `expand-assignment`. Note that the two-argument IR "const" is K"constdecl", whereas the one-argument K"const" only appears in the AST. Also note that the `wrap` parameter is omitted throughout assignment desugaring. As far as I'm aware, all this plumbing was just to support `const a,b,c = 1,2,3` having `b` and `c` inherit the `const`. TODO: find a better way of doing the same thing (a ScopedValue might be a clean solution; we currently throw an error). The check for `let; const x = 1; end`, (which should throw) is in scope analysis (lisp has it in `compile`). Co-authored-by: Claire Foster <aka.c42f@gmail.com> * Add `isdefinedglobal` builtin #54999, #56985 * :global no longer valid_ir_argument; rm `is_defined_nothrow_global` #56746. Also call :slot and :static_parameter valid (for now) * Fix `is_defined_and_owned_global` (Core.Binding changes) Adapt to bpart changes in #54788 * Struct desugaring: "Undo decision to publish incomplete types..." #56497; Add self-referencing struct shim I have doubts about how long this solution will stay in the base repository, and how complete it is (doesn't seem to work with M1.M2.S), but we are testing for it here. Also change the expected value of a test changed in the same PR. * Emit `latestworld` world age increments For method defs, `latestworld` is produced in desugaring rather than closure conversion for now (our closure conversion doesn't seem to cover the same cases as lisp lowering yet). Covers #56523, #56509, #57299. Also includes changes from #57102 (bpart: Start enforcing minimum world age for const bparts) and #57150 (bpart: Start enforcing min_world for global variable definitions) since the lowering changes from those appear to be amendments to the changes above (missing world age increments). Co-authored-by: Claire Foster <aka.c42f@gmail.com> * bpart changes: `Core._typebody!` signature `Core._typebody!` now takes a new "prev" argument, which we don't use yet here. Changes from #57253 * bpart changes: struct desugaring Changes from #57253 (bpart: Fully switch to partitioned semantics). This fixes one failing test and realigns struct desugaring to match lisp for now. Also changed: the expected result of redefining a primitive type (now allowed). * Additional argument in `new_opaque_closure` Fix segfaulting test. Thanks for the TODO * Adapt to different `GeneratedFunctionStub` signature Signature changed in #57230. Thanks @aviatesk for the help! * Fix `public` and `export` As of #57765, `jl_module_public` is no longer exported. Change our runtime to handle it like `public` and `export` like we handle `import` or `using` for now * Fix modules.jl test I believe this was a world age issue * Regenerate IR tests Too many to count. * Update README to known-good julia, JuliaSyntax versions Latest julia works. Changes are needed to work with the latest JuliaSyntax, but that isn't in base julia yet, and more changes are likely to come. * Fix small bug from JuliaLang/JuliaLowering.jl#16 so tests pass The change lifted the scope of `note`, so it was being changed in the loop * Changes from code review: const/global lowering Ping me if you'd like this squashed into the original const/global commit! Co-authored-by: Claire Foster <aka.c42f@gmail.com> * Remove a special case No longer needed since we no longer put `global` or `local` forms back into the expand_forms machine. Some error messages change slightly as a result. * Changes from code review Co-authored-by: Claire Foster <aka.c42f@gmail.com> * Fix + test for assignment in value but not tail position * Disallow `static_parameter` as `valid_ir_argument` See added comment, and discussion at c42f/JuliaLowering.jl#10 (comment) Co-authored-by: Claire Foster <aka.c42f@gmail.com> * Change printing of `K"latestworld"` Parens are nice, but it wasn't consistent. Also make it a leaf (remaining non-leaves are deleted in the next commit.) * Move most `latestworld`s to linearization From the docs: ``` The following statements raise the current world age: 1. An explicit invocation of Core.@latestworld 2. The start of every top-level statement 3. The start of every REPL prompt 4. Any type or struct definition 5. Any method definition 6. Any constant declaration 7. Any global variable declaration (but not a global variable assignment) 8. Any using, import, export or public statement 9. Certain other macros like eval (depends on the macro implementation) ``` This commit handles each case as follows: ``` 1. = 9 2. I'm not sure this actually happens (or needs to happen, unless we're being defensive? Doing it after each world-changing operation should suffice). But if we need it, this would just be emitting once at the beginning of every lowered output. 3. = 2 4. = 6 5. Emit seeing `method` in linearize 6. Emit seeing `constdecl` in linearize 7. Emit seeing `global` or `globaldecl` in linearize 8. We just defer to `eval`, but should probably go in desugaring later - using/import recently became builtin calls, and I haven't updated JL to use them yet. Base._import_using has an expr-based API that may change, and our importpath destructuring is worth keeping. - export and public (special forms) are handled in toplevel.c 9. Done for us ``` Other quirks: - `JuliaLowering.eval_closure_type` calls eval to assign a const, so we still need to deal with that in closure conversion. - The `include` hack isn't mentioned in the docs, but can stay in desugaring. I'm not certain why we don't do the same for non-macro `eval`. --------- Co-authored-by: Claire Foster <aka.c42f@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a follow up to resolve a TODO left in #54773 as part of preparatory work for #54654. Currently, our lowering for type definition contains an early
isdefinedthat forces a decision on binding resolution before the assignment of the actual binding. In the current implementation, this doesn't matter much, but with #54654, this would incur a binding invalidation we would like to avoid.To get around this, we extend the (internal)
isdefinedform to take an extra argument specifying whether or not to permit looking at imported bindings. If not, resolving the binding is not required semantically, but for the purposes of type definition (where assigning to an imported binding would error anyway), this is all we need.