Fix: make macro function argument resolution more robust

[georgesittas]

Fixes #2599

Prior to this PR, we would bind macro arguments to the corresponding formal parameters only if annotations were present, and the existing logic for that case was brittle and complicated. This PR addresses these points:

• Parameter binding will now always happen, ensuring that macros will get called with the correct arguments
• The binding logic has been offloaded to inspect.Signature.bind, which handles all sorts of edge cases for us that would otherwise be tedious and tricky to implement correctly (see added test cases).

[z3z1ma]

I like the use of BoundArguments here. Makes sense. However we need to use parameters to determine variadic positional args and variadic kwargs. The current check of tuple/dict is not correct.

This will correct the implementation
https://docs.python.org/3/library/inspect.html#inspect.Parameter

You can get Parameters from the Signature object.

[georgesittas]

Hey Alex 👋

I like the use of BoundArguments here. Makes sense. However we need to use parameters to determine variadic positional args and variadic kwargs. The current check of tuple/dict is not correct.

This will correct the implementation https://docs.python.org/3/library/inspect.html#inspect.Parameter

You can get Parameters from the Signature object.

Can you show an example that is incorrectly handled so I can better understand this? In my first iteration I was actually using the parameters attribute of Signature, but I refactored quite a bit of logic to end up with this one.

[georgesittas]

Hm I guess you were talking about this note in the docs?

Should be used in conjunction with Signature.parameters for any argument processing purposes.

I may have missed this one earlier, perhaps there could be ordering issues the way I'm currently accessing the arguments? Will double-check this tomorrow, but in any case - an example would still be appreciated :)

[z3z1ma]

I got you. Give me just a min

[georgesittas]

One bug I found with this approach is the following:

main:

>>> from sqlmesh.core.macros import *
>>> from sqlglot import *
>>>
>>> @macro()
... def foo(evaluator: MacroEvaluator, a1: int = 1, a2: int = exp.Literal.number(2)):
...     return sum([a1, a2])
...
>>> MacroEvaluator().evaluate(parse_one("@foo()")).sql()
'3'

this branch:

>>> from sqlmesh.core.macros import *
>>> from sqlglot import *
>>>
>>> @macro()
... def foo(evaluator: MacroEvaluator, a1: int = 1, a2: int = exp.Literal.number(2)):
...     return sum([a1, a2])
...
>>> MacroEvaluator().evaluate(parse_one("@foo()")).sql()
'1 + 2'

It seems like default arguments are not properly handled in the coercion loop.

[tobymao]

@georgesittas i'm a bit confused, isn't main's behavior correct? if the default value is a literal number, but the type says it's an int, it should be coerced.

[georgesittas]

@georgesittas i'm a bit confused, isn't main's behavior correct? if the default value is a literal number, but the type says it's an int, it should be coerced.

There's no issue with main, this is an issue with this branch. I was replying to Alex's comment above.

[z3z1ma]

Small changes below. cc @georgesittas
I havent ran this, but this is what I had in mind.

def send(
    self, name: str, *args: t.Any, **kwargs: t.Any
) -> t.Union[None, exp.Expression, t.List[exp.Expression]]:
    func = self.macros.get(normalize_macro_name(name))

    if not callable(func):
        raise SQLMeshError(f"Macro '{name}' does not exist.")

    try:
        # Bind the macro's actual parameters to its formal parameters
        sig = inspect.signature(func)
        bound = sig.bind(self, *args, **kwargs)
    except Exception as e:
        print_exception(e, self.python_env)
        raise MacroEvalError("Error trying to eval macro.") from e

    try:
        annotations = t.get_type_hints(func)
    except NameError:  # forward references aren't handled
        annotations = {}

    # If the macro is annotated, we try coerce the actual parameters to the corresponding types
    if annotations:
        for arg, value in bound.arguments.items():
            typ = annotations.get(arg)
            if not typ:
                continue

            # Changes to bound.arguments will reflect in bound.args and bound.kwargs
            # https://docs.python.org/3/library/inspect.html#inspect.BoundArguments.arguments
            param = sig.parameters[arg]
            if param.default is value:
                continue
            elif param.kind is inspect.Parameter.VAR_POSITIONAL:
                bound.arguments[arg] = tuple(self._coerce(v, typ) for v in value)
            elif param.kind is inspect.Parameter.VAR_KEYWORD:
                bound.arguments[arg] = {k: self._coerce(v, typ) for k, v in value.items()}
            else:
                bound.arguments[arg] = self._coerce(value, typ)

    try:
        return func(*bound.args, **bound.kwargs)
    except Exception as e:
        print_exception(e, self.python_env)
        raise MacroEvalError("Error trying to eval macro.") from e

[georgesittas]

Ah I see, makes sense. Thanks!

[georgesittas]

This ensures default values will also be coerced, even when omitted (see latest test case).

[georgesittas]

@z3z1ma refactored the previous approach, feel free to take another look. I thought a bit about the isinstance(value, tuple) and isinstance(value, dict) branches and I'm still not sure if there's a code path where they'd produce incorrect behavior, but I do agree that the suggested approach is more robust / clear. Let me know if you had a particular case in mind that would crash using that approach and I can add another test.

[z3z1ma]

You would have hit that codepath anytime someone asked passes in something that meets that isinstance check and has typing. This includes any arg instead of just variadics. And the top-level annotation was being passed inside the comprehension which could've also been wrong since generic tuples apply the inner types to the tuples values.

EDIT: I think this would've been an example that would've been incorrect with the isinstance code.

def foo(a: t.Tuple[exp.Literal, ...] = (1,2,3)): ...

sig = inspect.signature(foo)
# <Signature (a: Tuple[sqlglot.expressions.Literal, ...] = (1, 2, 3))>

bound.apply_defaults()
# <BoundArguments (a=(1, 2, 3))>

annotations = t.get_type_hints(foo)
(arg, value) = next(iter(bound.arguments.items()))

annotations[arg]
# typing.Tuple[sqlglot.expressions.Literal, ...]

value
# (1, 2, 3)

value is a tuple and passes isinstance check (despite it NOT being a variadic of the form *something), then we tuple(coerce(v, typ) for v in value) but the typ would have been t.Tuple[exp.Literal, ...] which would not have worked or been correct. We should only do that for true variadic args.

So the current version leveraging Parameters and the kind enum handles that.