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

Evaluate terms before extracting documentation #1463

Merged
merged 4 commits into from
Jul 25, 2023
Merged

Conversation

vkleen
Copy link
Contributor

@vkleen vkleen commented Jul 19, 2023

With this change, nickel doc evaluates a term before trying to extract
documentation. The evaluation mode needed here is somewhat between evaluating to
WHNF and a full deep evaluation. Namely, we first evaluate the term to WHNF. If
it isn't a record then there is no documentation to extract and we're done. If
it is, we iterate through all fields with a defined value and call the evaluator
recursively.

The result will be a tree of records in WHNF which our previous documentation
extraction procedure knows how to handle. In fact, the only difference to a full
deep evaluation is that we do not descend into arrays. Consequently, this change
will incur a performance penalty compared to the previous nickel doc. For
now, we're willing to accept this penalty for two reasons. First, documentation
extraction is expected to be rarer than fully evaluating a Nickel configuration.
Second, the evaluation algorithm presented here does strictly less work than a
full evaluation and therefore shouldn't be any slower.

There is another caveat with this implementation. For simplicity, I've
implemented an explicitly recursive version of the algorithm described above.
This means that we're using the Rust callstack when descending into deeply
nested records and we might encounter stack overflows for particularly deep
records. The Nickel evaluator itself uses a custom stack stored in the heap to
circumvent this problem. If we ever encounter stack overflows in nickel doc in
the wild, we should do the same here.

Fixes #1462.

@github-actions github-actions bot temporarily deployed to pull request July 19, 2023 11:40 Inactive
@github-actions github-actions bot temporarily deployed to pull request July 21, 2023 07:47 Inactive
@github-actions github-actions bot temporarily deployed to pull request July 21, 2023 08:12 Inactive
@vkleen vkleen changed the title [WIP] Try evaluating terms before extracting documentation Evaluate terms before extracting documentation Jul 21, 2023
@vkleen vkleen marked this pull request as ready for review July 21, 2023 08:33
Copy link
Member

@yannham yannham left a comment

Choose a reason for hiding this comment

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

As discussed before, this approach sounds reasonable. We might actually be slower than evaluation in some cases (we reset the machine, we map over fields, etc.), but most probably still close to the performance of full evaluation in the worst case, so it's fine.

core/src/program.rs Outdated Show resolved Hide resolved
core/src/program.rs Show resolved Hide resolved
Co-authored-by: Yann Hamdaoui <yann.hamdaoui@tweag.io>
@github-actions github-actions bot temporarily deployed to pull request July 25, 2023 11:08 Inactive
@vkleen vkleen enabled auto-merge July 25, 2023 11:14
@vkleen vkleen added this pull request to the merge queue Jul 25, 2023
Merged via the queue into master with commit 835f15c Jul 25, 2023
4 checks passed
@vkleen vkleen deleted the fix/doc-let-bindings branch July 25, 2023 12:15
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.

nickel doc appears not to work for Let x in y expressions
2 participants