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

hclsyntax: Don't panic when function returns invalid ArgError #472

Merged
merged 1 commit into from Jun 21, 2021

Conversation

apparentlymart
Copy link
Member

When a function returns a functions.ArgError it's supposed to set the argument index to a valid index within the range of the given argument slice.

However, we typically try to be robust in the face of incorrectly-implemented functions, so rather than panicking in that case
as we would've before we'll now return a slightly-less-precise error message.

The if i > len(e.Args)-1 case in the previous code here was actually trying to handle this situation before, but it didn't quite manage to do it because it was incorrectly referring to e.Args rather than args (so it couldn't take into account expanded arguments) and because it incorrectly assumed that there would always be more declared parameters than arguments (which is only true when there's a variadic parameter that has no arguments at all in the call).

Now we'll handle out of range a little differently for variadic vs. non-variadic functions. For variadics we'll assume that the function was trying to talk about the variadic parameter and name that in the error message. For non-variadics we can't do anything special so we end up just treating it the same as any other error type, simply stating that the function call failed without naming a particular argument.

Functions that end up in this error-handling codepath should still be fixed, because they'll likely end up returning a slightly confusing error message which doesn't accurately reflect the source of the problem. The goal here is just avoid a panic in that case.


This originally surfaced in hashicorp/waypoint#1675. The root cause of that crash is that Waypoint's templatefile incorrectly reports an error in its vars argument even in situations where that argument isn't specified in the call. This change should make it stop panicking, but it'd also be good to update Waypoint to properly handle the situation where there is no vars but yet the template still refers to variables.

When a function returns a functions.ArgError it's supposed to set the
argument index to a valid index within the range of the given argument
slice.

However, we typically try to be robust in the face of
incorrectly-implemented functions, so rather than panicking in that case
as we would've before we'll now return a slightly-less-precise error
message.

The "if i > len(e.Args)-1" case in the previous code here was actually
trying to handle this situation before, but it didn't quite manage to do
it because it was incorrectly referring to e.Args rather than "args"
(so it couldn't take into account expanded arguments) and because it
incorrectly assumed that there would always be more declared parameters
than arguments, which is actually never true in any valid call.

Now we'll handle out of range a little differently for variadic vs.
non-variadic functions. For variadics we'll assume that the function was
trying to talk about the variadic parameter and name that in the error
message. For non-variadics we can't do anything special so we end up
just treating it the same as any other error type, simply stating that
the function call failed without naming a particular argument.

Functions that end up in this error-handling codepath should still be
fixed, because they'll likely end up returning a slightly confusing error
message which doesn't accurately reflect the source of the problem. The
goal here is just avoid a panic in that case.
@apparentlymart apparentlymart added v2 Relates to the v2 line of releases syntax/native labels Jun 21, 2021
@apparentlymart apparentlymart self-assigned this Jun 21, 2021
Copy link
Contributor

@mitchellh mitchellh left a comment

Choose a reason for hiding this comment

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

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
syntax/native v2 Relates to the v2 line of releases
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants