DML doesn't correctly handle return statements within injected functions

[EvanKirshenbaum]

In DMFCompiler.visitRet(), the return value is processed as

        def run(env: Environment) -> Delayed[MaybeError[Any]]:
            return value.evaluate(env, return_type).transformed(error_check(MacroReturn))

The value is evaluated and on the final check, if it isn't an error (as checked by error_check()), it is wrapped in a MacroReturn object, which is an error and so will propagate, causing the rest of the body to be ignored.

This error is caught and unwrapped in visitFunction_expr():

        def catch_return(val: MaybeError[Any]) -> Any:
            if isinstance(val, MacroReturn):
                return val.value
            # Either it's a fall-off-the-end value, which should only be allowed
            # if it's okay, or it's some other error, which we should propagate.
            return val

        def dispatch(fn: CallableValue, *args: Sequence[Any]) -> Delayed[Any]:
            return fn.apply(args).transformed(catch_return)

This all works fine with f(a,b,c) syntax, but we don't have the same check with injuections, so if a function is injected as a : f, where f returns a value, the MacroReturn "error" will continue to be propagated, rather than being transformed into a value. This has three consequences:

1. Any following code will be ignored. This is most notable in expressions like a : f : g, where the result should be passed to g, but won't be.
2. The error will propagate. If it makes it up to top level, an incorrect error message will be printed.
3. Worse, if the injection is inside a function body, it will be incorrectly taken to be the value of the enclosing function.

The straightforward way to patch this is to put the catch_return logic within visitInjection_expr(). The more robust way is to figure out how to put the logic into (probably) use_callable() to ensure that any MacroReturn value is always unwrapped. (visitInjection_expr() calls use_function(), which in turn calls use_callable().)

Migrated from internal repository. Originally created by @EvanKirshenbaum on Jun 09, 2023 at 1:44 PM PDT. Closed on Jun 09, 2023 at 2:57 PM PDT.

[EvanKirshenbaum]

This issue was referenced by the following commit before migration:

• 612c58f58b4937064ea316674d89ea39d882d9bf by @EvanKirshenbaum on Jun 09, 2023 at 2:54 PM PDT

[EvanKirshenbaum]

On second thought, it is probably safest to put it in MacroValue.apply(), since it's only user-defined functions ("macros") that can have return statements in them. Currently, this is defined as

class MacroValue(CallableValue):
    def apply(self, args:Sequence[Any])->Delayed[Any]:
        bindings = dict(zip(self.param_names, args))
        local_env = self.static_env.new_child(bindings)
        return self.body.evaluate(local_env)

It should (and appears to be) sufficient to change this to

class MacroValue(CallableValue):
    def apply(self, args:Sequence[Any])->Delayed[Any]:
        bindings = dict(zip(self.param_names, args))
        local_env = self.static_env.new_child(bindings)
        def unwrap_return(val: MaybeError[Any]) -> MaybeError[Any]:
            return val.value if isinstance(val, MacroReturn) else val
        return self.body.evaluate(local_env).transformed(unwrap_return)

Having done that, I can simply remove the catch_return() logic from visitFunction_expr().

Migrated from internal repository. Originally created by @EvanKirshenbaum on Jun 09, 2023 at 2:39 PM PDT.