# mojo-toml v0.2.0 - Code Review

**Reviewer:** Warp AI Agent\
**Date:** 2026-01-07\
**Version:** v0.2.0 (commit: c002d57)\
**Scope:** Full codebase review for issues, simplification opportunities, and improvements

------------------------------------------------------------------------


# Executive Summary

Overall, the codebase is **well-structured** and demonstrates solid engineering practices. The nested table implementation is correct and tests are comprehensive. This review identifies several opportunities for improvement, ranging from minor simplifications to potential correctness issues.


## Severity Levels

- 🔴 **CRITICAL**: Correctness issues that may cause bugs
- 🟠 **IMPORTANT**: Performance or usability issues that should be addressed
- 🟡 **MODERATE**: Code quality improvements worth considering
- 🟢 **MINOR**: Style/documentation suggestions

------------------------------------------------------------------------


# Critical Issues 🔴


## 1. Dotted Key Implementation is Incomplete (Line 614-617)

**File:** `src/toml/parser.mojo`

**Issue:**

``` mojo
# For simple key (not dotted), return it
if len(key_parts) == 1:
    return KeyValuePair(key_parts[0], value^)
else:
    # TODO: Handle dotted keys by creating nested dicts
    # For now, just use the last part
    return KeyValuePair(key_parts[len(key_parts) - 1], value^)
```

**Problem:** Dotted keys in key-value pairs (e.g. `a.b.c = "value"`) are parsed but only the last key is used. This is **incorrect behaviour** according to TOML spec and will silently produce wrong results.

**Impact:** Users writing `database.host = "localhost"` will get `host = "localhost"` at the current table level instead of creating nested structure.

**Recommendation:** - Either implement proper dotted key support now (creates nested tables like `[a.b]`) - OR raise an error: `raise Error("Dotted keys not yet supported")` - Don't silently do the wrong thing

**Suggested Fix:**

``` mojo
else:
    # Dotted keys not yet supported - fail explicitly
    raise Error("Dotted keys in key-value pairs not yet supported: use [" + 
                key_parts[0] + "." + key_parts[1] + "...] table headers instead")
```

------------------------------------------------------------------------


## 2. No Duplicate Key Detection

**File:** `src/toml/parser.mojo`

**Issue:** Parser allows overwriting keys without warning.

**Example:**

``` toml
name = "first"
name = "second"  # Should raise error but silently overwrites
```

**TOML Spec:** "Defining a key multiple times is invalid."

**Impact:** Configuration errors go undetected, potentially causing production issues.

**Recommendation:** Add duplicate key detection in v0.3.0. For now, document this limitation clearly in README.

------------------------------------------------------------------------


## 3. Error Messages Lack Context

**File:** `src/toml/parser.mojo` (multiple locations)

**Issue:** Most errors don't include line/column information despite Position being tracked.

**Example:**

``` mojo
raise Error("Expected key in table header")  # Which line?
```

**Impact:** Users can't easily fix syntax errors in large TOML files.

**Recommendation:**

``` mojo
raise Error("Expected key in table header at line " + 
            str(token.pos.line) + ", column " + str(token.pos.column))
```

Add helper method:

``` mojo
fn format_error(self, message: String, pos: Position) -> String:
    return message + " at line " + str(pos.line) + ", column " + str(pos.column)
```

------------------------------------------------------------------------


# Important Issues 🟠


## 4. Excessive Copying in `as_table()` and `as_array()`

**File:** `src/toml/parser.mojo` (lines 197-205, 187-195)

**Issue:**

``` mojo
fn as_table(self) raises -> Dict[String, TomlValue]:
    if not self.is_table():
        raise Error("Value is not a table")
    # Return a copy of the table
    var result = Dict[String, TomlValue]()
    for entry in self.table_value.items():
        result[entry.key] = entry.value.copy()  # Deep copy!
    return result^
```

**Problem:** Every call to `as_table()` performs a deep copy of the entire nested structure. For large TOML files with deep nesting, this is expensive.

**Impact:** Performance degrades with file size. Parsing pixi.toml repeatedly copies the same data.

**Recommendation:** - Consider adding `as_table_ref()` that returns a reference/borrow (if Mojo supports this pattern) - OR document that users should call `as_table()` once and store the result - OR investigate if Dict can be returned by reference with proper lifetime annotations

**Note:** This may be a Mojo limitation. If ownership model requires copies, document this in README under "Performance Considerations".

------------------------------------------------------------------------


## 5. Parser Functions Return Functional Copies

**File:** `src/toml/parser.mojo` (lines 494-534, 536-568)

**Issue:** `ensure_table_path()` and `set_in_table_path()` create new Dict instances recursively:

``` mojo
var new_result = Dict[String, TomlValue]()
for entry in result.items():
    new_result[entry.key] = entry.value.copy()  # Copy entire dict
```

**Problem:** For deeply nested tables, this creates O(depth × keys) copies during parsing.

**Example:** Parsing `[a.b.c.d.e]` creates 5 full dict copies even if adding one key.

**Impact:** Parsing performance degrades with nesting depth.

**Recommendation:** - If Mojo supports in-place mutation, refactor to mutate the dict directly - Otherwise, document this as a known performance characteristic - Consider benchmarking against real-world TOML files

**Alternative Approach:** Could you refactor to mutate `result` in place? Something like:

``` mojo
fn ensure_table_path_mut(mut self, mut result: Dict[String, TomlValue], path: List[String]) raises:
    # Mutate result directly instead of copying
```

This depends on whether Mojo's ownership model allows this pattern.

------------------------------------------------------------------------


## 6. `current_table_path` is Mutable State

**File:** `src/toml/parser.mojo` (line 222, 657)

**Issue:** Parser maintains `current_table_path` that gets updated during parsing:

``` mojo
self.current_table_path = self.parse_table_header()
```

**Problem:** Makes parser non-reentrant and harder to reason about. If parsing fails midway, state is corrupted.

**Impact:** Can't reuse same Parser instance for multiple files. Debugging is harder.

**Recommendation:** Consider passing `current_path` as a parameter through recursive calls instead of storing as field. This makes parsing more functional.

**Alternative:** If keeping mutable state, add `reset()` method to allow parser reuse.

------------------------------------------------------------------------


# Moderate Issues 🟡


## 7. TomlValue Type Discrimination Uses Magic Numbers

**File:** `src/toml/parser.mojo` (lines 53, 121-137)

**Issue:**

``` mojo
var value_type: Int  # 0=STRING, 1=INTEGER, 2=FLOAT, 3=BOOLEAN, 4=ARRAY, 5=TABLE
```

**Problem:** Magic numbers are error-prone and hard to maintain.

**Recommendation:** Use constants or enum-like pattern:

``` mojo
alias TYPE_STRING = 0
alias TYPE_INTEGER = 1
alias TYPE_FLOAT = 2
alias TYPE_BOOLEAN = 3
alias TYPE_ARRAY = 4
alias TYPE_TABLE = 5
```

Or better, if Mojo supports:

``` mojo
struct TomlValueType:
    alias STRING = 0
    alias INTEGER = 1
    alias FLOAT = 2
    alias BOOLEAN = 3
    alias ARRAY = 4
    alias TABLE = 5
```

Then use: `self.value_type = TomlValueType.STRING`

------------------------------------------------------------------------


## 8. Redundant Copy Operations

**File:** `src/toml/parser.mojo` (lines 658-664, 677-681)

**Issue:**

``` mojo
# Copy path to avoid aliasing issues
var path_copy = List[String]()
for i in range(len(self.current_table_path)):
    path_copy.append(self.current_table_path[i])
```

**Problem:** This pattern appears twice. If it's necessary, it should be a helper method.

**Recommendation:**

``` mojo
fn copy_path(path: List[String]) -> List[String]:
    var result = List[String]()
    for i in range(len(path)):
        result.append(path[i])
    return result^
```

Then use: `var path_copy = copy_path(self.current_table_path)`

**Question:** Is this copy actually necessary? Does Mojo's List have value semantics already? If so, just: `var path_copy = self.current_table_path` might work.

------------------------------------------------------------------------


## 9. Unused String Copies in Parser

**File:** `src/toml/parser.mojo` (lines 673-674)

**Issue:**

``` mojo
var parsed_key = String(pair.key)      # Why copy?
var parsed_value = pair.value.copy()   # Why copy?
```

**Problem:** These copies might be unnecessary if `pair.key` is already a String.

**Recommendation:** Test if these copies can be removed. They add overhead for every key-value pair parsed.

------------------------------------------------------------------------


## 10. Lexer Escape Handling is Limited

**File:** `src/toml/lexer.mojo` (lines 336-357)

**Issue:** Only handles basic escapes: `\n`, `\t`, `\\`, `\"`, `\'`

**Missing:** - Unicode escapes: `\u0041`, `\U00000041` - Other escapes: `\b`, `\f`, `\r`

**Impact:** Some valid TOML files won't parse correctly.

**Recommendation:** Add to v0.3.0 roadmap. Document limitation in README.

------------------------------------------------------------------------


## 11. Array Trailing Comma Logic

**File:** `src/toml/parser.mojo` (lines 387-395)

**Issue:**

``` mojo
if token.kind == TokenKind.COMMA():
    _ = self.advance()
    self.skip_whitespace_and_newlines()
    token = self.current()
    if token.kind == TokenKind.RIGHT_BRACKET():
        _ = self.advance()
        break
```

**Problem:** Trailing commas are allowed in arrays but forbidden in inline tables (line 345). This is correct per TOML spec, but the different handling could confuse maintainers.

**Recommendation:** Add comment explaining the difference:

``` mojo
# Arrays allow trailing commas per TOML spec
if token.kind == TokenKind.RIGHT_BRACKET():
```

------------------------------------------------------------------------


## 12. Special Float Handling Uses String Comparison

**File:** `src/toml/parser.mojo` (lines 437-442)

**Issue:**

``` mojo
if token.value == "inf":
    return TomlValue(inf[DType.float64]())
elif token.value == "-inf":
    return TomlValue(-inf[DType.float64]())
elif token.value == "nan":
    return TomlValue(nan[DType.float64]())
```

**Problem:** String comparisons for every float. Could use TokenKind metadata instead.

**Recommendation:** Minor optimization - lexer could set a flag for special floats. Low priority.

------------------------------------------------------------------------


# Minor Issues 🟢


## 13. Inconsistent Comment Style

**Files:** Multiple

**Issue:** Some files use `#` comments, others use docstrings. `TomlValue` has inline comments for types but `TokenKind` uses docstrings.

**Recommendation:** Standardize on docstrings for struct fields where possible.

------------------------------------------------------------------------


## 14. Module Docstrings Could Be More Concise

**Files:** `lexer.mojo`, `parser.mojo`

**Issue:** The "Why/What/How" docstrings are great for learning but quite long.

**Recommendation:** Move detailed explanations to `docs/ARCHITECTURE.md` and keep module docstrings shorter.

------------------------------------------------------------------------


## 15. Test File Has No `main()` Runner

**File:** `tests/test_basic.mojo`

**Issue:** Unlike `test_tables.mojo` which has a `main()` function, `test_basic.mojo` doesn't.

**Recommendation:** Either all test files should have `main()` or none. Check if test runner expects this.

------------------------------------------------------------------------


## 16. Unused Variable in Parser

**File:** `src/toml/parser.mojo` (line 468)

**Issue:**

``` mojo
var pos = Position(self.line, self.column)
```

This `pos` is defined but used inconsistently (sometimes used for token creation, sometimes not).

**Recommendation:** Clean up - either always use `pos` or never store it.

------------------------------------------------------------------------


## 17. EOF Token Value

**File:** `src/toml/lexer.mojo` (line 465)

**Issue:** EOF token has empty string value: `Token(TokenKind.EOF(), "", Position(...))`

**Recommendation:** Consider using a sentinel value like `"<EOF>"` for debugging clarity.

------------------------------------------------------------------------


## 18. Position Struct Could Be Simplified

**File:** `src/toml/lexer.mojo` (lines 40-52)

**Issue:** Position is `@register_passable("trivial")` but could benefit from a `__str__` method for error messages.

**Recommendation:**

``` mojo
fn __str__(self) -> String:
    return "line " + str(self.line) + ", column " + str(self.column)
```

------------------------------------------------------------------------


# Code Quality Observations


## Strengths ✅

1.  **Excellent separation of concerns**: Lexer and Parser are cleanly separated
2.  **Comprehensive tests**: 79 tests covering edge cases
3.  **Good documentation**: Docstrings explain purpose and usage
4.  **Proper error handling**: Uses `raises` appropriately
5.  **Nested structure implementation**: The Dict iteration pattern is correct and well-tested
6.  **Real-world validation**: pixi.toml parsing demonstrates practical usage


## Areas for Improvement 📈

1.  **Performance**: Excessive copying could be reduced
2.  **Error messages**: Need line/column context
3.  **Incomplete features**: Dotted keys silently do wrong thing
4.  **Validation**: No duplicate key detection
5.  **Code reuse**: Some patterns repeated (path copying)

------------------------------------------------------------------------


# Recommendations by Priority


## High Priority (Do Now or Soon)

1.  **Fix dotted key bug** - Either implement or raise error (don't silently fail)
2.  **Add line/column to error messages** - Critical for usability
3.  **Document performance characteristics** - Users should know about copying behaviour


## Medium Priority (v0.3.0)

4.  **Add duplicate key detection** - Required for TOML 1.0 compliance
5.  **Optimize copying** - Profile real-world files and reduce copies if possible
6.  **Complete escape sequence support** - Unicode escapes needed
7.  **Replace magic numbers** - Use named constants for type enum


## Low Priority (Future)

8.  **Refactor mutable state** - Make parser more functional
9.  **Standardize test structure** - All test files should be consistent
10. **Extract helper methods** - Reduce duplication (path copying, error formatting)

------------------------------------------------------------------------


# Testing Gaps

All current tests pass, but consider adding:

1.  **Error cases**: Invalid TOML that should raise specific errors
2.  **Performance tests**: Large files with deep nesting
3.  **Edge cases**: Empty tables, empty arrays, nested empty structures
4.  **Regression tests**: For the dotted key bug when fixed
5.  **Fuzz testing**: Random TOML generation to find corner cases

------------------------------------------------------------------------


# Security Considerations

No obvious security issues, but note:

1.  **No input size limits**: Very large TOML files could cause memory issues
2.  **Recursive depth**: Deeply nested structures could overflow stack (though unlikely)
3.  **String escapes**: Incomplete Unicode handling could allow bypass of validation

For a parser library, these are acceptable. Document them if targeting production use.

------------------------------------------------------------------------


# Documentation Suggestions

Current docs are good. Consider adding:

1.  **Performance guide**: When to worry about copying, how to optimize usage
2.  **Migration guide**: For when dotted keys are implemented
3.  **Limitations page**: Clear list of TOML 1.0 features not yet supported
4.  **Architecture doc**: Move detailed design explanations from code to docs

------------------------------------------------------------------------


# Final Assessment

**Overall Grade: B+**

The codebase is well-structured and demonstrates good engineering practices. The nested table implementation is correct and represents a significant breakthrough. However, there are some correctness issues (dotted keys) and performance concerns (excessive copying) that should be addressed.

**Recommendation:** - Fix the dotted key bug immediately (high risk of user confusion) - Improve error messages before wider adoption - Profile performance with real-world files to validate copy overhead - Document limitations clearly

The code is **ready for v0.2.0 release** as planned, but should have these issues addressed before v0.3.0 "TOML 1.0 Compliance" release.

------------------------------------------------------------------------

**Review Complete**\
If you'd like me to elaborate on any section or provide specific code examples for fixes, let me know!