From d3e52ba974ef816e60341a9c5250f88e5e7b9bbf Mon Sep 17 00:00:00 2001
From: Jameson Nash <vtjnash@gmail.com>
Date: Mon, 24 Jul 2017 11:37:33 -0400
Subject: [PATCH] more predictable global binding resolution

This changes the meaning of `global` from being a directive when used at toplevel,
which forces the introduction of a new global when used in certain contexts,
to always being just an scope annotation that there should exist a global binding for the given name.

fix #18933
fix #17387 (for the syntactic case)
---
 NEWS.md                                 |  9 +++
 base/poll.jl                            |  3 +-
 doc/src/manual/modules.md               |  6 +-
 doc/src/manual/variables-and-scoping.md | 22 ++----
 src/codegen.cpp                         | 11 ++-
 src/dump.c                              |  4 +-
 src/interpreter.c                       | 48 ++++---------
 src/julia-syntax.scm                    | 91 +++++++++++++------------
 src/julia.h                             |  2 +-
 src/method.c                            |  8 ++-
 src/module.c                            | 60 ++++++++--------
 src/staticdata.c                        |  2 +-
 src/toplevel.c                          | 28 +++++++-
 test/codegen.jl                         |  9 ++-
 test/core.jl                            | 37 ++++++++++
 test/parse.jl                           |  5 +-
 16 files changed, 199 insertions(+), 146 deletions(-)

diff --git a/NEWS.md b/NEWS.md
index 7e461a30be6a32..207e8a216f1b42 100644
--- a/NEWS.md
+++ b/NEWS.md
@@ -32,6 +32,15 @@ Language changes
   * `{ }` expressions now use `braces` and `bracescat` as expression heads instead
     of `cell1d` and `cell2d`, and parse similarly to `vect` and `vcat` ([#8470]).
 
+  * The `global` keyword now only introduces a new binding if one doesn't already exist
+    in the module.
+    This means that assignment to a global (`global sin = 3`) may now throw the error:
+    "cannot assign variable Base.sin from module Main", rather than emitting a warning.
+    Additionally, the new bindings are now created before the statement is executed.
+    For example, `f() = (global sin = "gluttony"; nothing)` will now resolve which module
+    contains `sin` eagerly, rather than delaying that decision until `f` is run. ([#22984]).
+
+
 Breaking changes
 ----------------
 
diff --git a/base/poll.jl b/base/poll.jl
index 28deeb33fabf51..87b157b1208b30 100644
--- a/base/poll.jl
+++ b/base/poll.jl
@@ -145,8 +145,7 @@ mutable struct _FDWatcher
             end
         end
 
-        global uvfinalize
-        function uvfinalize(t::_FDWatcher)
+        function Base.uvfinalize(t::_FDWatcher)
             if t.handle != C_NULL
                 disassociate_julia_struct(t)
                 ccall(:jl_close_uv, Void, (Ptr{Void},), t.handle)
diff --git a/doc/src/manual/modules.md b/doc/src/manual/modules.md
index 3063b1d45c41cb..921f195c7a3176 100644
--- a/doc/src/manual/modules.md
+++ b/doc/src/manual/modules.md
@@ -214,9 +214,8 @@ in other modules can be invoked as `Mod.@mac` or `@Mod.mac`.
 The syntax `M.x = y` does not work to assign a global in another module; global assignment is
 always module-local.
 
-A variable can be "reserved" for the current module without assigning to it by declaring it as
-`global x` at the top level. This can be used to prevent name conflicts for globals initialized
-after load time.
+A variable name can be "reserved" without assigning to it by declaring it as `global x`.
+This prevents name conflicts for globals initialized after load time.
 
 ### Module initialization and precompilation
 
@@ -283,6 +282,7 @@ const foo_data_ptr = Ref{Ptr{Void}}(0)
 function __init__()
     ccall((:foo_init, :libfoo), Void, ())
     foo_data_ptr[] = ccall((:foo_data, :libfoo), Ptr{Void}, ())
+    nothing
 end
 ```
 
diff --git a/doc/src/manual/variables-and-scoping.md b/doc/src/manual/variables-and-scoping.md
index e519050bd4e0b2..f1df11b7ed5a36 100644
--- a/doc/src/manual/variables-and-scoping.md
+++ b/doc/src/manual/variables-and-scoping.md
@@ -174,19 +174,6 @@ julia> x
 12
 ```
 
-Within soft scopes, the *global* keyword is never necessary, although allowed. The only case
-when it would change the semantics is (currently) a syntax error:
-
-```jldoctest
-julia> let
-           local j = 2
-           let
-               global j = 3
-           end
-       end
-ERROR: syntax: `global j`: j is local variable in the enclosing scope
-```
-
 ### Hard Local Scope
 
 Hard local scopes are introduced by function definitions (in all their forms), struct type definition blocks,
@@ -336,8 +323,7 @@ constructing [closures](https://en.wikipedia.org/wiki/Closure_%28computer_progra
 have a private state, for instance the `state` variable in the following example:
 
 ```jldoctest
-julia> let
-           state = 0
+julia> let state = 0
            global counter
            counter() = state += 1
        end;
@@ -483,13 +469,13 @@ julia> const e  = 2.71828182845904523536;
 julia> const pi = 3.14159265358979323846;
 ```
 
-The `const` declaration is allowed on both global and local variables, but is especially useful
-for globals. It is difficult for the compiler to optimize code involving global variables, since
+The `const` declaration should only be used in global scope on globals.
+It is difficult for the compiler to optimize code involving global variables, since
 their values (or even their types) might change at almost any time. If a global variable will
 not change, adding a `const` declaration solves this performance problem.
 
 Local constants are quite different. The compiler is able to determine automatically when a local
-variable is constant, so local constant declarations are not necessary for performance purposes.
+variable is constant, so local constant declarations are not necessary, and are currently just ignored.
 
 Special top-level assignments, such as those performed by the `function` and `struct` keywords,
 are constant by default.
diff --git a/src/codegen.cpp b/src/codegen.cpp
index 8724918761ee1b..05b6c208c15fa7 100644
--- a/src/codegen.cpp
+++ b/src/codegen.cpp
@@ -3000,8 +3000,15 @@ static Value *global_binding_pointer(jl_codectx_t &ctx, jl_module_t *m, jl_sym_t
 {
     jl_binding_t *b = NULL;
     if (assign) {
-        b = jl_get_binding_wr(m, s);
+        b = jl_get_binding_wr(m, s, 0);
         assert(b != NULL);
+        if (b->owner != m) {
+            char *msg;
+            asprintf(&msg, "cannot assign variable %s.%s from module %s",
+                     jl_symbol_name(b->owner->name), jl_symbol_name(s), jl_symbol_name(m->name));
+            emit_error(ctx, msg);
+            free(msg);
+        }
     }
     else {
         b = jl_get_binding(m, s);
@@ -3736,7 +3743,7 @@ static jl_cgval_t emit_expr(jl_codectx_t &ctx, jl_value_t *expr)
                 jl_ptls_t ptls = jl_get_ptls_states();
                 jl_value_t *e = ptls->exception_in_transit;
                 // errors. boo. root it somehow :(
-                bnd = jl_get_binding_wr(ctx.module, (jl_sym_t*)jl_gensym());
+                bnd = jl_get_binding_wr(ctx.module, (jl_sym_t*)jl_gensym(), 1);
                 bnd->value = e;
                 bnd->constp = 1;
                 raise_exception(ctx, literal_pointer_val(ctx, e));
diff --git a/src/dump.c b/src/dump.c
index 4383079e2f20e6..0480d1a5b09c03 100644
--- a/src/dump.c
+++ b/src/dump.c
@@ -1515,7 +1515,7 @@ static jl_value_t *jl_deserialize_value_module(jl_serializer_state *s)
         jl_sym_t *name = (jl_sym_t*)jl_deserialize_value(s, NULL);
         if (name == NULL)
             break;
-        jl_binding_t *b = jl_get_binding_wr(m, name);
+        jl_binding_t *b = jl_get_binding_wr(m, name, 1);
         b->value = jl_deserialize_value(s, &b->value);
         jl_gc_wb_buf(m, b, sizeof(jl_binding_t));
         if (b->value != NULL) jl_gc_wb(m, b->value);
@@ -1981,7 +1981,7 @@ static void jl_reinit_item(jl_value_t *v, int how, arraylist_t *tracee_list)
             }
             case 2: { // reinsert module v into parent (const)
                 jl_module_t *mod = (jl_module_t*)v;
-                jl_binding_t *b = jl_get_binding_wr(mod->parent, mod->name);
+                jl_binding_t *b = jl_get_binding_wr(mod->parent, mod->name, 1);
                 jl_declare_constant(b); // this can throw
                 if (b->value != NULL) {
                     if (!jl_is_module(b->value)) {
diff --git a/src/interpreter.c b/src/interpreter.c
index c1ebb4ff6f3d61..1eac50d7da4808 100644
--- a/src/interpreter.c
+++ b/src/interpreter.c
@@ -302,20 +302,15 @@ static jl_value_t *eval(jl_value_t *e, interpreter_state *s)
         assert(jl_expr_nargs(ex) != 1 || jl_is_symbol(fname));
 
         if (jl_is_symbol(fname)) {
-            jl_value_t **bp=NULL;
-            jl_value_t *bp_owner=NULL;
-            jl_binding_t *b=NULL;
-            if (bp == NULL) {
-                b = jl_get_binding_for_method_def(modu, fname);
-                bp = &b->value;
-                bp_owner = (jl_value_t*)modu;
-            }
-            jl_value_t *gf = jl_generic_function_def(fname, modu, bp, bp_owner, b);
+            jl_value_t *bp_owner = (jl_value_t*)modu;
+            jl_binding_t *b = jl_get_binding_for_method_def(modu, fname);
+            jl_value_t **bp = &b->value;
+            jl_value_t *gf = jl_generic_function_def(b->name, b->owner, bp, bp_owner, b);
             if (jl_expr_nargs(ex) == 1)
                 return gf;
         }
 
-        jl_value_t *atypes=NULL, *meth=NULL;
+        jl_value_t *atypes = NULL, *meth = NULL;
         JL_GC_PUSH2(&atypes, &meth);
         atypes = eval(args[1], s);
         meth = eval(args[2], s);
@@ -330,26 +325,10 @@ static jl_value_t *eval(jl_value_t *e, interpreter_state *s)
             sym = jl_globalref_name(sym);
         }
         assert(jl_is_symbol(sym));
-        jl_binding_t *b = jl_get_binding_wr(modu, sym);
+        jl_binding_t *b = jl_get_binding_wr(modu, sym, 1);
         jl_declare_constant(b);
         return (jl_value_t*)jl_nothing;
     }
-    else if (ex->head == global_sym) {
-        // create uninitialized mutable binding for "global x" decl
-        // TODO: handle type decls
-        size_t i, l = jl_array_len(ex->args);
-        for (i = 0; i < l; i++) {
-            jl_sym_t *gsym = (jl_sym_t*)args[i];
-            jl_module_t *gmodu = modu;
-            if (jl_is_globalref(gsym)) {
-                gmodu = jl_globalref_mod(gsym);
-                gsym = jl_globalref_name(gsym);
-            }
-            assert(jl_is_symbol(gsym));
-            jl_get_binding_wr(gmodu, gsym);
-        }
-        return (jl_value_t*)jl_nothing;
-    }
     else if (ex->head == abstracttype_sym) {
         if (inside_typedef)
             jl_error("cannot eval a new abstract type definition while defining another type");
@@ -368,7 +347,7 @@ static jl_value_t *eval(jl_value_t *e, interpreter_state *s)
         assert(jl_is_symbol(name));
         dt = jl_new_abstracttype(name, modu, NULL, (jl_svec_t*)para);
         w = dt->name->wrapper;
-        jl_binding_t *b = jl_get_binding_wr(modu, (jl_sym_t*)name);
+        jl_binding_t *b = jl_get_binding_wr(modu, (jl_sym_t*)name, 1);
         temp = b->value;
         check_can_assign_type(b, w);
         b->value = w;
@@ -416,7 +395,7 @@ static jl_value_t *eval(jl_value_t *e, interpreter_state *s)
                       jl_symbol_name((jl_sym_t*)name));
         dt = jl_new_primitivetype(name, modu, NULL, (jl_svec_t*)para, nb);
         w = dt->name->wrapper;
-        jl_binding_t *b = jl_get_binding_wr(modu, (jl_sym_t*)name);
+        jl_binding_t *b = jl_get_binding_wr(modu, (jl_sym_t*)name, 1);
         temp = b->value;
         check_can_assign_type(b, w);
         b->value = w;
@@ -461,7 +440,7 @@ static jl_value_t *eval(jl_value_t *e, interpreter_state *s)
                              0, args[5]==jl_true ? 1 : 0, jl_unbox_long(args[6]));
         w = dt->name->wrapper;
 
-        jl_binding_t *b = jl_get_binding_wr(modu, (jl_sym_t*)name);
+        jl_binding_t *b = jl_get_binding_wr(modu, (jl_sym_t*)name, 1);
         temp = b->value;  // save old value
         // temporarily assign so binding is available for field types
         check_can_assign_type(b, w);
@@ -573,17 +552,14 @@ static jl_value_t *eval_body(jl_array_t *stmts, interpreter_state *s, int start,
                     s->locals[n-1] = rhs;
                 }
                 else {
-                    jl_module_t *m;
+                    jl_module_t *modu = s->module;
                     if (jl_is_globalref(sym)) {
-                        m = jl_globalref_mod(sym);
+                        modu = jl_globalref_mod(sym);
                         sym = (jl_value_t*)jl_globalref_name(sym);
                     }
-                    else {
-                        m = s->module;
-                    }
                     assert(jl_is_symbol(sym));
                     JL_GC_PUSH1(&rhs);
-                    jl_binding_t *b = jl_get_binding_wr(m, (jl_sym_t*)sym);
+                    jl_binding_t *b = jl_get_binding_wr(modu, (jl_sym_t*)sym, 1);
                     jl_checked_assignment(b, rhs);
                     JL_GC_POP();
                 }
diff --git a/src/julia-syntax.scm b/src/julia-syntax.scm
index c5d6941004ae28..dde8766165030f 100644
--- a/src/julia-syntax.scm
+++ b/src/julia-syntax.scm
@@ -2704,8 +2704,8 @@
          (let ((vi (var-info-for (cadr e) env)))
               (vinfo:set-never-undef! vi #t)))
         ((=)
-         (let ((vi (var-info-for (cadr e) env)))
-           (if vi
+         (let ((vi (and (symbol? (cadr e)) (var-info-for (cadr e) env))))
+           (if vi ; if local or captured
                (begin (if (vinfo:asgn vi)
                           (vinfo:set-sa! vi #f)
                           (vinfo:set-sa! vi #t))
@@ -2848,35 +2848,45 @@ f(x) = yt(x)
 ;; when doing this, the original value needs to be preserved, to
 ;; ensure the expression `a=b` always returns exactly `b`.
 (define (convert-assignment var rhs0 fname lam interp)
-  (let* ((vi (assq var (car  (lam:vinfo lam))))
-         (cv (assq var (cadr (lam:vinfo lam))))
-         (vt  (or (and vi (vinfo:type vi))
-                  (and cv (vinfo:type cv))
-                  '(core Any)))
-         (closed (and cv (vinfo:asgn cv) (vinfo:capt cv)))
-         (capt   (and vi (vinfo:asgn vi) (vinfo:capt vi))))
-    (if (and (not closed) (not capt) (equal? vt '(core Any)))
-        `(= ,var ,rhs0)
-        (let* ((rhs1 (if (or (ssavalue? rhs0) (simple-atom? rhs0)
-                             (equal? rhs0 '(the_exception)))
-                         rhs0
-                         (make-ssavalue)))
-               (rhs  (if (equal? vt '(core Any))
-                         rhs1
-                         (convert-for-type-decl rhs1 (cl-convert vt fname lam #f #f interp))))
-               (ex (cond (closed `(call (core setfield!)
-                                        ,(if interp
-                                             `($ ,var)
-                                             `(call (core getfield) ,fname (inert ,var)))
-                                        (inert contents)
-                                        ,rhs))
-                         (capt `(call (core setfield!) ,var (inert contents) ,rhs))
-                         (else `(= ,var ,rhs)))))
-          (if (eq? rhs1 rhs0)
-              `(block ,ex ,rhs0)
-              `(block (= ,rhs1 ,rhs0)
-                      ,ex
-                      ,rhs1))))))
+  (cond
+    ((symbol? var)
+     (let* ((vi (assq var (car  (lam:vinfo lam))))
+            (cv (assq var (cadr (lam:vinfo lam))))
+            (vt  (or (and vi (vinfo:type vi))
+                     (and cv (vinfo:type cv))
+                     '(core Any)))
+            (closed (and cv (vinfo:asgn cv) (vinfo:capt cv)))
+            (capt   (and vi (vinfo:asgn vi) (vinfo:capt vi))))
+       (if (and (not closed) (not capt) (equal? vt '(core Any)))
+           `(= ,var ,rhs0)
+           (let* ((rhs1 (if (or (ssavalue? rhs0) (simple-atom? rhs0)
+                                (equal? rhs0 '(the_exception)))
+                            rhs0
+                            (make-ssavalue)))
+                  (rhs  (if (equal? vt '(core Any))
+                            rhs1
+                            (convert-for-type-decl rhs1 (cl-convert vt fname lam #f #f interp))))
+                  (ex (cond (closed `(call (core setfield!)
+                                           ,(if interp
+                                                `($ ,var)
+                                                `(call (core getfield) ,fname (inert ,var)))
+                                           (inert contents)
+                                           ,rhs))
+                            (capt `(call (core setfield!) ,var (inert contents) ,rhs))
+                            (else `(= ,var ,rhs)))))
+             (if (eq? rhs1 rhs0)
+                 `(block ,ex ,rhs0)
+                 `(block (= ,rhs1 ,rhs0)
+                         ,ex
+                         ,rhs1))))))
+     ((and (pair? var) (or (eq? (car var) 'outerref)
+                           (eq? (car var) 'globalref)))
+
+      `(= ,var ,rhs0))
+     ((ssavalue? var)
+      `(= ,var ,rhs0))
+     (else
+       (error (string "invalid assignment location \"" (deparse var) "\"")))))
 
 ;; replace leading (function) argument type with `typ`
 (define (fix-function-arg-type te typ iskw namemap type-sp)
@@ -3063,9 +3073,7 @@ f(x) = yt(x)
           ((=)
            (let ((var (cadr e))
                  (rhs (cl-convert (caddr e) fname lam namemap toplevel interp)))
-             (if (ssavalue? var)
-                 `(= ,var ,rhs)
-                 (convert-assignment var rhs fname lam interp))))
+             (convert-assignment var rhs fname lam interp)))
           ((local-def) ;; make new Box for local declaration of defined variable
            (let ((vi (assq (cadr e) (car (lam:vinfo lam)))))
              (if (and vi (vinfo:asgn vi) (vinfo:capt vi))
@@ -3107,10 +3115,10 @@ f(x) = yt(x)
                   (lam2  (if short #f (cadddr e)))
                   (vis   (if short '(() () ()) (lam:vinfo lam2)))
                   (cvs   (map car (cadr vis)))
-                  (local? (lambda (s) (and (symbol? s)
+                  (local? (lambda (s) (and lam (symbol? s)
                                (or (assq s (car  (lam:vinfo lam)))
                                    (assq s (cadr (lam:vinfo lam)))))))
-                  (local (and lam (local? name)))
+                  (local (local? name))
                   (sig      (and (not short) (caddr e)))
                   (sp-inits (if (or short (not (eq? (car sig) 'block)))
                                 '()
@@ -3187,7 +3195,7 @@ f(x) = yt(x)
                                                                             (and (symbol? s)
                                                                                  (not (eq? name s))
                                                                                  (not (memq s capt-sp))
-                                                                                 (or ;(local? s) ; TODO: make this work for local variables too?
+                                                                                 (or ;(local? s) ; TODO: error for local variables
                                                                                    (memq s (lam:sp lam)))))))
                                                       (caddr methdef)
                                                       (lambda (e) (cadr e)))))
@@ -3313,7 +3321,7 @@ f(x) = yt(x)
 ;; numbered slots (or be simple immediate values), and then those will be the
 ;; only possible returned values.
 (define (compile-body e vi lam)
-  (let ((code '())
+  (let ((code '())            ;; statements (emitted in reverse order)
         (filename 'none)
         (first-line #t)
         (current-loc #f)
@@ -3615,13 +3623,12 @@ f(x) = yt(x)
              (if (not (and (pair? code) (equal? (car code) e)))
                  (emit e)
                  #f))
-            ((global) ; remove global declarations
+            ((global) ; keep global declarations as statements
              (if value (error "misplaced \"global\" declaration"))
              (let ((vname (cadr e)))
-               (if (var-info-for vname vi)
-                   ;; issue #7264
+               (if (var-info-for vname vi) ;; issue #7264
                    (error (string "`global " vname "`: " vname " is local variable in the enclosing scope"))
-                   #f)))
+                   (emit e))))
             ((local-def) #f)
             ((local) #f)
             ((implicit-global) #f)
diff --git a/src/julia.h b/src/julia.h
index c1225481aa0ca1..9f71006aa3d45b 100644
--- a/src/julia.h
+++ b/src/julia.h
@@ -1236,7 +1236,7 @@ JL_DLLEXPORT jl_binding_t *jl_get_binding(jl_module_t *m, jl_sym_t *var);
 JL_DLLEXPORT jl_binding_t *jl_get_binding_or_error(jl_module_t *m, jl_sym_t *var);
 JL_DLLEXPORT jl_value_t *jl_module_globalref(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_sym_t *var);
+JL_DLLEXPORT jl_binding_t *jl_get_binding_wr(jl_module_t *m, jl_sym_t *var, int error);
 JL_DLLEXPORT jl_binding_t *jl_get_binding_for_method_def(jl_module_t *m,
                                                          jl_sym_t *var);
 JL_DLLEXPORT int jl_boundp(jl_module_t *m, jl_sym_t *var);
diff --git a/src/method.c b/src/method.c
index db2d44e8369fcb..8384726a7b468b 100644
--- a/src/method.c
+++ b/src/method.c
@@ -25,8 +25,14 @@ jl_value_t *jl_resolve_globals(jl_value_t *expr, jl_module_t *module, jl_svec_t
     }
     else if (jl_is_expr(expr)) {
         jl_expr_t *e = (jl_expr_t*)expr;
+        if (e->head == global_sym) {
+            // execute the side-effects of "global x" decl immediately:
+            // creates uninitialized mutable binding in module for each global
+            jl_toplevel_eval_flex(module, expr, 0, 1);
+            expr = jl_nothing;
+        }
         if (jl_is_toplevel_only_expr(expr) || e->head == const_sym || e->head == copyast_sym ||
-            e->head == global_sym || e->head == quote_sym || e->head == inert_sym ||
+            e->head == quote_sym || e->head == inert_sym ||
             e->head == line_sym || e->head == meta_sym || e->head == inbounds_sym ||
             e->head == boundscheck_sym || e->head == simdloop_sym) {
             // ignore these
diff --git a/src/module.c b/src/module.c
index d0735a2d25c287..c1ca45cc9a9dec 100644
--- a/src/module.c
+++ b/src/module.c
@@ -92,24 +92,22 @@ static jl_binding_t *new_binding(jl_sym_t *name)
 }
 
 // get binding for assignment
-JL_DLLEXPORT jl_binding_t *jl_get_binding_wr(jl_module_t *m, jl_sym_t *var)
+JL_DLLEXPORT jl_binding_t *jl_get_binding_wr(jl_module_t *m, jl_sym_t *var, int error)
 {
     jl_binding_t **bp = (jl_binding_t**)ptrhash_bp(&m->bindings, var);
-    jl_binding_t *b;
+    jl_binding_t *b = *bp;
 
-    if (*bp != HT_NOTFOUND) {
-        if ((*bp)->owner == NULL) {
-            (*bp)->owner = m;
-            return *bp;
-        }
-        else if ((*bp)->owner != m) {
-            // TODO: change this to an error soon
-            jl_printf(JL_STDERR,
-                      "WARNING: imported binding for %s overwritten in module %s\n", jl_symbol_name(var), jl_symbol_name(m->name));
-        }
-        else {
-            return *bp;
+    if (b != HT_NOTFOUND) {
+        if (b->owner != m) {
+            if (b->owner == NULL) {
+                b->owner = m;
+            }
+            else if (error) {
+                jl_errorf("cannot assign variable %s.%s from module %s",
+                          jl_symbol_name(b->owner->name), jl_symbol_name(var), jl_symbol_name(m->name));
+            }
         }
+        return *bp;
     }
 
     b = new_binding(var);
@@ -129,26 +127,30 @@ JL_DLLEXPORT jl_module_t *jl_get_module_of_binding(jl_module_t *m, jl_sym_t *var
 }
 
 // get binding for adding a method
-// like jl_get_binding_wr, but uses existing imports instead of warning
-// and overwriting.
+// like jl_get_binding_wr, but has different error paths
 JL_DLLEXPORT jl_binding_t *jl_get_binding_for_method_def(jl_module_t *m, jl_sym_t *var)
 {
     jl_binding_t **bp = (jl_binding_t**)ptrhash_bp(&m->bindings, var);
     jl_binding_t *b = *bp;
 
     if (b != HT_NOTFOUND) {
-        if (b->owner != m && b->owner != NULL) {
-            jl_binding_t *b2 = jl_get_binding(b->owner, var);
-            if (b2 == NULL)
-                jl_errorf("invalid method definition: imported function %s.%s does not exist", jl_symbol_name(b->owner->name), jl_symbol_name(var));
-            // TODO: we might want to require explicitly importing types to add constructors
-            if (!b->imported && (b2->value == NULL || !jl_is_type(b2->value))) {
-                jl_errorf("error in method definition: function %s.%s must be explicitly imported to be extended", jl_symbol_name(b->owner->name),
-                          jl_symbol_name(var));
+        if (b->owner != m) {
+            if (b->owner == NULL) {
+                b->owner = m;
+            }
+            else {
+                jl_binding_t *b2 = jl_get_binding(b->owner, var);
+                if (b2 == NULL || b2->value == NULL)
+                    jl_errorf("invalid method definition: imported function %s.%s does not exist",
+                              jl_symbol_name(b->owner->name), jl_symbol_name(var));
+                // TODO: we might want to require explicitly importing types to add constructors
+                if (!b->imported && !jl_is_type(b2->value)) {
+                    jl_errorf("error in method definition: function %s.%s must be explicitly imported to be extended",
+                              jl_symbol_name(b->owner->name), jl_symbol_name(var));
+                }
+                return b2;
             }
-            return b2;
         }
-        b->owner = m;
         return b;
     }
 
@@ -199,7 +201,7 @@ static jl_binding_t *jl_get_binding_(jl_module_t *m, jl_sym_t *var, modstack_t *
                               jl_symbol_name(imp->name), jl_symbol_name(var),
                               jl_symbol_name(m->name));
                     // mark this binding resolved, to avoid repeating the warning
-                    (void)jl_get_binding_wr(m, var);
+                    (void)jl_get_binding_wr(m, var, 0);
                     return NULL;
                 }
                 if (owner == NULL || !tempb->deprecated) {
@@ -450,7 +452,7 @@ JL_DLLEXPORT jl_value_t *jl_get_global(jl_module_t *m, jl_sym_t *var)
 
 JL_DLLEXPORT void jl_set_global(jl_module_t *m, jl_sym_t *var, jl_value_t *val)
 {
-    jl_binding_t *bp = jl_get_binding_wr(m, var);
+    jl_binding_t *bp = jl_get_binding_wr(m, var, 1);
     if (!bp->constp) {
         bp->value = val;
         jl_gc_wb(m, val);
@@ -459,7 +461,7 @@ JL_DLLEXPORT void jl_set_global(jl_module_t *m, jl_sym_t *var, jl_value_t *val)
 
 JL_DLLEXPORT void jl_set_const(jl_module_t *m, jl_sym_t *var, jl_value_t *val)
 {
-    jl_binding_t *bp = jl_get_binding_wr(m, var);
+    jl_binding_t *bp = jl_get_binding_wr(m, var, 1);
     if (!bp->constp) {
         bp->value = val;
         bp->constp = 1;
diff --git a/src/staticdata.c b/src/staticdata.c
index f2ab5700293ed5..2a2b00c94cd735 100644
--- a/src/staticdata.c
+++ b/src/staticdata.c
@@ -1055,7 +1055,7 @@ static void jl_reinit_item(jl_value_t *v, int how, arraylist_t *tracee_list)
             case 2: { // reinsert module v into parent (const)
                 jl_module_t *mod = (jl_module_t*)v;
                 assert(jl_is_module(mod));
-                jl_binding_t *b = jl_get_binding_wr(mod->parent, mod->name);
+                jl_binding_t *b = jl_get_binding_wr(mod->parent, mod->name, 1);
                 jl_declare_constant(b); // this can throw
                 if (b->value != NULL) {
                     if (!jl_is_module(b->value)) {
diff --git a/src/toplevel.c b/src/toplevel.c
index c90f335ea8b116..31e157fcfb60a6 100644
--- a/src/toplevel.c
+++ b/src/toplevel.c
@@ -141,7 +141,7 @@ jl_value_t *jl_eval_module_expr(jl_module_t *parent_module, jl_expr_t *ex)
     if (!jl_is_symbol(name)) {
         jl_type_error("module", (jl_value_t*)jl_sym_type, (jl_value_t*)name);
     }
-    jl_binding_t *b = jl_get_binding_wr(parent_module, name);
+    jl_binding_t *b = jl_get_binding_wr(parent_module, name, 1);
     jl_declare_constant(b);
     if (b->value != NULL) {
         if (!jl_is_module(b->value)) {
@@ -442,6 +442,7 @@ int jl_is_toplevel_only_expr(jl_value_t *e)
          ((jl_expr_t*)e)->head == using_sym ||
          ((jl_expr_t*)e)->head == export_sym ||
          ((jl_expr_t*)e)->head == thunk_sym ||
+         ((jl_expr_t*)e)->head == global_sym ||
          ((jl_expr_t*)e)->head == toplevel_sym);
 }
 
@@ -530,9 +531,32 @@ jl_value_t *jl_toplevel_eval_flex(jl_module_t *m, jl_value_t *e, int fast, int e
         return jl_nothing;
     }
     else if (ex->head == line_sym) {
-        jl_lineno = jl_unbox_long(jl_exprarg(ex,0));
+        jl_lineno = jl_unbox_long(jl_exprarg(ex, 0));
         return jl_nothing;
     }
+    else if (ex->head == global_sym) {
+        // create uninitialized mutable binding for "global x" decl
+        size_t i, l = jl_array_len(ex->args);
+        for (i = 0; i < l; i++) {
+            jl_value_t *a = jl_exprarg(ex, i);
+            if (!jl_is_symbol(a) && !jl_is_globalref(a))
+                break;
+        }
+        if (i == l) {
+            for (i = 0; i < l; i++) {
+                jl_sym_t *gs = (jl_sym_t*)jl_exprarg(ex, i);
+                jl_module_t *gm = m;
+                if (jl_is_globalref(gs)) {
+                    gm = jl_globalref_mod(gs);
+                    gs = jl_globalref_name(gs);
+                }
+                assert(jl_is_symbol(gs));
+                jl_get_binding_wr(gm, gs, 0);
+            }
+            return jl_nothing;
+        }
+        // fall-through to expand to normalize the syntax
+    }
 
     jl_method_instance_t *li = NULL;
     jl_value_t *result;
diff --git a/test/codegen.jl b/test/codegen.jl
index 5c8c44e4fd2410..a5009875ef290d 100644
--- a/test/codegen.jl
+++ b/test/codegen.jl
@@ -207,21 +207,24 @@ end
 let was_gced = false
     @noinline make_tuple(x) = tuple(x)
     @noinline use(x) = ccall(:jl_breakpoint, Void, ())
-    @noinline assert_not_gced() = @assert !was_gced
+    @noinline assert_not_gced() = @test !was_gced
 
     function foo22770()
         b = Ref(2)
-        finalizer(b, x->(global was_gced; was_gced=true))
+        finalizer(b, x -> was_gced = true)
         y = make_tuple(b)
         x = y[1]
         a = Ref(1)
         use(x); use(a); use(y)
         c = Ref(3)
-        gc(); assert_not_gced();
+        gc()
+        assert_not_gced()
         use(x)
         use(c)
     end
     foo22770()
+    gc()
+    @test was_gced
 end
 
 function egal_svecs()
diff --git a/test/core.jl b/test/core.jl
index 36e308a4564d1e..ed3dc4465ddfd3 100644
--- a/test/core.jl
+++ b/test/core.jl
@@ -5114,3 +5114,40 @@ m22929_2.x = m22929_1
 @test !isdefined_22929_x(m22929_1)
 @test isdefined_22929_1(m22929_2)
 @test isdefined_22929_x(m22929_2)
+
+# issue 18933
+module GlobalDef18933
+    using Base.Test
+    import Base.sqrt
+    # test that global declaration vs assignment operates correctly in local scope
+    f() = (global sin; nothing)
+    g() = (global cos; cos = 2; nothing)
+    h() = (global sqrt; nothing)
+    @test !@isdefined sin
+    @test !@isdefined cos
+    @test @isdefined sqrt
+    f()
+    g()
+    h()
+    @test !@isdefined sin
+    @test @isdefined cos
+    @test sqrt === Base.sqrt
+    @test cos === 2
+    # test that function definitions declared global
+    # introduce a new, local global
+    let
+        global tan
+        @test !@isdefined tan
+        tan() = nothing
+        @test @isdefined tan
+        @test tan() === nothing
+    end
+    # test that global declaration side-effects don't ignore conditionals
+    if false
+        global sincos
+        nothing
+    end
+    @test @which(sincos) === Base.Math
+    @test @isdefined sincos
+    @test sincos === Base.sincos
+end
diff --git a/test/parse.jl b/test/parse.jl
index a7822e679c841f..e68c7564456026 100644
--- a/test/parse.jl
+++ b/test/parse.jl
@@ -1184,10 +1184,7 @@ end
 # comment 298107224 on pull #21607
 module Test21607
     using Base.Test
-
-    @test_warn(
-    "WARNING: imported binding for Any overwritten in module Test21607",
-    @eval const Any = Integer)
+    const Any = Integer
 
     # check that X <: Core.Any, not Integer
     mutable struct X; end