Switch Python import machinery to focus primarily on Python modules (#1176)

Fixes #1165 and #1155

This PR changes the Python `importlib` machinery for Basilisp to simply
focus on importing modules, without getting itself too mixed up in the
business of figuring out what a namespace is actually _called_ from the
module name. When `importlib.import_module` is called, Basilisp just
figures out the location of the module on disk and starts loading it up
as if it were a Basilisp module. It sets the newly introduced
`basilisp.core/*import-module*` dynamic Var to the value of the
importing module and lets the Basilisp runtime fetch that value down and
attach it to a new namespace if and when it is appropriate to do so.

There may be some unintended side effects here for strange cases, so
noting those here for now:
* If multiple namespaces are created within a single invocation of
`import.import_lib` (excluding recursive calls for other imports or
requires), a module could be attached to multiple namespaces. I'm not
sure if that's actually an issue, but it's certainly bizarre and could
lead to some unintended consequences. That being said, I don't really
think it is a priority for the project to support this style of bizarre
metaprogramming, so that may be fine.
* Namespace names and module names aren't necessarily "linked" anymore,
although they never were linked directly before due to the required
munging of module names for Python code.
 * Others?

Author: chrisrink10

CHANGELOG.md

@@ -11,6 +11,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ### Changed
  * `import` now returns nil instead of the last module's string representation (#1174)
  *  The `:decorators` key now works in `defn` when passed as a metadata name key, expanding its support to `defn`-derived macros like `defasync` (#1178).
+ * Change Python import machinery to be centered around Python code, rather than Basilisp namespaces (#1155, #1165)
 
 ### Fixed
  * Fix a bug in `defn` where the `attr-map?` and function metdata were merged into a seq instead of a map, causing `macroexpand` to fail in some cases (#1186)

docs/testing.rst

@@ -41,28 +41,59 @@ For asserting repeatedly against different inputs, you can use the :lpy:fn:`are`
 Testing and ``PYTHONPATH``
 --------------------------
 
-Typical Clojure projects will have parallel ``src/`` and ``tests/`` folders in the project root.
+Typical Clojure projects will have parallel ``src/`` and ``test/`` folders in the project root.
 Project management tooling typically constructs the Java classpath to include both parallel trees for development and only ``src/`` for deployed software.
-Basilisp does not currently have such tooling (though it is planned!) and the recommended Python tooling is not configurable to allow for this distinction.
+Basilisp does not currently have such tooling, though it is planned.
 
-Due to this limitation, the easiest solution to facilitate test discovery with Pytest (Basilisp's default test runner) is to include a single, empty ``__init__.py`` file in the top-level ``tests`` directory:
+The easiest solution to facilitate test discovery with Pytest (Basilisp's default test runner) is to create a ``tests`` directory:
 
 .. code-block:: text
 
    tests
-   ├── __init__.py
    └── myproject
        └── core_test.lpy
 
 Test namespaces can then be created as if they are part of a giant ``tests`` package:
 
-.. code-block::
+.. code-block:: clojure
 
    (ns tests.myproject.core-test)
 
+Tests can be run with:
+
+.. code-block:: shell
+
+   $ basilisp test
+
+----
+
+Alternatively, you can follow the more traditional Clojure project structure by creating a ``test`` directory for your test namespaces:
+
+.. code-block:: text
+
+   test
+   └── myproject
+       └── core_test.lpy
+
+In this case, the test namespace can start at ``myproject``:
+
+.. code-block:: clojure
+
+   (ns myproject.core-test)
+
+
+However, the ``test`` directory must be explicitly added to the ``PYTHONPATH`` using the ``--include-path`` (or ``-p``) option when running the tests:
+
+.. code-block:: shell
+
+   $ basilisp test --include-path test
+
 .. note::
 
-   The project maintainers acknowledge that this is not an ideal solution and would like to provide a more Clojure-like solution in the future.
+   Test directory names can be arbitrary.
+   By default, the test runner searches all subdirectories for tests.
+   In the first example above (``tests``, a Python convention), the top-level directory is already in the ``PYTHONPATH``, allowing ``tests.myproject.core-test`` to be resolvable.
+   In the second example (``test``, a Clojure convention), the test directory is explicitly added to the ``PYTHONPATH``, enabling ``myproject.core-test`` to be resolvable.
 
 .. _test_fixtures:
 
@@ -107,4 +138,4 @@ You can see below that the fixture uses a :ref:`dynamic Var <dynamic_vars>` to c
 
 .. warning::
 
-   Basilisp test fixtures are not related to PyTest fixtures and they cannot be used interchangeably.
+   Basilisp test fixtures are not related to PyTest fixtures and they cannot be used interchangeably.

src/basilisp/contrib/pytest/testrunner.py

@@ -57,9 +57,7 @@ def pytest_unconfigure(config):
         runtime.pop_thread_bindings()
 
 
-def pytest_collect_file(  # pylint: disable=unused-argument
-    file_path: Path, path, parent
-):
+def pytest_collect_file(file_path: Path, parent):
     """Primary PyTest hook to identify Basilisp test files."""
     if file_path.suffix == ".lpy":
         if file_path.name.startswith("test_") or file_path.stem.endswith("_test"):
@@ -177,27 +175,21 @@ def _is_package(path: Path) -> bool:
     return False
 
 
-def _get_fully_qualified_module_name(file: Path) -> str:
+def _get_fully_qualified_module_names(file: Path) -> list[str]:
     """Return the fully qualified module name (from the import root) for a module given
     its location.
 
     This works by traversing up the filesystem looking for the top-most package. From
     there, we derive a Python module name referring to the given module path."""
-    top = None
-    for p in file.parents:
-        if _is_package(p):
-            top = p
-        else:
-            break
-
-    if top is None or top == file.parent:
-        return file.stem
-
-    root = top.parent
-    elems = list(file.with_suffix("").relative_to(root).parts)
-    if elems[-1] == "__init__":
-        elems.pop()
-    return ".".join(elems)
+    paths = []
+    for pth in sys.path:
+        root = Path(pth)
+        if file.is_relative_to(root):
+            elems = list(file.with_suffix("").relative_to(pth).parts)
+            if elems[-1] == "__init__":
+                elems.pop()
+            paths.append(".".join(elems))
+    return paths
 
 
 class BasilispFile(pytest.File):
@@ -251,10 +243,21 @@ def teardown(self) -> None:
         self._fixture_manager.teardown()
 
     def _import_module(self) -> runtime.BasilispModule:
-        modname = _get_fully_qualified_module_name(self.path)
-        module = importlib.import_module(modname)
-        assert isinstance(module, runtime.BasilispModule)
-        return module
+        modnames = _get_fully_qualified_module_names(self.path)
+        assert modnames, "Must have at least one module name"
+
+        exc: Optional[ModuleNotFoundError] = None
+        for modname in modnames:
+            try:
+                module = importlib.import_module(modname)
+            except ModuleNotFoundError as e:
+                exc = e
+            else:
+                assert isinstance(module, runtime.BasilispModule)
+                return module
+
+        assert exc is not None, "Must have an exception or module"
+        raise exc
 
     def collect(self):
         """Collect all tests from the namespace (module) given.

src/basilisp/importer.py

@@ -297,7 +297,7 @@ def _exec_cached_module(
         fullname: str,
         loader_state: Mapping[str, str],
         path_stats: Mapping[str, int],
-        ns: runtime.Namespace,
+        module: BasilispModule,
     ) -> None:
         """Load and execute a cached Basilisp module."""
         filename = loader_state["filename"]
@@ -319,15 +319,15 @@ def _exec_cached_module(
                     filename=filename, opts=runtime.get_compiler_opts()
                 ),
                 compiler.PythonASTOptimizer(),
-                ns,
+                module,
             )
 
     def _exec_module(
         self,
         fullname: str,
         loader_state: Mapping[str, str],
         path_stats: Mapping[str, int],
-        ns: runtime.Namespace,
+        module: BasilispModule,
     ) -> None:
         """Load and execute a non-cached Basilisp module."""
         filename = loader_state["filename"]
@@ -356,7 +356,7 @@ def _exec_module(
                 compiler.CompilerContext(
                     filename=filename, opts=runtime.get_compiler_opts()
                 ),
-                ns,
+                module,
                 collect_bytecode=all_bytecode.append,
             )
 
@@ -394,21 +394,29 @@ def exec_module(self, module: types.ModuleType) -> None:
         # a blank module. If we do not replace the module here with the module we are
         # generating, then we will not be able to use advanced compilation features such
         # as direct Python variable access to functions and other def'ed values.
-        ns_name = demunge(fullname)
-        ns: runtime.Namespace = runtime.Namespace.get_or_create(sym.symbol(ns_name))
-        ns.module = module
-        module.__basilisp_namespace__ = ns
-
-        # 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, "").lower() == "true":
-            self._exec_module(fullname, spec.loader_state, path_stats, ns)
-        else:
-            try:
-                self._exec_cached_module(fullname, spec.loader_state, path_stats, ns)
-            except (EOFError, ImportError, OSError) as e:
-                logger.debug(f"Failed to load cached Basilisp module: {e}")
-                self._exec_module(fullname, spec.loader_state, path_stats, ns)
+        if fullname == runtime.CORE_NS:
+            ns: runtime.Namespace = runtime.Namespace.get_or_create(runtime.CORE_NS_SYM)
+            ns.module = module
+            module.__basilisp_namespace__ = ns
+
+        # Set the currently importing module so it can be attached to the namespace when
+        # it is created.
+        with runtime.bindings(
+            {runtime.Var.find_safe(runtime.IMPORT_MODULE_VAR_SYM): 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, "").lower() == "true":
+                self._exec_module(fullname, spec.loader_state, path_stats, module)
+            else:
+                try:
+                    self._exec_cached_module(
+                        fullname, spec.loader_state, path_stats, module
+                    )
+                except (EOFError, ImportError, OSError) as e:
+                    logger.debug(f"Failed to load cached Basilisp module: {e}")
+                    self._exec_module(fullname, spec.loader_state, path_stats, module)
 
 
 def hook_imports() -> None:

src/basilisp/lang/compiler/__init__.py

@@ -37,6 +37,7 @@
 from basilisp.lang.compiler.generator import statementize as _statementize
 from basilisp.lang.compiler.optimizer import PythonASTOptimizer
 from basilisp.lang.interfaces import ISeq
+from basilisp.lang.runtime import BasilispModule
 from basilisp.lang.typing import CompilerOpts, ReaderForm
 from basilisp.lang.util import genname
 from basilisp.util import Maybe
@@ -109,7 +110,6 @@ def compiler_opts(  # pylint: disable=too-many-arguments
 
 
 def _emit_ast_string(
-    ns: runtime.Namespace,
     module: ast.AST,
 ) -> None:  # pragma: no cover
     """Emit the generated Python AST string either to standard out or to the
@@ -125,7 +125,7 @@ def _emit_ast_string(
     if runtime.print_generated_python():
         print(to_py_str(module))
     else:
-        runtime.add_generated_python(to_py_str(module), which_ns=ns)
+        runtime.add_generated_python(to_py_str(module))
 
 
 def _flatmap_forms(forms: Iterable[ReaderForm]) -> Iterable[ReaderForm]:
@@ -156,7 +156,7 @@ def compile_and_exec_form(
         return None
 
     if not ns.module.__basilisp_bootstrapped__:
-        _bootstrap_module(ctx.generator_context, ctx.py_ast_optimizer, ns)
+        _bootstrap_module(ctx.generator_context, ctx.py_ast_optimizer, ns.module)
 
     last = _sentinel
     for unrolled_form in _flatmap_forms([form]):
@@ -181,7 +181,7 @@ def compile_and_exec_form(
         ast_module = ctx.py_ast_optimizer.visit(ast_module)
         ast.fix_missing_locations(ast_module)
 
-        _emit_ast_string(ns, ast_module)
+        _emit_ast_string(ast_module)
 
         bytecode = compile(ast_module, ctx.filename, "exec")
         if collect_bytecode:
@@ -199,7 +199,7 @@ def compile_and_exec_form(
 def _incremental_compile_module(
     optimizer: PythonASTOptimizer,
     py_ast: GeneratedPyAST,
-    ns: runtime.Namespace,
+    module: BasilispModule,
     source_filename: str,
     collect_bytecode: Optional[BytecodeCollector] = None,
 ) -> None:
@@ -213,39 +213,39 @@ def _incremental_compile_module(
         map(_statementize, itertools.chain(py_ast.dependencies, [py_ast.node]))
     )
 
-    module = ast.Module(body=list(module_body), type_ignores=[])
-    module = optimizer.visit(module)
-    ast.fix_missing_locations(module)
+    module_ast = ast.Module(body=list(module_body), type_ignores=[])
+    module_ast = optimizer.visit(module_ast)
+    ast.fix_missing_locations(module_ast)
 
-    _emit_ast_string(ns, module)
+    _emit_ast_string(module_ast)
 
-    bytecode = compile(module, source_filename, "exec")
+    bytecode = compile(module_ast, source_filename, "exec")
     if collect_bytecode:
         collect_bytecode(bytecode)
-    exec(bytecode, ns.module.__dict__)  # pylint: disable=exec-used  # nosec 6102
+    exec(bytecode, module.__dict__)  # pylint: disable=exec-used  # nosec 6102
 
 
 def _bootstrap_module(
     gctx: GeneratorContext,
     optimizer: PythonASTOptimizer,
-    ns: runtime.Namespace,
+    module: BasilispModule,
     collect_bytecode: Optional[BytecodeCollector] = None,
 ) -> None:
     """Bootstrap a new module with imports and other boilerplate."""
     _incremental_compile_module(
         optimizer,
-        py_module_preamble(ns),
-        ns,
+        py_module_preamble(),
+        module,
         source_filename=gctx.filename,
         collect_bytecode=collect_bytecode,
     )
-    ns.module.__basilisp_bootstrapped__ = True
+    module.__basilisp_bootstrapped__ = True
 
 
 def compile_module(
     forms: Iterable[ReaderForm],
     ctx: CompilerContext,
-    ns: runtime.Namespace,
+    module: BasilispModule,
     collect_bytecode: Optional[BytecodeCollector] = None,
 ) -> None:
     """Compile an entire Basilisp module into Python bytecode which can be
@@ -255,7 +255,7 @@ def compile_module(
     Basilisp import machinery, to allow callers to import Basilisp modules from
     Python code.
     """
-    _bootstrap_module(ctx.generator_context, ctx.py_ast_optimizer, ns)
+    _bootstrap_module(ctx.generator_context, ctx.py_ast_optimizer, module)
 
     for form in _flatmap_forms(forms):
         nodes = gen_py_ast(
@@ -264,7 +264,7 @@ def compile_module(
         _incremental_compile_module(
             ctx.py_ast_optimizer,
             nodes,
-            ns,
+            module,
             source_filename=ctx.filename,
             collect_bytecode=collect_bytecode,
         )
@@ -274,17 +274,17 @@ def compile_bytecode(
     code: list[types.CodeType],
     gctx: GeneratorContext,
     optimizer: PythonASTOptimizer,
-    ns: runtime.Namespace,
+    module: BasilispModule,
 ) -> None:
     """Compile cached bytecode into the given module.
 
     The Basilisp import hook attempts to cache bytecode while compiling Basilisp
     namespaces. When the cached bytecode is reloaded from disk, it needs to be
     compiled within a bootstrapped module. This function bootstraps the module
     and then proceeds to compile a collection of bytecodes into the module."""
-    _bootstrap_module(gctx, optimizer, ns)
+    _bootstrap_module(gctx, optimizer, module)
     for bytecode in code:
-        exec(bytecode, ns.module.__dict__)  # pylint: disable=exec-used  # nosec 6102
+        exec(bytecode, module.__dict__)  # pylint: disable=exec-used  # nosec 6102
 
 
 _LOAD_SYM = sym.symbol("load", ns=runtime.CORE_NS)