Skip to content

Add dot-access syntax for maps (u.foo)#8

Merged
aisrael merged 3 commits into
0.1.1from
feat/dot-field-access
Jul 2, 2026
Merged

Add dot-access syntax for maps (u.foo)#8
aisrael merged 3 commits into
0.1.1from
feat/dot-field-access

Conversation

@aisrael

@aisrael aisrael commented Jul 1, 2026

Copy link
Copy Markdown
Owner

Summary

  • Adds a highest-precedence postfix .field operator so u.foo reads a field out of a map (e.g. u = {foo: "bar"}; u.foo"bar")
  • Fixes a prerequisite bug: Expr::MapLiteral was never evaluated into Value::Map at all — map literals unconditionally raised InvalidOperandType

Changes

  • src/ast/expr.rs: new Expr::FieldAccess(Box<Expr>, String) variant, Display impl, field_access constructor, and Display tests
  • src/runtime.rs: MapLiteral now evaluates to Value::Map; new FieldAccess eval arm (looks up the key on Value::Map, InvalidOperandType on non-map bases)
  • src/errors.rs: new RuntimeError::NoSuchField(String) for missing keys
  • src/parser/expr.rs: new parse_postfix layer (plus a mirrored parse_if_condition_postfix for if-conditions) wired in between parse_unary and parse_primary, so .field binds tighter than unary operators (-u.foo == -(u.foo)) and works in if conditions (if u.active ...); . binds tightly with no surrounding whitespace, matching this codebase's existing "tight" unary-operator convention
  • src/eval/mod.rs, tests/features.rs, features/ast/dot_access.feature: new tests covering plain/chained access, missing-field and non-map errors, and parse-level precedence/if-condition behavior

Testing

  • cargo test — 174 tests passing (unit, integration, and cucumber feature suites)
  • cargo clippy --all-targets — no issues
  • Manually verified via the REPL:
    u = {foo: "bar"}
    u.foo        # => "bar"
    u.missing    # => eval error: RuntimeError(NoSuchField("missing"))
    
    and nested access u = {a: {b: 42}}; u.a.b42

Checklist

  • Tests pass
  • Documentation updated (doc comments on new AST/parser items)
  • No breaking changes

aisrael and others added 2 commits July 1, 2026 17:05
Adds a highest-precedence postfix `.field` operator, parsed via a new
parse_postfix layer (mirrored for if-conditions) and evaluated against
Value::Map. Also fixes a prerequisite gap where MapLiteral expressions
were never evaluated into Value::Map at all.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Line in the new FieldAccess eval arm exceeded rustfmt's width limit.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@aisrael

aisrael commented Jul 2, 2026

Copy link
Copy Markdown
Owner Author

@claude Review this PR

@claude

claude Bot commented Jul 2, 2026

Copy link
Copy Markdown

Claude finished @aisrael's task in 4m 16s —— View job


Code Review: Add dot-access syntax for maps (u.foo)

Todo list

  • Gather context (read repo AGENTS.md, changed files, diff vs origin/0.1.1)
  • Understand the request (code review of dot-access PR)
  • Review src/ast/expr.rs changes
  • Review src/errors.rs changes
  • Review src/runtime.rs changes
  • Review src/parser/expr.rs changes (postfix parsing)
  • Review src/eval/mod.rs and test changes
  • Post final review feedback

Summary

This is a clean, well-scoped change. The prerequisite bug fix (MapLiteral never evaluating to Value::Map) is a real and valuable catch — the CHANGELOG explicitly documented that limitation. The new .field postfix parser is threaded consistently through every precedence layer that needs it (parse_unary/parse_unary_tight and their if-condition mirrors), so precedence claims in the PR description (-u.foo == -(u.foo), works in if conditions) check out by inspection. Test coverage is solid across Display, parser, eval, and cucumber-feature layers. CI is green on this PR (verified via mcp__github_ci__get_ci_status).

Observations (non-blocking)

  1. HashMap key ordering is now user-visibleValue::Map's Display impl (src/runtime.rs, ~line 58) iterates a std::collections::HashMap, whose order isn't guaranteed. That code pre-dates this PR, but since MapLiteral previously always errored, nobody could actually observe it. Now that map literals evaluate successfully, printing a map with 2+ keys (e.g. {a: 1, b: 2} in the REPL) can show keys in a nondeterministic order. Worth a follow-up to switch to an ordered map (e.g. indexmap::IndexMap<String, Value>) or sort keys in Display.
    Fix this →

  2. Number-literal / dot interactionsrc/parser/number.rs's fractional_digits falls back to digit0 (i.e. zero digits is OK) whenever a . follows a number, so 3.foo has its . swallowed by the number parser itself (yielding 3. plus unconsumed foo) rather than being parsed as FieldAccess(3, "foo") like other primaries (u.foo, {...}.foo, (x).foo) are. Pre-existing number-parser behavior, not introduced here, and field access on a bare numeric literal is a nonsensical program anyway — just flagging the inconsistency in case it ever matters (e.g. confusing parse-error messages instead of a clean InvalidOperandType runtime error).

  3. Minor nitsrc/runtime.rs:214, m.get(field.as_str()) can just be m.get(field) since field: &String and HashMap<String, Value>::get accepts &Q via Borrow. Purely cosmetic.

  4. Parser duplicationparse_postfix and parse_if_condition_postfix (and their interplay with the already-duplicated unary/tight variants) are structurally identical aside from which primary parser they wrap. This mirrors the codebase's pre-existing parse_unary vs parse_if_condition_unary duplication pattern, so it's consistent with precedent rather than new debt — just one more copy to keep in sync if precedence rules change again.

  5. No CHANGELOG.md entry — there's a v0.1.0 section but nothing yet for 0.1.1 (this PR's target branch). Optional, since AGENTS.md doesn't mandate it, but worth considering given this is a user-facing language feature plus a real bug fix.

None of the above are blockers — the implementation is correct, thoroughly tested, and consistent with the codebase's existing conventions.
(branch)

…add CHANGELOG

- Sort keys before printing Value::Map so Display output is deterministic
  now that map literals actually evaluate.
- Simplify m.get(field.as_str()) to m.get(field) (Borrow coercion).
- Add CHANGELOG entry for v0.1.1 covering the MapLiteral fix and dot-access syntax.

Addresses github.com//pull/8#issuecomment-4861585295 items 1, 3, 5.
Items 2 (number-literal/dot parsing) and 4 (parser duplication) left as-is per
the reviewer's own assessment that they're pre-existing / consistent with
precedent, not defects introduced by this PR.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@aisrael

aisrael commented Jul 2, 2026

Copy link
Copy Markdown
Owner Author

Addressed the review feedback in a7d78be:

Left as-is: #2 (number-literal/dot interaction) and #4 (parser duplication) — both were explicitly flagged as pre-existing behavior / consistent with existing precedent rather than defects introduced by this PR, so no change needed there.

@aisrael aisrael merged commit 4ada3d9 into 0.1.1 Jul 2, 2026
1 check passed
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.

1 participant