From 136928043008c47cbab9c8248f50f5984ca67bc6 Mon Sep 17 00:00:00 2001 From: Chris Rink Date: Tue, 17 Sep 2024 11:25:35 -0400 Subject: [PATCH 1/7] Emit warnings when requiring or importing with an already used alias --- CHANGELOG.md | 4 ++ docs/runtime.rst | 25 ++++++++++ src/basilisp/cli.py | 14 ++++++ src/basilisp/core.lpy | 6 --- src/basilisp/lang/compiler/__init__.py | 2 +- src/basilisp/lang/compiler/constants.py | 3 ++ src/basilisp/lang/compiler/generator.py | 33 ++++++++++--- src/basilisp/lang/compiler/optimizer.py | 3 +- src/basilisp/lang/runtime.py | 66 ++++++++++++++++++------- 9 files changed, 122 insertions(+), 34 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b0c6d9ead..3b80ba067 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] ### Added * Added a compiler metadata flag for suppressing warnings when Var indirection is unavoidable (#1052) + * Added the `--emit-generated-python` CLI argument to control whether generated Python code strings are stored by the runtime for each compiled namespace (#1045) + +### Changed + * The compiler will issue a warning when adding any alias that might conflict with any other alias (#1045) ### Fixed * Basilisp now respects the value of Python's `sys.dont_write_bytecode` flag when generating bytecode (#1054) diff --git a/docs/runtime.rst b/docs/runtime.rst index e640ff336..ffa739791 100644 --- a/docs/runtime.rst +++ b/docs/runtime.rst @@ -111,6 +111,31 @@ This is roughly analogous to the Java classpath in Clojure. These values may be set manually, but are more often configured by some project management tool such as Poetry or defined in your Python virtualenv. These values may also be set via :ref:`cli` arguments. +.. _namespace_imports: + +Namespace Imports +^^^^^^^^^^^^^^^^^ + +Basilisp compiles Lisp code into Python code in Python modules exactly the same way the Python compiler does. +The Python code compiled by the Basilisp compiler expects certain features to be available at runtime beyond the standard Python builtins. +To support this, the Python modules compiled by Basilisp automatically import a number of modules both from the Basilisp runtime, the Python standard library, and Basilisp's Python dependencies. +In Basilisp modules (particularly :lpy:ns:`basilisp.core`) you may find references to such modules without any corresponding :lpy:form:`import`. + +The modules imported by default are given below: + +- ``attr`` (from the `attrs `_ project) +- :external:py:mod:`builtins` (Basilisp users should prefer the ``python`` namespace for calling :ref:`python_builtins`) +- :external:py:mod:`functools` +- :external:py:mod:`io` +- :external:py:mod:`importlib` +- :external:py:mod:`operator` +- :external:py:mod:`sys` +- The majority of the modules in ``basilisp.lang.*`` + +.. warning:: + + Using any of these names (particularly the Python standard library module names) as an alias for a required namespace or imported Python module will trigger a warning. + .. _vars: Vars diff --git a/src/basilisp/cli.py b/src/basilisp/cli.py index b2ee352de..9543ced3d 100644 --- a/src/basilisp/cli.py +++ b/src/basilisp/cli.py @@ -307,6 +307,20 @@ def _add_debug_arg_group(parser: argparse.ArgumentParser) -> None: "(env: BASILISP_LOGGING_LEVEL; default: WARNING)" ), ) + group.add_argument( + "--emit-generated-python", + action=_set_envvar_action( + "BASILISP_EMIT_GENERATED_PYTHON", parent=argparse._StoreAction + ), + nargs="?", + const=True, + type=_to_bool, + help=( + "if true, store generated Python code in `*generated-python*` dynamic " + "Vars within each namespace (env: BASILISP_EMIT_GENERATED_PYTHON; " + "default: true)" + ), + ) def _add_import_arg_group(parser: argparse.ArgumentParser) -> None: diff --git a/src/basilisp/core.lpy b/src/basilisp/core.lpy index 5b6ff72fe..8a3239c8a 100644 --- a/src/basilisp/core.lpy +++ b/src/basilisp/core.lpy @@ -6,8 +6,6 @@ datetime decimal fractions - functools - importlib importlib.util math multiprocessing @@ -20,10 +18,6 @@ [time :as py-time] uuid) -(import* attr) - -(import* basilisp.lang.multifn) - (def ^{:doc "Create a list from the arguments." :arglists '([& args])} list diff --git a/src/basilisp/lang/compiler/__init__.py b/src/basilisp/lang/compiler/__init__.py index 8f9e9c7e2..f09e76470 100644 --- a/src/basilisp/lang/compiler/__init__.py +++ b/src/basilisp/lang/compiler/__init__.py @@ -139,7 +139,7 @@ def _emit_ast_string( # TODO: eventually, this default should become "false" but during this # period of heavy development, having it set to "true" by default # is tremendously useful - if os.getenv("BASILISP_EMIT_GENERATED_PYTHON", "true") != "true": + if os.getenv("BASILISP_EMIT_GENERATED_PYTHON", "true").lower() != "true": return if runtime.print_generated_python(): diff --git a/src/basilisp/lang/compiler/constants.py b/src/basilisp/lang/compiler/constants.py index 5ac11fc75..21a084cab 100644 --- a/src/basilisp/lang/compiler/constants.py +++ b/src/basilisp/lang/compiler/constants.py @@ -1,5 +1,6 @@ from basilisp.lang import keyword as kw from basilisp.lang import symbol as sym +from basilisp.lang.util import genname class SpecialForm: @@ -32,6 +33,8 @@ class SpecialForm: DEFAULT_COMPILER_FILE_PATH = "NO_SOURCE_PATH" +OPERATOR_ALIAS = genname("operator") + SYM_ABSTRACT_META_KEY = kw.keyword("abstract") SYM_ABSTRACT_MEMBERS_META_KEY = kw.keyword("abstract-members") SYM_ASYNC_META_KEY = kw.keyword("async") diff --git a/src/basilisp/lang/compiler/generator.py b/src/basilisp/lang/compiler/generator.py index bdd79da0c..a16cbed1b 100644 --- a/src/basilisp/lang/compiler/generator.py +++ b/src/basilisp/lang/compiler/generator.py @@ -46,6 +46,7 @@ from basilisp.lang.compiler.constants import ( DEFAULT_COMPILER_FILE_PATH, INTERFACE_KW, + OPERATOR_ALIAS, REST_KW, SYM_DYNAMIC_META_KEY, SYM_REDEF_META_KEY, @@ -684,6 +685,13 @@ def _var_ns_as_python_sym(name: str) -> str: ####################### +_ATTR_ALIAS = genname("attr") +_BUILTINS_ALIAS = genname("builtins") +_FUNCTOOLS_ALIAS = genname("functools") +_IMPORTLIB_ALIAS = genname("importlib") +_IO_ALIAS = genname("io") +_SYS_ALIAS = genname("sys") + _ATOM_ALIAS = genname("atom") _COMPILER_ALIAS = genname("compiler") _CORE_ALIAS = genname("core") @@ -706,9 +714,17 @@ def _var_ns_as_python_sym(name: str) -> str: _VEC_ALIAS = genname("vec") _VOLATILE_ALIAS = genname("volatile") _VAR_ALIAS = genname("Var") +_UNION_ALIAS = genname("Union") _UTIL_ALIAS = genname("langutil") _MODULE_ALIASES = { + "attr": _ATTR_ALIAS, + "builtins": _BUILTINS_ALIAS, + "functools": _FUNCTOOLS_ALIAS, + "importlib": _IMPORTLIB_ALIAS, + "io": _IO_ALIAS, + "operator": OPERATOR_ALIAS, + "sys": _SYS_ALIAS, "basilisp.lang.atom": _ATOM_ALIAS, "basilisp.lang.compiler": _COMPILER_ALIAS, "basilisp.core": _CORE_ALIAS, @@ -732,6 +748,9 @@ def _var_ns_as_python_sym(name: str) -> str: "basilisp.lang.volatile": _VOLATILE_ALIAS, "basilisp.lang.util": _UTIL_ALIAS, } +assert set(_MODULE_ALIASES.keys()).issuperset( + map(lambda s: s.name, runtime.Namespace.DEFAULT_IMPORTS) +), "All default Namespace imports should have generator aliases" _NS_VAR_VALUE = f"{_NS_VAR}.value" @@ -753,16 +772,16 @@ def _var_ns_as_python_sym(name: str) -> str: _INTERN_VAR_FN_NAME = _load_attr(f"{_VAR_ALIAS}.intern") _INTERN_UNBOUND_VAR_FN_NAME = _load_attr(f"{_VAR_ALIAS}.intern_unbound") _FIND_VAR_FN_NAME = _load_attr(f"{_VAR_ALIAS}.find_safe") -_ATTR_CLASS_DECORATOR_NAME = _load_attr("attr.define") -_ATTR_FROZEN_DECORATOR_NAME = _load_attr("attr.frozen") -_ATTRIB_FIELD_FN_NAME = _load_attr("attr.field") +_ATTR_CLASS_DECORATOR_NAME = _load_attr(f"{_ATTR_ALIAS}.define") +_ATTR_FROZEN_DECORATOR_NAME = _load_attr(f"{_ATTR_ALIAS}.frozen") +_ATTRIB_FIELD_FN_NAME = _load_attr(f"{_ATTR_ALIAS}.field") _COERCE_SEQ_FN_NAME = _load_attr(f"{_RUNTIME_ALIAS}.to_seq") _BASILISP_FN_FN_NAME = _load_attr(f"{_RUNTIME_ALIAS}._basilisp_fn") _FN_WITH_ATTRS_FN_NAME = _load_attr(f"{_RUNTIME_ALIAS}._with_attrs") _BASILISP_TYPE_FN_NAME = _load_attr(f"{_RUNTIME_ALIAS}._basilisp_type") _BASILISP_WITH_META_INTERFACE_NAME = _load_attr(f"{_INTERFACES_ALIAS}.IWithMeta") -_BUILTINS_IMPORT_FN_NAME = _load_attr("builtins.__import__") -_IMPORTLIB_IMPORT_MODULE_FN_NAME = _load_attr("importlib.import_module") +_BUILTINS_IMPORT_FN_NAME = _load_attr(f"{_BUILTINS_ALIAS}.__import__") +_IMPORTLIB_IMPORT_MODULE_FN_NAME = _load_attr(f"{_IMPORTLIB_ALIAS}.import_module") _LISP_FN_APPLY_KWARGS_FN_NAME = _load_attr(f"{_RUNTIME_ALIAS}._lisp_fn_apply_kwargs") _LISP_FN_COLLECT_KWARGS_FN_NAME = _load_attr( f"{_RUNTIME_ALIAS}._lisp_fn_collect_kwargs" @@ -1964,7 +1983,7 @@ def fn(*args): ret_ann_deps.extend(ret_ann.dependencies) ret_ann_ast = ( ast.Subscript( - value=ast.Name(id="Union", ctx=ast.Load()), + value=ast.Name(id=_UNION_ALIAS, ctx=ast.Load()), slice=ast_index(ast.Tuple(elts=ret_ann_asts, ctx=ast.Load())), ctx=ast.Load(), ) @@ -3922,7 +3941,7 @@ def _from_module_imports() -> Iterable[ast.ImportFrom]: ast.ImportFrom( module="typing", names=[ - ast.alias(name="Union", asname=None), + ast.alias(name="Union", asname=_UNION_ALIAS), ], level=0, ), diff --git a/src/basilisp/lang/compiler/optimizer.py b/src/basilisp/lang/compiler/optimizer.py index 71459e7cd..1479d0208 100644 --- a/src/basilisp/lang/compiler/optimizer.py +++ b/src/basilisp/lang/compiler/optimizer.py @@ -4,6 +4,7 @@ from contextlib import contextmanager from typing import Deque, Iterable, List, Optional, Set +from basilisp.lang.compiler.constants import OPERATOR_ALIAS from basilisp.lang.compiler.utils import ast_FunctionDef, ast_index @@ -43,7 +44,7 @@ def _optimize_operator_call_attr( # pylint: disable=too-many-return-statements Using Python operators directly will allow for more direct bytecode to be emitted by the Python compiler and take advantage of any additional performance improvements in future versions of Python.""" - if isinstance(fn.value, ast.Name) and fn.value.id == "operator": + if isinstance(fn.value, ast.Name) and fn.value.id == OPERATOR_ALIAS: binop = { "add": ast.Add, "and_": ast.BitAnd, diff --git a/src/basilisp/lang/runtime.py b/src/basilisp/lang/runtime.py index b24532f19..bcea70fe4 100644 --- a/src/basilisp/lang/runtime.py +++ b/src/basilisp/lang/runtime.py @@ -503,31 +503,42 @@ def pop_bindings(self) -> Frame: class Namespace(ReferenceBase): - """Namespaces serve as organizational units in Basilisp code, just as - they do in Clojure code. Vars are mutable containers for functions and - data which may be interned in a namespace and referred to by a Symbol. - Namespaces additionally may have aliases to other namespaces, so code - organized in one namespace may conveniently refer to code or data in - other namespaces using that alias as the Symbol's namespace. - Namespaces are constructed def-by-def as Basilisp reads in each form - in a file (which will typically declare a namespace at the top). + """Namespaces serve as organizational units in Basilisp code, just as they do in + Clojure code. + + Vars are mutable containers for functions and data which may be interned in a + namespace and referred to by a Symbol. + + Namespaces additionally may have aliases to other namespaces, so code organized in + one namespace may conveniently refer to code or data in other namespaces using that + alias as the Symbol's namespace. + + Namespaces are constructed def-by-def as Basilisp reads in each form in a file + (which will typically declare a namespace at the top). + Namespaces have the following fields of interest: - - `aliases` is a mapping between a symbolic alias and another - Namespace. The fully qualified name of a namespace is also - an alias for itself. - - `imports` is a mapping of names to Python modules imported - into the current namespace. + - `aliases` is a mapping between a symbolic alias and another Namespace. The fully + qualified name of a namespace is also an alias for itself. - - `interns` is a mapping between a symbolic name and a Var. The - Var may point to code, data, or nothing, if it is unbound. Vars - in `interns` are interned in _this_ namespace. + - `imports` is a mapping of names to Python modules imported into the current + namespace. - - `refers` is a mapping between a symbolic name and a Var. Vars in - `refers` are interned in another namespace and are only referred - to without an alias in this namespace. + - `import_aliases` is a mapping of aliases for Python modules to the true module + name. + + - `interns` is a mapping between a symbolic name and a Var. The Var may point to + code, data, or nothing, if it is unbound. Vars in `interns` are interned in + _this_ namespace. + + - `refers` is a mapping between a symbolic name and a Var. Vars in `refers` are + interned in another namespace and are only referred to without an alias in + this namespace. """ + # If this set is updated, be sure to update the following two locations: + # - basilisp.lang.compiler.generator._MODULE_ALIASES + # - the `namespace_imports` section in the documentation DEFAULT_IMPORTS = lset.set( map( sym.symbol, @@ -659,6 +670,20 @@ def __repr__(self): def __hash__(self): return hash(self._name) + def _check_potential_name_conflicts(self, name: sym.Symbol) -> None: + if name in self._aliases: + logger.warning( + f"name '{name}' may be shadowed by existing alias in '{self}'" + ) + if name in self._import_aliases: + logger.warning( + f"name '{name}' may be shadowed by existing import alias in '{self}'" + ) + if name in self._imports: + logger.warning( + f"name '{name}' may be shadowed by existing import in '{self}'" + ) + def require(self, ns_name: str, *aliases: sym.Symbol) -> BasilispModule: """Require the Basilisp Namespace named by `ns_name` and add any aliases given to this Namespace. @@ -684,6 +709,7 @@ def add_alias(self, namespace: "Namespace", *aliases: sym.Symbol) -> None: with self._lock: new_m = self._aliases for alias in aliases: + self._check_potential_name_conflicts(alias) new_m = new_m.assoc(alias, namespace) self._aliases = new_m @@ -725,10 +751,12 @@ def add_import(self, sym: sym.Symbol, module: Module, *aliases: sym.Symbol) -> N """Add the Symbol as an imported Symbol in this Namespace. If aliases are given, the aliases will be applied to the""" with self._lock: + self._check_potential_name_conflicts(sym) self._imports = self._imports.assoc(sym, module) if aliases: m = self._import_aliases for alias in aliases: + self._check_potential_name_conflicts(alias) m = m.assoc(alias, sym) self._import_aliases = m From e664a74f067a73a065daa08a6c0af08cf81b27fa Mon Sep 17 00:00:00 2001 From: Chris Rink Date: Tue, 17 Sep 2024 10:14:58 -0400 Subject: [PATCH 2/7] Unused --- src/basilisp/io.lpy | 1 - 1 file changed, 1 deletion(-) diff --git a/src/basilisp/io.lpy b/src/basilisp/io.lpy index 8323bc8ca..a07e8616c 100644 --- a/src/basilisp/io.lpy +++ b/src/basilisp/io.lpy @@ -5,7 +5,6 @@ streams from a wide variety of different input types as well as utility functions for interacting with the filesystem." (:import - io os.path pathlib shutil From c8837f7ab449d32ab6d4bd56875a54b9de3da5d8 Mon Sep 17 00:00:00 2001 From: Chris Rink Date: Tue, 17 Sep 2024 12:49:25 -0400 Subject: [PATCH 3/7] Different --- src/basilisp/lang/compiler/analyzer.py | 61 ++++++++++++++++++++++++++ src/basilisp/lang/runtime.py | 17 ------- tests/basilisp/compiler_test.py | 4 ++ 3 files changed, 65 insertions(+), 17 deletions(-) diff --git a/src/basilisp/lang/compiler/analyzer.py b/src/basilisp/lang/compiler/analyzer.py index 9105eb06f..ea5a64b23 100644 --- a/src/basilisp/lang/compiler/analyzer.py +++ b/src/basilisp/lang/compiler/analyzer.py @@ -9,6 +9,7 @@ import re import sys import uuid +from collections import defaultdict from datetime import datetime from decimal import Decimal from fractions import Fraction @@ -2505,6 +2506,60 @@ def _if_ast(form: ISeq, ctx: AnalyzerContext) -> If: ) +def _do_warn_on_import_name_clash( + ctx: AnalyzerContext, alias_nodes: List[ImportAlias] +) -> None: + assert alias_nodes, "Must have at least one alias" + + # Fetch these locally to avoid triggering more locks than we need to + current_ns = ctx.current_ns + aliases, import_aliases, imports = ( + current_ns.aliases, + current_ns.import_aliases, + current_ns.imports, + ) + + def _node_loc(env: NodeEnv) -> str: + if env.line is None: + return str(env.ns) + return f"{env.ns}:{env.line}" + + # Identify duplicates in the import list first + name_to_nodes = defaultdict(list) + for node in alias_nodes: + name_to_nodes[(node.alias or node.name)].append(node) + + for name, nodes in name_to_nodes.items(): + if len(nodes) < 2: + continue + + locs = [_node_loc(node.env) for node in nodes] + logger.warning( + f"duplicate name or alias '{name}' in import ({'; '.join(locs)})" + ) + + # Now check against names in the namespace + for name, nodes in name_to_nodes.items(): + name_sym = sym.symbol(name) + node, *_ = nodes + + if name_sym in aliases: + logger.warning( + f"name '{name}' may be shadowed by existing alias in '{current_ns}' " + f"({_node_loc(node.env)})" + ) + if name_sym in import_aliases: + logger.warning( + f"name '{name}' may be shadowed by existing import alias in " + f"'{current_ns}' ({_node_loc(node.env)})" + ) + if name_sym in imports: + logger.warning( + f"name '{name}' may be shadowed by existing import in '{current_ns}' " + f"({_node_loc(node.env)})" + ) + + def _import_ast(form: ISeq, ctx: AnalyzerContext) -> Import: assert form.first == SpecialForm.IMPORT @@ -2567,6 +2622,12 @@ def _import_ast(form: ISeq, ctx: AnalyzerContext) -> Import: ) ) + if not aliases: + raise ctx.AnalyzerException( + "import forms must name at least one module", form=form + ) + + _do_warn_on_import_name_clash(ctx, aliases) return Import( form=form, aliases=aliases, diff --git a/src/basilisp/lang/runtime.py b/src/basilisp/lang/runtime.py index bcea70fe4..1ddd6530a 100644 --- a/src/basilisp/lang/runtime.py +++ b/src/basilisp/lang/runtime.py @@ -670,20 +670,6 @@ def __repr__(self): def __hash__(self): return hash(self._name) - def _check_potential_name_conflicts(self, name: sym.Symbol) -> None: - if name in self._aliases: - logger.warning( - f"name '{name}' may be shadowed by existing alias in '{self}'" - ) - if name in self._import_aliases: - logger.warning( - f"name '{name}' may be shadowed by existing import alias in '{self}'" - ) - if name in self._imports: - logger.warning( - f"name '{name}' may be shadowed by existing import in '{self}'" - ) - def require(self, ns_name: str, *aliases: sym.Symbol) -> BasilispModule: """Require the Basilisp Namespace named by `ns_name` and add any aliases given to this Namespace. @@ -709,7 +695,6 @@ def add_alias(self, namespace: "Namespace", *aliases: sym.Symbol) -> None: with self._lock: new_m = self._aliases for alias in aliases: - self._check_potential_name_conflicts(alias) new_m = new_m.assoc(alias, namespace) self._aliases = new_m @@ -751,12 +736,10 @@ def add_import(self, sym: sym.Symbol, module: Module, *aliases: sym.Symbol) -> N """Add the Symbol as an imported Symbol in this Namespace. If aliases are given, the aliases will be applied to the""" with self._lock: - self._check_potential_name_conflicts(sym) self._imports = self._imports.assoc(sym, module) if aliases: m = self._import_aliases for alias in aliases: - self._check_potential_name_conflicts(alias) m = m.assoc(alias, sym) self._import_aliases = m diff --git a/tests/basilisp/compiler_test.py b/tests/basilisp/compiler_test.py index 441bd5c55..5626dd1ae 100644 --- a/tests/basilisp/compiler_test.py +++ b/tests/basilisp/compiler_test.py @@ -3294,6 +3294,10 @@ def test_truthiness(self, lcompile: CompileFn): class TestImport: + def test_import_must_have_at_least_one_module(self, lcompile: CompileFn): + with pytest.raises(compiler.CompilerException): + lcompile("(import*)") + @pytest.mark.parametrize( "code", [ From 8d718c2d6b22280a27f57d9c1b0331a8d88daf45 Mon Sep 17 00:00:00 2001 From: Chris Rink Date: Tue, 17 Sep 2024 12:54:25 -0400 Subject: [PATCH 4/7] Lock down require forms --- src/basilisp/lang/compiler/analyzer.py | 5 +++++ tests/basilisp/compiler_test.py | 4 ++++ 2 files changed, 9 insertions(+) diff --git a/src/basilisp/lang/compiler/analyzer.py b/src/basilisp/lang/compiler/analyzer.py index ea5a64b23..2a11f40bb 100644 --- a/src/basilisp/lang/compiler/analyzer.py +++ b/src/basilisp/lang/compiler/analyzer.py @@ -3164,6 +3164,11 @@ def _require_ast(form: ISeq, ctx: AnalyzerContext) -> Require: ) ) + if not aliases: + raise ctx.AnalyzerException( + "require forms must name at least one namespace", form=form + ) + return Require( form=form, aliases=aliases, diff --git a/tests/basilisp/compiler_test.py b/tests/basilisp/compiler_test.py index 5626dd1ae..d80680414 100644 --- a/tests/basilisp/compiler_test.py +++ b/tests/basilisp/compiler_test.py @@ -5402,6 +5402,10 @@ def test_reify_property_may_not_be_multi_arity(self, lcompile: CompileFn): class TestRequire: + def test_require_must_have_at_least_one_namespace(self, lcompile: CompileFn): + with pytest.raises(compiler.CompilerException): + lcompile("(require*)") + @pytest.mark.parametrize( "code", [ From 3e8c0fbc7fafc0874e95cd161e775aba29c954dc Mon Sep 17 00:00:00 2001 From: Chris Rink Date: Wed, 18 Sep 2024 08:38:22 -0400 Subject: [PATCH 5/7] yeah --- src/basilisp/lang/compiler/analyzer.py | 26 +++++++++++++------------- tests/basilisp/compiler_test.py | 20 ++++++++++++++++++++ 2 files changed, 33 insertions(+), 13 deletions(-) diff --git a/src/basilisp/lang/compiler/analyzer.py b/src/basilisp/lang/compiler/analyzer.py index 2a11f40bb..a69c3e1f7 100644 --- a/src/basilisp/lang/compiler/analyzer.py +++ b/src/basilisp/lang/compiler/analyzer.py @@ -612,7 +612,7 @@ def syntax_position(self) -> NodeSyntacticPosition: def get_node_env(self, pos: Optional[NodeSyntacticPosition] = None) -> NodeEnv: """Return the current Node environment. - If a synax position is given, it will be included in the environment. + If a syntax position is given, it will be included in the environment. Otherwise, the position will be set to None.""" return NodeEnv( ns=self.current_ns, file=self.filename, pos=pos, func_ctx=self.func_ctx @@ -2519,10 +2519,12 @@ def _do_warn_on_import_name_clash( current_ns.imports, ) - def _node_loc(env: NodeEnv) -> str: - if env.line is None: - return str(env.ns) - return f"{env.ns}:{env.line}" + def _node_loc(node: ImportAlias) -> str: + if (line := node.env.line) is None: + if (form_loc := _loc(node.form)) is None: + return str(node.env.ns) + line = form_loc[0] + return f"{node.env.ns}:{line}" # Identify duplicates in the import list first name_to_nodes = defaultdict(list) @@ -2533,10 +2535,8 @@ def _node_loc(env: NodeEnv) -> str: if len(nodes) < 2: continue - locs = [_node_loc(node.env) for node in nodes] - logger.warning( - f"duplicate name or alias '{name}' in import ({'; '.join(locs)})" - ) + loc = _node_loc(next(iter(nodes))) + logger.warning(f"duplicate name or alias '{name}' in import ({loc})") # Now check against names in the namespace for name, nodes in name_to_nodes.items(): @@ -2546,17 +2546,17 @@ def _node_loc(env: NodeEnv) -> str: if name_sym in aliases: logger.warning( f"name '{name}' may be shadowed by existing alias in '{current_ns}' " - f"({_node_loc(node.env)})" + f"({_node_loc(node)})" ) if name_sym in import_aliases: logger.warning( f"name '{name}' may be shadowed by existing import alias in " - f"'{current_ns}' ({_node_loc(node.env)})" + f"'{current_ns}' ({_node_loc(node)})" ) if name_sym in imports: logger.warning( f"name '{name}' may be shadowed by existing import in '{current_ns}' " - f"({_node_loc(node.env)})" + f"({_node_loc(node)})" ) @@ -4040,7 +4040,7 @@ def _const_node(form: ReaderForm, ctx: AnalyzerContext) -> Const: ) if hasattr(form, "meta"): - form_meta = _clean_meta(form.meta) # type: ignore + form_meta = form.meta # _clean_meta(form.meta) # type: ignore if form_meta is not None: meta_ast = _const_node(form_meta, ctx) assert isinstance(meta_ast, MapNode) or ( diff --git a/tests/basilisp/compiler_test.py b/tests/basilisp/compiler_test.py index d80680414..1728def08 100644 --- a/tests/basilisp/compiler_test.py +++ b/tests/basilisp/compiler_test.py @@ -3379,6 +3379,26 @@ def test_aliased_nested_import_refers_to_child(self, lcompile: CompileFn): assert os.path.exists is lcompile("(import [os.path :as path]) path/exists") + def test_warn_on_duplicated_import_name( + self, lcompile: CompileFn, assert_matching_logs + ): + lcompile("(import os.path os.path)") + assert_matching_logs( + "basilisp.lang.compiler.analyzer", + logging.WARNING, + r"duplicate name or alias 'os.path' in import ([^)]+)", + ) + + def test_warn_on_duplicated_import_name_with_alias( + self, lcompile: CompileFn, assert_matching_logs + ): + lcompile("(import abc [collections.abc :as abc])") + assert_matching_logs( + "basilisp.lang.compiler.analyzer", + logging.WARNING, + r"duplicate name or alias 'abc' in import ([^)]+)", + ) + class TestInvoke: @pytest.mark.parametrize( From a13c152600000b2afb20267bed5b1a0073ea6f19 Mon Sep 17 00:00:00 2001 From: Chris Rink Date: Wed, 18 Sep 2024 11:57:37 -0400 Subject: [PATCH 6/7] Warn --- pyproject.toml | 2 + src/basilisp/lang/compiler/analyzer.py | 35 +++++------ tests/basilisp/compiler_test.py | 86 +++++++++++++++++++++++++- 3 files changed, 102 insertions(+), 21 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index b4358c16c..5dabedf31 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -85,6 +85,7 @@ exclude = ''' \.git | \.hg | \.cache + | \.graalvenv | \.mypy_cache | \.pytest_cache | \.tox @@ -133,6 +134,7 @@ skip = [ ".env", ".hg", ".git", + ".graalvenv", ".mypy_cache", ".pytest_cache", ".tox", diff --git a/src/basilisp/lang/compiler/analyzer.py b/src/basilisp/lang/compiler/analyzer.py index a69c3e1f7..1d3ce9588 100644 --- a/src/basilisp/lang/compiler/analyzer.py +++ b/src/basilisp/lang/compiler/analyzer.py @@ -36,6 +36,7 @@ ) import attr +from typing_extensions import Literal from basilisp.lang import keyword as kw from basilisp.lang import list as llist @@ -2506,8 +2507,13 @@ def _if_ast(form: ISeq, ctx: AnalyzerContext) -> If: ) -def _do_warn_on_import_name_clash( - ctx: AnalyzerContext, alias_nodes: List[ImportAlias] +T_alias_node = TypeVar("T_alias_node", ImportAlias, RequireAlias) + + +def _do_warn_on_import_or_require_name_clash( + ctx: AnalyzerContext, + alias_nodes: List[T_alias_node], + action: Literal["import", "require"], ) -> None: assert alias_nodes, "Must have at least one alias" @@ -2519,13 +2525,6 @@ def _do_warn_on_import_name_clash( current_ns.imports, ) - def _node_loc(node: ImportAlias) -> str: - if (line := node.env.line) is None: - if (form_loc := _loc(node.form)) is None: - return str(node.env.ns) - line = form_loc[0] - return f"{node.env.ns}:{line}" - # Identify duplicates in the import list first name_to_nodes = defaultdict(list) for node in alias_nodes: @@ -2535,8 +2534,7 @@ def _node_loc(node: ImportAlias) -> str: if len(nodes) < 2: continue - loc = _node_loc(next(iter(nodes))) - logger.warning(f"duplicate name or alias '{name}' in import ({loc})") + logger.warning(f"duplicate name or alias '{name}' in {action}") # Now check against names in the namespace for name, nodes in name_to_nodes.items(): @@ -2545,18 +2543,16 @@ def _node_loc(node: ImportAlias) -> str: if name_sym in aliases: logger.warning( - f"name '{name}' may be shadowed by existing alias in '{current_ns}' " - f"({_node_loc(node)})" + f"name '{name}' may shadow an existing alias in '{current_ns}'" ) if name_sym in import_aliases: logger.warning( - f"name '{name}' may be shadowed by existing import alias in " - f"'{current_ns}' ({_node_loc(node)})" + f"name '{name}' may be shadowed by an existing import alias in " + f"'{current_ns}'" ) if name_sym in imports: logger.warning( - f"name '{name}' may be shadowed by existing import in '{current_ns}' " - f"({_node_loc(node)})" + f"name '{name}' may be shadowed by an existing import in '{current_ns}'" ) @@ -2627,7 +2623,7 @@ def _import_ast(form: ISeq, ctx: AnalyzerContext) -> Import: "import forms must name at least one module", form=form ) - _do_warn_on_import_name_clash(ctx, aliases) + _do_warn_on_import_or_require_name_clash(ctx, aliases, "import") return Import( form=form, aliases=aliases, @@ -3169,6 +3165,7 @@ def _require_ast(form: ISeq, ctx: AnalyzerContext) -> Require: "require forms must name at least one namespace", form=form ) + _do_warn_on_import_or_require_name_clash(ctx, aliases, "require") return Require( form=form, aliases=aliases, @@ -4040,7 +4037,7 @@ def _const_node(form: ReaderForm, ctx: AnalyzerContext) -> Const: ) if hasattr(form, "meta"): - form_meta = form.meta # _clean_meta(form.meta) # type: ignore + form_meta = _clean_meta(form.meta) # type: ignore if form_meta is not None: meta_ast = _const_node(form_meta, ctx) assert isinstance(meta_ast, MapNode) or ( diff --git a/tests/basilisp/compiler_test.py b/tests/basilisp/compiler_test.py index 1728def08..5dbc7e1f5 100644 --- a/tests/basilisp/compiler_test.py +++ b/tests/basilisp/compiler_test.py @@ -3386,7 +3386,7 @@ def test_warn_on_duplicated_import_name( assert_matching_logs( "basilisp.lang.compiler.analyzer", logging.WARNING, - r"duplicate name or alias 'os.path' in import ([^)]+)", + r"duplicate name or alias 'os.path' in import", ) def test_warn_on_duplicated_import_name_with_alias( @@ -3396,7 +3396,37 @@ def test_warn_on_duplicated_import_name_with_alias( assert_matching_logs( "basilisp.lang.compiler.analyzer", logging.WARNING, - r"duplicate name or alias 'abc' in import ([^)]+)", + r"duplicate name or alias 'abc' in import", + ) + + def test_warn_on_shadowing_by_existing_import( + self, ns: runtime.Namespace, lcompile: CompileFn, assert_matching_logs + ): + lcompile("(import abc) (import [collections.abc :as abc])") + assert_matching_logs( + "basilisp.lang.compiler.analyzer", + logging.WARNING, + rf"name 'abc' may be shadowed by an existing import in '{ns}'", + ) + + def test_warn_on_shadowing_by_existing_import_alias( + self, ns: runtime.Namespace, lcompile: CompileFn, assert_matching_logs + ): + lcompile("(import [collections.abc :as abc]) (import abc)") + assert_matching_logs( + "basilisp.lang.compiler.analyzer", + logging.WARNING, + rf"name 'abc' may be shadowed by an existing import alias in '{ns}'", + ) + + def test_warn_on_shadowing_by_existing_namespace_alias( + self, ns: runtime.Namespace, lcompile: CompileFn, assert_matching_logs + ): + lcompile("(require '[basilisp.string :as abc]) (import abc)") + assert_matching_logs( + "basilisp.lang.compiler.analyzer", + logging.WARNING, + rf"name 'abc' may shadow an existing alias in '{ns}'", ) @@ -5507,6 +5537,58 @@ def test_single_require(self, lcompile: CompileFn, string_ns, code: str): def test_multi_require(self, lcompile: CompileFn, string_ns, set_ns, code: str): assert [string_ns.join, set_ns.union] == list(lcompile(code)) + def test_warn_on_duplicated_require_name( + self, lcompile: CompileFn, assert_matching_logs + ): + lcompile("(require* basilisp.string basilisp.string)") + assert_matching_logs( + "basilisp.lang.compiler.analyzer", + logging.WARNING, + r"duplicate name or alias 'basilisp.string' in require", + ) + + def test_warn_on_duplicated_import_name_with_alias( + self, lcompile: CompileFn, assert_matching_logs + ): + lcompile("(require* [basilisp.edn :as serde] [basilisp.json :as serde])") + assert_matching_logs( + "basilisp.lang.compiler.analyzer", + logging.WARNING, + r"duplicate name or alias 'serde' in require", + ) + + def test_warn_on_shadowing_by_existing_import( + self, ns: runtime.Namespace, lcompile: CompileFn, assert_matching_logs + ): + lcompile("(import json) (require* [basilisp.json :as json])") + assert_matching_logs( + "basilisp.lang.compiler.analyzer", + logging.WARNING, + rf"name 'json' may be shadowed by an existing import in '{ns}'", + ) + + def test_warn_on_shadowing_by_existing_import_alias( + self, ns: runtime.Namespace, lcompile: CompileFn, assert_matching_logs + ): + lcompile("(import [collections.abc :as abc]) (require* [basilisp.edn :as abc])") + assert_matching_logs( + "basilisp.lang.compiler.analyzer", + logging.WARNING, + rf"name 'abc' may be shadowed by an existing import alias in '{ns}'", + ) + + def test_warn_on_shadowing_by_existing_namespace_alias( + self, ns: runtime.Namespace, lcompile: CompileFn, assert_matching_logs + ): + lcompile( + "(require* [basilisp.string :as edn]) (require* [basilisp.json :as edn])" + ) + assert_matching_logs( + "basilisp.lang.compiler.analyzer", + logging.WARNING, + rf"name 'edn' may shadow an existing alias in '{ns}'", + ) + class TestSetBang: def test_num_elems(self, lcompile: CompileFn): From 477626949d922e76511f850cae9865217716e551 Mon Sep 17 00:00:00 2001 From: Chris Rink Date: Wed, 18 Sep 2024 11:59:35 -0400 Subject: [PATCH 7/7] Ignore graal --- .gitignore | 1 + 1 file changed, 1 insertion(+) diff --git a/.gitignore b/.gitignore index d5465e578..ee8544007 100644 --- a/.gitignore +++ b/.gitignore @@ -51,6 +51,7 @@ docs/_build/ # Environments .env +.graalvenv .venv env/ venv/