"Extra args dropped" was too lenient. Throw error instead

[erights]

At https://github.com/endojs/endo/pull/1764 I raise three alternatives

• Fix only method guards, so that they reject extra args by default, by changing how it delegates to splitArray. This "solution" would not change the semantics of splitArray at all.
• Grandfather in exactly the current tolerant behavior of all three: method guards, splitArray, and splitRecord. This tolerant behavior of methodGuards can be rationalized as reflecting the tolerant behavior of JS positional function parameters.
• Preserve the tolerant behavior, but not pass the values of the extra args through to the protected raw behavior method. This would preserve the input validation intent of method guards while still accommodating API growth and thus version slippage. (suggested by @turadg )

At https://github.com/endojs/endo/pull/1765 I implemented and merged the third option. Experience since then has shown that this behavior is too lenient, in that it is still too likely to mask programmer bugs.

Rather, we need to implement/enforce the first option, where unexpected extra args cause an error. Any method that does want to be tolerant of extra args would them need to say so explicitly with a .rest(M.any()).

We need to explore the consequences with any customers that may have accidentally written code that depends on the current too-lenient behavior, so that their interfaces become more explicit and thereby enforce stricter input validation.

[mhofman]

Experience since then has shown that this behavior is too lenient, in that it is still too likely to mask programmer bugs.

Rather, we need to implement/enforce the first option, where unexpected extra args cause an error. Any method that does want to be tolerant of extra args would them need to say so explicitly with a .rest(M.any()).

I have been on the other side of that coin, where I would have gotten stuck in a bad situation had the method not allowed extra arguments. I'd argue that such programmer errors do not need to result in runtime errors, but that we need typing inference to work sufficiently to bring up these errors.

[erights]

The current example remains a good test case. Given the mistake that was made, to reliably recover from this mistake, we must not allow erroneous cases to silently do the wrong thing.

As for reliable help from static types, I remain open to the possibility. Unlike TS in general, the TS types inferred from guards and patterns can be (and probably will be) sound, so there's hope. But I'll proceed with this "throw error" plan until I understand how some alternative works reliably.

[turadg]

must not allow erroneous cases to silently do the wrong thing

Agree. However I think static type checking is sufficient noise (and doesn't risk runtime breakage).

I'll proceed with this "throw error" plan until I understand how some alternative works reliably.

I suppose we could throw for now and relax if warranted, but we'd be risking compatibility pain that @mhofman brings up.

Produce static type for guards is not a research problem, just a matter of prioritization.

• https://github.com/Agoric/agoric-sdk/issues/6160
• https://github.com/Agoric/agoric-sdk/pull/8394

[erights]

What I still don't understand is how sound static types for guards solves this problem.

we'd be risking compatibility pain that @mhofman brings up

Examples would help!

[turadg]

how sound static types for guards solves this problem

Here's an example of how static types would alert.

It acknowledges the peril of a static type not matching the runtime type, and thus not getting any error. As of now I'm convinced that it's better to throw. Maybe @mhofman has more/better examples.

[mhofman]

It acknowledges the peril of a static type not matching the runtime type

How is a static type error that is more strict than runtime check a hazard?

Maybe @mhofman has more/better examples.

A few months ago I walked @markm through what motivated me (and how I discovered the current implementations was too lax): https://github.com/Agoric/agoric-sdk/compare/3b4db93413e3caf11efb0e4aafe67f2ea7c864e5...24e5e6e5e5aca31b9748c170e3ac19e4dc1df4c3

[mhofman]

It acknowledges the peril of a static type not matching the runtime type

How is a static type error that is more strict than runtime check a hazard?

Maybe @mhofman has more/better examples.

A few months ago I walked @markm through what motivated me (and how I discovered the current implementations was too lax): Agoric/agoric-sdk@3b4db93...24e5e6e

Edit: More precisely, the vat code accepts a new optional parameter rref in the inbound implementation, and if present uses it. The host provides that new parameter when calling bridgeInbound, but must do so before the vat has been upgraded. Arguably it is tight coupling we should avoid, but I couldn't find alternative approaches.

[turadg]

How is a static type error that is more strict than runtime check a hazard?

The hazard I was referring to was:

• v1 has no arguments; v2 takes an optional argument
• consumer thinks they're using v2 but they're getting v1
• they call the method with an argument, expecting v2 behavior
• with truncation (current Endo master) the function call succeeds, but with the wrong behavior

[mhofman]

Right, and that is the exact behavior that my branch is (ab)using.