Prohibit binding replacement in closed modules during precompile (#57425

)

This applies the existing prohibition against introducing new bindings
in a closed module to binding replacement as well (for the same reason -
the change won't be persisted after reload). It is pretty hard to even
reach this point, since `eval` into closed modules is already prohibited
and there's no surface syntax for cross-module declaration, but it is
technically reachable from lowered IR. Further, in the future we may
make all of these builtins, which would make it easier. Thus, be
consistent now and fully disallow binding replacement in closed modules
during precompile.

src/julia_internal.h

@@ -1185,7 +1185,9 @@ _Atomic(jl_value_t*) *jl_table_peek_bp(jl_genericmemory_t *a, jl_value_t *key) J
 
 JL_DLLEXPORT jl_method_t *jl_new_method_uninit(jl_module_t*);
 
+JL_DLLEXPORT jl_module_t *jl_new_module__(jl_sym_t *name, jl_module_t *parent);
 JL_DLLEXPORT jl_module_t *jl_new_module_(jl_sym_t *name, jl_module_t *parent, uint8_t default_using_core, uint8_t self_name);
+JL_DLLEXPORT void jl_add_default_names(jl_module_t *m, uint8_t default_using_core, uint8_t self_name);
 JL_DLLEXPORT jl_methtable_t *jl_new_method_table(jl_sym_t *name, jl_module_t *module);
 JL_DLLEXPORT jl_method_instance_t *jl_get_specialization1(jl_tupletype_t *types JL_PROPAGATES_ROOT, size_t world, int mt_cache);
 jl_method_instance_t *jl_get_specialized(jl_method_t *m, jl_value_t *types, jl_svec_t *sp) JL_PROPAGATES_ROOT;

src/module.c

@@ -221,7 +221,7 @@ jl_binding_partition_t *jl_get_binding_partition_all(jl_binding_t *b, size_t min
     return bpart;
 }
 
-JL_DLLEXPORT jl_module_t *jl_new_module_(jl_sym_t *name, jl_module_t *parent, uint8_t default_using_core, uint8_t self_name)
+JL_DLLEXPORT jl_module_t *jl_new_module__(jl_sym_t *name, jl_module_t *parent)
 {
     jl_task_t *ct = jl_current_task;
     const jl_uuid_t uuid_zero = {0, 0};
@@ -253,7 +253,11 @@ JL_DLLEXPORT jl_module_t *jl_new_module_(jl_sym_t *name, jl_module_t *parent, ui
     jl_atomic_store_relaxed(&m->bindings, jl_emptysvec);
     jl_atomic_store_relaxed(&m->bindingkeyset, (jl_genericmemory_t*)jl_an_empty_memory_any);
     arraylist_new(&m->usings, 0);
-    JL_GC_PUSH1(&m);
+    return m;
+}
+
+JL_DLLEXPORT void jl_add_default_names(jl_module_t *m, uint8_t default_using_core, uint8_t self_name)
+{
     if (jl_core_module) {
         // Bootstrap: Before jl_core_module is defined, we don't have enough infrastructure
         // for bindings, so Core itself gets special handling in jltypes.c
@@ -262,14 +266,22 @@ JL_DLLEXPORT jl_module_t *jl_new_module_(jl_sym_t *name, jl_module_t *parent, ui
         }
         if (self_name) {
             // export own name, so "using Foo" makes "Foo" itself visible
-            jl_set_const(m, name, (jl_value_t*)m);
-            jl_module_public(m, name, 1);
+            jl_set_const(m, m->name, (jl_value_t*)m);
+            jl_module_public(m, m->name, 1);
         }
     }
+}
+
+JL_DLLEXPORT jl_module_t *jl_new_module_(jl_sym_t *name, jl_module_t *parent, uint8_t default_using_core, uint8_t self_name)
+{
+    jl_module_t *m = jl_new_module__(name, parent);
+    JL_GC_PUSH1(&m);
+    jl_add_default_names(m, default_using_core, self_name);
     JL_GC_POP();
     return m;
 }
 
+
 // Precondition: world_counter_lock is held
 JL_DLLEXPORT jl_binding_partition_t *jl_declare_constant_val3(
     jl_binding_t *b, jl_module_t *mod, jl_sym_t *var, jl_value_t *val,
@@ -1273,6 +1285,7 @@ JL_DLLEXPORT jl_binding_partition_t *jl_replace_binding_locked(jl_binding_t *b,
 JL_DLLEXPORT jl_binding_partition_t *jl_replace_binding_locked2(jl_binding_t *b,
     jl_binding_partition_t *old_bpart, jl_value_t *restriction_val, size_t kind, size_t new_world)
 {
+    check_safe_newbinding(b->globalref->mod, b->globalref->name);
     assert(jl_atomic_load_relaxed(&b->partitions) == old_bpart);
     jl_atomic_store_release(&old_bpart->max_world, new_world-1);
     jl_binding_partition_t *new_bpart = new_binding_partition();

src/toplevel.c

@@ -138,13 +138,15 @@ static jl_value_t *jl_eval_module_expr(jl_module_t *parent_module, jl_expr_t *ex
     // If we have `Base`, don't also try to import `Core` - the `Base` exports are a superset.
     // While we allow multiple imports of the same binding from different modules, various error printing
     // performs reflection on which module a binding came from and we'd prefer users see "Base" here.
-    jl_module_t *newm = jl_new_module_(name, is_parent__toplevel__ ? NULL : parent_module, std_imports && jl_base_module != NULL ? 0 : 1, 1);
+    jl_module_t *newm = jl_new_module__(name, is_parent__toplevel__ ? NULL : parent_module);
     jl_value_t *form = (jl_value_t*)newm;
     JL_GC_PUSH1(&form);
     JL_LOCK(&jl_modules_mutex);
     ptrhash_put(&jl_current_modules, (void*)newm, (void*)((uintptr_t)HT_NOTFOUND + 1));
     JL_UNLOCK(&jl_modules_mutex);
 
+    jl_add_default_names(newm, std_imports && jl_base_module != NULL ? 0 : 1, 1);
+
     jl_module_t *old_toplevel_module = jl_precompile_toplevel_module;
 
     // copy parent environment info into submodule

test/precompile.jl

@@ -2311,4 +2311,25 @@ precompile_test_harness("llvmcall validation") do load_path
     end
 end
 
+precompile_test_harness("BindingReplaceDisallow") do load_path
+    write(joinpath(load_path, "BindingReplaceDisallow.jl"),
+        """
+        module BindingReplaceDisallow
+        const sinreplace = try
+            eval(Expr(:block,
+                Expr(:const, GlobalRef(Base, :sin), 1),
+                nothing))
+        catch ex
+            ex isa ErrorException || rethrow()
+            ex
+        end
+        end
+        """)
+    ji, ofile = Base.compilecache(Base.PkgId("BindingReplaceDisallow"))
+    @eval using BindingReplaceDisallow
+    invokelatest() do
+        @test BindingReplaceDisallow.sinreplace.msg == "Creating a new global in closed module `Base` (`sin`) breaks incremental compilation because the side effects will not be permanent."
+    end
+end
+
 finish_precompile_test!()