Disallow set!s on non-threadbound Vars (#638)

* Disallow set!s on non-threadbound Vars

* Update changelog

* Simplifications

* Tests and such

* Fix that one test

* Improve code generation logic

* Simplify

Author: chrisrink10

CHANGELOG.md

@@ -9,7 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
  * Added a bootstrapping function for easily bootstrapping Basilisp projects from Python (#620)
  * Added support for watchers and validator functions on Atoms and Vars (#627)
  * Added support for Taps (#631)
- * Added support for hierarchies (#???)
+ * Added support for hierarchies (#633)
 
 ### Changed
  * PyTest is now an optional extra dependency, rather than a required dependency (#622)
@@ -18,9 +18,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ### Fixed
  * Fixed a bug where `seq`ing co-recursive lazy sequences would cause a stack overflow (#632)
  * Fixed a spurious failure in the test runner and switched to using macro forms for test line numbers (#631)
+ * Fixed a bug that allowed dynamic Vars to be `set!` even if they weren't thread-bound (#638)
 
 ### Removed
- * Removed Click as a dependency in favor of builtin `argparse` (#622, #624)
+ * Removed Click as a dependency in favor of builtin `argparse` (#622, #624, #636)
  * Removed Atomos as a dependency in favor of `readerwriterlock` (#624)
 
 ## [v0.1.dev15]

mypy.ini

@@ -131,5 +131,18 @@ skip = [
   "pip-wheel-metadata",
 ]
 
+[tool.mypy]
+check_untyped_defs = true
+warn_unused_ignores = true
+
+[[tool.mypy.overrides]]
+module = [
+    "astor.*",
+    "prompt_toolkit.*",
+    "pygments.*",
+    "pytest.*"
+]
+ignore_missing_imports = true
+
 [tool.pytest.ini_options]
 junit_family = "legacy"

pyproject.toml

@@ -5,7 +5,7 @@
 import sys
 import traceback
 import types
-from typing import Any, Callable, Optional, Sequence
+from typing import Any, Callable, Optional, Sequence, Type
 
 from basilisp import main as basilisp
 from basilisp.lang import compiler as compiler
@@ -20,6 +20,11 @@
 STDIN_INPUT_FILE_PATH = "<stdin>"
 STDIN_FILE_NAME = "-"
 
+BOOL_TRUE = frozenset({"true", "t", "1", "yes", "y"})
+BOOL_FALSE = frozenset({"false", "f", "0", "no", "n"})
+
+DEFAULT_COMPILER_OPTS = {k.name: v for k, v in compiler.compiler_opts().items()}
+
 
 def eval_file(filename: str, ctx: compiler.CompilerContext, ns: runtime.Namespace):
     """Evaluate a file with the given name into a Python module AST node."""
@@ -56,42 +61,137 @@ def bootstrap_repl(ctx: compiler.CompilerContext, which_ns: str) -> types.Module
     return importlib.import_module(REPL_NS)
 
 
+def _to_bool(v: Optional[str]) -> Optional[bool]:
+    """Coerce a string argument to a boolean value, if possible."""
+    if v is None:
+        return v
+    elif v.lower() in BOOL_TRUE:
+        return True
+    elif v.lower() in BOOL_FALSE:
+        return False
+    else:
+        raise argparse.ArgumentTypeError("Unable to coerce flag value to boolean.")
+
+
+def _set_envvar_action(
+    var: str, parent: Type[argparse.Action] = argparse.Action
+) -> Type[argparse.Action]:
+    """Return an argparse.Action instance (deriving from `parent`) that sets the value
+    as the default value of the environment variable `var`."""
+
+    class EnvVarSetterAction(parent):  # type: ignore
+        def __call__(  # pylint: disable=signature-differs
+            self,
+            parser: argparse.ArgumentParser,
+            namespace: argparse.Namespace,
+            values: Any,
+            option_string: str,
+        ):
+            os.environ.setdefault(var, str(values))
+
+    return EnvVarSetterAction
+
+
 def _add_compiler_arg_group(parser: argparse.ArgumentParser) -> None:
-    group = parser.add_argument_group("compiler arguments")
+    group = parser.add_argument_group(
+        "compiler arguments",
+        description=(
+            "The compiler arguments below can be used to tweak warnings emitted by the "
+            "compiler during compilation and in some cases, tweak emitted code. Note "
+            "that Basilisp, like Python, aggressively caches compiled namespaces so "
+            "you may need to disable namespace caching or modify your file to see the "
+            "compiler argument changes take effect."
+        ),
+    )
     group.add_argument(
         "--warn-on-shadowed-name",
-        action="store_const",
-        const=True,
-        default=os.getenv("BASILISP_WARN_ON_SHADOWED_NAME"),
-        help="if provided, emit warnings if a local name is shadowed by another local name",
+        action="store",
+        nargs="?",
+        const=os.getenv("BASILISP_WARN_ON_SHADOWED_NAME"),
+        type=_to_bool,
+        help=(
+            "if true, emit warnings if a local name is shadowed by another local "
+            "name (env: BASILISP_WARN_ON_SHADOWED_NAME; default: "
+            f"{DEFAULT_COMPILER_OPTS['warn-on-shadowed-name']})"
+        ),
     )
     group.add_argument(
         "--warn-on-shadowed-var",
-        action="store_const",
-        const=True,
-        default=os.getenv("BASILISP_WARN_ON_SHADOWED_VAR"),
-        help="if provided, emit warnings if a Var name is shadowed by a local name",
+        action="store",
+        nargs="?",
+        const=os.getenv("BASILISP_WARN_ON_SHADOWED_VAR"),
+        type=_to_bool,
+        help=(
+            "if true, emit warnings if a Var name is shadowed by a local name "
+            "(env: BASILISP_WARN_ON_SHADOWED_VAR; default: "
+            f"{DEFAULT_COMPILER_OPTS['warn-on-shadowed-var']})"
+        ),
     )
     group.add_argument(
         "--warn-on-unused-names",
-        action="store_const",
-        const=True,
-        default=os.getenv("BASILISP_WARN_ON_UNUSED_NAMES"),
-        help="if provided, emit warnings if a local name is bound and unused",
+        action="store",
+        nargs="?",
+        const=os.getenv("BASILISP_WARN_ON_UNUSED_NAMES"),
+        type=_to_bool,
+        help=(
+            "if true, emit warnings if a local name is bound and unused "
+            "(env: BASILISP_WARN_ON_UNUSED_NAMES; default: "
+            f"{DEFAULT_COMPILER_OPTS['warn-on-unused-names']})"
+        ),
+    )
+    group.add_argument(
+        "--warn-on-non-dynamic-set",
+        action="store",
+        nargs="?",
+        const=os.getenv("BASILISP_WARN_ON_NON_DYNAMIC_SET"),
+        type=_to_bool,
+        help=(
+            "if true, emit warnings if the compiler detects an attempt to set! "
+            "a Var which is not marked as ^:dynamic (env: "
+            "BASILISP_WARN_ON_NON_DYNAMIC_SET; default: "
+            f"{DEFAULT_COMPILER_OPTS['warn-on-non-dynamic-set']})"
+        ),
     )
     group.add_argument(
         "--use-var-indirection",
-        action="store_const",
-        const=True,
-        default=os.getenv("BASILISP_USE_VAR_INDIRECTION"),
-        help="if provided, all Var accesses will be performed via Var indirection",
+        action="store",
+        nargs="?",
+        const=os.getenv("BASILISP_USE_VAR_INDIRECTION"),
+        type=_to_bool,
+        help=(
+            "if true, all Var accesses will be performed via Var indirection "
+            "(env: BASILISP_USE_VAR_INDIRECTION; default: "
+            f"{DEFAULT_COMPILER_OPTS['use-var-indirection']})"
+        ),
     )
     group.add_argument(
         "--warn-on-var-indirection",
-        action="store_const",
+        action="store",
+        nargs="?",
+        const=os.getenv("BASILISP_WARN_ON_VAR_INDIRECTION"),
+        type=_to_bool,
+        help=(
+            "if true, emit warnings if a Var reference cannot be direct linked "
+            "(env: BASILISP_WARN_ON_VAR_INDIRECTION; default: "
+            f"{DEFAULT_COMPILER_OPTS['warn-on-var-indirection']})"
+        ),
+    )
+
+
+def _add_debug_arg_group(parser: argparse.ArgumentParser) -> None:
+    group = parser.add_argument_group("debug options")
+    group.add_argument(
+        "--disable-ns-cache",
+        action=_set_envvar_action(
+            "BASILISP_DO_NOT_CACHE_NAMESPACES", parent=argparse._StoreAction
+        ),
+        nargs="?",
         const=True,
-        default=os.getenv("BASILISP_WARN_ON_VAR_INDIRECTION"),
-        help="if provided, emit warnings if a Var reference cannot be direct linked",
+        type=_to_bool,
+        help=(
+            "if true, disable attempting to load cached namespaces "
+            "(env: BASILISP_DO_NOT_CACHE_NAMESPACES; default: false)"
+        ),
     )
 
 
@@ -131,41 +231,71 @@ def repl(
     )
     basilisp.init(opts)
     ctx = compiler.CompilerContext(filename=REPL_INPUT_FILE_PATH, opts=opts)
-    repl_module = bootstrap_repl(ctx, args.default_ns)
-    ns_var = runtime.set_current_ns(args.default_ns)
     prompter = get_prompter()
     eof = object()
-    while True:
-        ns: runtime.Namespace = ns_var.value
-        try:
-            lsrc = prompter.prompt(f"{ns.name}=> ")
-        except EOFError:
-            break
-        except KeyboardInterrupt:  # pragma: no cover
-            print("")
-            continue
-
-        if len(lsrc) == 0:
-            continue
-
-        try:
-            result = eval_str(lsrc, ctx, ns, eof)
-            if result is eof:  # pragma: no cover
+
+    # Bind user-settable dynamic Vars to their existing value to allow users to
+    # conveniently (set! *var* val) at the REPL without needing `binding`.
+    with runtime.bindings(
+        {
+            var: var.value
+            for var in map(
+                lambda name: runtime.Var.find_safe(
+                    sym.symbol(name, ns=runtime.CORE_NS)
+                ),
+                [
+                    "*e",
+                    "*1",
+                    "*2",
+                    "*3",
+                    "*assert*",
+                    "*data-readers*",
+                    "*resolver*",
+                    runtime.PRINT_DUP_VAR_NAME,
+                    runtime.PRINT_LEVEL_VAR_NAME,
+                    runtime.PRINT_READABLY_VAR_NAME,
+                    runtime.PRINT_LEVEL_VAR_NAME,
+                    runtime.PRINT_META_VAR_NAME,
+                ],
+            )
+        }
+    ):
+        repl_module = bootstrap_repl(ctx, args.default_ns)
+        ns_var = runtime.set_current_ns(args.default_ns)
+
+        while True:
+            ns: runtime.Namespace = ns_var.value
+            try:
+                lsrc = prompter.prompt(f"{ns.name}=> ")
+            except EOFError:
+                break
+            except KeyboardInterrupt:  # pragma: no cover
+                print("")
+                continue
+
+            if len(lsrc) == 0:
+                continue
+
+            try:
+                result = eval_str(lsrc, ctx, ns, eof)
+                if result is eof:  # pragma: no cover
+                    continue
+                prompter.print(runtime.lrepr(result))
+                repl_module.mark_repl_result(result)  # type: ignore
+            except reader.SyntaxError as e:
+                traceback.print_exception(reader.SyntaxError, e, e.__traceback__)
+                repl_module.mark_exception(e)  # type: ignore
+                continue
+            except compiler.CompilerException as e:
+                traceback.print_exception(
+                    compiler.CompilerException, e, e.__traceback__
+                )
+                repl_module.mark_exception(e)  # type: ignore
+                continue
+            except Exception as e:
+                traceback.print_exception(Exception, e, e.__traceback__)
+                repl_module.mark_exception(e)  # type: ignore
                 continue
-            prompter.print(runtime.lrepr(result))
-            repl_module.mark_repl_result(result)  # type: ignore
-        except reader.SyntaxError as e:
-            traceback.print_exception(reader.SyntaxError, e, e.__traceback__)
-            repl_module.mark_exception(e)  # type: ignore
-            continue
-        except compiler.CompilerException as e:
-            traceback.print_exception(compiler.CompilerException, e, e.__traceback__)
-            repl_module.mark_exception(e)  # type: ignore
-            continue
-        except Exception as e:
-            traceback.print_exception(Exception, e, e.__traceback__)
-            repl_module.mark_exception(e)  # type: ignore
-            continue
 
 
 @_subcommand(
@@ -181,6 +311,7 @@ def _add_repl_subcommand(parser: argparse.ArgumentParser) -> None:
         help="default namespace to use for the REPL",
     )
     _add_compiler_arg_group(parser)
+    _add_debug_arg_group(parser)
 
 
 def run(
@@ -248,6 +379,7 @@ def _add_run_subcommand(parser: argparse.ArgumentParser):
         "--in-ns", default=runtime.REPL_DEFAULT_NS, help="namespace to use for the code"
     )
     _add_compiler_arg_group(parser)
+    _add_debug_arg_group(parser)
 
 
 def test(parser: argparse.ArgumentParser, args: argparse.Namespace):  # pragma: no cover

src/basilisp/cli.py

@@ -330,7 +330,7 @@ def exec_module(self, module):
 
         # Check if a valid, cached version of this Basilisp namespace exists and, if so,
         # load it and bypass the expensive compilation process below.
-        if os.getenv(_NO_CACHE_ENVVAR, None) == "true":
+        if os.getenv(_NO_CACHE_ENVVAR, "").lower() == "true":
             self._exec_module(fullname, spec.loader_state, path_stats, ns)
         else:
             try:

src/basilisp/importer.py

@@ -9,6 +9,7 @@
 from basilisp.lang import map as lmap
 from basilisp.lang import runtime as runtime
 from basilisp.lang.compiler.analyzer import (  # noqa
+    WARN_ON_NON_DYNAMIC_SET,
     WARN_ON_SHADOWED_NAME,
     WARN_ON_SHADOWED_VAR,
     WARN_ON_UNUSED_NAMES,
@@ -69,10 +70,11 @@ def py_ast_optimizer(self) -> PythonASTOptimizer:
         return self._optimizer
 
 
-def compiler_opts(
+def compiler_opts(  # pylint: disable=too-many-arguments
     warn_on_shadowed_name: Optional[bool] = None,
     warn_on_shadowed_var: Optional[bool] = None,
     warn_on_unused_names: Optional[bool] = None,
+    warn_on_non_dynamic_set: Optional[bool] = None,
     use_var_indirection: Optional[bool] = None,
     warn_on_var_indirection: Optional[bool] = None,
 ) -> CompilerOpts:
@@ -83,6 +85,7 @@ def compiler_opts(
             WARN_ON_SHADOWED_NAME: warn_on_shadowed_name or False,
             WARN_ON_SHADOWED_VAR: warn_on_shadowed_var or False,
             WARN_ON_UNUSED_NAMES: warn_on_unused_names or True,
+            WARN_ON_NON_DYNAMIC_SET: warn_on_non_dynamic_set or True,
             # Generator options
             USE_VAR_INDIRECTION: use_var_indirection or False,
             WARN_ON_VAR_INDIRECTION: warn_on_var_indirection or True,