Only generate globally unique parameter names for inline functions (#1229)

Fixes #1212

Author: chrisrink10

CHANGELOG.md

@@ -9,7 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
  * Added `afor` and `awith` macros to support async Python interop (#1179, #1181)
 
 ### Changed
- * Single arity functions can be tagged with `^:allow-unsafe-names` to preserve their parameter names (#1212)
+ * Function parameter names will not be automatically generated with unique suffixes unless the function meta key `^:safe-py-params` is provided (#1212)
 
 ### Fixed
  * Fix an issue where the compiler would generate an illegal `return` statement for asynchronous generators (#1180)

docs/compiler.rst

@@ -177,6 +177,16 @@ Functions not meeting these criteria will trigger compile time errors if they ar
 
    Users should consider inlining primarily a Basilisp internal feature and use it extremely sparingly in user code.
 
+.. warning::
+
+   Due to the nature of how inline functions are applied, there is a potential for name clashes between the inline function parameter names and names defined elsewhere in the containing Python module.
+   Therefore, it is recommended for any inline function to set the meta key ``^:safe-py-params`` to ensure that the compiler generates globally unique Python parameter names.
+   For inline functions generated automatically by the compiler, this setting is automatically enabled.
+
+   .. code-block::
+
+      ^:safe-py-params (fn [a b] ...)
+
 .. _compiler_debugging:
 
 Debugging

docs/pyinterop.rst

@@ -22,14 +22,6 @@ When compiled, a ``kebab-case`` identifier always becomes a ``snake_case`` ident
 
    The Basilisp compiler munges *all* unsafe Basilisp identifiers to safe Python identifiers, but other cases are unlikely to appear in standard Python interop usage.
 
-.. note::
-
-   By default, the compiler munges function parameter names and makes them globally unique by appending a monotically increasing number suffix to support function inlining. To enable interop with Python libraries that rely on preserved parameter names, you can use the ```^:allow-unsafe-names`` metadata key to retain the (munged) parameter names. This flag only applies to single-arity Basilisp functions.
-
-   .. code-block::
-
-      (defn ^:allow-unsafe-names afun [a b] ...)
-
 .. _python_builtins:
 
 Python Builtins
@@ -316,7 +308,7 @@ These decorators are applied from right to left, similar to how Python decorator
     ;;; Decorators with arguments, and order of application (right to left)
     ;;
     ;; example decorator
-    (defn mult-x-decorator 
+    (defn mult-x-decorator
       [x]
       (fn [f]
         (fn [] (* (f) x))))
@@ -327,7 +319,7 @@ These decorators are applied from right to left, similar to how Python decorator
     ;;; defasync support
     ;;
     ;; example async decorator
-    (defn add-7-async-decorator 
+    (defn add-7-async-decorator
       [f]
       ^:async (fn [] (+ (await (f)) 7)))
 

src/basilisp/core.lpy

@@ -307,27 +307,27 @@
 ;; Define inline functions for earlier core functions now that we have all the
 ;; functionality required to do so.
 
-(.alter-meta #'first assoc :inline (fn [seq] `(basilisp.lang.runtime/first ~seq)))
-(.alter-meta #'rest assoc :inline (fn [seq] `(basilisp.lang.runtime/rest ~seq)))
-(.alter-meta #'next assoc :inline (fn [seq] `(basilisp.lang.runtime/next ~seq)))
-(.alter-meta #'second assoc :inline (fn [seq] `(first (rest ~seq))))
-(.alter-meta #'ffirst assoc :inline (fn [seq] `(first (first ~seq))))
-(.alter-meta #'identity assoc :inline (fn [o] `~o))
-(.alter-meta #'instance? assoc :inline (fn [class obj] `(python/isinstance ~obj ~class)))
-(.alter-meta #'boolean? assoc :inline (fn [o] `(instance? python/bool ~o)))
-(.alter-meta #'float? assoc :inline (fn [o] `(instance? python/float ~o)))
-(.alter-meta #'integer? assoc :inline (fn [o] `(instance? python/int ~o)))
-(.alter-meta #'string? assoc :inline (fn [o] `(instance? python/str ~o)))
-(.alter-meta #'symbol? assoc :inline (fn [o] `(instance? basilisp.lang.symbol/Symbol ~o)))
-(.alter-meta #'keyword? assoc :inline (fn [o] `(instance? basilisp.lang.keyword/Keyword ~o)))
-(.alter-meta #'list? assoc :inline (fn [o] `(instance? basilisp.lang.interfaces/IPersistentList ~o)))
-(.alter-meta #'map? assoc :inline (fn [o] `(instance? basilisp.lang.interfaces/IPersistentMap ~o)))
-(.alter-meta #'set? assoc :inline (fn [o] `(instance? basilisp.lang.interfaces/IPersistentSet ~o)))
-(.alter-meta #'vector? assoc :inline (fn [o] `(instance? basilisp.lang.interfaces/IPersistentVector ~o)))
-(.alter-meta #'seq? assoc :inline (fn [o] `(instance? basilisp.lang.interfaces/ISeq ~o)))
-(.alter-meta #'seq assoc :inline (fn [o] `(basilisp.lang.runtime/to-seq ~o)))
-(.alter-meta #'set assoc :inline (fn [coll] `(basilisp.lang.runtime/to-set ~coll)))
-(.alter-meta #'vec assoc :inline (fn [coll] `(basilisp.lang.runtime/vector ~coll)))
+(.alter-meta #'first assoc :inline ^:safe-py-params (fn [seq] `(basilisp.lang.runtime/first ~seq)))
+(.alter-meta #'rest assoc :inline ^:safe-py-params (fn [seq] `(basilisp.lang.runtime/rest ~seq)))
+(.alter-meta #'next assoc :inline ^:safe-py-params (fn [seq] `(basilisp.lang.runtime/next ~seq)))
+(.alter-meta #'second assoc :inline ^:safe-py-params (fn [seq] `(first (rest ~seq))))
+(.alter-meta #'ffirst assoc :inline ^:safe-py-params (fn [seq] `(first (first ~seq))))
+(.alter-meta #'identity assoc :inline ^:safe-py-params (fn [o] `~o))
+(.alter-meta #'instance? assoc :inline ^:safe-py-params (fn [class obj] `(python/isinstance ~obj ~class)))
+(.alter-meta #'boolean? assoc :inline ^:safe-py-params (fn [o] `(instance? python/bool ~o)))
+(.alter-meta #'float? assoc :inline ^:safe-py-params (fn [o] `(instance? python/float ~o)))
+(.alter-meta #'integer? assoc :inline ^:safe-py-params (fn [o] `(instance? python/int ~o)))
+(.alter-meta #'string? assoc :inline ^:safe-py-params (fn [o] `(instance? python/str ~o)))
+(.alter-meta #'symbol? assoc :inline ^:safe-py-params (fn [o] `(instance? basilisp.lang.symbol/Symbol ~o)))
+(.alter-meta #'keyword? assoc :inline ^:safe-py-params (fn [o] `(instance? basilisp.lang.keyword/Keyword ~o)))
+(.alter-meta #'list? assoc :inline ^:safe-py-params (fn [o] `(instance? basilisp.lang.interfaces/IPersistentList ~o)))
+(.alter-meta #'map? assoc :inline ^:safe-py-params (fn [o] `(instance? basilisp.lang.interfaces/IPersistentMap ~o)))
+(.alter-meta #'set? assoc :inline ^:safe-py-params (fn [o] `(instance? basilisp.lang.interfaces/IPersistentSet ~o)))
+(.alter-meta #'vector? assoc :inline ^:safe-py-params (fn [o] `(instance? basilisp.lang.interfaces/IPersistentVector ~o)))
+(.alter-meta #'seq? assoc :inline ^:safe-py-params (fn [o] `(instance? basilisp.lang.interfaces/ISeq ~o)))
+(.alter-meta #'seq assoc :inline ^:safe-py-params (fn [o] `(basilisp.lang.runtime/to-seq ~o)))
+(.alter-meta #'set assoc :inline ^:safe-py-params (fn [coll] `(basilisp.lang.runtime/to-set ~coll)))
+(.alter-meta #'vec assoc :inline ^:safe-py-params (fn [coll] `(basilisp.lang.runtime/vector ~coll)))
 
 (def
   ^{:macro    true
@@ -4796,7 +4796,7 @@
   as the result. Returns the new value.
 
   .. note::
-  
+
      Due to Basilisp's :ref:`Direct Linking Optimization <direct_linking>`, changes to
      a Var's root value may not be reflected in the code unless the Var is dynamic.
      To ensure updates propagate, set the Var's `^:redef` metadata, which disables
@@ -6068,7 +6068,7 @@
       generated function from right to left.
 
    .. note::
-   
+
       The ``name`` metadata (i.e., ``(fn ^{...} <name>  ...)``) takes
       precedence over  the ``form`` metadata (i.e., ``^{...} (fn <name?> ...)``)
       when both specify the same key.

src/basilisp/lang/compiler/analyzer.py

@@ -56,6 +56,7 @@
     SYM_CLASSMETHOD_META_KEY,
     SYM_DEFAULT_META_KEY,
     SYM_DYNAMIC_META_KEY,
+    SYM_GEN_SAFE_PYTHON_PARAM_NAMES_META_KEY,
     SYM_INLINE_META_KW,
     SYM_KWARGS_META_KEY,
     SYM_MACRO_META_KEY,
@@ -1070,7 +1071,7 @@ def _def_ast(  # pylint: disable=too-many-locals,too-many-statements
                     "Cannot have a user-generated inline function and an automatically "
                     "generated inline function"
                 )
-                var.meta.assoc(SYM_INLINE_META_KW, init.inline_fn)  # type: ignore[union-attr]
+                var.alter_meta(lambda m: m.assoc(SYM_INLINE_META_KW, init.inline_fn))  # type: ignore[misc]
                 def_meta = def_meta.assoc(SYM_INLINE_META_KW, init.inline_fn.form)  # type: ignore[union-attr]
 
             if tag_ast is not None and any(
@@ -1570,7 +1571,7 @@ def __deftype_or_reify_method_node_from_arities(
     )
 
 
-def __deftype_or_reify_impls(  # pylint: disable=too-many-branches,too-many-locals  # noqa: MC0001
+def __deftype_or_reify_impls(  # pylint: disable=too-many-branches,too-many-locals
     form: ISeq, ctx: AnalyzerContext, special_form: sym.Symbol
 ) -> tuple[list[DefTypeBase], list[DefTypeMember]]:
     """Roll up `deftype*` and `reify*` declared bases and method implementations."""
@@ -2210,11 +2211,12 @@ def _inline_fn_ast(
         *([sym.symbol(genname(f"{name.name}-inline"))] if name is not None else []),
         vec.vector(binding.form for binding in inline_arity.params),
         macroed_form,
+        meta=lmap.map({SYM_GEN_SAFE_PYTHON_PARAM_NAMES_META_KEY: True}),
     )
     return _fn_ast(inline_fn_form, ctx)
 
 
-@_with_meta  # noqa: MC0001
+@_with_meta
 def _fn_ast(  # pylint: disable=too-many-locals,too-many-statements
     form: Union[llist.PersistentList, ISeq], ctx: AnalyzerContext
 ) -> Fn:
@@ -3601,7 +3603,7 @@ def __resolve_namespaced_symbol_in_ns(
     return None
 
 
-def __resolve_namespaced_symbol(  # pylint: disable=too-many-branches  # noqa: MC0001
+def __resolve_namespaced_symbol(  # pylint: disable=too-many-branches
     ctx: AnalyzerContext, form: sym.Symbol
 ) -> Union[Const, HostField, MaybeClass, MaybeHostForm, VarRef]:
     """Resolve a namespaced symbol into a Python name or Basilisp Var."""

src/basilisp/lang/compiler/constants.py

@@ -37,7 +37,7 @@ class SpecialForm:
 
 SYM_ABSTRACT_META_KEY = kw.keyword("abstract")
 SYM_ABSTRACT_MEMBERS_META_KEY = kw.keyword("abstract-members")
-SYM_ALLOW_UNSAFE_NAMES_META_KEY = kw.keyword("allow-unsafe-names")
+SYM_GEN_SAFE_PYTHON_PARAM_NAMES_META_KEY = kw.keyword("safe-py-params")
 SYM_ASYNC_META_KEY = kw.keyword("async")
 SYM_KWARGS_META_KEY = kw.keyword("kwargs")
 SYM_PRIVATE_META_KEY = kw.keyword("private")

src/basilisp/lang/compiler/generator.py

@@ -37,8 +37,8 @@
     INTERFACE_KW,
     OPERATOR_ALIAS,
     REST_KW,
-    SYM_ALLOW_UNSAFE_NAMES_META_KEY,
     SYM_DYNAMIC_META_KEY,
+    SYM_GEN_SAFE_PYTHON_PARAM_NAMES_META_KEY,
     SYM_REDEF_META_KEY,
     VAR_IS_PROTOCOL_META_KEY,
 )
@@ -654,13 +654,12 @@ def with_lineno_and_col(
 MetaNode = Union[Const, MapNode]
 
 
-def _is_allow_unsafe_names(fn_meta_node: Optional[MetaNode]) -> bool:
-    """Return True if the `fn_meta_node` has the meta key set to
-    retain functio parameter names.
-
-    """
+def _should_gen_safe_python_param_names(fn_meta_node: Optional[MetaNode]) -> bool:
+    """Return True if the `fn_meta_node` has the meta key set to generate globally
+    unique function parameter names."""
     return (
-        bool(fn_meta_node.form.val_at(SYM_ALLOW_UNSAFE_NAMES_META_KEY, False)) is True
+        bool(fn_meta_node.form.val_at(SYM_GEN_SAFE_PYTHON_PARAM_NAMES_META_KEY, False))
+        is True
         if fn_meta_node is not None and isinstance(fn_meta_node.form, IPersistentMap)
         else False
     )
@@ -1659,7 +1658,7 @@ def __fn_args_to_py_ast(
     ctx: GeneratorContext,
     params: Iterable[Binding],
     body: Do,
-    allow_unsafe_param_names: bool = True,
+    should_generate_safe_names: bool = False,
 ) -> tuple[list[ast.arg], Optional[ast.arg], list[ast.stmt], Iterable[PyASTNode]]:
     """Generate a list of Python AST nodes from function method parameters.
 
@@ -1675,7 +1674,11 @@ def __fn_args_to_py_ast(
         assert binding.init is None, ":fn nodes cannot have binding :inits"
         assert varg is None, "Must have at most one variadic arg"
         arg_name = munge(binding.name)
-        if not allow_unsafe_param_names:
+        # Always generate a unique name for bindings named "_" since those are
+        # typically ignored parameters. Python doesn't allow duplicate param
+        # names (even including "_"), so this is a hack to support something
+        # Clojure allows.
+        if should_generate_safe_names or binding.name == "_":
             arg_name = genname(arg_name)
 
         arg_tag: Optional[ast.expr]
@@ -1822,14 +1825,12 @@ def __single_arity_fn_to_py_ast(  # pylint: disable=too-many-locals
                 sym.symbol(lisp_fn_name), py_fn_name, LocalType.FN
             )
 
-        # check if we should preserve the original parameter names
-        allow_unsafe_param_names = _is_allow_unsafe_names(meta_node)
-
         fn_args, varg, fn_body_ast, fn_def_deps = __fn_args_to_py_ast(
             ctx,
             method.params,
             method.body,
-            allow_unsafe_param_names=allow_unsafe_param_names,
+            # check if we should preserve the original parameter names
+            should_generate_safe_names=_should_gen_safe_python_param_names(meta_node),
         )
         meta_deps, meta_decorators = __fn_meta(ctx, meta_node)
 
@@ -2138,7 +2139,13 @@ def __multi_arity_fn_to_py_ast(  # pylint: disable=too-many-locals
                 )
 
             fn_args, varg, fn_body_ast, fn_def_deps = __fn_args_to_py_ast(
-                ctx, arity.params, arity.body
+                ctx,
+                arity.params,
+                arity.body,
+                # check if we should preserve the original parameter names
+                should_generate_safe_names=_should_gen_safe_python_param_names(
+                    meta_node
+                ),
             )
             all_arity_def_deps.extend(fn_def_deps)
 

src/basilisp/lang/reader.py

@@ -1644,7 +1644,7 @@ def _resolve_tagged_literal(
             raise ctx.syntax_error(e.message).with_traceback(e.__traceback__) from None
 
 
-def _read_reader_macro(ctx: ReaderContext) -> LispReaderForm:  # noqa: MC0001
+def _read_reader_macro(ctx: ReaderContext) -> LispReaderForm:
     """Return a data structure evaluated as a reader macro from the input stream."""
     start = ctx.reader.advance()
     assert start == "#"
@@ -1731,7 +1731,7 @@ def _read_next_consuming_whitespace(ctx: ReaderContext) -> LispReaderForm:
     return _read_next(ctx)
 
 
-def _read_next(ctx: ReaderContext) -> LispReaderForm:  # noqa: C901 MC0001
+def _read_next(ctx: ReaderContext) -> LispReaderForm:  # noqa: C901
     """Read the next full form from the input stream."""
     reader = ctx.reader
     char = reader.peek()

tests/basilisp/compiler_test.py

@@ -2776,6 +2776,50 @@ def test_fn_method_allows_empty_body(
         assert callable(fvar.value)
         assert None is fvar.value()
 
+    @pytest.mark.parametrize(
+        "code,args",
+        [
+            ("(defn test_dfn1a [a b] 5)", ["a", "b"]),
+            ("(def test_dfn2 (fn [a b & c] 5))", ["a", "b", "c"]),
+            ("(def test_dfn3 (fn [a b {:as c}] 5))", ["a", "b", "c"]),
+        ],
+    )
+    def test_fn_argument_names_unchanged(
+        self, lcompile: CompileFn, code: str, args: list[str]
+    ):
+        fvar = lcompile(code)
+        fn_args = list(inspect.signature(fvar.deref()).parameters.keys())
+        assert fn_args == args
+
+    @pytest.mark.parametrize(
+        "code",
+        [
+            "(defn ^:safe-py-params test_dfn0 [a b] 5)",
+            "(def ^:safe-py-params test_dfn2 (fn [a b & c] 5))",
+            "(def test_dfn2 ^:safe-py-params (fn [a b & c] 5))",
+            "(def test_dfn2 ^:safe-py-params (fn [a b {:as c}] 5))",
+        ],
+    )
+    def test_fn_argument_names_globally_unique(self, lcompile: CompileFn, code: str):
+        fvar = lcompile(code)
+        args = list(inspect.signature(fvar.deref()).parameters.keys())
+        assert all(
+            re.fullmatch(r"[a-z]_\d+", arg) for arg in args
+        ), f"unexpected argument names {args}"
+
+    def test_fn_argument_names_unchanged_except_ignored_argument(
+        self, lcompile: CompileFn
+    ):
+        code = "(fn [a b _ _] 5)"
+        fvar = lcompile(code)
+        fn_args = set(inspect.signature(fvar).parameters.keys())
+        assert len(fn_args) == 4
+        assert {"a", "b"}.issubset(fn_args)
+        remaining_args = fn_args.difference({"a", "b"})
+        assert all(
+            re.fullmatch(r"__\d+", arg) for arg in remaining_args
+        ), f"unexpected argument names {remaining_args}"
+
     def test_single_arity_fn(self, lcompile: CompileFn, ns: runtime.Namespace):
         code = """
         (def string-upper (fn* string-upper [s] (.upper s)))
@@ -7222,57 +7266,3 @@ def test_yield_as_coroutine_with_multiple_yields(
         assert kw.keyword("coroutine-value") == state.deref()
         assert None is next(coro, None)
         assert kw.keyword("done") == state.deref()
-
-
-def test_defn_argument_names(lcompile: CompileFn):
-    # By default, function parameter names are made globally unique.
-    #
-    # For example, notice how defn generate parameter names in the
-    # pattern <parameter-name>_<monotonically-increasing-number> to
-    # ensure global uniqueness.
-    code = """
-    (defn test_dfn0 [a b] 5)
-    """
-    fvar = lcompile(code)
-    args = list(inspect.signature(fvar.deref()).parameters.keys())
-    assert all(
-        re.fullmatch(p, s) for p, s in zip([r"a_\d+", r"b_\d+"], args)
-    ), f"unexpected argument names {args}"
-
-    code = """
-    (defn ^:allow-unsafe-names test_dfn1a [a b] 5)
-    """
-    fvar = lcompile(code)
-    args = list(inspect.signature(fvar.deref()).parameters.keys())
-    assert args == ["a", "b"]
-
-    code = """
-    (defn ^{:allow-unsafe-names false} test_dfn1b [a b] 5)
-    """
-    fvar = lcompile(code)
-    args = list(inspect.signature(fvar.deref()).parameters.keys())
-    assert all(
-        re.fullmatch(p, s) for p, s in zip([r"a_\d+", r"b_\d+"], args)
-    ), f"unexpected argument names {args}"
-
-    code = """
-    (def ^:allow-unsafe-names test_dfn2 (fn [a b & c] 5))
-    """
-    fvar = lcompile(code)
-    args = list(inspect.signature(fvar.deref()).parameters.keys())
-    assert args == ["a", "b", "c"]
-
-    code = """
-    (def test_dfn3 ^:allow-unsafe-names (fn [a b & c] 5))
-    """
-    fvar = lcompile(code)
-    args = list(inspect.signature(fvar.deref()).parameters.keys())
-    assert args == ["a", "b", "c"]
-
-    code = """
-    ^{:kwargs :collect}
-    (defn ^:allow-unsafe-names test_dfn4 [a b {:as xyz}] 5)
-    """
-    fvar = lcompile(code)
-    args = list(inspect.signature(fvar.deref()).parameters.keys())
-    assert args == ["a", "b", "xyz"]