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(vrl): mark set_semantic_meaning impure #21896

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

sfackler
Copy link

@sfackler sfackler commented Nov 26, 2024

Summary

The VRL compiler previously warned about the unused result of set_semantic_meaning, even though it doesn't return anything due to it being incorrectly considered a pure function.

Change Type

  • Bug fix
  • New feature
  • Non-functional (chore, refactoring, docs)
  • Performance

Is this a breaking change?

  • Yes
  • No

How did you test this PR?

Does this PR include user facing changes?

  • Yes. Please add a changelog fragment based on our guidelines.
  • No. A maintainer will apply the "no-changelog" label to this PR.

Checklist

  • Please read our Vector contributor resources.
  • If this PR introduces changes Vector dependencies (modifies Cargo.lock), please
    run dd-rust-license-tool write to regenerate the license inventory and commit the changes (if any). More details here.

References

Closes #21887

@sfackler sfackler requested a review from a team as a code owner November 26, 2024 19:03
@bits-bot
Copy link

bits-bot commented Nov 26, 2024

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@pront pront left a comment

Choose a reason for hiding this comment

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

Thanks @sfackler! Did you have a chance to test that the warning went away?

@sfackler
Copy link
Author

I have not - what's the easiest way to do that?

@pront
Copy link
Member

pront commented Nov 26, 2024

I have not - what's the easiest way to do that?

You can build Vector from this PR and use a Remap with set_semantic_meaning in its source. Something like:

sources:
  demo_logs_1:
    type: demo_logs
    format: json
transforms:
  remap0:
    type: remap
    inputs: ["demo_logs_1"]
    source: |
      set_semantic_meaning(.host, "a")
sinks:
  console:
    type: console
    inputs: ["remap0"]
    encoding:
      codec: json

@sfackler
Copy link
Author

Huh - it seems like this change doesn't actually fix the warning? I had to add a second statement to the VRL script (does it treat the last expression as an implicit return to the caller?) for the warning to trigger at all, but with that setup it warns both on master and this branch:

$ cat config.yaml
sources:
  demo_logs:
    type: demo_logs
    format: json
transforms:
  remap:
    type: remap
    inputs: [demo_logs]
    source: |
      set_semantic_meaning(.foo, "a")
      .foo = "bar"
sinks:
  console:
    type: console
    inputs: [remap]
    encoding:
      codec: json
$ cargo run -p vector -- --config config.yaml
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.33s
     Running `target/debug/vector --config config.yaml`
2024-11-26T21:09:00.300855Z  INFO vector::app: Log level is enabled. level="info"
2024-11-26T21:09:00.319805Z  INFO vector::app: Loading configs. paths=["config.yaml"]
2024-11-26T21:09:00.329959Z  WARN transform{component_kind="transform" component_id=remap component_type=remap}: vector::transforms::remap: VRL compilation warning. warnings=
warning[E900]: unused result for function call `set_semantic_meaning(.foo, "a")`
  ┌─ :1:1
  │
1 │ set_semantic_meaning(.foo, "a")
  │ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ 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

2024-11-26T21:09:00.331415Z  INFO vector::topology::running: Running healthchecks.
2024-11-26T21:09:00.331730Z  INFO vector::topology::builder: Healthcheck passed.
2024-11-26T21:09:00.331916Z  INFO vector: Vector has started. debug="true" version="0.43.0" arch="x86_64" revision=""
2024-11-26T21:09:00.332007Z  INFO vector::app: API is disabled, enable by setting `api.enabled` to `true` and use commands like `vector top`.
{"foo":"bar"}

@sfackler
Copy link
Author

Oh - looks like the lint doesn't actually check if the function is pure or not: https://github.com/vectordotdev/vrl/blob/e3852056d7f2f52a6127c5407ab840f52e545a88/src/compiler/unused_expression_checker.rs#L354-L372

@pront
Copy link
Member

pront commented Nov 26, 2024

does it treat the last expression as an implicit return to the caller?

Yes, the result of the last expression is considered "used". I think we need to add set_semantic_meaning to this list.

But we should also merge this PR, since this a 'strange' function which modifies internal state.

config.yaml Outdated
@@ -0,0 +1,17 @@
sources:
Copy link
Member

Choose a reason for hiding this comment

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

Should also git rm this one. Probably was committed by accident.

Copy link
Author

Choose a reason for hiding this comment

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

Oops, fixed

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.

VRL compiler warns about the unused result of set_semantic_meaning
3 participants