Repository: ysugimoto/falco
Pull request #96 (2022-08-22 16:29:56)

Allow req.backend to be read as a string
Fastly [documents](https://developer.fastly.com/reference/vcl/variables/backend-connection/req-backend/) some curious behaviour for the `req.backend` variable:

> Upon reading in a [STRING](https://developer.fastly.com/reference/vcl/types/string/) context, `req.backend` is converted to a [STRING](https://developer.fastly.com/reference/vcl/types/string/) and prepended with the backend's `share_key` property, which is your service ID (for backends created via the API, CLI, or web interface), the string "fastlyshield" (for [shield](https://developer.fastly.com/learning/concepts/shielding) backends), or whatever you defined (for backends [defined in VCL](https://developer.fastly.com/reference/vcl/declarations/backend/)).

So `req.backend` is typically a `BACKEND` but it can be cast to a `STRING` as well, unlike other `BACKEND` values! We make use of this behaviour in our code to track what backend has been used for debugging purposes, i.e.:
```vcl
set resp.http.Debug-Fastly-Backend = req.backend;
```
Currently falco (understandably) prints a linter error when encountering this code
```
🔥 [ERROR] resp.http.Debug-Fastly-Backend wants type STRING but assign BACKEND (operator/assignment)
```

I'm honestly not sure what the best workaround for this would be. I think the big question is: do any other Fastly variables exhibit similar magical casting behaviour? If so, it would probably be worth expanding the `predefined.yml` specification to allow `get` and `set` fields to return an array or their different possible types.

For now, however, I'm assuming (or hoping) that this isn't the case and we can instead construct a specific hack for `req.backend`. I've started this off by adding an additional check in `lintAssignOperator` for whether you're reading the `req.backend` identifier. For this to work consistently we'll need to copy the check to all other places where a value is read, such as in `lintFunctionArguments`.

It's very messy work though and I'm wondering if anyone knows a better way to handle these values which can have multiple types. Is there anything in code already that supports this behaviour?

P.S. Thanks for giving me write access to the repository; I've made this change a branch in the repository rather than our fork!

davinci26: Also a bit orthogonal but we should be adding a warning if someone uses req.backend on `vcl_log` or `vcl_deliver` since it will be `null`  or incorrect. If you dont want to do that now we should open an issue for that so that we dont forget 
ysugimoto: VCL behaves mysteriously type conversion that the type conversion depends on the right-side member is a variable or literal.
When I started to make `falco`, I investigated type conversion systems of all types [here](https://docs.google.com/spreadsheets/d/16xRPugw9ubKA1nXHIc5ysVZKokLLhysI-jAu3qbOFJ8/edit#gid=0) (hope this helps somewhere) but this lacks BACKEND type 😓 

In my understanding, the backend **variable** will be converted to string but literal will be an error, so,

```vcl
set req.backend = F_origin_0;
set req.http.Debug-Fastly-Backend = req.backend; // Valid

----

set req.http.Debug-Fastly-Backend = F_origin_0; // Invalid, confusing...
```

Then, I agree with @davinci26 , I'm just thinking that we could have an extensible method in `types.Type` (e.g  types.Type.CanConvertTo(types.Type) bool)` and check the type conversion is allowed or not through this method.
Almost VCL type conversion problem may be STRING type (e.g. HTTP Headers) but each types could do so for our future. What do you think about this?


ivomurrell: > I'm just thinking that we could have an extensible method in types.Type (e.g types.Type.CanConvertTo(types.Type) bool)` and check the type conversion is allowed or not through this method.

I'm not sure this would work as the `BACKEND` type itself is not convertable to a `STRING` as your table actually shows in [this cell](https://docs.google.com/spreadsheets/d/16xRPugw9ubKA1nXHIc5ysVZKokLLhysI-jAu3qbOFJ8/edit#gid=0&range=G6), but the `req.backend` variable is an exception to the rule. That's why `set req.http.Debug-Fastly-Backend = req.backend;` works but `set req.http.Debug-Fastly-Backend = F_origin_0;` doesn't. So this means that for the returned value of `types.Type.CanConvertTo` to be correct, we'd have to know the name of the identifier (so we can see whether it's `req.backend` or not), which we don't have access to in the `types.Backend` struct.

The 'cleanest' solution might be defining a special `REQBACKEND` type to describe the unique behaviour, but it would probably be simpler and more maintainable to just hack the identifier checks into the linter phase like I started doing 😅 Does that sound acceptable?
ysugimoto: > I'm not sure this would work as the BACKEND type itself is not convertable to a STRING as your table actually shows in [this cell](https://docs.google.com/spreadsheets/d/16xRPugw9ubKA1nXHIc5ysVZKokLLhysI-jAu3qbOFJ8/edit#gid=0&range=G6) even if it's a variable, but the req.backend variable is an exception to the rule.

yes, In my investigation, the backend identifier like F_origin_0 was treated as a variable 😅 so I didn't try `req.backend`.

> The 'cleanest' solution might be defining a special REQBACKEND type to describe the unique behaviour, but it would probably be simpler and more maintainable to just hack the identifier checks into the linter phase like I started doing 😅 Does that sound acceptable?

That sounds fine, but we should think if there are other specific type conversion problems (but currently I don't know at the moment). If it might be, we should implement the underlying feature.

And I have one question, what does the `REQBACKEND` type work? I'm guessing when the `BACKEND` type is substituted to `req.backend`, then we'll change its type, but I'd know your thought with a short example 😉 

However, defining `types.REQBACKEND` solution just solves this problem so I could accept your suggestion!
@davinci26 do you have any opinions on this?


davinci26: Works for me, I think `REQBACKEND` is a good tradeoff but we should handle the cast from `BACKEND` to `REQBACKEND`
ivomurrell: > And I have one question, what does the `REQBACKEND` type work? I'm guessing when the `BACKEND` type is substituted to `req.backend`, then we'll change its type, but I'd know your thought with a short example 😉

Yes, I think the semantics should look like this:

```vcl
backend F_origin_0 {} // BACKEND
set req.backend = F_origin_0; // implicit cast from BACKEND to REQBACKEND
set req.http.Debug-Fastly-Backend = req.backend; // implicit cast from REQBACKEND to STRING

declare local var.backend BACKEND;
set var.backend = req.backend; // implicit cast from REQBACKEND to BACKEND
set req.http.Debug-Fastly-Backend = var.backend // ERROR: cannot cast a BACKEND to a STRING
```

Patch:
diff --git a/__generator__/main.go b/__generator__/main.go
index f41522e..dfc5483 100644
--- a/__generator__/main.go
+++ b/__generator__/main.go
@@ -14,6 +14,7 @@ var typeToType = map[string]string{
 	"RTIME":       "types.RTimeType",
 	"TIME":        "types.TimeType",
 	"BACKEND":     "types.BackendType",
+	"REQBACKEND":  "types.ReqBackendType",
 	"IP":          "types.IPType",
 	"ID":          "types.IDType",
 	"ACL":         "types.AclType",
diff --git a/__generator__/predefined.yml b/__generator__/predefined.yml
index 45a37ba..443bf62 100644
--- a/__generator__/predefined.yml
+++ b/__generator__/predefined.yml
@@ -223,8 +223,8 @@ beresp.used_alternate_path_to_origin:
 req.backend:
   reference: "https://developer.fastly.com/reference/vcl/variables/backend-connection/req-backend/"
   on: [RECV, HASH, HIT, MISS, PASS, FETCH, ERROR, DELIVER, LOG]
-  get: BACKEND
-  set: BACKEND
+  get: REQBACKEND
+  set: REQBACKEND
 
 req.backend.healthy:
   reference: "https://developer.fastly.com/reference/vcl/variables/backend-connection/req-backend-healthy/"
diff --git a/context/predefined.go b/context/predefined.go
index 5a80a21..ffce231 100644
--- a/context/predefined.go
+++ b/context/predefined.go
@@ -2940,8 +2940,8 @@ func predefinedVariables() Variables {
 						},
 					},
 					Value: &Accessor{
-						Get:       types.BackendType,
-						Set:       types.BackendType,
+						Get:       types.ReqBackendType,
+						Set:       types.ReqBackendType,
 						Unset:     false,
 						Scopes:    RECV | HASH | HIT | MISS | PASS | FETCH | ERROR | DELIVER | LOG,
 						Reference: "https://developer.fastly.com/reference/vcl/variables/backend-connection/req-backend/",
diff --git a/docs/rules.md b/docs/rules.md
index a53dea5..f5e13fd 100644
--- a/docs/rules.md
+++ b/docs/rules.md
@@ -751,14 +751,14 @@ Calling function argument type mismatch.
 
 Problem:
 ```vcl
-declare local var.dir STRING;
-set var.dir = std.dirname(req.backend); // std.dirname expects 1st argument as STRING but BACKEND supplied
+declare local var.lat FLOAT;
+set var.lat = math.floor("2.2"); // math.floor expects 1st argument as FLOAT but STRING supplied
 ```
 
 Fix:
 ```vcl
-declare local var.dir STRING;
-set var.dir = std.dirname(req.url);
+declare local var.lat FLOAT;
+set var.lat = math.floor(2.2);
 ```
 
 ## include/module-not-found
diff --git a/linter/function.go b/linter/function.go
index 597c75c..dcd3149 100644
--- a/linter/function.go
+++ b/linter/function.go
@@ -104,6 +104,14 @@ func (l *Linter) lintFunctionArguments(fn *context.BuiltinFunction, calledFn fun
 					calledFn.meta, calledFn.name, i+1, v, arg,
 				).Match(FUNCTION_ARGUMENT_TYPE).Ref(fn.Reference))
 			}
+		case types.StringType:
+			// fuzzy type check: some builtin function expects STRING type,
+			// then actual argument type could be REQBACKEND because VCL STRING type can be cast from REQBACKEND.
+			if !expectType(arg, types.StringType, types.ReqBackendType) {
+				l.Error(FunctionArgumentTypeMismatch(
+					calledFn.meta, calledFn.name, i+1, v, arg,
+				).Match(FUNCTION_ARGUMENT_TYPE).Ref(fn.Reference))
+			}
 		default:
 			// Otherwise, strict type check
 			if v != arg {
diff --git a/linter/linter.go b/linter/linter.go
index 388bac8..e793618 100644
--- a/linter/linter.go
+++ b/linter/linter.go
@@ -1343,6 +1343,15 @@ func (l *Linter) lintInfixExpression(exp *ast.InfixExpression, ctx *context.Cont
 
 	switch exp.Operator {
 	case "==", "!=":
+		// Cast req.backend to standard backend type for comparisons
+		// Fiddle demonstrating these comparisons are valid:
+		// https://fiddle.fastly.dev/fiddle/06865e2d
+		if left == types.ReqBackendType {
+			left = types.BackendType
+		}
+		if right == types.ReqBackendType {
+			right = types.BackendType
+		}
 		// Equal operator could compare any types but both left and right type must be the same.
 		if left != right {
 			l.Error(InvalidTypeComparison(exp.GetMeta(), left, right).Match(OPERATOR_CONDITIONAL))
diff --git a/linter/operator.go b/linter/operator.go
index cff2308..29d5aef 100644
--- a/linter/operator.go
+++ b/linter/operator.go
@@ -45,7 +45,7 @@ func (l *Linter) lintAssignOperator(op *ast.Operator, name string, left, right t
 		case types.StringType, types.BoolType:
 			return
 		// allows variable only, disallow literal
-		case types.IntegerType, types.FloatType, types.RTimeType, types.TimeType, types.IPType:
+		case types.IntegerType, types.FloatType, types.RTimeType, types.TimeType, types.IPType, types.ReqBackendType:
 			if isLiteral {
 				l.Error(InvalidType(op.Meta, name, left, right).Match(OPERATOR_ASSIGNMENT))
 			}
@@ -76,7 +76,25 @@ func (l *Linter) lintAssignOperator(op *ast.Operator, name string, left, right t
 		default:
 			l.Error(InvalidType(op.Meta, name, left, right).Match(OPERATOR_ASSIGNMENT))
 		}
-	default: // types.BackendType or types.BoolType
+	case types.BackendType:
+		switch right {
+		// allows both variable and literal
+		case types.BackendType, types.ReqBackendType:
+			return
+		// disallow
+		default:
+			l.Error(InvalidType(op.Meta, name, left, right).Match(OPERATOR_ASSIGNMENT))
+		}
+	case types.ReqBackendType:
+		switch right {
+		// allows both variable and literal
+		case types.BackendType, types.ReqBackendType:
+			return
+		// disallow
+		default:
+			l.Error(InvalidType(op.Meta, name, left, right).Match(OPERATOR_ASSIGNMENT))
+		}
+	default: // types.BoolType
 		if left != right {
 			l.Error(InvalidType(op.Meta, name, left, right).Match(OPERATOR_ASSIGNMENT))
 		}
diff --git a/types/types.go b/types/types.go
index 2627df5..caa8c03 100644
--- a/types/types.go
+++ b/types/types.go
@@ -35,6 +35,11 @@ const (
 	// This type is used for variadic arguments of builtin function like h2.disable_header_compression(string...)
 	// Note that this type must not be used in VCL codes, only use for linting function arguments.
 	StringListType Type = 0x100000000001000
+	// ReqBackendType is a virtual type used to represent the special behavior of
+	// the req.backend variable. Unlike typical backend variables, it can be cast
+	// to a string.
+	// Any backend is implicitly cast to ReqBackendType when set to req.backend.
+	ReqBackendType Type = 0x100000000010000
 )
 
 func (t Type) String() string {
@@ -45,6 +50,8 @@ func (t Type) String() string {
 		return "ACL"
 	case BackendType:
 		return "BACKEND"
+	case ReqBackendType:
+		return "REQBACKEND"
 	case BoolType:
 		return "BOOL"
 	case FloatType:


Test patch:
diff --git a/context/context_test.go b/context/context_test.go
index a179ddf..c0e36d5 100644
--- a/context/context_test.go
+++ b/context/context_test.go
@@ -37,7 +37,7 @@ func TestContextSet(t *testing.T) {
 	t.Run("Can return right variable type", func(t *testing.T) {
 		c := New()
 		c.Scope(RECV)
-		expectedType := types.BackendType
+		expectedType := types.ReqBackendType
 		variableType, _ := c.Set("req.backend")
 		if variableType != expectedType {
 			t.Errorf("expected %s but got %s", expectedType, variableType)
diff --git a/linter/linter_test.go b/linter/linter_test.go
index 79d1688..0e4b148 100644
--- a/linter/linter_test.go
+++ b/linter/linter_test.go
@@ -449,6 +449,25 @@ sub foo {
 		assertNoError(t, input)
 	})
 
+	t.Run("set backend as req.backend", func(t *testing.T) {
+		input := `
+backend foo {}
+sub bar {
+	set req.backend = foo;
+}`
+
+		assertNoError(t, input)
+	})
+
+	t.Run("pass req.backend as string", func(t *testing.T) {
+		input := `
+sub foo {
+	set req.http.Debug-Backend = req.backend;
+}`
+
+		assertNoError(t, input)
+	})
+
 	t.Run("invalid variable name", func(t *testing.T) {
 		input := `
 sub foo {
@@ -831,6 +850,17 @@ sub foo {
 }`
 		assertError(t, input)
 	})
+
+	t.Run("req.backend is comparable with BACKEND type", func(t *testing.T) {
+		input := `
+backend foo {}
+sub foo {
+	if (req.backend == foo) {
+		restart;
+	}
+}`
+		assertNoError(t, input)
+	})
 }
 
 func TestLintNotEqualOperator(t *testing.T) {
@@ -1278,6 +1308,17 @@ sub foo {
 }`
 		assertNoError(t, input)
 	})
+
+	t.Run("fuzzy type check for STRING type argument", func(t *testing.T) {
+		input := `
+sub foo {
+	declare local var.S STRING;
+
+	set var.S = substr(req.backend, 1);
+}
+`
+		assertNoError(t, input)
+	})
 }
 
 func TestReturnStatement(t *testing.T) {