feat: REPL highlighting + live completion, forge doc/fmt fixes#9
Conversation
REPL: - Syntax highlighting: keywords (magenta), builtins (blue), modules (green), strings (yellow), numbers (cyan), comments (dim) - Tab completion now includes user-defined variables and functions from the interpreter environment, updated after each evaluation - `env` command shows all defined variables instead of just _last forge doc: - Fix variable extraction: let/let mut declarations now appear in output - Extract // comments preceding declarations using SpannedStmt.line - Remove unused `self` import Interpreter: - Add Environment::all_names() for REPL tab completion
Formatter: - Add parenthesis depth tracking for multi-line function call indentation - 2 new tests (13 total formatter tests) forge doc: - Fix let/let mut declarations being silently skipped in doc output - Implement comment extraction using SpannedStmt.line - Remove unused ast self-import Roadmap: Phase 1 items 1.6-1.8 complete
humancto
left a comment
There was a problem hiding this comment.
Review: REPL highlighting + live completion, forge doc/fmt fixes
Nice batch of improvements — clean, focused changes across three areas with good test coverage. The REPL syntax highlighting, doc generator fixes, and formatter paren tracking are all well-implemented.
What looks good
extract_preceding_comments— Clean implementation. The 1-based → 0-based conversion viaspanned.line.saturating_sub(1)is correct, and the backward walk with proper bounds checking is solid.Environment::all_names()— Good use ofunwrap_or_else(|p| p.into_inner())for poisoned mutex recovery, consistent with the rest of the interpreter.Arc<Mutex<Vec<String>>>for shared tab-completion state — The right pattern for sharing mutable state between theHelperand the REPL main loop.forge fmtparen tracking — Minimal, correct change. The two new tests cover the important cases.forge docStmt::Lethandling — Removes the_ => {}catch-all swallowing declarations. Good catch.- CHANGELOG and ROADMAP updates — Well-structured entries.
- Unused
ast::selfimport cleanup — Correct; nothing uses theast::prefix.
Issues
- Bug (highlighter):
true/falseboolean highlighting is dead code — they're already in theKEYWORDSarray so they match the keyword branch first and get magenta, never reaching the intended cyan branch. See inline comment. - Minor (highlighter): Number highlighting consumes
..(range operator) as part of the number.1..10highlights as one cyan token instead of1,..,10. - Minor (highlighter): Single-quoted strings aren't highlighted — the formatter already treats
'as a string delimiter, so the highlighter should too for consistency. - Nit (formatter):
count_bracesnow counts parens but the name doesn't reflect it.
None of these are blocking — the dead-code boolean bug (#1) is worth fixing before merge, the rest are minor polish.
Generated by Claude Code
| result.push_str("\x1B[0m"); | ||
| } else if word == "true" || word == "false" { | ||
| result.push_str("\x1B[36m"); // cyan for booleans | ||
| result.push_str(&word); |
There was a problem hiding this comment.
Bug: true and false are already in the KEYWORDS array (lines 148-149), so they'll be matched by the KEYWORDS.contains() check above and highlighted in magenta. This else if branch is unreachable dead code.
To get the intended cyan highlighting for booleans, either:
- Remove
"true"and"false"fromKEYWORDSand rely on this explicit check, or - Move this check before the
KEYWORDS.contains()check
Option 1 seems cleaner since true/false are really literals, not keywords.
Generated by Claude Code
| // Numbers | ||
| if chars[i].is_ascii_digit() { | ||
| result.push_str("\x1B[36m"); // cyan | ||
| while i < len && (chars[i].is_ascii_digit() || chars[i] == '.') { | ||
| result.push(chars[i]); | ||
| i += 1; |
There was a problem hiding this comment.
Minor: This will consume the range operator .. as part of a number literal. For example, range(1..10) would highlight 1..10 as a single cyan number instead of 1, .., 10.
Consider tightening to allow at most one . (and only if followed by a digit):
if chars[i].is_ascii_digit() {
result.push_str("\x1B[36m");
while i < len && chars[i].is_ascii_digit() {
result.push(chars[i]);
i += 1;
}
// Allow one decimal point followed by digits
if i < len && chars[i] == '.' && i + 1 < len && chars[i + 1].is_ascii_digit() {
result.push(chars[i]);
i += 1;
while i < len && chars[i].is_ascii_digit() {
result.push(chars[i]);
i += 1;
}
}
result.push_str("\x1B[0m");
continue;
}Not blocking, but would improve highlighting accuracy for Forge's range() and similar patterns.
Generated by Claude Code
| result.push_str("\x1B[33m\""); // yellow | ||
| i += 1; | ||
| while i < len && chars[i] != '"' { | ||
| if chars[i] == '\\' && i + 1 < len { |
There was a problem hiding this comment.
Nit: The highlighter handles double-quoted strings but not single-quoted ones. The formatter's count_braces already treats both '"' and '\'' as string delimiters, so Forge does support single-quoted strings. Adding a chars[i] == '\'' branch here (same logic, same color) would be a nice consistency improvement.
Generated by Claude Code
|
|
||
| // Count braces and brackets outside strings | ||
| if c == '{' || c == '[' { | ||
| // Count braces, brackets, and parens outside strings |
There was a problem hiding this comment.
Nit: Now that this function counts parentheses too, the name count_braces is slightly misleading. Consider renaming to count_delimiters or count_nesting. Not blocking — just a readability note since the test name count_braces_includes_parens already hints at the mismatch.
Generated by Claude Code
…xes, rename - Move true/false check before KEYWORDS check so booleans get cyan (not magenta) - Numbers no longer consume range operator (..) — require digit after dot - Add single-quoted string highlighting - Rename count_braces to count_delimiters (now tracks braces, brackets, parens)
Summary
Phase 1 final batch — REPL, formatter, and doc generator improvements.
REPL:
envcommand shows all defined variables and values (not just_last)forge doc:
let/let mutdeclarations being silently skipped (the_ => {}catch-all was swallowing them)//comments preceding declarations are now extracted and displayed usingSpannedStmt.lineforge fmt:
Infrastructure:
Environment::all_names()method for REPL tab completionTest plan
forge run examples/hello.fgpassesforge run examples/functional.fgpassesforge repl— verify syntax colors, define a variable, tab-complete itforge doc examples/— verify variables and comments appearforge fmt --checkon a file with multi-line function call