Skip to content

Only generate globally unique parameter names for inline functions #1229

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Apr 13, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
10 changes: 10 additions & 0 deletions docs/compiler.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
12 changes: 2 additions & 10 deletions docs/pyinterop.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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))))
Expand All @@ -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)))

Expand Down
46 changes: 23 additions & 23 deletions src/basilisp/core.lpy
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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.
Expand Down
10 changes: 6 additions & 4 deletions src/basilisp/lang/compiler/analyzer.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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."""
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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."""
Expand Down
2 changes: 1 addition & 1 deletion src/basilisp/lang/compiler/constants.py
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand Down
35 changes: 21 additions & 14 deletions src/basilisp/lang/compiler/generator.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
)
Expand Down Expand Up @@ -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
)
Expand Down Expand Up @@ -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.

Expand All @@ -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]
Expand Down Expand Up @@ -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)

Expand Down Expand Up @@ -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)

Expand Down
4 changes: 2 additions & 2 deletions src/basilisp/lang/reader.py
Original file line number Diff line number Diff line change
Expand Up @@ -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 == "#"
Expand Down Expand Up @@ -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()
Expand Down
98 changes: 44 additions & 54 deletions tests/basilisp/compiler_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)))
Expand Down Expand Up @@ -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"]
Loading