feat(sprintf): support full %q format specifications - #810
Open
Vitalii Tverdokhlib (vitaliytv) wants to merge 4 commits into
Open
feat(sprintf): support full %q format specifications#810Vitalii Tverdokhlib (vitaliytv) wants to merge 4 commits into
Vitalii Tverdokhlib (vitaliytv) wants to merge 4 commits into
Conversation
8 tasks
Collaborator
|
Vitalii Tverdokhlib (@vitaliytv) Thank you for the contribution. Going to take a deeper look at this today/tomorrow. |
Copilot started reviewing on behalf of
Anand Krishnamoorthi (anakrish)
September 9, 2026 15:10
View session
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Format compatibility gaps and unchecked allocation paths can produce incorrect output or substantially exceed configured memory limits.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Completes Go/OPA-compatible %q formatting for sprintf.
Changes:
- Adds format-spec parsing, quoting flags, dynamic/indexed operands, and value conversions.
- Ports Go Unicode printability tables.
- Adds compatibility and memory-limit tests.
File summaries
| File | Description |
|---|---|
src/builtins/strings.rs |
Implements %q formatting and integration. |
src/builtins/strings/sprintf_format.rs |
Parses Go-style format specifications. |
src/builtins/strings/go_is_print.rs |
Provides Go-compatible Unicode classification. |
tests/interpreter/cases/builtins/strings/sprintf.yaml |
Adds interpreter compatibility cases. |
tests/memory_limits.rs |
Tests quoting memory-limit propagation. |
LICENSE |
Attributes Go-derived source. |
Review details
Suppressed comments (1)
src/builtins/strings/sprintf_format.rs:119
- A negative dynamic precision must produce Go's
%!(BADPREC)diagnostic and then format without precision. Withsprintf("%.*q", [-1, "x"]), this silently returns"x"instead of%!(BADPREC)"x", so a valid integer*operand has incompatible OPA behavior. Preserve a bad-precision marker in the parsed spec and emit it before formatting the value.
let precision = take_integer(args, args_idx, args_span)?;
if precision >= 0 {
spec.precision = Some(checked_dynamic_value(precision as u64, args_span)?);
}
- Files reviewed: 6/6 changed files
- Comments generated: 7
- Review effort level: Balanced
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
Author
|
Also addressed the suppressed review note in 3568dac: negative dynamic precision now emits Go/OPA-compatible |
Anand Krishnamoorthi (anakrish)
previously approved these changes
Sep 11, 2026
Anand Krishnamoorthi (anakrish)
left a comment
Anand Krishnamoorthi (anakrish)
left a comment
Collaborator
There was a problem hiding this comment.
LGTM.
Olena Tverdokhlib (ElenaTv)
force-pushed
the
codex/sprintf-format-spec
branch
from
September 13, 2026 05:19
60b38bb to
36fe599
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to the merged #809, now rebased directly onto
main.This completes
%qsupport with a dedicatedsprintfformat-spec parser instead of extending the original PR's single-verb parsing incrementally.What this adds
*operands, explicit argument indexes such as%[3]*.[2]*[1]q, and Go/OPA-compatibleBADINDEXdiagnostics for malformed indexed specifications.%+q(QuoteToASCIIbehavior),%#q(backquoted strings when valid), left/right alignment, zero padding, and combined width/precision.%qoperands using OPA-compatible value conversion for strings, booleans, null, collections, integral runes, invalid runes, fractional numbers, and out-of-range integers.qverbs. Newly parsed flags that those verbs do not support are rejected explicitly.no_stdbuilds by avoiding the platformlog10symbol.Compatibility boundary
Regorus currently normalizes numbers into its internal
Numbervariants and does not retain the original Rego numeric token spelling. OPA's%qbehavior can depend on that spelling: for example, OPA distinguishes65.0,65e0, and65, while Regorus may normalize them to different shared numeric variants. Exact parity for those lexical-number edge cases requires a separate change to the number representation; this PR does not disguise or broaden that architectural change.The common
%qmatrix and parser edge cases were compared directly with OPA, including ASCII quoting, raw quoting and fallback, width/precision combinations, dynamic and indexed operands,BADINDEX/BADPRECdiagnostics, runes, invalid runes, collections, float rounding boundaries, and oversized padding requests.Validation
cargo fmt --all -- --checkcargo test --locked(full local suite)cargo check --locked --no-default-featurestests/interpreter/cases/builtins/strings/sprintf.yamltests/rvm/rego/cases/sprintf.yamlsprintf_quote_propagates_memory_limit_errorswithmimallocandallocator-memory-limitsThe commits already merged through #809 were removed from this branch during the rebase, so the PR now contains only the format-spec extension and its follow-up fixes.