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

Improve error messages when reference resolution fails #72

Merged
merged 1 commit into from
Jan 9, 2024

Conversation

simu
Copy link
Member

@simu simu commented Jan 5, 2024

To improve the error messages for reference resolution failure, we add a Vec<String> to track the current parameter key for which we're resolving references into the ResolveState struct.

We also extract the error rendering in Token::resolve() into methods on ResolveState, since we mostly need data from ResolveState to render the error messages.

The new error messages all print the parameter key for which we're resolving references. If applicable, the error messages also contain the full reference, and the particular key for which resolution failed.

To reduce repetition for the error handling, we also refactor Token::resolve() to always either interpolate or copy the Value which is the base for the key lookup. While this introduces a bunch of unnecessary calls to clone() if we're not doing a lookup into a Value::String or Value::ValueList, the performance impact is negligible, and the resulting code is much less convoluted.

Resolves #69

Checklist

  • The PR has a meaningful title. The title will be used to auto generate the changelog
  • PR contains a single logical change (to build a better changelog).
  • Update the documentation.
  • Update tests.
  • Categorize the PR by setting a good title and adding one of the labels:
    bug, enhancement, documentation, change, breaking, dependency, internal
    as they show up in the changelog
  • Link this PR to related PRs or issues.

@simu simu added the change label Jan 5, 2024
@simu simu changed the title Fix error messages when reference resolution fails Improve error messages when reference resolution fails Jan 5, 2024
@simu simu force-pushed the fix/reference-resolution-error branch from 1b402a4 to a38a5db Compare January 5, 2024 15:34
@simu
Copy link
Member Author

simu commented Jan 5, 2024

Note that the PR has a slight performance regression (the benchmarks in #47 report a performance regression of 2-4%).

As far as I'm able to tell, this is caused mostly by the additional state that we're tracking:

  1. The ResolveState struct is larger, which makes cloning it during reference resolution more expensive
  2. We introduce a bunch of extra allocations because we track the current key in a Vec<String> -- note that tracking the current key in a String isn't faster.

To improve the error messages for reference resolution failure, we add a
`Vec<String>` to track the current parameter key for which we're
resolving references into the `ResolveState` struct.

We also extract the error rendering in `Token::resolve()` into methods
on `ResolveState`, since we mostly need data from `ResolveState` to
render the error messages.

The new error messages all print the parameter key for which we're
resolving references. If applicable, the error messages also contain the
full reference, and the particular key for which resolution failed.

To reduce repetition for the error handling, we also refactor
`Token::resolve()` to always either interpolate or copy the Value which
is the base for the key lookup. While this introduces a bunch of
unnecessary calls to `clone()` if we're not doing a lookup into a
`Value::String` or `Value::ValueList`, the performance impact is
negligible, and the resulting code is much less convoluted.

Resolves #69
@simu
Copy link
Member Author

simu commented Jan 8, 2024

Follow-up regarding performance: I tested a bit more and have found that pre-allocating the variable-sized ResolveState fields (seen_paths: HashSet::with_capacity() and current_keys: Vec::with_capacity()) doesn't seem to make a significant difference in runtime.

@simu simu force-pushed the fix/reference-resolution-error branch from a38a5db to df3d1ea Compare January 8, 2024 09:56
@simu simu requested a review from a team January 8, 2024 10:15
@simu simu merged commit fe9b6a3 into main Jan 9, 2024
17 checks passed
@simu simu deleted the fix/reference-resolution-error branch January 9, 2024 10:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve error message for failed reference lookups
2 participants