Handle implicit type conversion when calling functions #298
Conversation
These tests will fail and generate the following: ``` <========= Request finished Request Incoming =========> [regsub] Argument 1 expects STRING type but BACKEND provided <========= Request finished Request Incoming =========> [regsub] Argument 1 expects STRING type but TIME provided <========= Request finished ```
This fixes errors like `[regsub] Argument 1 expects STRING type but BACKEND provided` when calling builtin functions and passing a non-string for a string argument. Fastly handles that by implicitly converting the argument to a string: > These types all have implicit conversions to strings, such that their values may be used in contexts where a STRING value is necessary. https://www.fastly.com/documentation/reference/vcl/types/ I wasn't sure the cleanist way of implementing. Originally, I was thinking of doing this conversion in `ProcessFunctionCallExpression()` and `ProcessFunctionCallStatement()` (the two places that call functions) like we're already doing for the `IsIdentArgument()` check, but it needs to know the argument types for every function to do that, which means encoding that information in the `Function` struct.
There was a problem hiding this comment.
Funny I had actually gotten started on this problem due to the exact same thing you mention in the PR description, regsub with a backend value. But I ran a can of worms with edge cases and got distracted with other things and hadn't gotten to submitting a PR yet.
Largely we took the same general approach except I did the type coercion in the various function definition files themselves vs in builtin_functions.go.
On my first look I initially considered your solution a better approach. However as I think more about the various edge case thorns I ran into when working on this I'm now not so sure. My main concern is that by having the coercion happen outside the function definition it separates the argument handling logic from the function implementation itself. This creates cases of unexpected "magic" happening to the arguments before the code in the function implementation sees it. Given the amount of edge cases there are I think it makes sense to have all the logic related to handling the arguments with the implementation of the functions.
For example we have variadic functions (header.filter, early_hints, etc), functions that require literal string arguments (regsub*, header.filter, etc), functions that require expressions of string literals (querystring.filter*), and I wouldn't be surprised if there are others that haven't been found yet.
By keeping the logic of type coercion with the other argument handling logic we make it easier to know where to look when trying to identify all of the details of how arguments are handled for a specific built-in function which is especially important for the edge case functions.
For reference this is the diff for my WIP branch: main...richardmarshall:falco:regsub_backend_type
| if argTypes[i] == value.StringType && args[i].Type() != value.StringType { | ||
| // Handle implicit conversion to string | ||
| // "These types all have implicit conversions to strings, such that their values may be used in contexts where a STRING value is necessary." | ||
| // https://www.fastly.com/documentation/reference/vcl/types/" | ||
| args[i] = &value.String{Value: args[i].String()} | ||
| } |
There was a problem hiding this comment.
The type coercion rules in VCL are slightly more nuanced than this. If you look at my branch you can see that not all values can be coerced to a string, notably most literal values.
Additionally integers can be coerced into floats.
This fixes errors like
[regsub] Argument 1 expects STRING type but BACKEND providedwhen calling builtin functions and passing a non-string for a string argument. Fastly handles that by implicitly converting the non-string argument to a string:Fiddle: https://fiddle.fastly.dev/fiddle/2f830b56
I wasn't sure the cleanest way of implementing this. Originally, I was thinking of doing this conversion in
ProcessFunctionCallExpression()andProcessFunctionCallStatement()(the two places that call functions) like we're already doing for theIsIdentArgument()check:falco/interpreter/statement.go
Lines 418 to 421 in 65518ed
But it would need to know the argument types for every function to do that, which means encoding that information in the
Functionstruct.