navicore/patch-seq

Fork 0

list and variant sync #438

Merged

navicore merged 2 commits from list-v into main

2026-04-27 21:34:22 +00:00

navicore commented

2026-04-27 21:02:51 +00:00

(Migrated from github.com)

New runtime functions

crates/runtime/src/list_ops/access.rs: patch_seq_list_first, patch_seq_list_last — both return the (value Bool) failure tuple, matching list.get. Empty list → placeholder Int 0 + false.
crates/runtime/src/variant_ops/modify.rs: patch_seq_variant_first — mirrors patch_seq_variant_last. Panics on a fieldless variant (same convention as variant.last).

Compiler wiring

Builtin signatures + docs in builtins/list.rs and builtins/adt.rs.
Codegen runtime decls + symbol mappings in codegen/runtime/collections.rs and codegen/runtime/adt.rs.
AST program well-known list updated.
error_flag_lint/state.rs knows list.first/list.last shapes.
lints.toml: new unchecked-list-first and unchecked-list-last rules so dropping the Bool produces the same hint pattern as list.get drop.

Tests

8 runtime unit tests (3 list.first, 3 list.last, 2 variant.first) covering non-empty / singleton / empty paths.
5 Seq integration tests in test-list.seq and a new test-variant-accessors.seq.
All pass under just ci.

New runtime functions - crates/runtime/src/list_ops/access.rs: patch_seq_list_first, patch_seq_list_last — both return the (value Bool) failure tuple, matching list.get. Empty list → placeholder Int 0 + false. - crates/runtime/src/variant_ops/modify.rs: patch_seq_variant_first — mirrors patch_seq_variant_last. Panics on a fieldless variant (same convention as variant.last). Compiler wiring - Builtin signatures + docs in builtins/list.rs and builtins/adt.rs. - Codegen runtime decls + symbol mappings in codegen/runtime/collections.rs and codegen/runtime/adt.rs. - AST program well-known list updated. - error_flag_lint/state.rs knows list.first/list.last shapes. - lints.toml: new unchecked-list-first and unchecked-list-last rules so dropping the Bool produces the same hint pattern as list.get drop. Tests - 8 runtime unit tests (3 list.first, 3 list.last, 2 variant.first) covering non-empty / singleton / empty paths. - 5 Seq integration tests in test-list.seq and a new test-variant-accessors.seq. - All pass under just ci.

claude[bot] commented

2026-04-27 21:04:51 +00:00

(Migrated from github.com)

Code Review: list and variant sync (#438)

Overview

This PR adds list.first, list.last, and variant.first convenience accessors to the runtime and wires them all the way through the compiler (builtins, codegen, error-flag lint, lints.toml). The design is consistent with existing ops and the test coverage is solid.

Issues

Bug / Naming Inconsistency — panic messages in `patch_seq_variant_first`

crates/runtime/src/variant_ops/modify.rs

Both panic messages use a dash where the rest of the codebase uses a dot:

panic!("variant-first: variant has no fields");   // should be "variant.first"
panic!("variant-first: expected Variant, got {:?}", ...)  // same

The existing variant_last (and all other ops in this file) use the dot form ("variant.last: ..."). These messages surface in runtime error output, so the wrong prefix makes them harder to grep and confusing to users.

Code Duplication — `patch_seq_list_first` and `patch_seq_list_last`

crates/runtime/src/list_ops/access.rs

The two functions are ~85 lines of near-identical code differing only in the index used (fields[0] vs fields.last().unwrap()). A small private helper would remove the duplication without touching the public extern "C" surface — the only difference between the two call sites is the closure passed to retrieve the element. This matches the refactoring pattern already used elsewhere in the crate.

Suggestions

Missing test: `variant.first` empty-fields path

crates/runtime/src/variant_ops/tests.rs

The panic on an empty variant is documented but not exercised. A #[test] #[should_panic] case would lock in the contract and prevent accidental softening of the panic in a future refactor. It would also serve as a regression guard for the panic-message fix noted above.

Empty-path placeholder is unasserted in unit tests

crates/runtime/src/list_ops/tests.rs

test_list_first_empty_is_failure_tuple and test_list_last_empty_is_failure_tuple pop the placeholder as _placeholder without asserting its value. Since the placeholder (Int 0) is part of the documented public contract, asserting it would make the tests self-describing and prevent silent contract drift.

What is Good

The (value Bool) shape for list.first/list.last is consistent with list.get — the same error-flag lint infrastructure covers all three with no special-casing needed.
Lint rules in lints.toml follow the established pattern exactly.
The variant.first / variant.last asymmetry (panic vs. soft-failure) is documented clearly in both the Rust doc-comment and the PR description.
Integration tests cover the singleton agreement property, which is the subtlest correctness property here.
All compiler layers (AST well-known list, builtins, codegen decls, codegen symbols, error-flag state) are updated atomically — no half-wired state.

Summary

One real bug (panic message typo: variant-first should be variant.first) and one worthwhile refactor (extract the shared list-endpoint helper) before merge. Everything else is style/hardening. The feature itself is correct and well-integrated.

## Code Review: list and variant sync (#438) ### Overview This PR adds `list.first`, `list.last`, and `variant.first` convenience accessors to the runtime and wires them all the way through the compiler (builtins, codegen, error-flag lint, lints.toml). The design is consistent with existing ops and the test coverage is solid. --- ### Issues #### Bug / Naming Inconsistency — panic messages in `patch_seq_variant_first` `crates/runtime/src/variant_ops/modify.rs` Both panic messages use a dash where the rest of the codebase uses a dot: panic!("variant-first: variant has no fields"); // should be "variant.first" panic!("variant-first: expected Variant, got {:?}", ...) // same The existing `variant_last` (and all other ops in this file) use the dot form (`"variant.last: ..."`). These messages surface in runtime error output, so the wrong prefix makes them harder to grep and confusing to users. --- #### Code Duplication — `patch_seq_list_first` and `patch_seq_list_last` `crates/runtime/src/list_ops/access.rs` The two functions are ~85 lines of near-identical code differing only in the index used (`fields[0]` vs `fields.last().unwrap()`). A small private helper would remove the duplication without touching the public `extern "C"` surface — the only difference between the two call sites is the closure passed to retrieve the element. This matches the refactoring pattern already used elsewhere in the crate. --- ### Suggestions #### Missing test: `variant.first` empty-fields path `crates/runtime/src/variant_ops/tests.rs` The panic on an empty variant is documented but not exercised. A `#[test] #[should_panic]` case would lock in the contract and prevent accidental softening of the panic in a future refactor. It would also serve as a regression guard for the panic-message fix noted above. #### Empty-path placeholder is unasserted in unit tests `crates/runtime/src/list_ops/tests.rs` `test_list_first_empty_is_failure_tuple` and `test_list_last_empty_is_failure_tuple` pop the placeholder as `_placeholder` without asserting its value. Since the placeholder (`Int 0`) is part of the documented public contract, asserting it would make the tests self-describing and prevent silent contract drift. --- ### What is Good - The `(value Bool)` shape for `list.first`/`list.last` is consistent with `list.get` — the same error-flag lint infrastructure covers all three with no special-casing needed. - Lint rules in `lints.toml` follow the established pattern exactly. - The `variant.first` / `variant.last` asymmetry (panic vs. soft-failure) is documented clearly in both the Rust doc-comment and the PR description. - Integration tests cover the singleton agreement property, which is the subtlest correctness property here. - All compiler layers (AST well-known list, builtins, codegen decls, codegen symbols, error-flag state) are updated atomically — no half-wired state. --- ### Summary One real bug (panic message typo: `variant-first` should be `variant.first`) and one worthwhile refactor (extract the shared list-endpoint helper) before merge. Everything else is style/hardening. The feature itself is correct and well-integrated.

claude[bot] commented

2026-04-27 21:26:34 +00:00

(Migrated from github.com)

Code Review — PR #438: list and variant sync

Overview

This PR adds list.first / list.last convenience accessors (returning a (value Bool) error-flag tuple) and variant.first (panicking on fieldless variants, mirroring variant.last). The compiler wiring, lint rules, and test coverage are all thorough. The changes are well-scoped and follow established patterns in the codebase.

Code Quality & Style

Positives:

The list_endpoint helper cleanly factors out the shared logic between list.first and list.last. Using a plain fn pointer instead of a generic/closure to keep FFI wrappers trivial is a good call and the comment explains why.
Panic message updates ("variant-tag:" → "variant.tag:") across access.rs, make.rs, and modify.rs are a nice consistency cleanup bundled in sensibly.
The unsafe {} block inside unsafe fn list_endpoint is correct for edition 2024 (unsafe_op_in_unsafe_fn is deny-by-default).
Lint rules for unchecked-list-first / unchecked-list-last correctly mirror the existing list.get drop pattern.

Minor nit — alignment inconsistency in builtins/list.rs:

builtin!(sigs, "list.first", (a V -- a T Bool));
builtin!(sigs, "list.last",  (a V -- a T Bool));  // extra space before (

The trailing alignment space on list.last is inconsistent with list.first directly above it. Either align both or neither.

Potential Issues

1. Asymmetric error semantics between list.* and variant.* — documented but worth calling out

list.first / list.last return (value false) on an empty list. variant.first / variant.last panic on a fieldless variant. The PR description acknowledges this ("same convention as variant.last"), and the intent is sound — variants are expected to have statically-known structure. However, this asymmetry can surprise callers who assume all *.first / *.last operations behave the same way. A one-liner in the variant.first doc comment explicitly noting "unlike list.first, this panics on an empty variant" would help.

2. Wrong-type recovery path in list_endpoint pushes a failure tuple despite calling set_runtime_error

_ => {
    set_runtime_error(format!("{}: expected Variant (list), got {:?}", op_name, list_val));
    let stack = push(stack, Value::Int(0));
    return push(stack, Value::Bool(false));
}

On a type mismatch, the function both records the error and pushes (Int 0, false). The false flag means Seq code checking the Bool will see a failure, which is reasonable. But there's no unit test exercising this path — a test pushing a non-Variant (e.g., Value::Int(99)) through list_first and asserting (Int 0, false) would pin the contract for this edge case.

3. list_endpoint — fields[0].clone() is guarded but isolated

The is_empty() check before calling pick makes fields[0] safe. However, the pick callback is a free function pointer the callers supply; if a future caller passed a closure that index into fields differently, there'd be no compile-time guard. The current callers are trivial, so this is a very minor design note rather than a real bug.

Test Coverage

Path	Unit test	Integration test
`list.first` — non-empty	✅	✅
`list.first` — singleton	✅	✅ (combined with `list.last`)
`list.first` — empty → failure tuple	✅	✅
`list.last` — non-empty	✅	✅
`list.last` — singleton	✅	✅
`list.last` — empty → failure tuple	✅	✅
`list.first/last` — wrong type (non-Variant)	❌	❌
`variant.first` — non-empty	✅	✅
`variant.first` — singleton	✅	✅
`variant.first` — fieldless (panic)	❌ documented	❌ documented

The one gap worth filling (non-blocking) is a unit test for the wrong-type path in list_endpoint. The fieldless-variant panic test omission is clearly documented and the reasoning (process-abort across extern "C" in edition 2024) is correct.

Summary

This is a clean, well-tested addition. The two things worth addressing before merge:

(Nit) Align the trailing spaces on list.last in builtins/list.rs.
(Suggestion) Add a sentence to variant.first's doc comment that it panics on a fieldless variant (unlike list.first), to make the asymmetry explicit at the call site.
(Optional) Add a unit test for the wrong-type recovery path in list_endpoint.

Everything else looks correct and consistent with existing conventions.

## Code Review — PR #438: list and variant sync ### Overview This PR adds `list.first` / `list.last` convenience accessors (returning a `(value Bool)` error-flag tuple) and `variant.first` (panicking on fieldless variants, mirroring `variant.last`). The compiler wiring, lint rules, and test coverage are all thorough. The changes are well-scoped and follow established patterns in the codebase. --- ### Code Quality & Style **Positives:** - The `list_endpoint` helper cleanly factors out the shared logic between `list.first` and `list.last`. Using a plain `fn` pointer instead of a generic/closure to keep FFI wrappers trivial is a good call and the comment explains why. - Panic message updates (`"variant-tag:"` → `"variant.tag:"`) across `access.rs`, `make.rs`, and `modify.rs` are a nice consistency cleanup bundled in sensibly. - The `unsafe {}` block inside `unsafe fn list_endpoint` is correct for edition 2024 (`unsafe_op_in_unsafe_fn` is deny-by-default). - Lint rules for `unchecked-list-first` / `unchecked-list-last` correctly mirror the existing `list.get drop` pattern. **Minor nit — alignment inconsistency in `builtins/list.rs`:** ```rust builtin!(sigs, "list.first", (a V -- a T Bool)); builtin!(sigs, "list.last", (a V -- a T Bool)); // extra space before ( ``` The trailing alignment space on `list.last` is inconsistent with `list.first` directly above it. Either align both or neither. --- ### Potential Issues **1. Asymmetric error semantics between `list.*` and `variant.*` — documented but worth calling out** `list.first` / `list.last` return `(value false)` on an empty list. `variant.first` / `variant.last` **panic** on a fieldless variant. The PR description acknowledges this ("same convention as variant.last"), and the intent is sound — variants are expected to have statically-known structure. However, this asymmetry can surprise callers who assume all `*.first` / `*.last` operations behave the same way. A one-liner in the `variant.first` doc comment explicitly noting "unlike list.first, this panics on an empty variant" would help. **2. Wrong-type recovery path in `list_endpoint` pushes a failure tuple despite calling `set_runtime_error`** ```rust _ => { set_runtime_error(format!("{}: expected Variant (list), got {:?}", op_name, list_val)); let stack = push(stack, Value::Int(0)); return push(stack, Value::Bool(false)); } ``` On a type mismatch, the function both records the error *and* pushes `(Int 0, false)`. The `false` flag means Seq code checking the Bool will see a failure, which is reasonable. But there's no unit test exercising this path — a test pushing a non-Variant (e.g., `Value::Int(99)`) through `list_first` and asserting `(Int 0, false)` would pin the contract for this edge case. **3. `list_endpoint` — `fields[0].clone()` is guarded but isolated** The `is_empty()` check before calling `pick` makes `fields[0]` safe. However, the `pick` callback is a free function pointer the callers supply; if a future caller passed a closure that index into `fields` differently, there'd be no compile-time guard. The current callers are trivial, so this is a very minor design note rather than a real bug. --- ### Test Coverage | Path | Unit test | Integration test | |------|-----------|-----------------| | `list.first` — non-empty | ✅ | ✅ | | `list.first` — singleton | ✅ | ✅ (combined with `list.last`) | | `list.first` — empty → failure tuple | ✅ | ✅ | | `list.last` — non-empty | ✅ | ✅ | | `list.last` — singleton | ✅ | ✅ | | `list.last` — empty → failure tuple | ✅ | ✅ | | `list.first/last` — wrong type (non-Variant) | ❌ | ❌ | | `variant.first` — non-empty | ✅ | ✅ | | `variant.first` — singleton | ✅ | ✅ | | `variant.first` — fieldless (panic) | ❌ documented | ❌ documented | The one gap worth filling (non-blocking) is a unit test for the wrong-type path in `list_endpoint`. The fieldless-variant panic test omission is clearly documented and the reasoning (process-abort across `extern "C"` in edition 2024) is correct. --- ### Summary This is a clean, well-tested addition. The two things worth addressing before merge: 1. **(Nit)** Align the trailing spaces on `list.last` in `builtins/list.rs`. 2. **(Suggestion)** Add a sentence to `variant.first`'s doc comment that it panics on a fieldless variant (unlike `list.first`), to make the asymmetry explicit at the call site. 3. **(Optional)** Add a unit test for the wrong-type recovery path in `list_endpoint`. Everything else looks correct and consistent with existing conventions.