Reject argument with default values before arguments without

[Dridi]

I ran into this while I was testing the robustness of my patch in #2873 (comment) and after some testing it seems that it has never been possible to do something like this:

$Function VOID default_first(STRING first = "default", INT mandatory)

And then call it like this:

debug.default_first(123);

The compile error would look like this:

Message from VCC-compiler:
Argument 'mandatory' missing
('<vcl.inline>' Line 8 Pos 40)
                debug.default_first(123);
---------------------------------------#-

('<vcl.inline>' Line 8 Pos 17) -- (Pos 39)
                debug.default_first(123);
----------------#######################--

My suggestion is to forbid arguments with default values preceding arguments without one in $Function and $Method signatures when [optional] arguments are not involved. I tested this on current master and 4.1 since the feature was introduced in the 4.1.0 release.

[slimhazard]

My suggestion is to forbid arguments with default values preceding arguments without one ...

-1, because we have no way of knowing how many VMODs in the wild would become illegal after such a change. Doing that could create a lot of havoc.

Sometimes that has to happen if it's justified, but I don't think it is in this case, because it really is a user error. If you're not using named arguments, then you're using positional arguments. So if you place an INT in the first position, where a STRING is specified, then of course it's an error.

The failure above can easily be fixed with:

debug.default_first(mandatory=123);

The use of named and positional arguments can probably be better documented (like a lot of our stuff). I'd be +1 for a doc improvement, but not for a change that could cause a lot of unnecessary incompatibilities.

[Dridi]

@slimhazard in general I would agree with your point but I'm not too worried about VMODs relying on this in the wild. I wrote this in the issue description:

after some testing it seems that it has never been possible

[bsdphk]

I think @Dridi is right here, mandatory args should go before optional args, anything else doesn't make sense.

[bsdphk]

I thought this would be an easy one, but after playing around with it all morning, I have to admit that vmod_blob.{encode|decode} fences us in here.

I think their definition is wrong and the optional arguments should be last, I don't think we should break their API, so we are stuck with supporting mandatory after optional arguments.

That leaves us two options: Either we can try to write DWIM code to cope with the kind of example @Dridi brought up, and which would also allow things like blob.decode("foobar") which today fails along the same lines, or we can improve the error messages.

I am not going anywhere near code which heuristically tries to guess which argument you intended to pass to a function, so it will have to be better error messages.