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

fix(stdlib): tm_ternary() does not track expression scoped vars. #1276

Merged
merged 2 commits into from
Dec 6, 2023

Conversation

i4ki
Copy link
Contributor

@i4ki i4ki commented Dec 5, 2023

What this PR does / why we need it:

Some variables are scoped by their expression declaration, that's the case for [for ...], {for ...} and tm_dynamic. In this case, the tm_ternary() implementation must use the provided closure evaluator variables instead of the outer Terramate evaluator.

This fixes an issue where tm_ternary() reported the error below:

eval expression: evaluating global["val"] from /stack scope: Call to function "tm_ternary" failed: /sandbox/stack/terramate.tm.hcl:5,14-15: partial evaluation failed: evaluating tm_ternary branch: eval expression: There is no variable named "a".

Which issue(s) this PR fixes:

Special notes for your reviewer:

  • This is a fix for PR feat: evaluator v2 #909
  • This issue was reported by @zied-elouaer and reproduced in a customer repository.
  • It was fixed in the Hackathon but just now isolated into a separate PR.

Does this PR introduce a user-facing change?

no

@i4ki i4ki requested a review from a team as a code owner December 5, 2023 19:18
Some variables are scoped by their expression declaration, that's the
case for `[for ...]`, `{for ...}` and `tm_dynamic`.
In this case, the `tm_ternary()` implementation must use the provided
closure evaluator variables instead of the outer Terramate evaluator.

Signed-off-by: Tiago Natel <[email protected]>
Copy link

github-actions bot commented Dec 5, 2023

metric: time/op
CloudReadLines-4: old 954µs ± 3%: new 960µs ± 4%: delta: 0.00%
CloudReadLine-4: old 7.14ms ± 1%: new 7.23ms ± 1%: delta: 1.27%
ListFiles-4: old 51.1µs ± 1%: new 51.5µs ± 1%: delta: 0.80%
Generate-4: old 2.60s ± 1%: new 0.03s ± 1%: delta: -98.97%
GenerateRegex-4: old 1.78s ± 1%: new 0.05s ± 2%: delta: -97.22%
TokensForExpressionComplex-4: old 1.13ms ± 1%: new 1.15ms ± 2%: delta: 1.83%
TokensForExpressionPlainStringNoNewline-4: old 928ns ± 1%: new 942ns ± 1%: delta: 1.59%
TokensForExpressionStringWith100Newlines-4: old 32.1µs ± 1%: new 34.7µs ± 1%: delta: 8.01%
TokensForExpressionObjectWith100KeysWithNumberValues-4: old 1.29ms ± 2%: new 1.29ms ± 1%: delta: 0.00%
TokensForExpression-4: old 1.13ms ± 0%: new 1.14ms ± 1%: delta: 1.29%
PartialEvalComplex-4: old 435µs ± 0%: new 891µs ± 1%: delta: 104.86%
PartialEvalSmallString-4: old 3.61µs ± 0%: new 3.73µs ± 1%: delta: 3.21%
PartialEvalHugeString-4: old 1.85ms ± 0%: new 1.84ms ± 0%: delta: -0.71%
PartialEvalHugeInterpolatedString-4: old 4.85ms ± 1%: new 9.18ms ± 1%: delta: 89.27%
PartialEvalObject-4: old 22.1µs ± 1%: new 35.2µs ± 1%: delta: 59.00%
check failed: time/op=+20%
metric: alloc/op
CloudReadLines-4: old 3.12MB ± 0%: new 3.12MB ± 0%: delta: 0.00%
CloudReadLine-4: old 3.37MB ± 0%: new 3.37MB ± 0%: delta: 0.00%
ListFiles-4: old 22.0kB ± 0%: new 22.0kB ± 0%: delta: 0.01%
Generate-4: old 2.31GB ± 0%: new 0.02GB ± 0%: delta: -99.12%
GenerateRegex-4: old 950MB ± 0%: new 33MB ± 0%: delta: -96.51%
TokensForExpressionComplex-4: old 412kB ± 0%: new 412kB ± 0%: delta: 0.00%
TokensForExpressionPlainStringNoNewline-4: old 592B ± 0%: new 592B ± 0%: delta: 0.00%
TokensForExpressionStringWith100Newlines-4: old 14.0kB ± 0%: new 14.0kB ± 0%: delta: 0.00%
TokensForExpressionObjectWith100KeysWithNumberValues-4: old 402kB ± 0%: new 402kB ± 0%: delta: 0.00%
TokensForExpression-4: old 412kB ± 0%: new 412kB ± 0%: delta: 0.00%
PartialEvalComplex-4: old 353kB ± 0%: new 673kB ± 0%: delta: 90.79%
PartialEvalSmallString-4: old 1.74kB ± 0%: new 1.74kB ± 0%: delta: 0.00%
PartialEvalHugeString-4: old 166kB ± 0%: new 166kB ± 0%: delta: 0.00%
PartialEvalHugeInterpolatedString-4: old 4.38MB ± 0%: new 8.06MB ± 0%: delta: 84.10%
PartialEvalObject-4: old 20.4kB ± 0%: new 31.6kB ± 0%: delta: 54.88%
metric: allocs/op
CloudReadLines-4: old 5.54k ± 0%: new 5.54k ± 0%: delta: 0.00%
CloudReadLine-4: old 60.0k ± 0%: new 60.0k ± 0%: delta: 0.00%
ListFiles-4: old 321 ± 0%: new 321 ± 0%: delta: 0.00%
Generate-4: old 25.9M ± 0%: new 0.1M ± 0%: delta: -99.56%
GenerateRegex-4: old 18.3M ± 0%: new 0.2M ± 0%: delta: -98.80%
TokensForExpressionComplex-4: old 4.93k ± 0%: new 4.93k ± 0%: delta: 0.00%
TokensForExpressionPlainStringNoNewline-4: old 21.0 ± 0%: new 21.0 ± 0%: delta: 0.00%
TokensForExpressionStringWith100Newlines-4: old 328 ± 0%: new 328 ± 0%: delta: 0.00%
TokensForExpressionObjectWith100KeysWithNumberValues-4: old 3.29k ± 0%: new 3.29k ± 0%: delta: 0.00%
TokensForExpression-4: old 4.93k ± 0%: new 4.93k ± 0%: delta: 0.00%
PartialEvalComplex-4: old 2.83k ± 0%: new 6.36k ± 0%: delta: 124.78%
PartialEvalSmallString-4: old 23.0 ± 0%: new 23.0 ± 0%: delta: 0.00%
PartialEvalHugeString-4: old 35.0 ± 0%: new 35.0 ± 0%: delta: 0.00%
PartialEvalHugeInterpolatedString-4: old 23.1k ± 0%: new 61.1k ± 0%: delta: 164.80%
PartialEvalObject-4: old 125 ± 0%: new 245 ± 0%: delta: 96.00%
check failed: allocs/op=+20%

Copy link
Contributor

@snakster snakster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noticed a typo in a comment, looks goods otherwise 👍

stdlib/ternary.go Outdated Show resolved Hide resolved
Co-authored-by: Sebastian <[email protected]>
@i4ki i4ki merged commit 6ff219d into i4k-globals-v2-impl Dec 6, 2023
8 checks passed
@i4ki i4ki deleted the i4k-fix-eval-v2-ternary branch December 6, 2023 13:02
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.

3 participants