[checking] Generate Traversable derived members - #230
Conversation
Summary
Confidence 5/5 - Broad coverage supports a safe mergeThe implementation covers the requested derivation paths and includes targeted tests for nested, recursive, record, unary, binary, and rejected function-shaped cases. The reported validation checks pass, with 30 WalkthroughChangesThe compiler now supports derived Traversable derivation
Sequence Diagram(s)sequenceDiagram
participant DeriveDispatch
participant VarianceRecipes
participant TraversableGenerator
participant KnownTermsCore
DeriveDispatch->>VarianceRecipes: build traversal recipes
VarianceRecipes->>TraversableGenerator: provide traversal metadata
TraversableGenerator->>KnownTermsCore: resolve traversal operations
KnownTermsCore-->>TraversableGenerator: return known term references
TraversableGenerator-->>DeriveDispatch: generate instance members
Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #230 +/- ##
==========================================
- Coverage 83.87% 83.78% -0.09%
==========================================
Files 157 158 +1
Lines 34050 34700 +650
Branches 34050 34700 +650
==========================================
+ Hits 28560 29075 +515
- Misses 2963 3036 +73
- Partials 2527 2589 +62 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
compiler-core/checking/src/source/derive/generate/traversable.rs (1)
1042-1046: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueResolve
known_terms.applybefore the loop.The loop re-checks
self.builder.context.known_terms.applyon every effect. Ifapplyis absent, the generator has already emitted themapapplication, and it then discards the work. Hoisting the check above the loop fails earlier and removes the repeated lookup.♻️ Proposed refactor
+ let Some(apply) = self.builder.context.known_terms.apply else { + return Ok(None); + }; for effect in remaining_effects { - let Some(apply) = self.builder.context.known_terms.apply else { - return Ok(None); - }; let apply = self.builder.term_reference(apply)?;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@compiler-core/checking/src/source/derive/generate/traversable.rs` around lines 1042 - 1046, Resolve and convert self.builder.context.known_terms.apply to the term reference before entering the remaining_effects loop. If it is absent, return Ok(None) before any map application is emitted; inside the loop, reuse the hoisted apply value and remove the repeated lookup and conversion.compiler-core/checking/src/source/derive/generate.rs (1)
149-194: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBind the generated member before collecting it.
The four single-member arms chain
.into_iter().collect()directly onto the call result. The coding guidelines require an iterator expression to be bound to a local variable before collecting it. Binding the optional member first also removes the nested chain and makes each arm read as one step.♻️ Proposed refactor for the single-member arms
let members = match dispatch { DeriveDispatch::Eq => { - eq_ord::generate_eq_member(state, context, result, &freshened.arguments)? - .into_iter() - .collect() + let member = + eq_ord::generate_eq_member(state, context, result, &freshened.arguments)?; + let members = member.into_iter(); + members.collect() } - DeriveDispatch::Eq1 => generate_delegated_member( - state, - context, - result, - &freshened.arguments, - context.known_terms.eq, - )? - .into_iter() - .collect(), + DeriveDispatch::Eq1 => { + let member = generate_delegated_member( + state, + context, + result, + &freshened.arguments, + context.known_terms.eq, + )?; + let members = member.into_iter(); + members.collect() + } DeriveDispatch::Ord => { - eq_ord::generate_ord_member(state, context, result, &freshened.arguments)? - .into_iter() - .collect() + let member = + eq_ord::generate_ord_member(state, context, result, &freshened.arguments)?; + let members = member.into_iter(); + members.collect() } - DeriveDispatch::Ord1 => generate_delegated_member( - state, - context, - result, - &freshened.arguments, - context.known_terms.compare, - )? - .into_iter() - .collect(), + DeriveDispatch::Ord1 => { + let member = generate_delegated_member( + state, + context, + result, + &freshened.arguments, + context.known_terms.compare, + )?; + let members = member.into_iter(); + members.collect() + }Based on the coding guideline "Always bind an iterator expression to a local variable before collecting or folding it."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@compiler-core/checking/src/source/derive/generate.rs` around lines 149 - 194, Update the four single-member dispatch arms in the members match—Eq, Eq1, Ord, and Ord1—to bind each generated member result to a local variable before calling into_iter().collect(). Preserve the existing optional-member behavior and collected result while removing the direct chained collection from generate_eq_member, generate_ord_member, and generate_delegated_member calls.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@compiler-core/checking/src/source/derive/foldable.rs`:
- Around line 44-45: Change the FunctionPolicy setting in both Foldable and
Bifoldable derivation paths from Allow to Reject, so function types containing a
derived parameter are rejected while fixed function fields remain valid. Add
checking fixtures covering unary and binary function-shaped fold derives.
---
Nitpick comments:
In `@compiler-core/checking/src/source/derive/generate.rs`:
- Around line 149-194: Update the four single-member dispatch arms in the
members match—Eq, Eq1, Ord, and Ord1—to bind each generated member result to a
local variable before calling into_iter().collect(). Preserve the existing
optional-member behavior and collected result while removing the direct chained
collection from generate_eq_member, generate_ord_member, and
generate_delegated_member calls.
In `@compiler-core/checking/src/source/derive/generate/traversable.rs`:
- Around line 1042-1046: Resolve and convert
self.builder.context.known_terms.apply to the term reference before entering the
remaining_effects loop. If it is absent, return Ok(None) before any map
application is emitted; inside the loop, reuse the hoisted apply value and
remove the repeated lookup and conversion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b2514d86-4da7-4a0f-93b8-31e131e9afae
⛔ Files ignored due to path filters (15)
tests-integration/fixtures/checking/1785484200_derive_traversable_function_rejection/Main.snapis excluded by!**/*.snaptests-integration/fixtures/semantic/1785484440_derive_bitraversable_either_body/Main.snapis excluded by!**/*.snaptests-integration/fixtures/semantic/1785484440_derive_bitraversable_nested_binary_body/Main.snapis excluded by!**/*.snaptests-integration/fixtures/semantic/1785484440_derive_bitraversable_nested_unary_body/Main.snapis excluded by!**/*.snaptests-integration/fixtures/semantic/1785484440_derive_bitraversable_pair_body/Main.snapis excluded by!**/*.snaptests-integration/fixtures/semantic/1785484440_derive_bitraversable_record_body/Main.snapis excluded by!**/*.snaptests-integration/fixtures/semantic/1785484440_derive_traversable_binary_both_body/Main.snapis excluded by!**/*.snaptests-integration/fixtures/semantic/1785484440_derive_traversable_binary_left_body/Main.snapis excluded by!**/*.snaptests-integration/fixtures/semantic/1785484440_derive_traversable_const_body/Main.snapis excluded by!**/*.snaptests-integration/fixtures/semantic/1785484440_derive_traversable_identity_body/Main.snapis excluded by!**/*.snaptests-integration/fixtures/semantic/1785484440_derive_traversable_maybe_body/Main.snapis excluded by!**/*.snaptests-integration/fixtures/semantic/1785484440_derive_traversable_nested_body/Main.snapis excluded by!**/*.snaptests-integration/fixtures/semantic/1785484440_derive_traversable_product_body/Main.snapis excluded by!**/*.snaptests-integration/fixtures/semantic/1785484440_derive_traversable_record_body/Main.snapis excluded by!**/*.snaptests-integration/fixtures/semantic/1785484440_derive_traversable_recursive_body/Main.snapis excluded by!**/*.snap
📒 Files selected for processing (25)
compiler-core/checking/src/context.rscompiler-core/checking/src/source/derive/contravariant.rscompiler-core/checking/src/source/derive/foldable.rscompiler-core/checking/src/source/derive/functor.rscompiler-core/checking/src/source/derive/generate.rscompiler-core/checking/src/source/derive/generate/functor.rscompiler-core/checking/src/source/derive/generate/traversable.rscompiler-core/checking/src/source/derive/member.rscompiler-core/checking/src/source/derive/traversable.rscompiler-core/checking/src/source/derive/variance.rstests-integration/fixtures/checking/1785484200_derive_traversable_function_rejection/Main.purstests-integration/fixtures/semantic/1785484440_derive_bitraversable_either_body/Main.purstests-integration/fixtures/semantic/1785484440_derive_bitraversable_nested_binary_body/Main.purstests-integration/fixtures/semantic/1785484440_derive_bitraversable_nested_unary_body/Main.purstests-integration/fixtures/semantic/1785484440_derive_bitraversable_pair_body/Main.purstests-integration/fixtures/semantic/1785484440_derive_bitraversable_record_body/Main.purstests-integration/fixtures/semantic/1785484440_derive_traversable_binary_both_body/Main.purstests-integration/fixtures/semantic/1785484440_derive_traversable_binary_left_body/Main.purstests-integration/fixtures/semantic/1785484440_derive_traversable_const_body/Main.purstests-integration/fixtures/semantic/1785484440_derive_traversable_identity_body/Main.purstests-integration/fixtures/semantic/1785484440_derive_traversable_maybe_body/Main.purstests-integration/fixtures/semantic/1785484440_derive_traversable_nested_body/Main.purstests-integration/fixtures/semantic/1785484440_derive_traversable_product_body/Main.purstests-integration/fixtures/semantic/1785484440_derive_traversable_record_body/Main.purstests-integration/fixtures/semantic/1785484440_derive_traversable_recursive_body/Main.purs
| function_policy: FunctionPolicy::Allow, | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Find relevant files"
git ls-files | rg '(^|/)foldable\.rs$|FunctionPolicy|Foldable|Bifoldable|derive.*foldable|derive.*bifold' || true
echo
echo "Inspect foldable.rs outline"
if [ -f compiler-core/checking/src/source/derive/foldable.rs ]; then
wc -l compiler-core/checking/src/source/derive/foldable.rs
ast-grep outline compiler-core/checking/src/source/derive/foldable.rs --view expanded || true
echo
sed -n '1,140p' compiler-core/checking/src/source/derive/foldable.rs | cat -n
fi
echo
echo "Search FunctionPolicy usages"
rg -n "FunctionPolicy|foldable|bifoldable|Foldable|Bifoldable" compiler-core/checking/src -S || trueRepository: purefunctor/purescript-alexandrite
Length of output: 8411
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Inspect variance implementation"
sed -n '1,340p' compiler-core/checking/src/source/derive/variance.rs | cat -n
echo
echo "Inspect traversable derive for comparison"
sed -n '1,120p' compiler-core/checking/src/source/derive/traversable.rs | cat -n
echo
echo "Find current fixtures related to function fields in Foldable/Bifoldable"
rg -n "fn ->|-> .*Foldable|Foldable|Bifoldable|Bifunctor|traversable|Traversable" tests-integration/fixtures/checking -S || true
echo
echo "Read relevant fixture/snap files"
for d in \
tests-integration/fixtures/checking/1772565960_derive_foldable_simple \
tests-integration/fixtures/checking/1772566020_derive_foldable_higher_kinded \
tests-integration/fixtures/checking/1772566200_derive_bifoldable_simple \
tests-integration/fixtures/checking/1772566260_derive_bifoldable_higher_kinded
do
echo "--- $d ---"
find "$d" -maxdepth 1 -type f -print | sort | xargs -r cat -n
doneRepository: purefunctor/purescript-alexandrite
Length of output: 50390
🌐 Web query:
PureScript Foldable f => Foldable (Reader r) foldM foldMap Reader function field
💡 Result:
In PureScript, the Reader monad is defined as type Reader r = ReaderT r Identity [1][2][3]. It does not have a Foldable instance because it is a functional wrapper (a computation that depends on an environment) rather than a container of values that can be folded [4][5][3]. The Foldable type class is intended for data structures that contain elements, such as Array, Maybe, or Either [4][5]. Because Reader r a represents a function r -> a, it does not store multiple elements in a structure that foldMap or foldM can traverse [4][1][2]. If you are looking for foldMap or foldM functionality related to Reader, you may be confusing it with other concepts: 1. Monad instances: Reader r is a Monad, which allows for chaining computations (bind), but this is distinct from folding over a structure [3][6]. 2. Applicative instances: Reader r is an Applicative functor [6]. 3. Mapping: You can modify the result of a Reader computation using mapReader (which is effectively map from the Functor instance) or withReader to modify the environment r [1][2][3]. If you need to perform a fold, the structure being folded must implement the Foldable type class [4]. If your goal is to fold values within a Reader context (i.e., folding while having access to an environment), you would typically fold the underlying container and perform the monadic operations within the Reader monad using standard traverse or foldM functions adapted for monads [4][7].
Citations:
- 1: https://pursuit.purescript.org/packages/purescript-transformers/3.2.0/docs/Control.Monad.Reader
- 2: https://github.com/purescript/purescript-transformers/blob/v4.2.0/src/Control/Monad/Reader.purs
- 3: https://pursuit.purescript.org/packages/purescript-transformers/6.1.0/docs/Control.Monad.Reader
- 4: https://pursuit.purescript.org/packages/purescript-foldable-traversable/6.0.0/docs/Data.Foldable
- 5: https://github.com/purescript/purescript-foldable-traversable/blob/f45f03a5b4a568c79ba36a8764edbb0cbee203e6/src/Data/Foldable.purs
- 6: https://github.com/purescript/purescript-transformers/blob/v4.2.0/src/Control/Monad/Reader/Trans.purs
- 7: https://hackage.haskell.org/package/rio-0.1.15.1/docs/RIO-Prelude.html
Reject function-shaped occurrences in Foldable and Bifoldable derives.
FunctionPolicy::Allow permits fields such as Int -> a, but foldMap/foldM must consume the a values in f a; a function field does not provide those values to the fold callback. Keep fixed function fields valid, but reject function types that contain a derived parameter. Apply FunctionPolicy::Reject at both derivation points and add checking fixtures for unary and binary function-shaped folds.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@compiler-core/checking/src/source/derive/foldable.rs` around lines 44 - 45,
Change the FunctionPolicy setting in both Foldable and Bifoldable derivation
paths from Allow to Reject, so function types containing a derived parameter are
rejected while fixed function fields remain valid. Add checking fixtures
covering unary and binary function-shaped fold derives.
Source: MCP tools
1 participant
Summary
Alexandrite already validates and registers derived
TraversableandBitraversableinstance heads, but it does not generate their dictionary members. This change completes those instances by generating:traverseandsequenceforTraversablebitraverseandbisequenceforBitraversableConstructor fields are traversed in source order and reconstructed applicatively with
pure,map, andapply. Records follow canonical row order, and recursive derived instances are resolved through the instance head registered before member generation.Implementation
sequenceastraversewith an identity lambda andbisequenceasbitraversewith two identity lambdas.Applicative,Apply,Traversable, andBitraversableknown terms.Bitraversable.Int -> a, which can be mapped but cannot be traversed, during variance checking.This follows the canonical PureScript class signatures and keeps member generation all-or-nothing when a required known term or decoded member signature is unavailable.
Tests
Semantic fixtures cover:
TraversabletypesBitraversablesums, products, unary and binary nesting, and recordsValidation completed:
just formatgit diff --checkjust t checkingjust t semanticcargo check -p checking --testscargo nextest run -p checking(30 tests passed)Stack
This is the base PR for the deriving stack. The Foldable implementation follows in a separate PR based on
traversable-deriving.