Skip to content

feat: add struct definitions with field access and assignment πŸ—οΈ - #197

Merged
timfennis merged 24 commits into
masterfrom
feature/structs
Aug 26, 2026
Merged

feat: add struct definitions with field access and assignment πŸ—οΈ#197
timfennis merged 24 commits into
masterfrom
feature/structs

Conversation

@timfennis

@timfennis timfennis commented Aug 23, 2026

Copy link
Copy Markdown
Owner

Context

While attempting to implement an advent of code puzzle I needed access to linked lists because currently that can only be emulated with maps which is cumbersome to read and write. This PR is a step in the right direction, but since self references are not allowed yet it doesn't really solve the problem πŸͺ¦.

What this adds

struct Point { x: Int, y: Int }

let p = Point(1, 2);   // positional constructor
p.x                    // getter β€” exactly `x(p)`, nothing special
p.x = 10;              // setter β€” calls the `x=` binding
p.x += 5;              // augmented, receiver evaluated once
  • Lexer/parser/AST β€” struct keyword, field declarations with required type
    annotations, trailing commas allowed.
  • StructRegistry in ndc_core β€” owns StructInfo; shared by the analyser and
    compiler via Rc<RefCell<…>> so the REPL keeps struct definitions across lines.
  • Accessors are ordinary function values. A declaration binds a constructor, plus a
    getter and setter per field. Identity is nominal: two structs with an x field produce
    two distinct getters, so overload resolution picks the right one.
  • Expression::MemberAccess β€” p.x is its own AST node rather than a desugared call,
    while p.x() still lowers to x(p). This is what makes assignability decidable at
    parse time: p.x = v is valid, p.x() = v is not.
  • VM β€” Object::Struct plus Function::{Constructor, GetField, SetField}, all
    completing inline without pushing a frame.
  • Editor support β€” tree-sitter grammar and corpus tests, TextMate grammar, and LSP
    document symbols, inlay hints and go-to-definition.
  • Struct names are types. fn dist(p: Point), let p: Point = ...,
    struct Line { a: Point }, and List<Point> all resolve (built on the TypeExpr
    refactor from refactor(parser): resolve type annotations in the analyser 🧭 #198; built-in names take precedence). Struct names are globally
    unique β€” redeclaration is an analysis error β€” and the analyser checkpoint rolls the
    registry back, so a failed REPL line frees its struct names.

Bugs found while building this

Three panics, two of which predate the branch:

  • Passing a constructor or accessor to a HOF (points.map(x)) corrupted the VM stack and
    overran the chunk. call_callback assumed every dispatch pushed a frame; dispatch now
    reports which happened. This also fixes pure fn as a HOF callback, which hit
    unreachable! on master.
  • let l[0] = 5 panicked in the compiler (pre-existing) and let p.x = 5 in the
    analyser. Both are write targets, not new bindings β€” now rejected while parsing.
  • Hashing a field accessor hit todo!(), so %{x: 1} aborted.

Notes for reviewers

  • Dispatch enum touches all call dispatch. dispatch_call now returns
    FramePushed / Completed. Shape follows Lua's luaD_precall.
  • Augmented assignment on fields is partial by design. +=, -=, *=, %=, &=,
    |= work; /= and ^= on an Int field are rejected because the operator's result
    type widens to Number β€” identically to an annotated container
    (let l: List<Int> = [10]; l[0] /= 2 gives the same error). A field is a non-widenable
    typed location, so it lands in the behaviour chore: augmented assignment review follow-ups 🧹 #195 established. Deferred with the rest of
    the subtyping rework, not a new regression. (An earlier revision of this note listed
    -=/&=/|= as rejected too; the Binding::None overload work fixed those.)
  • Two commits were cherry-picked from the pre-rebase feature/struct-oops branch, with
    authorship preserved; that branch still holds the old history.
  • Earlier commits also changed emit_set_var to bump source_locals on every local store,
    and made produces_value exhaustive instead of _ => true.

Remaining work

  • Struct names are not usable as types. Done β€” see "What this adds". Deferred
    to the subtyping/unification rework: self-referential structs
    (struct Node { next: Option<Node> } errors with unknown type, since usable
    recursion also needs None to be assignable to any Option<T>).
  • Manual page. Done β€” manual/src/reference/types/struct.md covers declaration
    syntax, positional constructors, p.x ≑ x(p), accessors as function values,
    field assignment (including the augmented-assignment rules above), reference
    semantics, nominal equality/hashing, JSON encoding, and current limitations. Every
    code block was executed against this branch.
  • to_json on a struct is unreachable. Done β€” the call itself was already fixed
    by the Binding::None overload work. This branch is now stacked on feat(stdlib): make json_encode strict and add json_encode_lossy 🧊 #199 (strict
    json_encode + json_encode_lossy, targeted at master): the strict json_encode
    rejects structs (a JSON object decodes back to a map, not a struct) and
    json_encode_lossy encodes them as objects with field names as keys.
  • Duplicate field names are silently accepted. Done β€” now an analyser error
    (Illegal redefinition of field 'a' in struct 'Dup'), reported at the duplicate
    field's own span; the declaration is not registered, so the name stays free.
  • Constructor arity/type errors are runtime-only. Done, and generally: when every
    same-name candidate is a fully-annotated function and none is compatible with the
    call, resolve_call now reports Binding::None instead of falling back to runtime
    dispatch β€” so Point(1), Point("x", 2), and dist(Other(1, 2)) all fail at
    compile time. Exposed a latent analyser bug (a map default's type wasn't part of the
    map's value type), fixed alongside.
  • Setter Display is indistinguishable from the getter β€” fixed in refactor(vm): model struct accessors as native function closures 🧩 #201 (stacked
    on this branch): the setter prints <fn Point.x=>.
  • Decide on struct Empty { } β€” decided: allowed. A one-flag parser change
    (delimited_comma_separated(..., allow_empty: true)); the rest of the pipeline
    already handled zero fields (construction, printing as Empty {}, equality,
    hashing, type annotations β€” covered by 015_struct/024_empty_struct.ndc). The
    tree-sitter grammar already accepted an empty body.
  • Test gaps. A setter can't be captured as a value, so its hash tag is untested.
    (Nested field assignment is now covered by 015_struct/012_nested_struct.ndc.)
    Moot after refactor(vm): model struct accessors as native function closures 🧩 #201: the setter-specific hash arm no longer exists.
  • Consider modelling constructors and accessors as NativeFunction closures instead of
    three Function variants. Would delete ~9 match arms, the per-dispatch StaticType
    allocation, and the getter/setter Display ambiguity. Done in refactor(vm): model struct accessors as native function closures 🧩 #201, stacked on this
    branch.

Unrelated and pre-existing: cargo clippy --all-features fails to compile because
ndc_vm/src/tracer.rs doesn't cover OpCode::CallVec. CI only builds default features.

πŸ€–

timfennis added a commit that referenced this pull request Aug 25, 2026
## Context

Working on PR #197's biggest TODO ("struct names are not usable as
types") revealed the underlying problem: the parser eagerly constructs
semantic `StaticType`s from annotations through a closed-world name
table (`StaticType::from_name_and_args`). That makes user-declared type
names unresolvable in principle (the parser only has tokens), turns a
typo in one annotation into a fatal parse error that blanks every LSP
feature for the document, loses the annotation's span, and blocks the
planned generics rework β€” a type variable is a binder, not a name
lookup, so it can't be represented by parse-time `StaticType`
construction.

This precursor separates type syntax from type semantics so struct names
(and later, type variables) become a small addition to one lowering
function instead of a special case.

## Changes

- **`ndc_parser`**: new syntactic `TypeExpr` (`Name { name, args, span
}` / `Tuple { elements, span }`) with `Display`. The type-annotation
parser builds `TypeExpr` and no longer validates names; the
`>>`/`>=`/`>>=` splitting is untouched. AST slots store the syntax plus
an analyser-filled resolved field (the existing
`Binding::Resolved`-style pattern): `FunctionParameter.resolved_type`
and `FunctionDeclaration.resolved_return_type`.
- **`ndc_analyser`**: `lower_type_expr` resolves `TypeExpr` β†’
`StaticType`. Unknown names and generic-arity mismatches are emitted as
span-precise analysis errors and degrade to `Any`, so the rest of the
program is still checked. Lowering runs before the `TypeSignature` is
built, so overload resolution and runtime dispatch see the resolved
types.
- **`ndc_vm` / `ndc_lsp`**: consume the resolved fields instead of
parse-time types.

## Behaviour change

`fn f(x: Unknown)` and `let a: Map<Int> = ...` now report
`error[resolver]` diagnostics (with the annotation's exact span) instead
of aborting the parse, and one bad annotation no longer suppresses other
errors or LSP features. New functional and REPL tests cover each
annotation position, the arity error, error recovery, and REPL state
integrity.

πŸ€–

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
@timfennis
timfennis changed the base branch from master to feature/strict-json August 26, 2026 08:55
@timfennis
timfennis force-pushed the feature/structs branch 2 times, most recently from dda1b7a to 3f3aea4 Compare August 26, 2026 09:43
Base automatically changed from feature/strict-json to master August 26, 2026 10:01
timfennis added a commit that referenced this pull request Aug 26, 2026
## Context

While working on struct support in #197 (the `to_json` TODO), we decided
JSON conversion should be perfect in both directions: `json_encode` must
be the exact inverse of `json_decode` and error whenever that's
impossible, instead of silently degrading values. Extracted as a
precursor PR off master so #197 can rebase on it and add only the struct
arm.

## Changes

- **`json_encode` is now strict.** It only accepts values for which
`json_decode(json_encode(v)) == v` holds: the unit value `()`, booleans,
ints, big ints, finite floats, strings, lists, string-keyed sets, and
maps with string keys. Everything else errors with a hint: rationals,
complex numbers, non-finite floats, options (both `Some` and `None`),
tuples, deques, iterators, heaps, functions, maps with non-string keys
or a default value, and cyclic values.
- **New `json_encode_lossy`** accepts the rejected values by degrading
them: rationals β†’ floats, complex β†’ strings, `Some(x)` unwrapped to `x`,
`None` β†’ `null`, tuples/deques β†’ arrays, heaps β†’ arrays in priority
order (deterministic, was arbitrary internal heap order), iterators
drained, non-string keys stringified, map defaults dropped, non-finite
floats β†’ `null`. It has no decode counterpart because these conversions
cannot be reversed. Functions and cyclic values still error.
- **`json_decode` fixes:** integers beyond `i64` decode exactly to big
ints via serde_json's `arbitrary_precision` (previously silently lossy
through `f64`), and numbers that overflow a float (`1e999`) error.
- **Cycle detection** replaces a stack-overflow abort: `json_encode([l
where l contains l])` used to crash the process, now it's an error.

---------

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Comment on lines +10 to +14
The reverse also holds: when every argument type is known at compile time and **no** overload
can accept them β€” every candidate has a fully-annotated signature of the wrong arity or with
conflicting parameter types β€” the call is rejected at compile time with a
`No function called '…' found that matches the arguments` error, instead of being deferred to
a guaranteed runtime failure.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way to phrase this with shorter sentences. This is a little rough to read for my pea sized brain.

timfennis added a commit that referenced this pull request Aug 26, 2026
)

## Context

Follow-up to #197, which noted that modelling constructors and accessors
as three dedicated `Function` variants forced ~9 extra match arms, a
`StaticType` allocation on every dispatch probe, and an ambiguous
`Display` (getter and setter both printed `<fn Point.x>`). Stacked on
`feature/structs` so it can be reviewed separately.

## What changed

- **`Function::{Constructor, GetField, SetField}` are gone.** A struct
declaration now builds three ordinary `NativeFunction` closures
(`Function::struct_constructor/struct_getter/struct_setter`) that
capture the `Rc<StructInfo>` and field index. Behaviour, runtime error
messages, and the nominal `Rc::ptr_eq` type check are unchanged.
- **All special-case match arms deleted** from dispatch
(`dispatch_call_with_memo`), equality, hashing, `name()`,
`static_type()`, `matches_arg_types`/`matches_value_args`, and
`Display`/`Debug`. Equality and hashing now use the `Native` arm's
pointer identity, which coincides with the old per-info-plus-index
identity because each accessor is constructed exactly once, as a chunk
constant.
- **The per-dispatch `StaticType` allocation is gone** β€” the type is
computed once at declaration and stored on the `NativeFunction`.
Struct-heavy code got measurably faster (~8% on a
constructor+getter+setter loop, hyperfine, pinned core); fib-style
compiled-function dispatch is unchanged within noise.
- **`Display` is now name-based and unambiguous**: `<fn Point>`, `<fn
Point.x>`, `<fn Point.x=>`. Covered by a unit test. This also resolves
the "two identical constants in `ndc disassemble`" item from #197.
- **Removed `Function::is_native`** β€” unused, and its doc comment
(natives bridge to the tree-walk interpreter) would have been wrong for
accessors.

## Notes for reviewers

- **Stdlib natives display differently now.** `print(len)` prints `<fn
len>` instead of `<native fn Function { … }>`, since `Display` for
`Function::Native` uses the function's name. No test relied on the old
string; the old type dump moved to `Debug`, which now also includes the
name.
- Accessor names are qualified (`Point.x`, `Point.x=`), so the rare
runtime dispatch errors that print a callee name (vec dispatch) name the
struct too. Compile-time messages are unaffected β€” the analyser resolves
accessors by their binding names (`x`, `x=`) as before.
- #197's "setter hash tag is untested" gap dissolves: there is no
setter-specific hash arm any more.

πŸ€–

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
timfennis and others added 17 commits August 26, 2026 12:54
`call_callback` assumed every dispatch pushed a frame, so passing a struct
constructor or field accessor to a HOF ran the caller's bytecode again and
then overran the chunk. Dispatch now reports whether it pushed a frame.

Routing through `dispatch_call` also unwraps `Memoized`, fixing a `pure fn`
passed as a callback hitting `unreachable!`.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`Hash for Object` left `GetField`/`SetField` as `todo!`, so using a getter as
a map key aborted. Hash the `StructInfo` pointer plus the field index, with
distinct tags for getter and setter, matching the `PartialEq` arm.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Only `Constructor` wrote a discriminant, so a `Compiled` and a `Closure`
sharing a prototype pointer hashed alike. Tag all seven variants.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`p.x += 1` now lowers through `PreparedAssignmentTarget`, caching the receiver
so it is evaluated once for the getter and the setter.

Rejection of `+=` on a member is dropped now that the compiler can lower it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`let l[0] = 5` panicked in the compiler and `let p.x = 5` panicked in the
analyser: both are write targets, not new bindings. Reject them while parsing.

Destructuring declarations are unaffected; only Index and Member targets go.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Adapts the structs branch to the TypeExpr refactor from #198: struct
fields store the syntactic annotation, and the analyser lowers it when
registering the struct.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Struct names now work as type annotations (parameters, return types, let
bindings, struct fields, inside generics). Built-in names take
precedence. Struct names are globally unique: redeclaration is an
analysis error, and the analyser checkpoint now rolls the registry back
so a failed REPL line frees its struct names.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
timfennis and others added 7 commits August 26, 2026 12:54
Also rewrites the duplicate-parameter check with the same
itertools duplicates_by/unique_by idiom.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
When every same-name candidate has a fully-annotated signature and none
is compatible with the call, resolution now reports Binding::None
instead of deferring to runtime dispatch. Covers constructor arity and
argument type errors.

Also fixes the map value type to include the default value's type,
which the stricter resolution would otherwise falsely reject.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
let x: Map<Int, Int> = %{} was rejected because the literal inferred
Map<Any, ()>, which is not a subtype of the annotation. Declarations
and assignments now analyse the initialiser with the expected type,
letting empty (and nested empty) container literals adopt it.
Temporary until type parameters and unification land.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Structs are rejected by the strict json_encode (a JSON object decodes
back to a map, not a struct) and encode as objects via json_encode_lossy.
The serde.rs arms landed while resolving the rebase onto #199.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
)

## Context

Follow-up to #197, which noted that modelling constructors and accessors
as three dedicated `Function` variants forced ~9 extra match arms, a
`StaticType` allocation on every dispatch probe, and an ambiguous
`Display` (getter and setter both printed `<fn Point.x>`). Stacked on
`feature/structs` so it can be reviewed separately.

## What changed

- **`Function::{Constructor, GetField, SetField}` are gone.** A struct
declaration now builds three ordinary `NativeFunction` closures
(`Function::struct_constructor/struct_getter/struct_setter`) that
capture the `Rc<StructInfo>` and field index. Behaviour, runtime error
messages, and the nominal `Rc::ptr_eq` type check are unchanged.
- **All special-case match arms deleted** from dispatch
(`dispatch_call_with_memo`), equality, hashing, `name()`,
`static_type()`, `matches_arg_types`/`matches_value_args`, and
`Display`/`Debug`. Equality and hashing now use the `Native` arm's
pointer identity, which coincides with the old per-info-plus-index
identity because each accessor is constructed exactly once, as a chunk
constant.
- **The per-dispatch `StaticType` allocation is gone** β€” the type is
computed once at declaration and stored on the `NativeFunction`.
Struct-heavy code got measurably faster (~8% on a
constructor+getter+setter loop, hyperfine, pinned core); fib-style
compiled-function dispatch is unchanged within noise.
- **`Display` is now name-based and unambiguous**: `<fn Point>`, `<fn
Point.x>`, `<fn Point.x=>`. Covered by a unit test. This also resolves
the "two identical constants in `ndc disassemble`" item from #197.
- **Removed `Function::is_native`** β€” unused, and its doc comment
(natives bridge to the tree-walk interpreter) would have been wrong for
accessors.

## Notes for reviewers

- **Stdlib natives display differently now.** `print(len)` prints `<fn
len>` instead of `<native fn Function { … }>`, since `Display` for
`Function::Native` uses the function's name. No test relied on the old
string; the old type dump moved to `Debug`, which now also includes the
name.
- Accessor names are qualified (`Point.x`, `Point.x=`), so the rare
runtime dispatch errors that print a callee name (vec dispatch) name the
struct too. Compile-time messages are unaffected β€” the analyser resolves
accessors by their binding names (`x`, `x=`) as before.
- #197's "setter hash tag is untested" gap dissolves: there is no
setter-specific hash arm any more.

πŸ€–

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
@timfennis
timfennis marked this pull request as ready for review August 26, 2026 11:12

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

πŸ’‘ Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 434543b0bc

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with πŸ‘.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread ndc_vm/src/value/mod.rs
@timfennis
timfennis merged commit 295fa3c into master Aug 26, 2026
1 check passed
@timfennis
timfennis deleted the feature/structs branch August 26, 2026 11:40
timfennis added a commit that referenced this pull request Aug 26, 2026
## Context

A final adversarial review of #197 (after it merged) surfaced four real
bugs and two smaller gaps. This PR fixes all six. Each bug was
reproduced before fixing and has a regression test.

## Fixes

- **Compiler panic:** `for (p.x) in [1, 2, 3]` hit `unreachable!("cannot
declare into a field")`. For-loop iteration variables now go through the
same `non_binding_target` check as `let`. The probe also uncovered that
`fn f(p.x)` / `fn f(l[0])` panicked in `FunctionParameter::from_params`
(the index case pre-dates structs) β€” parameters are now required to be
plain identifiers at parse time.
- **VM stack corruption:** a struct declaration in value position
(`[struct R { x: Int }, 5]` printed `[,5]`) pushed no value. Struct
declarations are now statements, parsed exactly like `let`, so value
position is a parse error.
- **`(s.f)()` silently called the getter method-style** instead of the
function stored in the field. Parenthesized member access now keeps an
`Expression::Grouping` wrapper so the postfix-call rewrite can tell it
apart from `s.f()`. Grouping was never constructed by the parser before,
so its (dead) lvalue arms were redefined as transparent: `(s.x) = 5`,
`(s.x) += 2`, and `(l[0]) = 9` behave exactly as before.
- **`clone()` aliased structs and `deepcopy()` shared nested state** β€”
`Object::Struct` fell into both catch-all arms. Structs now copy like
the other mutable containers: `clone` gives an independent instance
(nested containers shared, like lists), `deepcopy` shares nothing.
- The `struct` keyword was missing from the REPL highlighter and the LSP
keyword completion list.
- The analyser hand-built the constructor/getter/setter `StaticType`s
inline; it now uses
`StructInfo::constructor_type()/getter_type()/setter_type()` β€” the same
source of truth the runtime functions use.

πŸ€–

---------

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
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