Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve string concatenation logic #364

Merged
merged 2 commits into from
Oct 8, 2024
Merged

Conversation

ysugimoto
Copy link
Owner

@ysugimoto ysugimoto commented Oct 8, 2024

Fixes #360

This PR improves string concatenation for expressions.

What was improved

Before we evaluate for each expressions but it could not lint "time calculation" case.
The "time calculation" is special behavior that is not documented on Fastly (maybe Varnish feature).

Time Calculation

On evaluating expressions, the left hand is TIME time and right hand is RTIME(Literal) with explicit plus sign(+), it works as "time calculation". additionally, the minus sign (-) is also accepted for subtracting the TIME.

set req.http.Foo = now + 5m; // time addition
set req.http.Foo = now - 5m; // time subtraction

On the above example, the expression should be treated as time calculation, the result is TIME which is added (or subracted) 5 minutes.

This is very complicated syntax because we will misunderstand that is string concatenation or syntax error.
Followings are confusing example:

declare local var.R RTIME;
set var.R = 5m;

set req.http.Foo = now + 5m;    // (VALID) time calculation
set req.http.Foo = now 5m;      // (INVALID) syntax error 
set req.http.Foo = now + var.R; // (INVALID) syntax error 
set req.http.Foo = now var.R;   // (VALID) string concatenation 

Then we couldn't recognize above cases by current string concatenation linting process so we improved that.

  1. Extract and flatten to expression series only with checking valid expressions
  2. hold operator (explicit + or not) for each series
  3. lint expressions the order of series

You also will feel that time calculation is complicated, and Fastly undocumented I think we should not use this syntax, so we will raise an lint error as INFO level (VCL is valid, but we recommend to fix).

And I also improved interpreter implementation follows the above way to evaluate.

See actual Fastly behavior on the fiddle: https://fiddle.fastly.dev/fiddle/befec89e

@ysugimoto ysugimoto merged commit ea0ba33 into main Oct 8, 2024
3 checks passed
@ysugimoto ysugimoto deleted the fix/string-concatenation branch October 8, 2024 11:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Concatenating TIME to STRING lints as ERROR despite being valid
1 participant