Consolidate various isdefined functionality into a new builtin #56985

Keno · 2025-01-08T02:20:32Z

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:

Adding a new builtin isdefinedglobal that has the full feature set
Dropping :isdefined on globals as an IR form (the frontend form gets lowered to the intrinsic if called on globals)
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

Since we dropped the :isdefined for globals, do we now need to update some files modified in #54999, like verify.jl, validation.jl and ast.md?

Compiler/src/abstractinterpretation.jl

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`.

aviatesk reviewed Jan 8, 2025

View reviewed changes