It is now possible to create globals in a different module

[LilithHafner]

x@fedora:~$ julia +1.8 -q
julia> module M; end
Main.M

julia> M.x = 6
ERROR: cannot assign variables in other modules
Stacktrace:
 [1] setproperty!(x::Module, f::Symbol, v::Int64)
   @ Base ./Base.jl:32
 [2] top-level scope
   @ REPL[2]:1

julia> 
x@fedora:~$ julia +1.9 -q
julia> module M; end
Main.M

julia> M.x = 6
6

From the discussion at #44231, this seems unintentional.

[cstjean]

Personally, I love the change. Especially now that globals can be typed properly, being able to just MyPackage.some_global_setting = true is just a very nice and convenient interface for packages.

The former behaviour had that "You are a bad person for using globals" vibe that just felt out of sync with the general permissiveness of a language that allows me to redefine addition if I feel like it.

[vtjnash]

This is a little different, since it not only sets the global, it also creates it if it didn't yet exist and assigns its type. I have entertained the idea of disallowing setting globals that don't exist (it is a 2 line patch roughly), but it breaks a few minor things (Serialization specifically) so it isn't entirely obvious that is beneficial just to help catch typos.

[KristofferC]

From the discussion at #44231, this seems unintentional.

I'm reading through that PR but can't really find that discussion. Could you link to it explicitly?

Anyway, this seems fine to me, having to go via eval always felt silly to me.

[LilithHafner]

I suspect that it happened in a triage discussion referenced by #44231 (comment)

[bvdmitri]

@vtjnash could Serialization use a somewhat private API for it instead of relying on the = operator?

[vtjnash]

Sure, it could use Core.eval(m, :(global $var)) before setglobal!(m, var, val). The patch looks something like this, though apparently some code has refactored so it takes a bit more than 2 lines right now to add it:

diff --git a/src/module.c b/src/module.c
index 7a12552415..470bf3df98 100644
--- a/src/module.c
+++ b/src/module.c
@@ -219,13 +219,16 @@ static void check_safe_newbinding(jl_module_t *m, jl_sym_t *var)
 static jl_module_t *jl_binding_dbgmodule(jl_binding_t *b, jl_module_t *m, jl_sym_t *var) JL_GLOBALLY_ROOTED;
 
 // get binding for assignment
-JL_DLLEXPORT jl_binding_t *jl_get_binding_wr(jl_module_t *m JL_PROPAGATES_ROOT, jl_sym_t *var)
+JL_DLLEXPORT jl_binding_t *jl_get_binding_wr(jl_module_t *m JL_PROPAGATES_ROOT, jl_sym_t *var, int alloc)
 {
     jl_binding_t *b = jl_get_module_binding(m, var, 1);
     jl_binding_t *b2 = jl_atomic_load_relaxed(&b->owner);
     if (b2 != b) {
-        if (b2 == NULL)
+        if (b2 == NULL) {
             check_safe_newbinding(m, var);
+            if (!alloc)
+                jl_errorf("Global %s.%s cannot be set since it does not exist.", jl_symbol_name(m->name), jl_symbol_name(var));
+        }
         if (b2 != NULL || (!jl_atomic_cmpswap(&b->owner, &b2, b) && b2 != b)) {
             jl_module_t *from = jl_binding_dbgmodule(b, m, var);
             if (from == m)

[KristofferC]

I suspect that it happened in a triage discussion referenced by #44231 (comment)

But there is nothing in that comment saying that setting globals in different modules should not be allowed? In fact, it explicitly removes the test that checks that this is not allowed https://github.com/JuliaLang/julia/pull/44231/files#diff-829029ba7e34234a5360ccd30854edbcff7fb932c1d2f9a52f1027e881ae9a7a.

Removing this now would be breaking anyway so maybe just best to let this be.

[LilithHafner]

Perhaps I misunderstood the "don't do setfield!" sentiment as "don't do property access syntax". In any event, this issue has accomplished its objective of double checking that we removed that intentional error intentionally.

[KristofferC]

I think that the statement there is intended to be read that settings and reading globals from modules shouldn't piggy back on the setfield! / getfield intrinsics but have their own. Then setproperty!/getproperty just uses these new intrinsics.

[topolarity]

This is a little different, since it not only sets the global, it also creates it if it didn't yet exist and assigns its type.

I think that means that #53750 is incomplete and also needs to consider that binding types can revert to undef, unless we prohibit this behavior

[Keno]

Discussed in the context of the meeting we had for #54654 and decided that this is a serious enough oversight that we should attempt to correct this as quickly as possible, keeping in mind that it was in the system for two releases, so we need to at the very least pkgeval and may do a soft deprecation.

[LilithHafner]

Triage is are okay with keeping changing values of globals in other modules with Mod.x = 6 syntax and setglobal!.

We want to change setglobal! to no longer create new globals. Mod.x = 6 should keep calling setglobal!.
x = 6 within a module should be special in that it may create a new global (and increment world age)