Warn at compile time on arity mismatch for function invocation (#842)

Add a compile-time warning for arity mismatches on function invocations.
The warning is enabled by default (if the dev logger is enabled and
configured for at least `WARNING` level logs).

In order to facilitate this work, we also had to change the computation
of the Basilisp function `arities` attribute set. Previously this
included only fixed arities (the argument count for each non-variadic
function arity) and a `:rest` keyword if the function included a
variadic arity. Now `arities` will include all fixed arities including
the number of fixed (non-variadic) arguments to the varidic arity. This
allows for more sophisticated warnings.

As part of this bill of work, it was also required to update `partial`
to support computing a new set of `arities` for partial applications.
`functools.wraps` copies all values from `__dict__` to wrapped
functions, so partials were appearing to have the same set of arities as
their wrapped function which was never correct.

Fixes #671 
Fixes #847

Author: chrisrink10

CHANGELOG.md

@@ -7,9 +7,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ## [Unreleased]
 ### Added
  * Added filename metadata to compiler exceptions (#844)
+ * Added a compile-time warning for attempting to call a function with an unsupported number of arguments (#671)
 
 ### Fixed
  * Fix a bug where `basilisp.lang.compiler.exception.CompilerException` would nearly always suppress line information in it's `data` map (#845)
+ * Fix a bug where the function returned by `partial` retained the meta, arities, and `with_meta` method of the wrapped function rather than creating new ones (#847)
 
 ## [v0.1.0b1]
 ### Added

docs/compiler.rst

@@ -35,6 +35,12 @@ Warnings
 
 The following settings enable and disable warnings from the Basilisp compiler during compilation.
 
+* ``warn-on-arity-mismatch`` - if ``true``, emit warnings if a Basilisp function invocation is detected with an unsupported number of arguments
+
+  * Environment Variable: ``BASILISP_WARN_ON_ARITY_MISMATCH``
+  * Default: ``true``
+
+
 * ``warn-on-shadowed-name`` - if ``true``, emit warnings if a local name is shadowed by another local name
 
   * Environment Variable: ``BASILISP_WARN_ON_SHADOWED_NAME``

src/basilisp/cli.py

@@ -136,6 +136,19 @@ def _add_compiler_arg_group(parser: argparse.ArgumentParser) -> None:
             f"{DEFAULT_COMPILER_OPTS['inline-functions']})"
         ),
     )
+    group.add_argument(
+        "--warn-on-arity-mismatch",
+        action="store",
+        nargs="?",
+        const=os.getenv("BASILISP_WARN_ON_ARITY_MISMATCH"),
+        type=_to_bool,
+        help=(
+            "if true, emit warnings if a Basilisp function invocation is detected with "
+            "an unsupported number of arguments "
+            "(env: BASILISP_WARN_ON_ARITY_MISMATCH; default: "
+            f"{DEFAULT_COMPILER_OPTS['warn-on-arity-mismatch']})"
+        ),
+    )
     group.add_argument(
         "--warn-on-shadowed-name",
         action="store",

src/basilisp/contrib/pytest/testrunner.py

@@ -30,7 +30,7 @@ def pytest_collect_file(file_path: 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"):
-            return BasilispFile.from_parent(parent, fspath=path, path=file_path)
+            return BasilispFile.from_parent(parent, path=file_path)
     return None
 
 

src/basilisp/lang/compiler/__init__.py

@@ -10,6 +10,7 @@
 from basilisp.lang.compiler.analyzer import (  # noqa
     GENERATE_AUTO_INLINES,
     INLINE_FUNCTIONS,
+    WARN_ON_ARITY_MISMATCH,
     WARN_ON_NON_DYNAMIC_SET,
     WARN_ON_SHADOWED_NAME,
     WARN_ON_SHADOWED_VAR,
@@ -32,6 +33,7 @@
 from basilisp.lang.compiler.optimizer import PythonASTOptimizer
 from basilisp.lang.typing import CompilerOpts, ReaderForm
 from basilisp.lang.util import genname
+from basilisp.util import Maybe
 
 _DEFAULT_FN = "__lisp_expr__"
 
@@ -97,6 +99,7 @@ def py_ast_optimizer(self) -> PythonASTOptimizer:
 def compiler_opts(  # pylint: disable=too-many-arguments
     generate_auto_inlines: Optional[bool] = None,
     inline_functions: Optional[bool] = None,
+    warn_on_arity_mismatch: Optional[bool] = None,
     warn_on_shadowed_name: Optional[bool] = None,
     warn_on_shadowed_var: Optional[bool] = None,
     warn_on_unused_names: Optional[bool] = None,
@@ -108,15 +111,16 @@ def compiler_opts(  # pylint: disable=too-many-arguments
     return lmap.map(
         {
             # Analyzer options
-            GENERATE_AUTO_INLINES: generate_auto_inlines or True,
-            INLINE_FUNCTIONS: inline_functions or True,
-            WARN_ON_SHADOWED_NAME: warn_on_shadowed_name or False,
-            WARN_ON_SHADOWED_VAR: warn_on_shadowed_var or False,
-            WARN_ON_UNUSED_NAMES: warn_on_unused_names or True,
-            WARN_ON_NON_DYNAMIC_SET: warn_on_non_dynamic_set or True,
+            GENERATE_AUTO_INLINES: Maybe(generate_auto_inlines).or_else_get(True),
+            INLINE_FUNCTIONS: Maybe(inline_functions).or_else_get(True),
+            WARN_ON_ARITY_MISMATCH: Maybe(warn_on_arity_mismatch).or_else_get(True),
+            WARN_ON_SHADOWED_NAME: Maybe(warn_on_shadowed_name).or_else_get(False),
+            WARN_ON_SHADOWED_VAR: Maybe(warn_on_shadowed_var).or_else_get(False),
+            WARN_ON_UNUSED_NAMES: Maybe(warn_on_unused_names).or_else_get(True),
+            WARN_ON_NON_DYNAMIC_SET: Maybe(warn_on_non_dynamic_set).or_else_get(True),
             # Generator options
-            USE_VAR_INDIRECTION: use_var_indirection or False,
-            WARN_ON_VAR_INDIRECTION: warn_on_var_indirection or True,
+            USE_VAR_INDIRECTION: Maybe(use_var_indirection).or_else_get(False),
+            WARN_ON_VAR_INDIRECTION: Maybe(warn_on_var_indirection).or_else_get(True),
         }
     )
 

src/basilisp/lang/compiler/analyzer.py

@@ -54,6 +54,7 @@
     LINE_KW,
     NAME_KW,
     NS_KW,
+    REST_KW,
     SYM_ABSTRACT_META_KEY,
     SYM_ASYNC_META_KEY,
     SYM_CLASSMETHOD_META_KEY,
@@ -145,6 +146,7 @@
 # Analyzer options
 GENERATE_AUTO_INLINES = kw.keyword("generate-auto-inlines")
 INLINE_FUNCTIONS = kw.keyword("inline-functions")
+WARN_ON_ARITY_MISMATCH = kw.keyword("warn-on-arity-mismatch")
 WARN_ON_SHADOWED_NAME = kw.keyword("warn-on-shadowed-name")
 WARN_ON_SHADOWED_VAR = kw.keyword("warn-on-shadowed-var")
 WARN_ON_UNUSED_NAMES = kw.keyword("warn-on-unused-names")
@@ -362,6 +364,12 @@ def should_inline_functions(self) -> bool:
         """If True, function calls may be inlined if an inline def is provided."""
         return self._opts.val_at(INLINE_FUNCTIONS, True)
 
+    @property
+    def warn_on_arity_mismatch(self) -> bool:
+        """If True, warn when a Basilisp function invocation is detected with an
+        unsupported number of arguments."""
+        return self._opts.val_at(WARN_ON_ARITY_MISMATCH, True)
+
     @property
     def warn_on_unused_names(self) -> bool:
         """If True, warn when local names are unused."""
@@ -2456,6 +2464,40 @@ def __handle_macroexpanded_ast(
     )
 
 
+def _do_warn_on_arity_mismatch(
+    fn: VarRef, form: Union[llist.PersistentList, ISeq], ctx: AnalyzerContext
+) -> None:
+    if ctx.warn_on_arity_mismatch and getattr(fn.var.value, "_basilisp_fn", False):
+        arities: Optional[Tuple[Union[int, kw.Keyword]]] = getattr(
+            fn.var.value, "arities", None
+        )
+        if arities is not None:
+            has_variadic = REST_KW in arities
+            fixed_arities = set(filter(lambda v: v != REST_KW, arities))
+            max_fixed_arity = max(fixed_arities) if fixed_arities else None
+            # This count could be off by 1 for cases where kwargs are being passed,
+            # but only Basilisp functions intended to be called by Python code
+            # (e.g. with a :kwargs strategy) should ever be called with kwargs,
+            # so this seems unlikely enough.
+            num_args = runtime.count(form.rest)
+            if has_variadic and (max_fixed_arity is None or num_args > max_fixed_arity):
+                return
+            if num_args not in fixed_arities:
+                report_arities = cast(Set[Union[int, str]], set(fixed_arities))
+                if has_variadic:
+                    report_arities.discard(cast(int, max_fixed_arity))
+                    report_arities.add(f"{max_fixed_arity}+")
+                loc = (
+                    f" ({fn.env.file}:{fn.env.line})"
+                    if fn.env.line is not None
+                    else f" ({fn.env.file})"
+                )
+                logger.warning(
+                    f"calling function {fn.var}{loc} with {num_args} arguments; "
+                    f"expected any of: {', '.join(sorted(map(str, report_arities)))}",
+                )
+
+
 def _invoke_ast(form: Union[llist.PersistentList, ISeq], ctx: AnalyzerContext) -> Node:
     with ctx.expr_pos():
         fn = _analyze_form(form.first, ctx)
@@ -2492,6 +2534,8 @@ def _invoke_ast(form: Union[llist.PersistentList, ISeq], ctx: AnalyzerContext) -
                     phase=CompilerPhase.INLINING,
                 ) from e
 
+        _do_warn_on_arity_mismatch(fn, form, ctx)
+
     args, kwargs = _call_args_ast(form.rest, ctx)
     return Invoke(
         form=form,

src/basilisp/lang/compiler/constants.py

@@ -52,6 +52,8 @@ class SpecialForm:
 SYM_TAG_META_KEY = kw.keyword("tag")
 
 ARGLISTS_KW = kw.keyword("arglists")
+INTERFACE_KW = kw.keyword("interface")
+REST_KW = kw.keyword("rest")
 COL_KW = kw.keyword("col")
 DOC_KW = kw.keyword("doc")
 FILE_KW = kw.keyword("file")

src/basilisp/lang/compiler/generator.py

@@ -45,6 +45,8 @@
 from basilisp.lang import vector as vec
 from basilisp.lang.compiler.constants import (
     DEFAULT_COMPILER_FILE_PATH,
+    INTERFACE_KW,
+    REST_KW,
     SYM_DYNAMIC_META_KEY,
     SYM_NO_WARN_ON_REDEF_META_KEY,
     SYM_REDEF_META_KEY,
@@ -132,10 +134,6 @@
 _TRY_PREFIX = "lisp_try"
 _NS_VAR = "_NS"
 
-# Keyword constants used in generating code
-_INTERFACE_KW = kw.keyword("interface")
-_REST_KW = kw.keyword("rest")
-
 
 @attr.frozen
 class SymbolTableEntry:
@@ -209,7 +207,7 @@ class RecurType(Enum):
 class RecurPoint:
     loop_id: str
     type: RecurType
-    binding_names: Optional[Collection[str]] = None
+    binding_names: Optional[Iterable[str]] = None
     is_variadic: Optional[bool] = None
     has_recur: bool = False
 
@@ -273,15 +271,15 @@ def has_var_indirection_override(self) -> bool:
             return False
 
     @property
-    def recur_point(self):
+    def recur_point(self) -> RecurPoint:
         return self._recur_points[-1]
 
     @contextlib.contextmanager
     def new_recur_point(
         self,
         loop_id: str,
         type_: RecurType,
-        binding_names: Optional[Collection[str]] = None,
+        binding_names: Optional[Iterable[str]] = None,
         is_variadic: Optional[bool] = None,
     ):
         self._recur_points.append(
@@ -305,7 +303,7 @@ def new_symbol_table(self, name: str, is_context_boundary: bool = False):
             self._st.pop()
 
     @property
-    def current_this(self):
+    def current_this(self) -> sym.Symbol:
         return self._this[-1]
 
     @contextlib.contextmanager
@@ -1417,7 +1415,7 @@ def __deftype_or_reify_bases_to_py_ast(
                         ast.Call(
                             func=_NEW_KW_FN_NAME,
                             args=[
-                                ast.Constant(hash(_INTERFACE_KW)),
+                                ast.Constant(hash(INTERFACE_KW)),
                                 ast.Constant("interface"),
                             ],
                             keywords=[],
@@ -1695,7 +1693,7 @@ def __fn_decorator(
                                     ast.Call(
                                         func=_NEW_KW_FN_NAME,
                                         args=[
-                                            ast.Constant(hash(_REST_KW)),
+                                            ast.Constant(hash(REST_KW)),
                                             ast.Constant("rest"),
                                         ],
                                         keywords=[],
@@ -1810,11 +1808,7 @@ def __single_arity_fn_to_py_ast(  # pylint: disable=too-many-locals
                                     meta_decorators,
                                     [
                                         __fn_decorator(
-                                            (
-                                                (len(fn_args),)
-                                                if not method.is_variadic
-                                                else ()
-                                            ),
+                                            (len(fn_args),),
                                             has_rest_arg=method.is_variadic,
                                         )
                                     ],
@@ -1847,6 +1841,7 @@ def __multi_arity_dispatch_fn(  # pylint: disable=too-many-arguments,too-many-lo
     arity_map: Mapping[int, str],
     return_tags: Iterable[Optional[Node]],
     default_name: Optional[str] = None,
+    rest_arity_fixed_arity: Optional[int] = None,
     max_fixed_arity: Optional[int] = None,
     meta_node: Optional[MetaNode] = None,
     is_async: bool = False,
@@ -2013,7 +2008,16 @@ def fn(*args):
                             meta_decorators,
                             [
                                 __fn_decorator(
-                                    arity_map.keys(),
+                                    list(
+                                        chain(
+                                            arity_map.keys(),
+                                            (
+                                                [rest_arity_fixed_arity]
+                                                if rest_arity_fixed_arity is not None
+                                                else []
+                                            ),
+                                        )
+                                    ),
                                     has_rest_arg=default_name is not None,
                                 )
                             ],
@@ -2046,6 +2050,7 @@ def __multi_arity_fn_to_py_ast(  # pylint: disable=too-many-locals
 
     arity_to_name = {}
     rest_arity_name: Optional[str] = None
+    rest_arity_fixed_arity: Optional[int] = None
     fn_defs = []
     all_arity_def_deps: List[ast.AST] = []
     for arity in arities:
@@ -2054,6 +2059,7 @@ def __multi_arity_fn_to_py_ast(  # pylint: disable=too-many-locals
         )
         if arity.is_variadic:
             rest_arity_name = arity_name
+            rest_arity_fixed_arity = arity.fixed_arity
         else:
             arity_to_name[arity.fixed_arity] = arity_name
 
@@ -2109,6 +2115,7 @@ def __multi_arity_fn_to_py_ast(  # pylint: disable=too-many-locals
         arity_to_name,
         return_tags=[arity.tag for arity in arities],
         default_name=rest_arity_name,
+        rest_arity_fixed_arity=rest_arity_fixed_arity,
         max_fixed_arity=node.max_fixed_arity,
         meta_node=meta_node,
         is_async=node.is_async,
@@ -2567,6 +2574,7 @@ def __loop_recur_to_py_ast(
 ) -> GeneratedPyAST[ast.expr]:
     """Return a Python AST node for `recur` occurring inside a `loop`."""
     assert node.op == NodeOp.RECUR
+    assert ctx.recur_point.binding_names is not None
 
     recur_deps: List[ast.AST] = []
     recur_targets: List[ast.Name] = []
@@ -3223,7 +3231,6 @@ def _interop_prop_to_py_ast(
     assert node.op == NodeOp.HOST_FIELD
 
     target_ast = gen_py_ast(ctx, node.target)
-    assert not target_ast.dependencies
 
     return GeneratedPyAST(
         node=ast.Attribute(