From e9ef08b638685d3683bad0a491b30ab8a1aa1bce Mon Sep 17 00:00:00 2001 From: ikappaki Date: Sun, 30 Mar 2025 19:19:49 +0100 Subject: [PATCH 1/7] Maintain original fn argument names when fn is def'ined --- src/basilisp/lang/compiler/generator.py | 20 ++++++++-- tests/basilisp/compiler_test.py | 50 +++++++++++++++++++++++++ 2 files changed, 66 insertions(+), 4 deletions(-) diff --git a/src/basilisp/lang/compiler/generator.py b/src/basilisp/lang/compiler/generator.py index 87a4a184..13342a3b 100644 --- a/src/basilisp/lang/compiler/generator.py +++ b/src/basilisp/lang/compiler/generator.py @@ -1640,16 +1640,26 @@ def __fn_name(ctx: GeneratorContext, s: Optional[str]) -> str: def __fn_args_to_py_ast( - ctx: GeneratorContext, params: Iterable[Binding], body: Do + ctx: GeneratorContext, + params: Iterable[Binding], + body: Do, + globalize_param_names: bool = True, ) -> tuple[list[ast.arg], Optional[ast.arg], list[ast.stmt], Iterable[PyASTNode]]: - """Generate a list of Python AST nodes from function method parameters.""" + """Generate a list of Python AST nodes from function method parameters. + + Parameter names are munged and modified to ensure global + uniqueness by default. If `globalize_param_names` is set to + False, the original munged parameter names are retained. + """ fn_args, varg = [], None fn_body_ast: list[ast.stmt] = [] fn_def_deps: list[PyASTNode] = [] for binding in params: assert binding.init is None, ":fn nodes cannot have binding :inits" assert varg is None, "Must have at most one variadic arg" - arg_name = genname(munge(binding.name)) + arg_name = munge(binding.name) + if globalize_param_names: + arg_name = genname(arg_name) arg_tag: Optional[ast.expr] if ( @@ -1786,8 +1796,10 @@ def __single_arity_fn_to_py_ast( # pylint: disable=too-many-locals sym.symbol(lisp_fn_name), py_fn_name, LocalType.FN ) + # maintain original parameter names if function is def'd. + globalize_param_names = def_name is None fn_args, varg, fn_body_ast, fn_def_deps = __fn_args_to_py_ast( - ctx, method.params, method.body + ctx, method.params, method.body, globalize_param_names=globalize_param_names ) meta_deps, meta_decorators = __fn_meta(ctx, meta_node) diff --git a/tests/basilisp/compiler_test.py b/tests/basilisp/compiler_test.py index e409300f..d2eff34e 100644 --- a/tests/basilisp/compiler_test.py +++ b/tests/basilisp/compiler_test.py @@ -7186,3 +7186,53 @@ 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): + # Single arity function `def`'initions preserve their original + # parameter names when compiled down to Python. + code = """ + (defn test_dfn1 [a b] 5) + """ + fvar = lcompile(code) + args = list(inspect.signature(fvar.deref()).parameters.keys()) + assert args == ["a", "b"] + + code = """ + (def 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 (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 test_dfn4 [a b {:as xyz}] 5) + """ + fvar = lcompile(code) + args = list(inspect.signature(fvar.deref()).parameters.keys()) + assert args == ["a", "b", "xyz"] + + # Note: For other function definitions, the generated parameter + # names are an internal implementation detail and could change at + # any time. + # + # For example, notice how anonymous functions generate parameter + # names in the pattern + # _ to ensure + # global uniqueness. + code = """ + (fn [a b] 5) + """ + fn = lcompile(code) + args = list(inspect.signature(fn).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}" From 1d0ca297ceaecca4650d95752d0cb6cca27ff71d Mon Sep 17 00:00:00 2001 From: ikappaki Date: Sun, 30 Mar 2025 19:55:08 +0100 Subject: [PATCH 2/7] added changelog entry --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1758b059..215d87d3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ## [Unreleased] +### Changed + * Preserve original argument names in Python compiled code for `def`-defined functions (#1212) ## [v0.3.7] ### Fixed From be9e18a716337a6789e8fa5b206a33b55ac5a947 Mon Sep 17 00:00:00 2001 From: ikappaki Date: Mon, 31 Mar 2025 07:13:13 +0100 Subject: [PATCH 3/7] mention single-arity in changelog --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 215d87d3..14d06947 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,7 +6,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] ### Changed - * Preserve original argument names in Python compiled code for `def`-defined functions (#1212) + * Preserve original argument names in Python compiled code for `def`-defined single-arity functions (#1212) ## [v0.3.7] ### Fixed From 513a4ea5c0969ef6f8e4c9c9f26667169406442f Mon Sep 17 00:00:00 2001 From: ikappaki Date: Mon, 31 Mar 2025 07:22:48 +0100 Subject: [PATCH 4/7] empty line [skip ci] --- src/basilisp/lang/compiler/generator.py | 1 + 1 file changed, 1 insertion(+) diff --git a/src/basilisp/lang/compiler/generator.py b/src/basilisp/lang/compiler/generator.py index 13342a3b..02bea5d7 100644 --- a/src/basilisp/lang/compiler/generator.py +++ b/src/basilisp/lang/compiler/generator.py @@ -1798,6 +1798,7 @@ def __single_arity_fn_to_py_ast( # pylint: disable=too-many-locals # maintain original parameter names if function is def'd. globalize_param_names = def_name is None + fn_args, varg, fn_body_ast, fn_def_deps = __fn_args_to_py_ast( ctx, method.params, method.body, globalize_param_names=globalize_param_names ) From 9247439d3b1bf062130491f9b4d2c1a371195cb0 Mon Sep 17 00:00:00 2001 From: ikappaki Date: Wed, 9 Apr 2025 07:02:26 +0100 Subject: [PATCH 5/7] introduce ^:allow-unsage-names to retain fn param names --- CHANGELOG.md | 2 +- docs/compiler.rst | 2 +- docs/pyinterop.rst | 10 ++++- src/basilisp/lang/compiler/constants.py | 1 + src/basilisp/lang/compiler/generator.py | 35 +++++++++++---- tests/basilisp/compiler_test.py | 57 +++++++++++++++---------- 6 files changed, 72 insertions(+), 35 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 14d06947..0009f43e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,7 +6,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] ### Changed - * Preserve original argument names in Python compiled code for `def`-defined single-arity functions (#1212) + * Single arity functions can be tagged with `^:allow-unsafe-names` to preserve their parameter names (#1212) ## [v0.3.7] ### Fixed diff --git a/docs/compiler.rst b/docs/compiler.rst index 746a99fb..4475e53b 100644 --- a/docs/compiler.rst +++ b/docs/compiler.rst @@ -205,4 +205,4 @@ The former can be configured via the environment variable ``BASILISP_USE_DEV_LOG .. code-block:: bash export BASILISP_USE_DEV_LOGGER=true - export BASILISP_LOGGING_LEVEL=INFO \ No newline at end of file + export BASILISP_LOGGING_LEVEL=INFO diff --git a/docs/pyinterop.rst b/docs/pyinterop.rst index 29d8ef18..1e09c2a4 100644 --- a/docs/pyinterop.rst +++ b/docs/pyinterop.rst @@ -10,7 +10,7 @@ Basilisp features myriad options for interfacing with host Python code. Name Munging ------------ -Per Python's `PEP 8 naming conventions `_, Python method and function names frequently use ``snake_case``. +Per Python's `PEP 8 naming conventions `_, Python method and function and parameter names frequently use ``snake_case``. Basilisp is certainly capable of reading ``snake_case`` names without any special affordance. However, Basilisp code (like many Lisps) tends to prefer ``kebab-case`` for word separation. @@ -22,6 +22,14 @@ 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 diff --git a/src/basilisp/lang/compiler/constants.py b/src/basilisp/lang/compiler/constants.py index 21a084ca..d99c25f8 100644 --- a/src/basilisp/lang/compiler/constants.py +++ b/src/basilisp/lang/compiler/constants.py @@ -37,6 +37,7 @@ class SpecialForm: SYM_ABSTRACT_META_KEY = kw.keyword("abstract") SYM_ABSTRACT_MEMBERS_META_KEY = kw.keyword("abstract-members") +SYM_ALLOW_UNSAFE_NAMES = kw.keyword("allow-unsafe-names") SYM_ASYNC_META_KEY = kw.keyword("async") SYM_KWARGS_META_KEY = kw.keyword("kwargs") SYM_PRIVATE_META_KEY = kw.keyword("private") diff --git a/src/basilisp/lang/compiler/generator.py b/src/basilisp/lang/compiler/generator.py index 02bea5d7..c11aa94b 100644 --- a/src/basilisp/lang/compiler/generator.py +++ b/src/basilisp/lang/compiler/generator.py @@ -37,6 +37,7 @@ INTERFACE_KW, OPERATOR_ALIAS, REST_KW, + SYM_ALLOW_UNSAFE_NAMES, SYM_DYNAMIC_META_KEY, SYM_REDEF_META_KEY, VAR_IS_PROTOCOL_META_KEY, @@ -896,7 +897,9 @@ def _def_to_py_ast( # pylint: disable=too-many-locals assert node.init is not None # silence MyPy if node.init.op == NodeOp.FN: assert isinstance(node.init, Fn) - def_ast = _fn_to_py_ast(ctx, node.init, def_name=defsym.name) + def_ast = _fn_to_py_ast( + ctx, node.init, def_name=defsym.name, meta_node=node.meta + ) is_defn = True elif ( node.init.op == NodeOp.WITH_META @@ -1643,13 +1646,14 @@ def __fn_args_to_py_ast( ctx: GeneratorContext, params: Iterable[Binding], body: Do, - globalize_param_names: bool = True, + allow_unsafe_param_names: bool = True, ) -> tuple[list[ast.arg], Optional[ast.arg], list[ast.stmt], Iterable[PyASTNode]]: """Generate a list of Python AST nodes from function method parameters. Parameter names are munged and modified to ensure global - uniqueness by default. If `globalize_param_names` is set to - False, the original munged parameter names are retained. + uniqueness by default. If `allow_unsafe_param_names` is set to + True, the original munged parameter names are retained instead. + """ fn_args, varg = [], None fn_body_ast: list[ast.stmt] = [] @@ -1658,7 +1662,7 @@ 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 globalize_param_names: + if not allow_unsafe_param_names: arg_name = genname(arg_name) arg_tag: Optional[ast.expr] @@ -1780,7 +1784,14 @@ def __single_arity_fn_to_py_ast( # pylint: disable=too-many-locals def_name: Optional[str] = None, meta_node: Optional[MetaNode] = None, ) -> GeneratedPyAST[ast.expr]: - """Return a Python AST node for a function with a single arity.""" + """Return a Python AST node for a function with a single arity. + + By default, parameter names are globally uniquified to support + inlining of Basilisp functions. If the `meta_node` contains + `SYM_ALLOW_UNSAFE_NAMES` set to True, the original (munged) + parameter names are retained instead. + + """ assert node.op == NodeOp.FN assert method.op == NodeOp.FN_ARITY @@ -1796,11 +1807,17 @@ def __single_arity_fn_to_py_ast( # pylint: disable=too-many-locals sym.symbol(lisp_fn_name), py_fn_name, LocalType.FN ) - # maintain original parameter names if function is def'd. - globalize_param_names = def_name is None + # check if we should preserve the original parameter names + allow_unsafe_param_names = ( + meta_node is not None + and meta_node.form.val_at(SYM_ALLOW_UNSAFE_NAMES) is True + ) fn_args, varg, fn_body_ast, fn_def_deps = __fn_args_to_py_ast( - ctx, method.params, method.body, globalize_param_names=globalize_param_names + ctx, + method.params, + method.body, + allow_unsafe_param_names=allow_unsafe_param_names, ) meta_deps, meta_decorators = __fn_meta(ctx, meta_node) diff --git a/tests/basilisp/compiler_test.py b/tests/basilisp/compiler_test.py index d2eff34e..94d6dbf7 100644 --- a/tests/basilisp/compiler_test.py +++ b/tests/basilisp/compiler_test.py @@ -7189,50 +7189,61 @@ def test_yield_as_coroutine_with_multiple_yields( def test_defn_argument_names(lcompile: CompileFn): - # Single arity function `def`'initions preserve their original - # parameter names when compiled down to Python. + # By default, function parameter names are made globally unique. + # + # For example, notice how defn generate parameter names in the + # pattern _ to + # ensure global uniqueness. code = """ - (defn test_dfn1 [a b] 5) + (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 = """ - (def test_dfn2 (fn [a b & c] 5)) + (defn ^{:allow-unsafe-names false} test_dfn1b [a b] 5) """ fvar = lcompile(code) args = list(inspect.signature(fvar.deref()).parameters.keys()) - assert args == ["a", "b", "c"] + 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 test_dfn1 {:allow-unsafe-names true} [a b] 5) + """ + fvar = lcompile(code) + args = list(inspect.signature(fvar.deref()).parameters.keys()) + assert args == ["a", "b"] code = """ - (def test_dfn3 (fn [a b & c] 5)) + (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 = """ - ^{:kwargs :collect} - (defn test_dfn4 [a b {:as xyz}] 5) + (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", "xyz"] + assert args == ["a", "b", "c"] - # Note: For other function definitions, the generated parameter - # names are an internal implementation detail and could change at - # any time. - # - # For example, notice how anonymous functions generate parameter - # names in the pattern - # _ to ensure - # global uniqueness. code = """ - (fn [a b] 5) + ^{:kwargs :collect} + (defn ^:allow-unsafe-names test_dfn4 [a b {:as xyz}] 5) """ - fn = lcompile(code) - args = list(inspect.signature(fn).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}" + fvar = lcompile(code) + args = list(inspect.signature(fvar.deref()).parameters.keys()) + assert args == ["a", "b", "xyz"] From 374da7126a3baaca8e3914715dd47cbfcaf08cf0 Mon Sep 17 00:00:00 2001 From: ikappaki Date: Wed, 9 Apr 2025 17:49:49 +0100 Subject: [PATCH 6/7] fix type issue, refactor to fn --- src/basilisp/lang/compiler/constants.py | 2 +- src/basilisp/lang/compiler/generator.py | 36 +++++++++++++------------ 2 files changed, 20 insertions(+), 18 deletions(-) diff --git a/src/basilisp/lang/compiler/constants.py b/src/basilisp/lang/compiler/constants.py index d99c25f8..4ecea5b0 100644 --- a/src/basilisp/lang/compiler/constants.py +++ b/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 = kw.keyword("allow-unsafe-names") +SYM_ALLOW_UNSAFE_NAMES_META_KEY = kw.keyword("allow-unsafe-names") SYM_ASYNC_META_KEY = kw.keyword("async") SYM_KWARGS_META_KEY = kw.keyword("kwargs") SYM_PRIVATE_META_KEY = kw.keyword("private") diff --git a/src/basilisp/lang/compiler/generator.py b/src/basilisp/lang/compiler/generator.py index c11aa94b..22a28946 100644 --- a/src/basilisp/lang/compiler/generator.py +++ b/src/basilisp/lang/compiler/generator.py @@ -37,7 +37,7 @@ INTERFACE_KW, OPERATOR_ALIAS, REST_KW, - SYM_ALLOW_UNSAFE_NAMES, + SYM_ALLOW_UNSAFE_NAMES_META_KEY, SYM_DYNAMIC_META_KEY, SYM_REDEF_META_KEY, VAR_IS_PROTOCOL_META_KEY, @@ -111,7 +111,7 @@ ast_ClassDef, ast_FunctionDef, ) -from basilisp.lang.interfaces import IMeta, ISeq +from basilisp.lang.interfaces import IMeta, IPersistentMap, ISeq from basilisp.lang.runtime import CORE_NS from basilisp.lang.runtime import NS_VAR_NAME as LISP_NS_VAR from basilisp.lang.runtime import BasilispModule, Var @@ -650,6 +650,21 @@ def with_lineno_and_col( return 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. + + """ + return ( + bool(fn_meta_node.form.val_at(SYM_ALLOW_UNSAFE_NAMES_META_KEY, False)) is True + if fn_meta_node is not None and isinstance(fn_meta_node.form, IPersistentMap) + else False + ) + + def _is_dynamic(v: Var) -> bool: """Return True if the Var holds a value which should be compiled to a dynamic Var access.""" @@ -1625,9 +1640,6 @@ def _synthetic_do_to_py_ast( ) -MetaNode = Union[Const, MapNode] - - def __fn_name(ctx: GeneratorContext, s: Optional[str]) -> str: """Generate a safe Python function name from a function name symbol. @@ -1784,14 +1796,7 @@ def __single_arity_fn_to_py_ast( # pylint: disable=too-many-locals def_name: Optional[str] = None, meta_node: Optional[MetaNode] = None, ) -> GeneratedPyAST[ast.expr]: - """Return a Python AST node for a function with a single arity. - - By default, parameter names are globally uniquified to support - inlining of Basilisp functions. If the `meta_node` contains - `SYM_ALLOW_UNSAFE_NAMES` set to True, the original (munged) - parameter names are retained instead. - - """ + """Return a Python AST node for a function with a single arity.""" assert node.op == NodeOp.FN assert method.op == NodeOp.FN_ARITY @@ -1808,10 +1813,7 @@ def __single_arity_fn_to_py_ast( # pylint: disable=too-many-locals ) # check if we should preserve the original parameter names - allow_unsafe_param_names = ( - meta_node is not None - and meta_node.form.val_at(SYM_ALLOW_UNSAFE_NAMES) is True - ) + 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, From 9041747871eac74ce37e6cd923796fac34fce0d1 Mon Sep 17 00:00:00 2001 From: ikappaki Date: Wed, 9 Apr 2025 18:04:16 +0100 Subject: [PATCH 7/7] remove redudent test case --- tests/basilisp/compiler_test.py | 7 ------- 1 file changed, 7 deletions(-) diff --git a/tests/basilisp/compiler_test.py b/tests/basilisp/compiler_test.py index 94d6dbf7..6e72db39 100644 --- a/tests/basilisp/compiler_test.py +++ b/tests/basilisp/compiler_test.py @@ -7219,13 +7219,6 @@ def test_defn_argument_names(lcompile: CompileFn): re.fullmatch(p, s) for p, s in zip([r"a_\d+", r"b_\d+"], args) ), f"unexpected argument names {args}" - code = """ - (defn test_dfn1 {:allow-unsafe-names true} [a b] 5) - """ - fvar = lcompile(code) - args = list(inspect.signature(fvar.deref()).parameters.keys()) - assert args == ["a", "b"] - code = """ (def ^:allow-unsafe-names test_dfn2 (fn [a b & c] 5)) """