rokucommunity / brighterscript

A superset of Roku's BrightScript language
MIT License
152 stars 47 forks source link

Make callfuncs safer #1019

Open TwitchBronBron opened 5 months ago

TwitchBronBron commented 5 months ago

Callfuncs with mismatched arguments will silently fail (not even a console warning!).

To mitigate this, we should automatically append _p = invalid parameters to all callfunc-exposed functions for all components. That way, the functions will be much more likely to execute and cause a runtime error instead (so you can actually see what went wrong).

Key points:

For example,

<component name="TestComponent">
    <interface>
        <function name="doSomething" />
    </interface>
</component>
function doSomething()
end function

would transpile to

function doSomething(__unused = invalid, __unused = invalid, __unused = invalid, __unused = invalid, __unused = invalid, __unused = invalid, __unused = invalid, __unused = invalid, __unused = invalid, __unused = invalid)
end function

BrightScript doesn't care if you use the same parameter name multiple times, so we can reduce the list of unused variables by naming it __unused over and over again.

This implementation will cause the code to fail INSIDE the function if you try to use any of the parameters.

luis-soares-sky commented 5 months ago

Having more than 5 parameters in a callFunc (other than the function name) makes my 4670X crash, so it's likely that 10 might be too many 😄

TwitchBronBron commented 5 months ago

Hmm. The limitation used to be 5, but I think in RokuOS 12.0 or 12.5 they increased the limit to 10. What device and OS version are you running?

TwitchBronBron commented 5 months ago

Oh, also, here are some additional findings about callfunc functions:

normal brightscript functions can be defined with up to 63 parameters. Adding the 64th parameter will cause a compile error. This seems to work as far back as 11.5 callfuncs are limited to 10 args. However, the underlying function can have up to 63 total parameters, as long as params 11-63 have defaults.

So that said, doing this up to 10 parameters should still be safe, it's just that your device will crash after 5 being passed. But having them defined on the function is still safe.

luis-soares-sky commented 5 months ago

Ah shoot, you're right. I wasn't aware they increased the limit, my bad.

elv-stav commented 5 months ago

This has the added benefit of functions with 0 args not silently failing on older rOS and forcing devs to add _ = invalid just to have a single arg. +1 from me