Consolidate various isdefined functionality into a new builtin (#56985)

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

Author: Keno

Compiler/src/Compiler.jl

@@ -49,7 +49,7 @@ using Core: ABIOverride, Builtin, CodeInstance, IntrinsicFunction, MethodInstanc
 
 using Base
 using Base: @_foldable_meta, @_gc_preserve_begin, @_gc_preserve_end, @nospecializeinfer,
-    BINDING_KIND_GLOBAL, Base, BitVector, Bottom, Callable, DataTypeFieldDesc,
+    BINDING_KIND_GLOBAL, BINDING_KIND_UNDEF_CONST, Base, BitVector, Bottom, Callable, DataTypeFieldDesc,
     EffectsOverride, Filter, Generator, IteratorSize, JLOptions, NUM_EFFECTS_OVERRIDES,
     OneTo, Ordering, RefValue, SizeUnknown, _NAMEDTUPLE_NAME,
     _array_for, _bits_findnext, _methods_by_ftype, _uniontypes, all, allocatedinline, any,
@@ -58,7 +58,7 @@ using Base: @_foldable_meta, @_gc_preserve_begin, @_gc_preserve_end, @nospeciali
     datatype_pointerfree, decode_effects_override, diff_names, fieldindex,
     generating_output, get_nospecializeinfer_sig, get_world_counter, has_free_typevars,
     hasgenerator, hasintersect, indexed_iterate, isType, is_file_tracked, is_function_def,
-    is_meta_expr, is_meta_expr_head, is_nospecialized, is_nospecializeinfer,
+    is_meta_expr, is_meta_expr_head, is_nospecialized, is_nospecializeinfer, is_defined_const_binding,
     is_some_const_binding, is_some_guard, is_some_imported, is_valid_intrinsic_elptr,
     isbitsunion, isconcretedispatch, isdispatchelem, isexpr, isfieldatomic, isidentityfree,
     iskindtype, ismutabletypename, ismutationfree, issingletontype, isvarargtype, isvatuple,

Compiler/src/abstractinterpretation.jl

@@ -2637,21 +2637,14 @@ function abstract_call_known(interp::AbstractInterpreter, @nospecialize(f),
         elseif f === Core.getfield && argtypes_are_actually_getglobal(argtypes)
             return Future(abstract_eval_getglobal(interp, sv, si.saw_latestworld, argtypes))
         elseif f === Core.isdefined && argtypes_are_actually_getglobal(argtypes)
-            exct = Bottom
-            if length(argtypes) == 4
-                order = argtypes[4]
-                exct = global_order_exct(order, #=loading=#true, #=storing=#false)
-                if !(isa(order, Const) && get_atomic_order(order.val, #=loading=#true, #=storing=#false).x >= MEMORY_ORDER_UNORDERED.x)
-                    exct = Union{exct, ConcurrencyViolationError}
-                end
-            end
-            return Future(merge_exct(CallMeta(abstract_eval_isdefined(
-                    interp,
-                    GlobalRef((argtypes[2]::Const).val::Module,
-                              (argtypes[3]::Const).val::Symbol),
-                    si.saw_latestworld,
-                    sv),
-                NoCallInfo()), exct))
+            return Future(abstract_eval_isdefinedglobal(interp, argtypes[2], argtypes[3], Const(true),
+                length(argtypes) == 4 ? argtypes[4] : Const(:unordered),
+                si.saw_latestworld, sv))
+        elseif f === Core.isdefinedglobal && 3 <= length(argtypes) <= 5
+            return Future(abstract_eval_isdefinedglobal(interp, argtypes[2], argtypes[3],
+                length(argtypes) >= 4 ? argtypes[4] : Const(true),
+                length(argtypes) >= 5 ? argtypes[5] : Const(:unordered),
+                si.saw_latestworld, sv))
         elseif f === Core.get_binding_type
             return Future(abstract_eval_get_binding_type(interp, sv, argtypes))
         end
@@ -3203,21 +3196,73 @@ function abstract_eval_isdefined_expr(interp::AbstractInterpreter, e::Expr, ssta
     return abstract_eval_isdefined(interp, sym, sstate.saw_latestworld, sv)
 end
 
-function abstract_eval_isdefined(interp::AbstractInterpreter, @nospecialize(sym), saw_latestworld::Bool, sv::AbsIntState)
+const generic_isdefinedglobal_effects = Effects(EFFECTS_TOTAL, consistent=ALWAYS_FALSE, nothrow=false)
+function abstract_eval_isdefinedglobal(interp::AbstractInterpreter, mod::Module, sym::Symbol, allow_import::Union{Bool, Nothing}, saw_latestworld::Bool, sv::AbsIntState)
     rt = Bool
+    if saw_latestworld
+        return RTEffects(rt, Union{}, Effects(generic_isdefinedglobal_effects, nothrow=true))
+    end
+
     effects = EFFECTS_TOTAL
-    exct = Union{}
-    isa(sym, Symbol) && (sym = GlobalRef(frame_module(sv), sym))
-    if isa(sym, GlobalRef)
-        rte = abstract_eval_globalref(interp, sym, saw_latestworld, sv)
+    partition = lookup_binding_partition!(interp, GlobalRef(mod, sym), sv)
+    if allow_import !== true && is_some_imported(binding_kind(partition))
+        if allow_import === false
+            rt = Const(false)
+        else
+            effects = Effects(generic_isdefinedglobal_effects, nothrow=true)
+        end
+    else
+        partition = walk_binding_partition!(interp, partition, sv)
+        rte = abstract_eval_partition_load(interp, partition)
         if rte.exct == Union{}
             rt = Const(true)
         elseif rte.rt === Union{} && rte.exct === UndefVarError
             rt = Const(false)
         else
-            effects = Effects(EFFECTS_TOTAL; consistent=ALWAYS_FALSE)
+            effects = Effects(generic_isdefinedglobal_effects, nothrow=true)
+        end
+    end
+    return RTEffects(rt, Union{}, effects)
+end
+
+function abstract_eval_isdefinedglobal(interp::AbstractInterpreter, @nospecialize(M), @nospecialize(s), @nospecialize(allow_import_arg), @nospecialize(order_arg), saw_latestworld::Bool, sv::AbsIntState)
+    exct = Bottom
+    allow_import = true
+    if allow_import_arg !== nothing
+        if !isa(allow_import_arg, Const)
+            allow_import = nothing
+            if widenconst(allow_import_arg) != Bool
+                exct = Union{exct, TypeError}
+            end
+        else
+            allow_import = allow_import_arg.val
+        end
+    end
+    if order_arg !== nothing
+        exct = global_order_exct(order_arg, #=loading=#true, #=storing=#false)
+        if !(isa(order_arg, Const) && get_atomic_order(order_arg.val, #=loading=#true, #=storing=#false).x >= MEMORY_ORDER_UNORDERED.x)
+            exct = Union{exct, ConcurrencyViolationError}
+        end
+    end
+    if M isa Const && s isa Const
+        M, s = M.val, s.val
+        if M isa Module && s isa Symbol
+            return merge_exct(CallMeta(abstract_eval_isdefinedglobal(interp, M, s, allow_import, saw_latestworld, sv), NoCallInfo()), exct)
         end
-    elseif isexpr(sym, :static_parameter)
+        return CallMeta(Union{}, TypeError, EFFECTS_THROWS, NoCallInfo())
+    elseif !hasintersect(widenconst(M), Module) || !hasintersect(widenconst(s), Symbol)
+        return CallMeta(Union{}, TypeError, EFFECTS_THROWS, NoCallInfo())
+    elseif M ⊑ Module && s ⊑ Symbol
+        return CallMeta(Bool, Union{exct, UndefVarError}, generic_isdefinedglobal_effects, NoCallInfo())
+    end
+    return CallMeta(Bool, Union{exct, TypeError, UndefVarError}, generic_isdefinedglobal_effects, NoCallInfo())
+end
+
+function abstract_eval_isdefined(interp::AbstractInterpreter, @nospecialize(sym), saw_latestworld::Bool, sv::AbsIntState)
+    rt = Bool
+    effects = EFFECTS_TOTAL
+    exct = Union{}
+    if isexpr(sym, :static_parameter)
         n = sym.args[1]::Int
         if 1 <= n <= length(sv.sptypes)
             sp = sv.sptypes[n]
@@ -3443,22 +3488,31 @@ function abstract_eval_globalref_type(g::GlobalRef, src::Union{CodeInfo, IRCode,
     return partition_restriction(partition)
 end
 
-function abstract_eval_binding_partition!(interp::AbstractInterpreter, g::GlobalRef, sv::AbsIntState)
+function lookup_binding_partition!(interp::AbstractInterpreter, g::GlobalRef, sv::AbsIntState)
     force_binding_resolution!(g)
     partition = lookup_binding_partition(get_inference_world(interp), g)
     update_valid_age!(sv, WorldRange(partition.min_world, partition.max_world))
+    partition
+end
 
+function walk_binding_partition!(interp::AbstractInterpreter, partition::Core.BindingPartition, sv::AbsIntState)
     while is_some_imported(binding_kind(partition))
         imported_binding = partition_restriction(partition)::Core.Binding
         partition = lookup_binding_partition(get_inference_world(interp), imported_binding)
         update_valid_age!(sv, WorldRange(partition.min_world, partition.max_world))
     end
+    return partition
+end
 
+function abstract_eval_binding_partition!(interp::AbstractInterpreter, g::GlobalRef, sv::AbsIntState)
+    partition = lookup_binding_partition!(interp, g, sv)
+    partition = walk_binding_partition!(interp, partition, sv)
     return partition
 end
 
 function abstract_eval_partition_load(interp::AbstractInterpreter, partition::Core.BindingPartition)
-    if is_some_guard(binding_kind(partition))
+    kind = binding_kind(partition)
+    if is_some_guard(kind) || kind == BINDING_KIND_UNDEF_CONST
         if InferenceParams(interp).assume_bindings_static
             return RTEffects(Union{}, UndefVarError, EFFECTS_THROWS)
         else
@@ -3468,13 +3522,12 @@ function abstract_eval_partition_load(interp::AbstractInterpreter, partition::Co
         end
     end
 
-    if is_some_const_binding(binding_kind(partition))
+    if is_defined_const_binding(kind)
         rt = Const(partition_restriction(partition))
         return RTEffects(rt, Union{}, Effects(EFFECTS_TOTAL, inaccessiblememonly=is_mutation_free_argtype(rt) ? ALWAYS_TRUE : ALWAYS_FALSE))
     end
 
     rt = partition_restriction(partition)
-
     return RTEffects(rt, UndefVarError, generic_getglobal_effects)
 end
 

Compiler/src/ssair/verify.jl

@@ -366,7 +366,7 @@ function verify_ir(ir::IRCode, print::Bool=true,
                     @verify_error "Assignment should have been removed during SSA conversion"
                     raise_error()
                 elseif stmt.head === :isdefined
-                    if length(stmt.args) > 2 || (length(stmt.args) == 2 && !isa(stmt.args[2], Bool))
+                    if length(stmt.args) > 2
                         @verify_error "malformed isdefined"
                         raise_error()
                     end
@@ -382,7 +382,7 @@ function verify_ir(ir::IRCode, print::Bool=true,
                 elseif stmt.head === :foreigncall
                     isforeigncall = true
                 elseif stmt.head === :isdefined && length(stmt.args) == 1 &&
-                        (stmt.args[1] isa GlobalRef || isexpr(stmt.args[1], :static_parameter))
+                        isexpr(stmt.args[1], :static_parameter)
                     # a GlobalRef or static_parameter isdefined check does not evaluate its argument
                     continue
                 elseif stmt.head === :call

base/boot.jl

@@ -231,7 +231,7 @@ export
     fieldtype, getfield, setfield!, swapfield!, modifyfield!, replacefield!, setfieldonce!,
     nfields, throw, tuple, ===, isdefined, eval,
     # access to globals
-    getglobal, setglobal!, swapglobal!, modifyglobal!, replaceglobal!, setglobalonce!,
+    getglobal, setglobal!, swapglobal!, modifyglobal!, replaceglobal!, setglobalonce!, isdefinedglobal,
     # ifelse, sizeof    # not exported, to avoid conflicting with Base
     # type reflection
     <:, typeof, isa, typeassert,

base/docs/basedocs.jl

@@ -2754,6 +2754,9 @@ compatible with the stores to that location. Otherwise, if not declared as
 
 To test whether an array element is defined, use [`isassigned`](@ref) instead.
 
+The global variable variant is supported for compatibility with older julia
+releases. For new code, prefer [`isdefinedglobal`](@ref).
+
 See also [`@isdefined`](@ref).
 
 # Examples
@@ -2781,6 +2784,37 @@ false
 """
 isdefined
 
+
+"""
+    isdefinedglobal(m::Module, s::Symbol, [allow_import::Bool=true, [order::Symbol=:unordered]])
+
+Tests whether a global variable `s` is defined in module `m` (in the current world age).
+A variable is considered defined if and only if a value may be read from this global variable
+and an access will not throw. This includes both constants and global variables that have
+a value set.
+
+If `allow_import` is `false`, the global variable must be defined inside `m`
+and may not be imported from another module.
+
+See also [`@isdefined`](@ref).
+
+# Examples
+```jldoctest
+julia> isdefinedglobal(Base, :sum)
+true
+
+julia> isdefinedglobal(Base, :NonExistentMethod)
+false
+
+julia> isdefinedglobal(Base, :sum, false)
+true
+
+julia> isdefinedglobal(Main, :sum, false)
+false
+```
+"""
+isdefinedglobal
+
 """
     Memory{T}(undef, n)
 

doc/src/base/base.md

@@ -166,6 +166,7 @@ Core.replacefield!
 Core.swapfield!
 Core.setfieldonce!
 Core.isdefined
+Core.isdefinedglobal
 Base.@isdefined
 Base.convert
 Base.promote

doc/src/devdocs/ast.md

@@ -426,11 +426,8 @@ These symbols appear in the `head` field of [`Expr`](@ref)s in lowered form.
 
   * `isdefined`
 
-    `Expr(:isdefined, :x [, allow_import])` returns a Bool indicating whether `x` has
-    already been defined in the current scope. The optional second argument is a boolean
-    that specifies whether `x` should be considered defined by an import or if only constants
-    or globals in the current module count as being defined. If `x` is not a global, the argument
-    is ignored.
+    `Expr(:isdefined, :x)` returns a Bool indicating whether `x` has
+    already been defined in the current scope.
 
   * `the_exception`
 

src/builtin_proto.h

@@ -45,6 +45,7 @@ DECLARE_BUILTIN(invoke);
 DECLARE_BUILTIN(is);
 DECLARE_BUILTIN(isa);
 DECLARE_BUILTIN(isdefined);
+DECLARE_BUILTIN(isdefinedglobal);
 DECLARE_BUILTIN(issubtype);
 DECLARE_BUILTIN(memorynew);
 DECLARE_BUILTIN(memoryref);

src/builtins.c

@@ -1352,6 +1352,33 @@ JL_CALLABLE(jl_f_getglobal)
     return v;
 }
 
+JL_CALLABLE(jl_f_isdefinedglobal)
+{
+    jl_module_t *m = NULL;
+    jl_sym_t *s = NULL;
+    JL_NARGS(isdefined, 2, 3);
+    int allow_import = 1;
+    enum jl_memory_order order = jl_memory_order_unspecified;
+    JL_TYPECHK(isdefined, module, args[0]);
+    JL_TYPECHK(isdefined, symbol, args[1]);
+    if (nargs == 3) {
+        JL_TYPECHK(isdefined, bool, args[2]);
+        allow_import = jl_unbox_bool(args[2]);
+    }
+    if (nargs == 4) {
+        JL_TYPECHK(isdefined, symbol, args[3]);
+        order = jl_get_atomic_order_checked((jl_sym_t*)args[2], 1, 0);
+    }
+    m = (jl_module_t*)args[0];
+    s = (jl_sym_t*)args[1];
+    if (order == jl_memory_order_unspecified)
+        order = jl_memory_order_unordered;
+    if (order < jl_memory_order_unordered)
+        jl_atomic_error("isdefined: module binding cannot be accessed non-atomically");
+    int bound = jl_boundp(m, s, allow_import); // seq_cst always
+    return bound ? jl_true : jl_false;
+}
+
 JL_CALLABLE(jl_f_setglobal)
 {
     enum jl_memory_order order = jl_memory_order_release;
@@ -2451,6 +2478,7 @@ void jl_init_primitives(void) JL_GC_DISABLED
     // module bindings
     jl_builtin_getglobal = add_builtin_func("getglobal", jl_f_getglobal);
     jl_builtin_setglobal = add_builtin_func("setglobal!", jl_f_setglobal);
+    jl_builtin_isdefinedglobal = add_builtin_func("isdefinedglobal", jl_f_isdefinedglobal);
     add_builtin_func("get_binding_type", jl_f_get_binding_type);
     jl_builtin_swapglobal = add_builtin_func("swapglobal!", jl_f_swapglobal);
     jl_builtin_replaceglobal = add_builtin_func("replaceglobal!", jl_f_replaceglobal);