Skip to content

fix: harden struct parsing, copying, and tooling after review 🛡️ - #204

Merged
timfennis merged 6 commits into
masterfrom
bugfix/struct-review-findings
Aug 26, 2026
Merged

fix: harden struct parsing, copying, and tooling after review 🛡️#204
timfennis merged 6 commits into
masterfrom
bugfix/struct-review-findings

Conversation

@timfennis

@timfennis timfennis commented Aug 26, 2026

Copy link
Copy Markdown
Owner

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 stateObject::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 StaticTypes inline; it now uses StructInfo::constructor_type()/getter_type()/setter_type() — the same source of truth the runtime functions use.

🤖

timfennis and others added 5 commits August 26, 2026 14:06
- Member/index lvalues in for-loop iteration variables and function
  parameters are parse errors instead of compiler/parser panics
- Struct declarations are statements like let; value position no longer
  corrupts the VM stack
- (s.f)() keeps its Grouping so it calls the stored function instead of
  silently lowering to method-call form f(s)

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

@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: ffb6966313

ℹ️ 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
Comment thread manual/src/reference/types/struct.md Outdated
…agraph ✍️

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@timfennis
timfennis merged commit 1320fce into master Aug 26, 2026
1 check passed
@timfennis
timfennis deleted the bugfix/struct-review-findings branch August 26, 2026 14:32
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