feat: add format specifiers to f-strings #31
Merged
Conversation
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
…ure, and feature implementation workflow
… and improve memory management
…ring formatting support
…ments in f-strings
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
github-actions
bot
added
documentation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
📝 WalkthroughWalkthroughThis PR adds format specifier support to f-strings in the Kronos language. Changes span the parser to track per-part format specs, compiler to introduce the OP_FORMAT_VALUE opcode, VM runtime to implement formatting logic, LSP completions, comprehensive integration tests, and documentation updates. Changes
Sequence DiagramsequenceDiagram
participant User
participant Parser
participant Compiler
participant VM
User->>Parser: Parse f"{value:.2f}"
activate Parser
Parser->>Parser: Extract expression & format spec ".2f"
Parser->>Parser: Store in format_specs array
deactivate Parser
Parser-->>Compiler: AST with format_specs
activate Compiler
Compiler->>Compiler: For each f-string part with format_spec
Compiler->>Compiler: Generate OP_FORMAT_VALUE bytecode
deactivate Compiler
Compiler-->>VM: Bytecode with format spec constants
activate VM
VM->>VM: Execute OP_FORMAT_VALUE
VM->>VM: parse_format_spec(".2f") → FormatSpec
VM->>VM: format_value_with_spec(value, spec)
VM->>VM: apply_alignment for padding/alignment
deactivate VM
VM-->>User: Formatted string result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@CLAUDE.md`:
- Around line 16-17: Update the test count comment for the test runner entry
("./scripts/run_tests.sh # Run full test suite (129 tests)") to reflect the
current number of passing tests (133) or replace the fixed number with a dynamic
comment like "Run full test suite (current passing tests)" so it stays accurate;
edit the CLAUDE.md line containing "./scripts/run_tests.sh" to either show 133
tests or a non-hardcoded description.
In `@src/compiler/compiler.c`:
- Around line 896-914: In emit_expr_to_string: remove the extra
value_release(spec_val) when add_constant(c, spec_val) returns SIZE_MAX
(add_constant already takes ownership and frees on failure) and instead just
return; additionally guard the constant index before emitting by checking if
spec_idx <= UINT16_MAX, and if it exceeds UINT16_MAX call compiler_set_error(c,
"format specifier constant index overflow") (or similar) and return rather than
casting and emitting with emit_uint16; references: emit_expr_to_string,
add_constant, value_release, emit_uint16, compiler_set_error.
In `@src/frontend/parser.c`:
- Around line 1521-1535: The bug: after calling fstring_add_part (which
increments part_count) you attempt to realloc format_specs and on failure call
fstring_cleanup_parts with the new part_count while format_specs still has its
old capacity, allowing fstring_cleanup_format_specs to read out-of-bounds; fix
by keeping sizes consistent on the failure path — either grow format_specs
before calling fstring_add_part or, if you must call fstring_add_part first,
pass the old format_specs_capacity (not the incremented part_count capacity)
into fstring_cleanup_parts so cleanup only iterates existing entries; update the
call sites around fstring_add_part/format_specs realloc to ensure
format_specs_capacity and part_count remain synchronized on error.
- Around line 1553-1594: The bug is that format_specs can be left unresized
after fstring_add_part succeeds, so if realloc fails the cleanup may read past
the smaller array; to fix, ensure format_specs is grown before calling
fstring_add_part when part_capacity will exceed format_specs_capacity: check if
part_capacity > format_specs_capacity, perform realloc and initialize new_specs
(updating format_specs_capacity) and handle failure (free format_spec and call
fstring_cleanup_parts) before invoking fstring_add_part, then after
fstring_add_part assign format_specs[part_count - 1] = format_spec; use the same
symbols shown (format_specs, format_specs_capacity, part_capacity, part_count,
format_spec, fstring_add_part, fstring_cleanup_parts) so the growth/realloc
happens prior to adding the new part.
In `@src/vm/vm.c`:
- Around line 2571-2594: The integer-formatting path incorrectly allows
precision to force floating formatting and casts doubles to long long without
checking for NaN/Inf or out-of-range values; update the branch in the formatting
code handling value->type == VAL_NUMBER (and the logic using spec->type and
spec->precision) to: (1) treat 'd' as a strict integer format and explicitly
reject any spec->precision >= 0 when spec->type == 'd' by calling
vm_error(KRONOS_ERR_RUNTIME); (2) before casting the double to long long for the
integer path, check for isnan(value->as.number), isinf(value->as.number) and
that the value lies within LLONG_MIN..LLONG_MAX (use appropriate macros) and
call vm_error(KRONOS_ERR_RUNTIME) on failure; and (3) only use the
floating-point snprintf("%.*f", ...) when spec->type == 'f' (or other floating
specifiers) and spec->precision is allowed — adjust the conditional that
currently reads spec->type == 'f' || spec->precision >= 0 accordingly so
precision alone does not route to float formatting.
🧹 Nitpick comments (1)
CLAUDE.md (1)
34-36: Add language specifier to fenced code block.Per markdownlint (MD040), fenced code blocks should have a language specified. Since this is a text diagram, you can use
textorplaintext.-``` +```text Source (.kr) → Tokenizer → Parser → Compiler → VM</blockquote></details> </blockquote></details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Comment on lines
+16
to
+17
| ./scripts/run_tests.sh # Run full test suite (129 tests) | ||
| ``` |
There was a problem hiding this comment.
Test count may be outdated.
The script comment references 129 tests, but the PR summary indicates 133 integration tests now pass with the 4 new f-string format tests. Consider updating this to keep documentation accurate.
-./scripts/run_tests.sh # Run full test suite (129 tests)
+./scripts/run_tests.sh # Run full test suite (133 tests)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ./scripts/run_tests.sh # Run full test suite (129 tests) | |
| ``` | |
| ./scripts/run_tests.sh # Run full test suite (133 tests) |
🤖 Prompt for AI Agents
In `@CLAUDE.md` around lines 16 - 17, Update the test count comment for the test
runner entry ("./scripts/run_tests.sh # Run full test suite (129 tests)") to
reflect the current number of passing tests (133) or replace the fixed number
with a dynamic comment like "Run full test suite (current passing tests)" so it
stays accurate; edit the CLAUDE.md line containing "./scripts/run_tests.sh" to
either show 133 tests or a non-hardcoded description.
Comment on lines
+896
to
+914
| static void emit_expr_to_string(Compiler *c, const char *format_spec) { | ||
| if (format_spec) { | ||
| // Emit format specifier as constant and use OP_FORMAT_VALUE | ||
| KronosValue *spec_val = value_new_string(format_spec, strlen(format_spec)); | ||
| if (!spec_val) { | ||
| compiler_set_error(c, "Failed to allocate format specifier constant"); | ||
| return; | ||
| } | ||
| size_t spec_idx = add_constant(c, spec_val); | ||
| if (spec_idx == SIZE_MAX) { | ||
| value_release(spec_val); | ||
| return; | ||
| } | ||
| emit_byte(c, OP_FORMAT_VALUE); | ||
| if (compiler_has_error(c)) { | ||
| return; | ||
| } | ||
| emit_uint16(c, (uint16_t)spec_idx); | ||
| } else { |
There was a problem hiding this comment.
Fix double-free and guard format spec index overflow.
add_constant() already takes ownership and releases on error, so value_release(spec_val) on SIZE_MAX can double-free. Also guard spec_idx > UINT16_MAX before casting to uint16_t.
🐛 Proposed fix
- size_t spec_idx = add_constant(c, spec_val);
- if (spec_idx == SIZE_MAX) {
- value_release(spec_val);
- return;
- }
+ size_t spec_idx = add_constant(c, spec_val);
+ if (spec_idx == SIZE_MAX) {
+ return;
+ }
+ if (spec_idx > UINT16_MAX) {
+ compiler_set_error(c, "Too many constants (limit 65535)");
+ return;
+ }🤖 Prompt for AI Agents
In `@src/compiler/compiler.c` around lines 896 - 914, In emit_expr_to_string:
remove the extra value_release(spec_val) when add_constant(c, spec_val) returns
SIZE_MAX (add_constant already takes ownership and frees on failure) and instead
just return; additionally guard the constant index before emitting by checking
if spec_idx <= UINT16_MAX, and if it exceeds UINT16_MAX call
compiler_set_error(c, "format specifier constant index overflow") (or similar)
and return rather than casting and emitting with emit_uint16; references:
emit_expr_to_string, add_constant, value_release, emit_uint16,
compiler_set_error.
Comment on lines
+1521
to
+1535
| // Grow format_specs array if parts array grew | ||
| if (part_capacity > format_specs_capacity) { | ||
| char **new_specs = realloc(format_specs, part_capacity * sizeof(char *)); | ||
| if (!new_specs) { | ||
| fstring_cleanup_parts(parts, format_specs, part_count); | ||
| return NULL; | ||
| } | ||
| // Zero-initialize new entries | ||
| for (size_t j = format_specs_capacity; j < part_capacity; j++) { | ||
| new_specs[j] = NULL; | ||
| } | ||
| format_specs = new_specs; | ||
| format_specs_capacity = part_capacity; | ||
| } | ||
| format_specs[part_count - 1] = NULL; // String literals have no format spec |
There was a problem hiding this comment.
Potential out-of-bounds read on allocation failure path.
If fstring_add_part succeeds (incrementing part_count) but the subsequent format_specs realloc fails, fstring_cleanup_parts is called with the new part_count. However, format_specs still has the old capacity (format_specs_capacity), causing fstring_cleanup_format_specs to read past the array bounds.
🔧 Proposed fix
Pass the old format_specs_capacity to cleanup, or grow format_specs before adding the part:
if (!str_node ||
!fstring_add_part(&parts, &part_count, &part_capacity, str_node)) {
- fstring_cleanup_parts(parts, format_specs, part_count);
+ fstring_cleanup_format_specs(format_specs, format_specs_capacity);
+ if (parts) {
+ for (size_t k = 0; k < part_count; k++) {
+ ast_node_free(parts[k]);
+ }
+ free(parts);
+ }
return NULL;
}Alternatively, grow format_specs before calling fstring_add_part to keep them synchronized:
+ // Grow format_specs if parts array would need to grow
+ if (part_count >= part_capacity && part_capacity > format_specs_capacity) {
+ // Pre-grow format_specs
+ }
if (!str_node ||
!fstring_add_part(&parts, &part_count, &part_capacity, str_node)) {🤖 Prompt for AI Agents
In `@src/frontend/parser.c` around lines 1521 - 1535, The bug: after calling
fstring_add_part (which increments part_count) you attempt to realloc
format_specs and on failure call fstring_cleanup_parts with the new part_count
while format_specs still has its old capacity, allowing
fstring_cleanup_format_specs to read out-of-bounds; fix by keeping sizes
consistent on the failure path — either grow format_specs before calling
fstring_add_part or, if you must call fstring_add_part first, pass the old
format_specs_capacity (not the incremented part_count capacity) into
fstring_cleanup_parts so cleanup only iterates existing entries; update the call
sites around fstring_add_part/format_specs realloc to ensure
format_specs_capacity and part_count remain synchronized on error.
Comment on lines
+1553
to
+1594
| size_t expr_end = (colon_pos < brace_end) ? colon_pos : brace_end; | ||
|
|
||
| // Extract format specifier if present | ||
| char *format_spec = NULL; | ||
| if (colon_pos < brace_end) { | ||
| size_t spec_len = brace_end - colon_pos - 1; | ||
| if (spec_len > 0) { | ||
| format_spec = malloc(spec_len + 1); | ||
| if (!format_spec) { | ||
| fstring_cleanup_parts(parts, format_specs, part_count); | ||
| return NULL; | ||
| } | ||
| memcpy(format_spec, content + colon_pos + 1, spec_len); | ||
| format_spec[spec_len] = '\0'; | ||
| } | ||
| } | ||
|
|
||
| // Parse expression from f-string content | ||
| ASTNode *expr_node = | ||
| fstring_parse_expression(content, expr_start, brace_end); | ||
| fstring_parse_expression(content, expr_start, expr_end, NULL); | ||
| if (!expr_node || | ||
| !fstring_add_part(&parts, &part_count, &part_capacity, expr_node)) { | ||
| fstring_cleanup_parts(parts, part_count); | ||
| free(format_spec); | ||
| fstring_cleanup_parts(parts, format_specs, part_count); | ||
| return NULL; | ||
| } | ||
|
|
||
| // Grow format_specs array if parts array grew | ||
| if (part_capacity > format_specs_capacity) { | ||
| char **new_specs = realloc(format_specs, part_capacity * sizeof(char *)); | ||
| if (!new_specs) { | ||
| free(format_spec); | ||
| fstring_cleanup_parts(parts, format_specs, part_count); | ||
| return NULL; | ||
| } | ||
| for (size_t j = format_specs_capacity; j < part_capacity; j++) { | ||
| new_specs[j] = NULL; | ||
| } | ||
| format_specs = new_specs; | ||
| format_specs_capacity = part_capacity; | ||
| } | ||
| format_specs[part_count - 1] = format_spec; |
There was a problem hiding this comment.
Format spec extraction logic is correct, but same growth bug exists.
The format specifier extraction correctly handles the colon position and properly allocates/copies the spec string. Empty format specs ({x:}) are correctly treated as no format spec.
However, lines 1580-1594 have the same potential out-of-bounds issue as the string literal case - if fstring_add_part succeeds but format_specs realloc fails, the cleanup will read past the array bounds.
🔧 Apply same fix pattern as the string literal case
// Grow format_specs array if parts array grew
if (part_capacity > format_specs_capacity) {
char **new_specs = realloc(format_specs, part_capacity * sizeof(char *));
if (!new_specs) {
free(format_spec);
- fstring_cleanup_parts(parts, format_specs, part_count);
+ fstring_cleanup_format_specs(format_specs, format_specs_capacity);
+ for (size_t k = 0; k < part_count; k++) {
+ ast_node_free(parts[k]);
+ }
+ free(parts);
return NULL;
}🤖 Prompt for AI Agents
In `@src/frontend/parser.c` around lines 1553 - 1594, The bug is that format_specs
can be left unresized after fstring_add_part succeeds, so if realloc fails the
cleanup may read past the smaller array; to fix, ensure format_specs is grown
before calling fstring_add_part when part_capacity will exceed
format_specs_capacity: check if part_capacity > format_specs_capacity, perform
realloc and initialize new_specs (updating format_specs_capacity) and handle
failure (free format_spec and call fstring_cleanup_parts) before invoking
fstring_add_part, then after fstring_add_part assign format_specs[part_count -
1] = format_spec; use the same symbols shown (format_specs,
format_specs_capacity, part_capacity, part_count, format_spec, fstring_add_part,
fstring_cleanup_parts) so the growth/realloc happens prior to adding the new
part.
Comment on lines
+2445
to
+2483
| static int parse_format_spec(const char *spec, size_t len, FormatSpec *out) { | ||
| out->fill_char = ' '; | ||
| out->align = '\0'; | ||
| out->width = 0; | ||
| out->precision = -1; | ||
| out->type = '\0'; | ||
|
|
||
| if (!spec || len == 0) { | ||
| return 0; // Empty spec is valid (default formatting) | ||
| } | ||
|
|
||
| size_t i = 0; | ||
|
|
||
| // Check for fill and align: [[fill]align] | ||
| // If second char is an align char, first char is fill | ||
| if (len >= 2 && (spec[1] == '<' || spec[1] == '>' || spec[1] == '^')) { | ||
| out->fill_char = spec[0]; | ||
| out->align = spec[1]; | ||
| i = 2; | ||
| } else if (len >= 1 && (spec[0] == '<' || spec[0] == '>' || spec[0] == '^')) { | ||
| out->align = spec[0]; | ||
| i = 1; | ||
| } | ||
|
|
||
| // Parse width | ||
| while (i < len && spec[i] >= '0' && spec[i] <= '9') { | ||
| out->width = out->width * 10 + (spec[i] - '0'); | ||
| i++; | ||
| } | ||
|
|
||
| // Parse precision (.N) | ||
| if (i < len && spec[i] == '.') { | ||
| i++; | ||
| out->precision = 0; | ||
| while (i < len && spec[i] >= '0' && spec[i] <= '9') { | ||
| out->precision = out->precision * 10 + (spec[i] - '0'); | ||
| i++; | ||
| } | ||
| } |
There was a problem hiding this comment.
Validate width/precision to avoid signed overflow and reject incomplete precision.
width/precision accumulate into int without overflow guards, which is UB on large specs. Also :. currently parses as precision 0, which is likely an invalid spec. Recommend bounds checks and requiring at least one digit after ..
🔧 Proposed fix
static int parse_format_spec(const char *spec, size_t len, FormatSpec *out) {
out->fill_char = ' ';
out->align = '\0';
out->width = 0;
out->precision = -1;
out->type = '\0';
@@
- while (i < len && spec[i] >= '0' && spec[i] <= '9') {
- out->width = out->width * 10 + (spec[i] - '0');
- i++;
- }
+ while (i < len && spec[i] >= '0' && spec[i] <= '9') {
+ int digit = spec[i] - '0';
+ if (out->width > (INT_MAX - digit) / 10) {
+ return -1;
+ }
+ out->width = out->width * 10 + digit;
+ i++;
+ }
@@
- if (i < len && spec[i] == '.') {
- i++;
- out->precision = 0;
- while (i < len && spec[i] >= '0' && spec[i] <= '9') {
- out->precision = out->precision * 10 + (spec[i] - '0');
- i++;
- }
- }
+ if (i < len && spec[i] == '.') {
+ i++;
+ if (i >= len || spec[i] < '0' || spec[i] > '9') {
+ return -1; // precision must have at least one digit
+ }
+ out->precision = 0;
+ while (i < len && spec[i] >= '0' && spec[i] <= '9') {
+ int digit = spec[i] - '0';
+ if (out->precision > (INT_MAX - digit) / 10) {
+ return -1;
+ }
+ out->precision = out->precision * 10 + digit;
+ i++;
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| static int parse_format_spec(const char *spec, size_t len, FormatSpec *out) { | |
| out->fill_char = ' '; | |
| out->align = '\0'; | |
| out->width = 0; | |
| out->precision = -1; | |
| out->type = '\0'; | |
| if (!spec || len == 0) { | |
| return 0; // Empty spec is valid (default formatting) | |
| } | |
| size_t i = 0; | |
| // Check for fill and align: [[fill]align] | |
| // If second char is an align char, first char is fill | |
| if (len >= 2 && (spec[1] == '<' || spec[1] == '>' || spec[1] == '^')) { | |
| out->fill_char = spec[0]; | |
| out->align = spec[1]; | |
| i = 2; | |
| } else if (len >= 1 && (spec[0] == '<' || spec[0] == '>' || spec[0] == '^')) { | |
| out->align = spec[0]; | |
| i = 1; | |
| } | |
| // Parse width | |
| while (i < len && spec[i] >= '0' && spec[i] <= '9') { | |
| out->width = out->width * 10 + (spec[i] - '0'); | |
| i++; | |
| } | |
| // Parse precision (.N) | |
| if (i < len && spec[i] == '.') { | |
| i++; | |
| out->precision = 0; | |
| while (i < len && spec[i] >= '0' && spec[i] <= '9') { | |
| out->precision = out->precision * 10 + (spec[i] - '0'); | |
| i++; | |
| } | |
| } | |
| static int parse_format_spec(const char *spec, size_t len, FormatSpec *out) { | |
| out->fill_char = ' '; | |
| out->align = '\0'; | |
| out->width = 0; | |
| out->precision = -1; | |
| out->type = '\0'; | |
| if (!spec || len == 0) { | |
| return 0; // Empty spec is valid (default formatting) | |
| } | |
| size_t i = 0; | |
| // Check for fill and align: [[fill]align] | |
| // If second char is an align char, first char is fill | |
| if (len >= 2 && (spec[1] == '<' || spec[1] == '>' || spec[1] == '^')) { | |
| out->fill_char = spec[0]; | |
| out->align = spec[1]; | |
| i = 2; | |
| } else if (len >= 1 && (spec[0] == '<' || spec[0] == '>' || spec[0] == '^')) { | |
| out->align = spec[0]; | |
| i = 1; | |
| } | |
| // Parse width | |
| while (i < len && spec[i] >= '0' && spec[i] <= '9') { | |
| int digit = spec[i] - '0'; | |
| if (out->width > (INT_MAX - digit) / 10) { | |
| return -1; | |
| } | |
| out->width = out->width * 10 + digit; | |
| i++; | |
| } | |
| // Parse precision (.N) | |
| if (i < len && spec[i] == '.') { | |
| i++; | |
| if (i >= len || spec[i] < '0' || spec[i] > '9') { | |
| return -1; // precision must have at least one digit | |
| } | |
| out->precision = 0; | |
| while (i < len && spec[i] >= '0' && spec[i] <= '9') { | |
| int digit = spec[i] - '0'; | |
| if (out->precision > (INT_MAX - digit) / 10) { | |
| return -1; | |
| } | |
| out->precision = out->precision * 10 + digit; | |
| i++; | |
| } | |
| } |
Comment on lines
+2571
to
+2594
| if (value->type == VAL_NUMBER) { | ||
| double num = value->as.number; | ||
|
|
||
| if (spec->type == 'f' || spec->precision >= 0) { | ||
| // Floating-point format | ||
| int prec = (spec->precision >= 0) ? spec->precision : 6; | ||
| int written = snprintf(buf, sizeof(buf), "%.*f", prec, num); | ||
| if (written < 0 || (size_t)written >= sizeof(buf)) { | ||
| vm_error(vm, KRONOS_ERR_RUNTIME, "Number too large to format"); | ||
| return NULL; | ||
| } | ||
| str = buf; | ||
| str_len = (size_t)written; | ||
| } else if (spec->type == 'd') { | ||
| // Integer format | ||
| long long int_val = (long long)num; | ||
| int written = snprintf(buf, sizeof(buf), "%lld", int_val); | ||
| if (written < 0 || (size_t)written >= sizeof(buf)) { | ||
| vm_error(vm, KRONOS_ERR_RUNTIME, "Number too large to format"); | ||
| return NULL; | ||
| } | ||
| str = buf; | ||
| str_len = (size_t)written; | ||
| } else { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd src/vm && wc -l vm.cRepository: nedanwr/kronos
Length of output: 67
🏁 Script executed:
sed -n '2571,2594p' src/vm/vm.cRepository: nedanwr/kronos
Length of output: 942
🏁 Script executed:
# Check if isfinite is available in the codebase
rg -t c '#include.*math' src/vm/Repository: nedanwr/kronos
Length of output: 87
🏁 Script executed:
# Check for any existing isfinite usage
rg -t c 'isfinite' src/vm/Repository: nedanwr/kronos
Length of output: 40
🏁 Script executed:
# Check if limits.h is included
rg -t c '#include.*limits' src/vm/vm.cRepository: nedanwr/kronos
Length of output: 77
🏁 Script executed:
# Check if LLONG_MIN or LLONG_MAX are used anywhere
rg -t c 'LLONG_(MIN|MAX)' src/vm/Repository: nedanwr/kronos
Length of output: 40
🏁 Script executed:
# Search for other double-to-long-long casts
rg -t c '\(long long\).*num' src/vm/Repository: nedanwr/kronos
Length of output: 111
Guard integer formatting against NaN/Inf/out-of-range and disallow precision with d.
Casting a double to long long without range checks is undefined behavior for NaN/Inf/out-of-range values. Additionally, the condition spec->type == 'f' || spec->precision >= 0 causes requests for d format with precision to incorrectly route through float formatting. If d is intended as integer format, reject precision explicitly.
🔧 Proposed fix
- if (spec->type == 'f' || spec->precision >= 0) {
+ if (spec->type == 'd') {
+ if (spec->precision >= 0) {
+ vm_error(vm, KRONOS_ERR_RUNTIME,
+ "Precision not supported for integer format");
+ return NULL;
+ }
+ if (!isfinite(num) || num > (double)LLONG_MAX ||
+ num < (double)LLONG_MIN) {
+ vm_error(vm, KRONOS_ERR_RUNTIME, "Integer value out of range");
+ return NULL;
+ }
+ long long int_val = (long long)num;
+ int written = snprintf(buf, sizeof(buf), "%lld", int_val);
+ if (written < 0 || (size_t)written >= sizeof(buf)) {
+ vm_error(vm, KRONOS_ERR_RUNTIME, "Number too large to format");
+ return NULL;
+ }
+ str = buf;
+ str_len = (size_t)written;
+ } else if (spec->type == 'f' || spec->precision >= 0) {
// Floating-point format
int prec = (spec->precision >= 0) ? spec->precision : 6;
int written = snprintf(buf, sizeof(buf), "%.*f", prec, num);
if (written < 0 || (size_t)written >= sizeof(buf)) {
vm_error(vm, KRONOS_ERR_RUNTIME, "Number too large to format");
return NULL;
}
str = buf;
str_len = (size_t)written;
- } else if (spec->type == 'd') {
- // Integer format
- long long int_val = (long long)num;
- int written = snprintf(buf, sizeof(buf), "%lld", int_val);
- if (written < 0 || (size_t)written >= sizeof(buf)) {
- vm_error(vm, KRONOS_ERR_RUNTIME, "Number too large to format");
- return NULL;
- }
- str = buf;
- str_len = (size_t)written;
} else {🤖 Prompt for AI Agents
In `@src/vm/vm.c` around lines 2571 - 2594, The integer-formatting path
incorrectly allows precision to force floating formatting and casts doubles to
long long without checking for NaN/Inf or out-of-range values; update the branch
in the formatting code handling value->type == VAL_NUMBER (and the logic using
spec->type and spec->precision) to: (1) treat 'd' as a strict integer format and
explicitly reject any spec->precision >= 0 when spec->type == 'd' by calling
vm_error(KRONOS_ERR_RUNTIME); (2) before casting the double to long long for the
integer path, check for isnan(value->as.number), isinf(value->as.number) and
that the value lies within LLONG_MIN..LLONG_MAX (use appropriate macros) and
call vm_error(KRONOS_ERR_RUNTIME) on failure; and (3) only use the
floating-point snprintf("%.*f", ...) when spec->type == 'f' (or other floating
specifiers) and spec->precision is allowed — adjust the conditional that
currently reads spec->type == 'f' || spec->precision >= 0 accordingly so
precision alone does not route to float formatting.
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.
Summary
Add Python-like format specifiers to f-strings, enabling precision, width, alignment, and type control for formatted output.
Changes
format_specsparallel array to f-string AST structure for storing format specifiers:in f-string expressions (e.g.,{price:.2f})OP_FORMAT_VALUEopcode to compiler for emitting formatted value instructions.2f,.4f,.0f:10,:<10,:>10,:^10:0>5,:->5,:*^9:d,:f,:sTesting
New test files:
fstrings_format_precision.krfstrings_format_width.krfstrings_format_fill.krfstrings_format_combined.krAll 133 integration tests pass. Valgrind reports no memory leaks.
Documentation
Updated:
website/content/docs/language/strings.mdx- Full format specifier documentationwebsite/content/docs/quick-reference.mdx- Quick reference examplewebsite/content/docs/roadmap.mdx- Added feature to v0.5.0 sectionBreaking Changes
None
Summary by CodeRabbit
New Features
f"{price:.2f}")Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.