lowering: Don't resolve type bindings earlier than necessary

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.
JuliaLang · Jul 4, 2024 · 9615722 · 9615722
1 parent f2558c4
commit 9615722
Show file tree

Hide file tree

Showing 9 changed files with 73 additions and 22 deletions.
diff --git a/base/compiler/validation.jl b/base/compiler/validation.jl
@@ -24,7 +24,7 @@ const VALID_EXPR_HEADS = IdDict{Symbol,UnitRange{Int}}(
     :global => 1:1,
     :foreigncall => 5:typemax(Int), # name, RT, AT, nreq, (cconv, effects), args..., roots...
     :cfunction => 5:5,
-    :isdefined => 1:1,
+    :isdefined => 1:2,
     :code_coverage_effect => 0:0,
     :loopinfo => 0:typemax(Int),
     :gc_preserve_begin => 0:typemax(Int),

diff --git a/doc/src/devdocs/ast.md b/doc/src/devdocs/ast.md
@@ -426,8 +426,11 @@ These symbols appear in the `head` field of [`Expr`](@ref)s in lowered form.
 
   * `isdefined`
 
-    `Expr(:isdefined, :x)` returns a Bool indicating whether `x` has
-    already been defined in the current scope.
+    `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.
 
   * `the_exception`
 

diff --git a/src/builtins.c b/src/builtins.c
@@ -1278,7 +1278,7 @@ JL_CALLABLE(jl_f_isdefined)
             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); // seq_cst always
+        int bound = jl_boundp(m, s, 1); // seq_cst always
         return bound ? jl_true : jl_false;
     }
     jl_datatype_t *vt = (jl_datatype_t*)jl_typeof(args[0]);

diff --git a/src/codegen.cpp b/src/codegen.cpp
@@ -956,7 +956,7 @@ static const auto jlboundp_func = new JuliaFunction<>{
     [](LLVMContext &C) {
         auto T_pjlvalue = JuliaType::get_pjlvalue_ty(C);
         return FunctionType::get(getInt32Ty(C),
-                {T_pjlvalue, T_pjlvalue}, false);
+                {T_pjlvalue, T_pjlvalue, getInt32Ty(C)}, false);
     },
     nullptr,
 };
@@ -5545,7 +5545,7 @@ static jl_cgval_t emit_sparam(jl_codectx_t &ctx, size_t i)
     return mark_julia_type(ctx, sp, true, jl_any_type);
 }
 
-static jl_cgval_t emit_isdefined(jl_codectx_t &ctx, jl_value_t *sym)
+static jl_cgval_t emit_isdefined(jl_codectx_t &ctx, jl_value_t *sym, int allow_import)
 {
     Value *isnull = NULL;
     if (jl_is_slotnumber(sym) || jl_is_argument(sym)) {
@@ -5604,8 +5604,8 @@ static jl_cgval_t emit_isdefined(jl_codectx_t &ctx, jl_value_t *sym)
             modu = ctx.module;
             name = (jl_sym_t*)sym;
         }
-        jl_binding_t *bnd = jl_get_binding(modu, name);
-        if (bnd) {
+        jl_binding_t *bnd = allow_import ? jl_get_binding(modu, name) : jl_get_module_binding(modu, name, 0);
+        if (bnd && jl_atomic_load_relaxed(&bnd->owner) == bnd) {
             if (jl_atomic_load_acquire(&bnd->value) != NULL && bnd->constp)
                 return mark_julia_const(ctx, jl_true);
             Value *bp = julia_binding_gv(ctx, bnd);
@@ -5619,7 +5619,8 @@ static jl_cgval_t emit_isdefined(jl_codectx_t &ctx, jl_value_t *sym)
         else {
             Value *v = ctx.builder.CreateCall(prepare_call(jlboundp_func), {
                     literal_pointer_val(ctx, (jl_value_t*)modu),
-                    literal_pointer_val(ctx, (jl_value_t*)name)
+                    literal_pointer_val(ctx, (jl_value_t*)name),
+                    ConstantInt::get(getInt32Ty(ctx.builder.getContext()), allow_import)
                 });
             isnull = ctx.builder.CreateICmpNE(v, ConstantInt::get(getInt32Ty(ctx.builder.getContext()), 0));
         }
@@ -6304,8 +6305,13 @@ static jl_cgval_t emit_expr(jl_codectx_t &ctx, jl_value_t *expr, ssize_t ssaidx_
     // however, this is a good way to do it because it should *not* be easy
     // to add new node types.
     if (head == jl_isdefined_sym) {
-        assert(nargs == 1);
-        return emit_isdefined(ctx, args[0]);
+        assert(nargs == 1 || nargs == 2);
+        int allow_import = 1;
+        if (nargs == 2) {
+            assert(jl_is_bool(args[1]));
+            allow_import = args[1] == jl_true;
+        }
+        return emit_isdefined(ctx, args[0], allow_import);
     }
     else if (head == jl_throw_undef_if_not_sym) {
         assert(nargs == 2);

diff --git a/src/interpreter.c b/src/interpreter.c
@@ -232,17 +232,22 @@ static jl_value_t *eval_value(jl_value_t *e, interpreter_state *s)
     else if (head == jl_isdefined_sym) {
         jl_value_t *sym = args[0];
         int defined = 0;
+        int allow_import = 1;
+        if (nargs == 2) {
+            assert(jl_is_bool(args[1]) && "malformed IR");
+            allow_import = args[1] == jl_true;
+        }
         if (jl_is_slotnumber(sym) || jl_is_argument(sym)) {
             ssize_t n = jl_slot_number(sym);
             if (src == NULL || n > jl_source_nslots(src) || n < 1 || s->locals == NULL)
                 jl_error("access to invalid slot number");
             defined = s->locals[n - 1] != NULL;
         }
         else if (jl_is_globalref(sym)) {
-            defined = jl_boundp(jl_globalref_mod(sym), jl_globalref_name(sym));
+            defined = jl_boundp(jl_globalref_mod(sym), jl_globalref_name(sym), allow_import);
         }
         else if (jl_is_symbol(sym)) {
-            defined = jl_boundp(s->module, (jl_sym_t*)sym);
+            defined = jl_boundp(s->module, (jl_sym_t*)sym, allow_import);
         }
         else if (jl_is_expr(sym) && ((jl_expr_t*)sym)->head == jl_static_parameter_sym) {
             ssize_t n = jl_unbox_long(jl_exprarg(sym, 0));

diff --git a/src/julia-syntax.scm b/src/julia-syntax.scm
@@ -983,8 +983,6 @@
                      (error (string "field name \"" (deparse v) "\" is not a symbol"))))
                field-names)
      `(block
-       ;; This is to prevent the :isdefined below from resolving the binding to an import.
-       ;; This will be reworked to a different check with world-age partitioned bindings.
        (global ,name)
        (scope-block
         (block
@@ -998,7 +996,7 @@
                         (call (core svec) ,@attrs)
                         ,mut ,min-initialized))
          (call (core _setsuper!) ,name ,super)
-         (if (isdefined (globalref (thismodule) ,name))
+         (if (isdefined (globalref (thismodule) ,name) (false))
              (block
               (= ,prev (globalref (thismodule) ,name))
               (if (call (core _equiv_typedef) ,prev ,name)
@@ -1061,7 +1059,7 @@
        (= ,name (call (core _abstracttype) (thismodule) (inert ,name) (call (core svec) ,@params)))
        (call (core _setsuper!) ,name ,super)
        (call (core _typebody!) ,name)
-       (if (&& (isdefined (globalref (thismodule) ,name))
+       (if (&& (isdefined (globalref (thismodule) ,name) (false))
                (call (core _equiv_typedef) (globalref (thismodule) ,name) ,name))
            (null)
            (const (globalref (thismodule) ,name) ,name))
@@ -1081,7 +1079,7 @@
        (= ,name (call (core _primitivetype) (thismodule) (inert ,name) (call (core svec) ,@params) ,n))
        (call (core _setsuper!) ,name ,super)
        (call (core _typebody!) ,name)
-       (if (&& (isdefined (globalref (thismodule) ,name))
+       (if (&& (isdefined (globalref (thismodule) ,name) (false))
                (call (core _equiv_typedef) (globalref (thismodule) ,name) ,name))
            (null)
            (const (globalref (thismodule) ,name) ,name))

diff --git a/src/julia.h b/src/julia.h
@@ -1943,7 +1943,7 @@ JL_DLLEXPORT jl_value_t *jl_get_binding_type(jl_module_t *m, jl_sym_t *var);
 // get binding for assignment
 JL_DLLEXPORT jl_binding_t *jl_get_binding_wr(jl_module_t *m JL_PROPAGATES_ROOT, jl_sym_t *var, int alloc);
 JL_DLLEXPORT jl_binding_t *jl_get_binding_for_method_def(jl_module_t *m JL_PROPAGATES_ROOT, jl_sym_t *var);
-JL_DLLEXPORT int jl_boundp(jl_module_t *m, jl_sym_t *var);
+JL_DLLEXPORT int jl_boundp(jl_module_t *m, jl_sym_t *var, int allow_import);
 JL_DLLEXPORT int jl_defines_or_exports_p(jl_module_t *m, jl_sym_t *var);
 JL_DLLEXPORT int jl_binding_resolved_p(jl_module_t *m, jl_sym_t *var);
 JL_DLLEXPORT int jl_is_const(jl_module_t *m, jl_sym_t *var);

diff --git a/src/module.c b/src/module.c
@@ -681,10 +681,10 @@ JL_DLLEXPORT void jl_module_public(jl_module_t *from, jl_sym_t *s, int exported)
     b->exportp |= exported;
 }
 
-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
 {
-    jl_binding_t *b = jl_get_binding(m, var);
-    return b && (jl_atomic_load(&b->value) != NULL);
+    jl_binding_t *b = allow_import ? jl_get_binding(m, var) : jl_get_module_binding(m, var, 0);
+    return b && (jl_atomic_load_relaxed(&b->owner) == b) && (jl_atomic_load(&b->value) != NULL);
 }
 
 JL_DLLEXPORT int jl_defines_or_exports_p(jl_module_t *m, jl_sym_t *var)

diff --git a/test/syntax.jl b/test/syntax.jl
@@ -3865,3 +3865,42 @@ module UndefGlobal54954
 end
 using .UndefGlobal54954: theglobal54954
 @test Core.get_binding_type(@__MODULE__, :theglobal54954) === Int
+
+# Extended isdefined
+module ExtendedIsDefined
+    using Test
+    module Import
+        export x2, x3
+        x2 = 2
+        x3 = 3
+        x4 = 4
+    end
+    const x1 = 1
+    using .Import
+    import .Import.x4
+    @test x2 == 2 # Resolve the binding
+    @eval begin
+        @test $(Expr(:isdefined, GlobalRef(@__MODULE__, :x1)))
+        @test $(Expr(:isdefined, GlobalRef(@__MODULE__, :x2)))
+        @test $(Expr(:isdefined, GlobalRef(@__MODULE__, :x3)))
+        @test $(Expr(:isdefined, GlobalRef(@__MODULE__, :x4)))
+
+        @test $(Expr(:isdefined, GlobalRef(@__MODULE__, :x1), false))
+        @test !$(Expr(:isdefined, GlobalRef(@__MODULE__, :x2), false))
+        @test !$(Expr(:isdefined, GlobalRef(@__MODULE__, :x3), false))
+        @test !$(Expr(:isdefined, GlobalRef(@__MODULE__, :x4), false))
+    end
+
+    @eval begin
+        @Base.Experimental.force_compile
+        @test $(Expr(:isdefined, GlobalRef(@__MODULE__, :x1)))
+        @test $(Expr(:isdefined, GlobalRef(@__MODULE__, :x2)))
+        @test $(Expr(:isdefined, GlobalRef(@__MODULE__, :x3)))
+        @test $(Expr(:isdefined, GlobalRef(@__MODULE__, :x4)))
+
+        @test $(Expr(:isdefined, GlobalRef(@__MODULE__, :x1), false))
+        @test !$(Expr(:isdefined, GlobalRef(@__MODULE__, :x2), false))
+        @test !$(Expr(:isdefined, GlobalRef(@__MODULE__, :x3), false))
+        @test !$(Expr(:isdefined, GlobalRef(@__MODULE__, :x4), false))
+    end
+end