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

Unused checker reports variable used in if-condition inside for_each block as unused #1214

Open
B-Schmidt opened this issue Jan 9, 2025 · 4 comments · May be fixed by #1217
Open

Unused checker reports variable used in if-condition inside for_each block as unused #1214

B-Schmidt opened this issue Jan 9, 2025 · 4 comments · May be fixed by #1217
Labels
type: bug A code related bug vrl: compiler Changes to the compiler

Comments

@B-Schmidt
Copy link

In trying to implement a sort of take_while construct I have come across a variable being incorrectly marked as unused. Sample code to reproduce:

thing = [1, 2,  -1, 3]
.valid = []
done = false

for_each(thing) -> |_i, v| {
    if !done {
        .valid = push(.valid, v)
    }
    if v < 0 {
        done = true
    }
}

This results in an unused variable warning for the line done = false despite the use of done in the condition on line 6. Removing the expression as the warning suggests will of course fail to compile due to an undefined variable.

@pront pront added type: bug A code related bug vrl: compiler Changes to the compiler labels Jan 9, 2025
@pront
Copy link
Member

pront commented Jan 9, 2025

Thank for reporting this @B-Schmidt. Closure support is very limited. I will take a look.

@pront
Copy link
Member

pront commented Jan 9, 2025

I reproduced this:

done = false
for_each([1]) -> |_i, _v| {
    if !done {
        done = true
    }
}

Command:

cargo run --bin vrl .../vrl/lib/cli/Cargo.toml -- --input /Users/pavlos.rontidis/CLionProjects/vrl/pront/inputs/match.json --print-object --print-warnings --program .../vrl/pront/scripts/for_each_warning.vrl

Warning:

warning[E900]: unused variable `done`
  ┌─ :1:1
  │
1 │ done = false
  │ ^^^^ help: use the result of this expression or remove it
  │
  = this expression has no side-effects
  = see language documentation at https://vrl.dev
  = try your code in the VRL REPL, learn more at https://vrl.dev/examples

The checker marks done as:

  1. pending usage
  2. used
  3. pending usage

It didn't keep track that it was used in the previous iteration. VRL scripts don't allow loops so this cannot happen unless a closure is used. Will think a bit how to remove this warning. Since the checker doesn't handle closures, we could probably detect identifier usage inside closure and never consider the identifier again.

@B-Schmidt
Copy link
Author

Thinking about this some more with your explanation of what is actually happening, it could be argued that the assignment inside the closure is correctly detected as unused since the assignment on the last run through the loop would not be used. I am honestly unsure if that is more likely to lead to confusion like here or actually warn of a real issue.

What did surprise me though is that the location of the issue is not updated to the latest assignment, which would have been less confusing to me. Looking at it in a simpler case, this behavior feels off to me:

On the one hand, this would detect an issue in line 1 while the actual problem lies with line 3 (lines 1 and 2 being completely fine on their own):

foo = 1
.bar = foo
foo = 2

On the other, this would not detect an issue at all despite the first assignment being completely unused:

foo = 1
foo = 2
.bar = foo

Especially that latter one could miss problems e.g. with error handling. That should probably its own issue though.

@pront
Copy link
Member

pront commented Jan 9, 2025

it could be argued that the assignment inside the closure is correctly detected as unused since the assignment on the last run through the loop would not be used. I am honestly unsure if that is more likely to lead to confusion like here or actually warn of a real issue.

My take is that this is a bit confusing for users. The expression checker is a relatively new addition aiming to help users improve their VRL scripts. Once matured, the warnings can be treated as errors (in a future strict mode). Given this context and because closures are out of scope (for now) we don't want to make a decision right now, hence my idea to ignore identifiers which are used inside closures.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug A code related bug vrl: compiler Changes to the compiler
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants