fix: address special_map off-by-one #7068
Conversation
Coverage Report for CI Build 24833235977Warning Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes. Coverage increased (+0.03%) to 85.744%Details
Uncovered Changes
Coverage Regressions3174 previously-covered lines in 85 files lost coverage.
Coverage Stats
💛 - Coveralls |
cylewitruk-stacks
left a comment
cylewitruk-stacks
left a comment
There was a problem hiding this comment.
Looks materially better than the previous PR(s) -- no comments from me! I'll trigger a Copilot run just for an extra pair of eyes on any subtleties 👍
There was a problem hiding this comment.
Pull request overview
This PR refactors Clarity’s higher-order sequence operations to simplify special_map (and related fold/filter paths) by switching from precomputed argument matrices to lazy iteration over SequenceData, reducing intermediate allocations and removing an unintended extra iteration.
Changes:
- Refactor
special_map/special_fold/special_filterto use lazySequenceDataiteration andapply_evaluated()(pre-evaluated argument dispatch). - Add
SequenceIter+IntoIteratorforSequenceDataand updatetry_retainto passValueto predicates. - Add additional tests for variadic
mapshortest-sequence behavior and introduce a new end-to-end Criterion benchmark.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| clarity/src/vm/tests/sequences.rs | Adds test coverage for variadic map stopping at the shortest sequence. |
| clarity/src/vm/mod.rs | Introduces shared call precondition checking, dispatch_args, and apply_evaluated(). |
| clarity/src/vm/functions/sequences.rs | Refactors filter/fold/map to use iterators + apply_evaluated(). |
| clarity/benches/sequence_ops.rs | Updates benchmark helper to match try_retain predicate signature change. |
| clarity/benches/sequence_higher_order.rs | Adds new end-to-end benchmarks for fold/map/filter. |
| clarity/Cargo.toml | Registers the new sequence_higher_order benchmark target. |
| clarity-types/src/types/mod.rs | Adds SequenceIter, IntoIterator for SequenceData, and updates try_retain predicate type. |
| clarity-types/src/tests/types/mod.rs | Updates tests to match try_retain(Value) predicate signature. |
| changelog.d/7068-refactor-special-map.changed | Adds changelog entry for the special_map refactor. |
| changelog.d/7022-fold-map-optimization.changed | Adds changelog entry for fold/map/filter performance work. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
I think it's important to be sure that the error behavior here is the same as the prior implementation, otherwise, this PR will have consensus impacts. |
@aaronb-stacks here the in-depth discussion: https://github.com/stacks-network/stacks-core/pull/7022/changes#r2980910603 Basically the |
…ated to the empty list case
…on when used with empty list
francesco-stacks
left a comment
francesco-stacks
left a comment
There was a problem hiding this comment.
I think the off-by-one is a correct fix, but causes a consensus-breaking change. The current code erroneously handles some edge cases. We'll probably have to epoch gate the fix
/// `(map + empty-list non-empty-list)` with typed-but-runtime-empty first sequence.
///
/// Variadic native `+` tolerates a 1-arg call and returns the single value. The old
/// algorithm pushes the first element of the non-empty list at position 0, then
/// the apply loop runs with `previous_len = None`, invoking `apply_evaluated(+, [10])`.
///
/// - develop: `(ok (list 10))` - tx succeeds with a non-empty result.
/// - this PR: `(ok (list))` - tx succeeds with an empty result.
#[test]
fn map_empty_first_variadic_native_ccall() {
contract_call_consensus_test!(
contract_name: "map-mt-first-nat",
contract_code: "
(define-data-var xs (list 10 int) (list))
(define-data-var ys (list 10 int) (list 10 20))
(define-public (trigger)
(ok (map + (var-get xs) (var-get ys))))
",
function_name: "trigger",
function_args: &[],
);
}
/// `(map + non-empty empty non-empty)` with typed-but-runtime-empty middle sequence.
///
/// Old algorithm: first loop pushes `[[1], [2], [3]]`; second loop (empty) does
/// nothing but sets `min_args_len = 0`; third loop's element at index 0 satisfies
/// `0 > 0 == false`, appending into the first group to produce `[[1, 10], [2], [3]]`.
/// The apply loop processes `[1, 10]` with `previous_len = None`, invoking `+(1, 10) = 11`.
///
/// - develop: `(ok (list 11))` - tx succeeds with one element.
/// - this PR: `(ok (list))` - tx succeeds with empty result.
#[test]
fn map_empty_middle_variadic_native_ccall() {
contract_call_consensus_test!(
contract_name: "map-mt-mid-nat",
contract_code: "
(define-data-var a (list 10 int) (list 1 2 3))
(define-data-var b (list 10 int) (list))
(define-data-var c (list 10 int) (list 10 20))
(define-public (trigger)
(ok (map + (var-get a) (var-get b) (var-get c))))
",
function_name: "trigger",
function_args: &[],
);
}
/// `(map user-fn empty-list non-empty-list)` where the user function has fixed arity 2.
///
/// Old algorithm pushes `[[10]]` and invokes `apply_evaluated(add, [10])`. A
/// user-defined function with arity 2 rejects a 1-arg call in
/// `DefinedFunction::execute_apply` (callables.rs:189) -> `IncorrectArgumentCount(2, 1)`
/// runtime error, aborting the transaction.
///
/// - develop: tx aborted with `IncorrectArgumentCount(2, 1)`.
/// - this PR: `(ok (list))` - tx succeeds with an empty result.
#[test]
fn map_empty_first_user_fn_ccall() {
contract_call_consensus_test!(
contract_name: "map-mt-first-user",
contract_code: "
(define-data-var xs (list 10 int) (list))
(define-data-var ys (list 10 int) (list 10 20))
(define-private (add (x int) (y int)) (+ x y))
(define-public (trigger)
(ok (map add (var-get xs) (var-get ys))))
",
function_name: "trigger",
function_args: &[],
);
}
federico-stacks
changed the title
|
Converted to draft, since we want to epoch gate this change for the next hard-fork |
|
|
Description
As discussed in #7022 (comment), this PR aims to address two points about
special_mapfunction:(not a functional bug, but still unintended behavior). Actually demonstrated to be a relevant bug (fix: address special_map off-by-one #7068 (review))The proposed approach removes explicit index handling (addressing the first point) and reduces complexity by relying on iterators instead of precomputing a
Vec<Vec<Value>>of arguments (addressing the second point).As a side effect, this should reduce allocations and avoid reallocations that were previously needed for precomputed structures. In terms of execution speed, I haven’t observed any significant improvements so far.
Applicable issues
Additional info (benefits, drawbacks, caveats)
Checklist
docs/property-testing.md)changelog.d/README.md)rpc/openapi.yamlfor RPC endpoints,event-dispatcher.mdfor new events)clarity-benchmarkingrepo