defonce no longer throws a SyntaxError (#525)

* `defonce` no longer throws a SyntaxError

* Change the log

* Clean it up

* Refactor `def` generator

* Support unbound Vars

Author: chrisrink10

CHANGELOG.md

@@ -10,6 +10,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Fixed
  * Fixed a bug where the Basilisp AST nodes for return values of `deftype` members could be marked as _statements_ rather than _expressions_, resulting in an incorrect `nil` return (#523)
+ * Fixed a bug where `defonce` would throw a Python SyntaxError due to a superfluous `global` statement in the generated Python (#525)
 
 ## [v0.1.dev13] - 2020-03-16
 ### Added

src/basilisp/core.lpy

@@ -2826,8 +2826,8 @@
   a `name` is not already defined as a Var in this namespace. `expr` will not be
   evaluated if the Var already exists."
   [name expr]
-  `(let [v (def ~name)]
-     (when-not (.-is-bound v)
+  `(let [v# (def ~name)]
+     (when-not (.-is-bound v#)
        (def ~name ~expr))))
 
 (defmacro for
@@ -3698,7 +3698,7 @@
 
 (import importlib)
 
-(defn- require-libspec
+(defn ^:private require-libspec
   "Convert a user-specified require libspec into a map with well-defined keys.
 
   Required keys:
@@ -3720,7 +3720,7 @@
                    (ex-info "Invalid libspec for require"
                             {:value req}))))
 
-(defn- require-lib
+(defn ^:private require-lib
   "Require the library described by `libspec` into the Namespace `requiring-ns`."
   [requiring-ns libspec]
   (let [required-ns-sym  (:namespace libspec)]
@@ -3737,7 +3737,7 @@
     ;; during the require process
     (set! *ns* requiring-ns)))
 
-(defn- refer-filtered-interns
+(defn ^:private refer-filtered-interns
   "Return a map of symbols to interned Vars in the Namespace `referred-ns` subject
   to the filters described in `libspec`."
   [referred-ns libspec]
@@ -3759,7 +3759,7 @@
                                 m))
                             {}))))
 
-(defn- refer-lib
+(defn ^:private refer-lib
   "Refer Vars into `requiring-ns` as described by `libspec`.
 
   This function assumes the referred-to Namespace has already been loaded by
@@ -3985,6 +3985,9 @@
   [name & body]
   (let [doc         (when (string? (first body))
                       (first body))
+        name        (if doc
+                      (vary-meta name assoc :doc doc)
+                      name)
         body        (if doc
                       (rest body)
                       body)
@@ -4414,9 +4417,9 @@
   ([o & {:keys [keywordize-keys] :or {keywordize-keys true}}]
    (basilisp.lang.runtime/to-lisp o keywordize-keys)))
 
-;;;;;;;;;;;;;;;;;;;;;;;;;;;;
-;; Data Types & Protocols ;;
-;;;;;;;;;;;;;;;;;;;;;;;;;;;;
+;;;;;;;;;;;;;;;;
+;; Interfaces ;;
+;;;;;;;;;;;;;;;;
 
 (import* abc)
 
@@ -4492,6 +4495,10 @@
     `(def ~interface-name
        (gen-interface ~name-str [~@method-sigs]))))
 
+;;;;;;;;;;;;;;;;
+;; Data Types ;;
+;;;;;;;;;;;;;;;;
+
 (defn ^:private collect-methods
   "Collect method and interface declarations for `deftype` and `defrecord`
   into a map containing `:interfaces` and `:methods` keys."
@@ -4561,6 +4568,10 @@
        (def ~ctor-name ~type-name)
        ~type-name)))
 
+;;;;;;;;;;;;;
+;; Records ;;
+;;;;;;;;;;;;;
+
 (import* attr)
 
 (defn record?

src/basilisp/lang/compiler/analyzer.py

@@ -394,30 +394,57 @@ def should_allow_unresolved_symbols(self) -> bool:
 
     @property
     def should_macroexpand(self) -> bool:
+        """Return True if macros should be expanded."""
         return self._should_macroexpand
 
     @property
     def is_async_ctx(self) -> bool:
+        """If True, the current node appears inside of an async function definition.
+        It is possible that the current function is defined inside other functions,
+        so this does not imply anything about the nesting level of the current node."""
         try:
             return self._func_ctx[-1] is True
         except IndexError:
             return False
 
+    @property
+    def in_func_ctx(self) -> bool:
+        """If True, the current node appears inside of a function definition.
+        It is possible that the current function is defined inside other functions,
+        so this does not imply anything about the nesting level of the current node."""
+        try:
+            self._func_ctx[-1]
+        except IndexError:
+            return False
+        else:
+            return True
+
     @contextlib.contextmanager
     def new_func_ctx(self, is_async: bool = False):
+        """Context manager which can be used to set a function context for child
+        nodes to examine. A new function context is pushed onto the stack each time
+        the Analyzer finds a new function definition, so there may be many nested
+        function contexts."""
         self._func_ctx.append(is_async)
         yield
         self._func_ctx.pop()
 
     @property
     def recur_point(self) -> Optional[RecurPoint]:
+        """Return the current recur point which applies to the current node, if there
+        is one."""
         try:
             return self._recur_points[-1]
         except IndexError:
             return None
 
     @contextlib.contextmanager
     def new_recur_point(self, loop_id: str, args: Collection[Any] = ()):
+        """Context manager which can be used to set a recur point for child nodes.
+        A new recur point is pushed onto the stack each time the Analyzer finds a
+        form which supports recursion (such as `fn*` or `loop*`), so there may be
+        many recur points, though only one may be active at any given time for a
+        node."""
         self._recur_points.append(RecurPoint(loop_id, args=args))
         yield
         self._recur_points.pop()
@@ -801,6 +828,7 @@ def _def_ast(  # pylint: disable=too-many-branches,too-many-locals
         var=var,
         init=init,
         doc=doc,
+        in_func_ctx=ctx.in_func_ctx,
         children=children,
         env=def_node_env,
     )

src/basilisp/lang/compiler/generator.py

@@ -657,37 +657,6 @@ def _def_to_py_ast(  # pylint: disable=too-many-branches
     assert node.op == NodeOp.DEF
 
     defsym = node.name
-    is_defn = False
-    is_var_bound = node.var.is_bound
-    is_noop_redef_of_bound_var = is_var_bound and node.init is None
-
-    if node.init is not None:
-        # Since Python function definitions always take the form `def name(...):`,
-        # it is redundant to assign them to the their final name after they have
-        # been defined under a private alias. This codepath generates `defn`
-        # declarations by directly generating the Python `def` with the correct
-        # function name and short-circuiting the default double-declaration.
-        if node.init.op == NodeOp.FN:
-            assert isinstance(node.init, Fn)
-            def_ast: Optional[GeneratedPyAST] = _fn_to_py_ast(  # type: ignore[call-arg]
-                ctx, node.init, def_name=defsym.name
-            )
-            is_defn = True
-        elif (
-            node.init.op == NodeOp.WITH_META
-            and isinstance(node.init, WithMeta)
-            and node.init.expr.op == NodeOp.FN
-        ):
-            assert isinstance(node.init, WithMeta)
-            def_ast = _with_meta_to_py_ast(ctx, node.init, def_name=defsym.name)
-            is_defn = True
-        else:
-            def_ast = gen_py_ast(ctx, node.init)
-    elif is_noop_redef_of_bound_var:
-        def_ast = None
-    else:
-        def_ast = GeneratedPyAST(node=ast.Constant(None))
-
     ns_name = _load_attr(_NS_VAR_VALUE)
     def_name = ast.Call(
         func=_NEW_SYM_FN_NAME, args=[ast.Constant(defsym.name)], keywords=[]
@@ -709,74 +678,79 @@ def _def_to_py_ast(  # pylint: disable=too-many-branches
 
     # Warn if this symbol is potentially being redefined (if the Var was
     # previously bound)
-    if is_var_bound and __should_warn_on_redef(ctx, defsym, safe_name, def_meta):
+    if node.var.is_bound and __should_warn_on_redef(ctx, defsym, safe_name, def_meta):
         logger.warning(
             f"redefining local Python name '{safe_name}' in module "
             f"'{ctx.current_ns.module.__name__}' ({node.env.ns}:{node.env.line})"
         )
 
-    meta_ast = gen_py_ast(ctx, node.meta)
-
-    # For defn style def generation, we specifically need to generate the
-    # global declaration prior to emitting the Python `def` otherwise the
-    # Python compiler will throw an exception during compilation
-    # complaining that we assign the value prior to global declaration.
-    if is_defn:
-        assert def_ast is not None, "def_ast must be defined at this point"
-        def_dependencies = list(
-            chain(
-                [] if node.top_level else [ast.Global(names=[safe_name])],
-                def_ast.dependencies,
-                [] if meta_ast is None else meta_ast.dependencies,
+    if node.init is not None:
+        # Since Python function definitions always take the form `def name(...):`,
+        # it is redundant to assign them to the their final name after they have
+        # been defined under a private alias. This codepath generates `defn`
+        # declarations by directly generating the Python `def` with the correct
+        # function name and short-circuiting the default double-declaration.
+        assert node.init is not None  # silence MyPy
+        if node.init.op == NodeOp.FN:
+            assert isinstance(node.init, Fn)
+            def_ast = _fn_to_py_ast(  # type: ignore[call-arg]
+                ctx, node.init, def_name=defsym.name
             )
-        )
-    elif is_noop_redef_of_bound_var:
-        # Re-def-ing previously bound Vars without providing a value is
-        # essentially a no-op, which should not modify the Var root.
-        assert def_ast is None, "def_ast is not defined at this point"
-        def_dependencies = list(
-            chain(
-                [] if node.top_level else [ast.Global(names=[safe_name])],
-                [] if meta_ast is None else meta_ast.dependencies,
+            is_defn = True
+        elif (
+            node.init.op == NodeOp.WITH_META
+            and isinstance(node.init, WithMeta)
+            and node.init.expr.op == NodeOp.FN
+        ):
+            assert isinstance(node.init, WithMeta)
+            def_ast = _with_meta_to_py_ast(ctx, node.init, def_name=defsym.name)
+            is_defn = True
+        else:
+            def_ast = gen_py_ast(ctx, node.init)
+            is_defn = False
+
+        func = _INTERN_VAR_FN_NAME
+        extra_args = [ast.Name(id=safe_name, ctx=ast.Load())]
+        if is_defn:
+            # For defn style def generation, we specifically need to generate
+            # the global declaration prior to emitting the Python `def` otherwise
+            # the Python compiler will throw an exception during compilation
+            # complaining that we assign the value prior to global declaration.
+            def_dependencies = list(
+                chain(
+                    [ast.Global(names=[safe_name])] if node.in_func_ctx else [],
+                    def_ast.dependencies,
+                )
             )
-        )
-    else:
-        assert def_ast is not None, "def_ast must be defined at this point"
-        def_dependencies = list(
-            chain(
-                def_ast.dependencies,
-                [] if node.top_level else [ast.Global(names=[safe_name])],
-                [
-                    ast.Assign(
-                        targets=[ast.Name(id=safe_name, ctx=ast.Store())],
-                        value=def_ast.node,
-                    )
-                ],
-                [] if meta_ast is None else meta_ast.dependencies,
+        else:
+            def_dependencies = list(
+                chain(
+                    def_ast.dependencies,
+                    [ast.Global(names=[safe_name])] if node.in_func_ctx else [],
+                    [
+                        ast.Assign(
+                            targets=[ast.Name(id=safe_name, ctx=ast.Store())],
+                            value=def_ast.node,
+                        )
+                    ],
+                )
             )
-        )
+    else:
+        # Callers can either `(def v)` to declare an unbound Var or they
+        # can redef a bound Var with no init value to fetch a reference
+        # to the var. Re-def-ing previously bound Vars without providing
+        # a value is essentially a no-op, which should not modify the Var
+        # root.
+        func = _INTERN_UNBOUND_VAR_FN_NAME
+        extra_args = []
+        def_dependencies = [ast.Global(names=[safe_name])] if node.in_func_ctx else []
 
-    if is_noop_redef_of_bound_var:
-        return GeneratedPyAST(
-            node=ast.Call(
-                func=_INTERN_UNBOUND_VAR_FN_NAME,
-                args=[ns_name, def_name],
-                keywords=list(
-                    chain(
-                        dynamic_kwarg,
-                        []
-                        if meta_ast is None
-                        else [ast.keyword(arg="meta", value=meta_ast.node)],
-                    )
-                ),
-            ),
-            dependencies=def_dependencies,
-        )
+    meta_ast = gen_py_ast(ctx, node.meta)
 
     return GeneratedPyAST(
         node=ast.Call(
-            func=_INTERN_VAR_FN_NAME,
-            args=[ns_name, def_name, ast.Name(id=safe_name, ctx=ast.Load())],
+            func=func,
+            args=list(chain([ns_name, def_name], extra_args)),
             keywords=list(
                 chain(
                     dynamic_kwarg,
@@ -786,7 +760,9 @@ def _def_to_py_ast(  # pylint: disable=too-many-branches
                 )
             ),
         ),
-        dependencies=def_dependencies,
+        dependencies=list(
+            chain(def_dependencies, [] if meta_ast is None else meta_ast.dependencies,)
+        ),
     )
 
 

src/basilisp/lang/compiler/nodes.py

@@ -351,6 +351,7 @@ class Def(Node[SpecialForm]):
     var: Var
     init: Optional[Node]
     doc: Optional[str]
+    in_func_ctx: bool
     env: NodeEnv
     meta: NodeMeta = None
     children: Sequence[kw.Keyword] = vec.Vector.empty()