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

enhancement: Expand usage of resolve_constant and add support for variables #304

Merged
merged 4 commits into from
Jul 7, 2023

Conversation

fuchsnj
Copy link
Member

@fuchsnj fuchsnj commented Jul 7, 2023

related: #151
closes: #114

Expr::resolve_constant tries to resolve an expression with only compile-time information. This implementation has been improved to also resolve variables if the value of the variable is known at compile-time.

This is something that is useful for ArgumentList functions, such as ArgumentList::required_regex, so for example a variable with a literal regex can be used where a literal regex is required.

All of the ArgumentList::* functions were updated to use Expr::resolve_constant where approriate, so they can also resolve literal variables (and any other improvements that may come with the resolve_const function in the future).

There was also an internal refactor for the resolve_constant function. The TypeState is the "source of truth" for compile-time information, including variables with known values. The Variable expression also included this. This was removed and now the TypeState is always used.

@jszwedko jszwedko requested a review from pront July 7, 2023 18:56
@@ -20,22 +20,27 @@ impl Query {
// TODO:
// - error when trying to index into object
// - error when trying to path into array
#[must_use]
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps it's better to apply #[must_use] to the struct Query.

Copy link
Member Author

Choose a reason for hiding this comment

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

Does that mean the same thing? These were from clippy suggestions (I'm not sure why it wanted it on these functions all of a sudden)

Copy link
Member Author

@fuchsnj fuchsnj Jul 7, 2023

Choose a reason for hiding this comment

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

I tried it, unfortunately clippy starts complaining if I only put it on the struct itself 😞

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I see, thanks for trying it out. This might happen because std::hint::must_use is still experimental.

src/compiler/expression/query.rs Outdated Show resolved Hide resolved
src/compiler/expression/variable.rs Show resolved Hide resolved
@fuchsnj fuchsnj enabled auto-merge (squash) July 7, 2023 19:54
@fuchsnj fuchsnj merged commit 76be3e2 into main Jul 7, 2023
@fuchsnj fuchsnj deleted the fuchsnj/improve_resolve_constant branch July 7, 2023 19:54
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.

Allow literal regexes from variables as arguments to VRL functions
3 participants