[checking] Generate Contravariant and Profunctor derived members - #236
Merged
Conversation
|
Warning Review limit reached
Next review available in: 38 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (31)
📒 Files selected for processing (35)
Summary
Testing
Confidence 4/5 - Broad coverage with complex variance logicThe implementation has extensive fixture coverage across supported constructors and variance combinations. The main merge risk is the complexity of the new contravariant and profunctor traversal generator, especially around nested applications and constraint resolution. WalkthroughThe compiler now supports variance-aware derivation for ChangesVariance-aware derivation
Sequence Diagram(s)sequenceDiagram
participant DeriveMember
participant VarianceChecker
participant InstanceGenerator
participant KnownTermsCore
DeriveMember->>VarianceChecker: build variance recipe
VarianceChecker->>InstanceGenerator: pass traversal recipe
InstanceGenerator->>KnownTermsCore: resolve cmap or dimap
InstanceGenerator-->>DeriveMember: return generated instance member
🚥 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 #236 +/- ##
==========================================
- Coverage 83.67% 83.60% -0.08%
==========================================
Files 159 160 +1
Lines 35439 36102 +663
Branches 35439 36102 +663
==========================================
+ Hits 29655 30183 +528
- Misses 3131 3208 +77
- Partials 2653 2711 +58 ☔ 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.
🧹 Nitpick comments (6)
compiler-core/checking/src/source/derive/generate/foldable.rs (2)
534-534: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueConsider matching the covariant variance explicitly instead of ignoring it.
Fold derivation is only reachable with
FunctionPolicy::Reject, so the recorded variance is alwaysVariance::Covarianttoday. The..pattern hides that assumption. If a fold-style configuration later allows function fields, the mixed path would record a contravariant edge and this code would fold across it silently. Matchingargument_variance: Variance::Covariantand returningOk(None)for the contravariant case would make the invariant explicit.As per coding guidelines: "Investigate architectural root faults, avoid escape hatches and temporary fixes, and use the type system to encode correctness."
Also applies to: 586-586
🤖 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/foldable.rs` at line 534, Update the TraversalOperation::UnaryApplication match in the fold derivation logic to bind argument_variance and accept only Variance::Covariant; return Ok(None) for contravariant edges so unsupported variance cannot be folded silently. Apply the same explicit variance handling to the corresponding match around the second referenced location.Source: Coding guidelines
534-586: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueCovariant-only variance is assumed implicitly in the fold and traverse emitters. Both emitters now discard the new
argument_varianceandfirst_variancefields with... The assumption holds only because Foldable, Bifoldable, Traversable, and Bitraversable all configureFunctionPolicy::Reject, which preventssupports_contravariant_traversalfrom selecting the mixed path. That link is not visible at either call site.
compiler-core/checking/src/source/derive/generate/foldable.rs#L534-L586: matchargument_variance: Variance::Covariantandfirst_variance: Variance::Covariant, and returnOk(None)for the contravariant case.compiler-core/checking/src/source/derive/generate/traversable.rs#L632-L701: apply the same explicit match inemit_effect.🤖 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/foldable.rs` around lines 534 - 586, Make variance handling explicit in foldable.rs at lines 534-586 and traversable.rs at lines 632-701: in the relevant operation matches, accept only argument_variance: Variance::Covariant and first_variance: Variance::Covariant, returning Ok(None) for contravariant cases. Update foldable.rs and traversable.rs consistently; no other sites require changes.compiler-core/checking/src/source/derive/generate/traversable.rs (1)
632-632: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueConsider matching the covariant variance explicitly instead of ignoring it.
Traversable and Bitraversable use
FunctionPolicy::Reject, so the recorded variance is alwaysVariance::Covariant. The..pattern hides that assumption, in the same way ascompiler-core/checking/src/source/derive/generate/foldable.rs.Also applies to: 701-701
🤖 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` at line 632, Update the TraversalOperation::UnaryApplication matches in the traversable generation logic to bind or explicitly match the variance as Variance::Covariant instead of ignoring it with `..`; apply the same change at both referenced match sites while preserving the existing FunctionPolicy::Reject behavior.compiler-core/checking/src/source/derive/variance.rs (2)
824-834: 🚀 Performance & Scalability | 🔵 TrivialConsider caching the elaborated available constraints.
has_instance_headcanonicalises and elaborates the wholeavailable_constraintslist on every call.check_mixed_applicationcan call it up to four times per type application, and the checker visits every field of every constructor. The elaborated set does not change during a single derivation, so computing it once perVarianceFieldCheckerwould remove repeated work.🤖 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/variance.rs` around lines 824 - 834, The has_instance_head path currently canonicalises and elaborates available_constraints on every call, causing repeated work during a derivation. Cache the elaborated canonical constraints as state owned by VarianceFieldChecker, initialise it once per checker, and have has_instance_head reuse that cached result while preserving the existing filtering and error propagation behavior.
737-758: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider removing the
unreachable!by restructuring the guard.The condition already tests
second.is_some(), so theelsebranch cannot run. The type system can encode this instead of a runtime panic. Match on(first, second)before the instance lookup, or bindsecondwithif let Some(second) = secondas the first term of the chain.♻️ Suggested restructure
- if first.is_none() - && second.is_some() + if first.is_none() + && let Some(second_operation) = second.as_ref() && let Some(functor) = self.context.known_types.functor && has_instance_head( self.state, self.context, self.available_constraints, functor, application.function, )? { if *self.valid { tools::emit_constraint(self.context, self.state, functor, application.function); } - let Some(second) = second else { - unreachable!("right-only traversal requires a second operation") - }; return Ok(Some(TraversalOperation::UnaryApplication { argument_variance: Variance::Covariant, - argument: Box::new(second), + argument: Box::new(second_operation.clone()), })); }The clone can be avoided by restructuring the match order if
TraversalOperationis notClone.🤖 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/variance.rs` around lines 737 - 758, Restructure the unary-application guard around the relevant traversal logic so `second` is bound as `Some` before the functor instance lookup, eliminating the redundant `unreachable!` branch. Preserve the existing constraint emission and `TraversalOperation::UnaryApplication` construction, and avoid cloning the traversal operation if possible.compiler-core/checking/src/source/derive/generate/contravariant.rs (1)
460-644: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftShare the duplicated emitter helpers with
generate/functor.rs.
TraversalContext,emit_transformer,emit_identity,emit_record_traversal, andextract_record_roware identical to their counterparts incompiler-core/checking/src/source/derive/generate/functor.rs.emit_binaryalso matchesfunctor.rs::emit_binary_applicationapart from the documentation comments. Any future fix to variance orientation or record reconstruction must be applied twice, and the two copies can drift.Extract the shared pieces into a common module, for example
generate/traversal_context.rs, or define a small trait over the mapping lookup so both emitters reuse one implementation. Keep theFunctionarm specific to the contravariant emitter, because that arm is the genuine difference between the two.As per coding guidelines: "In Rust code, avoid abstractions for their own sake; introduce them when they improve clarity, reduce real complexity, or simplify repeated work."
🤖 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/contravariant.rs` around lines 460 - 644, Extract the duplicated traversal helpers—TraversalContext, emit_transformer, emit_identity, emit_record_traversal, extract_record_row, and the shared binary-emission logic—into a common generate module reusable by both the contravariant emitter and functor.rs. Update both emitters to call the shared implementation while retaining the contravariant emitter’s Function-specific arm and variance behavior where it genuinely differs.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.
Nitpick comments:
In `@compiler-core/checking/src/source/derive/generate/contravariant.rs`:
- Around line 460-644: Extract the duplicated traversal
helpers—TraversalContext, emit_transformer, emit_identity,
emit_record_traversal, extract_record_row, and the shared binary-emission
logic—into a common generate module reusable by both the contravariant emitter
and functor.rs. Update both emitters to call the shared implementation while
retaining the contravariant emitter’s Function-specific arm and variance
behavior where it genuinely differs.
In `@compiler-core/checking/src/source/derive/generate/foldable.rs`:
- Line 534: Update the TraversalOperation::UnaryApplication match in the fold
derivation logic to bind argument_variance and accept only Variance::Covariant;
return Ok(None) for contravariant edges so unsupported variance cannot be folded
silently. Apply the same explicit variance handling to the corresponding match
around the second referenced location.
- Around line 534-586: Make variance handling explicit in foldable.rs at lines
534-586 and traversable.rs at lines 632-701: in the relevant operation matches,
accept only argument_variance: Variance::Covariant and first_variance:
Variance::Covariant, returning Ok(None) for contravariant cases. Update
foldable.rs and traversable.rs consistently; no other sites require changes.
In `@compiler-core/checking/src/source/derive/generate/traversable.rs`:
- Line 632: Update the TraversalOperation::UnaryApplication matches in the
traversable generation logic to bind or explicitly match the variance as
Variance::Covariant instead of ignoring it with `..`; apply the same change at
both referenced match sites while preserving the existing FunctionPolicy::Reject
behavior.
In `@compiler-core/checking/src/source/derive/variance.rs`:
- Around line 824-834: The has_instance_head path currently canonicalises and
elaborates available_constraints on every call, causing repeated work during a
derivation. Cache the elaborated canonical constraints as state owned by
VarianceFieldChecker, initialise it once per checker, and have has_instance_head
reuse that cached result while preserving the existing filtering and error
propagation behavior.
- Around line 737-758: Restructure the unary-application guard around the
relevant traversal logic so `second` is bound as `Some` before the functor
instance lookup, eliminating the redundant `unreachable!` branch. Preserve the
existing constraint emission and `TraversalOperation::UnaryApplication`
construction, and avoid cloning the traversal operation if possible.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ee9b676e-1c90-40ea-9a5b-dbb2117b1dc3
⛔ Files ignored due to path filters (31)
tests-integration/fixtures/checking/1772565240_derive_bifunctor_missing_functor/Main.snapis excluded by!**/*.snaptests-integration/fixtures/checking/1772567520_derive_functor_higher_kinded/Main.snapis excluded by!**/*.snaptests-integration/fixtures/checking/1772567580_derive_bifunctor_higher_kinded/Main.snapis excluded by!**/*.snaptests-integration/fixtures/checking/1785319620_derive_bifunctor_captured_application_argument/Main.snapis excluded by!**/*.snaptests-integration/fixtures/checking/1785319980_preserve_unary_variance_application_checks/Main.snapis excluded by!**/*.snaptests-integration/fixtures/checking/1785598200_derive_contravariant_captured_application_head/Main.snapis excluded by!**/*.snaptests-integration/fixtures/checking/1785598200_derive_contravariant_missing_traversal_instance/Main.snapis excluded by!**/*.snaptests-integration/fixtures/checking/1785598200_derive_contravariant_nested_parity_rejection/Main.snapis excluded by!**/*.snaptests-integration/fixtures/checking/1785598200_derive_profunctor_bifunctor_priority_rejection/Main.snapis excluded by!**/*.snaptests-integration/fixtures/semantic/1785598200_derive_contravariant_empty_data_body/Main.snapis excluded by!**/*.snaptests-integration/fixtures/semantic/1785598200_derive_contravariant_function_body/Main.snapis excluded by!**/*.snaptests-integration/fixtures/semantic/1785598200_derive_contravariant_functor_application_body/Main.snapis excluded by!**/*.snaptests-integration/fixtures/semantic/1785598200_derive_contravariant_mixed_application_body/Main.snapis excluded by!**/*.snaptests-integration/fixtures/semantic/1785598200_derive_contravariant_record_body/Main.snapis excluded by!**/*.snaptests-integration/fixtures/semantic/1785598200_derive_profunctor_function_body/Main.snapis excluded by!**/*.snaptests-integration/fixtures/semantic/1785598200_derive_profunctor_nested_both_body/Main.snapis excluded by!**/*.snaptests-integration/fixtures/semantic/1785598200_derive_profunctor_nested_left_body/Main.snapis excluded by!**/*.snaptests-integration/fixtures/semantic/1785598200_derive_profunctor_unary_preference_body/Main.snapis excluded by!**/*.snaptests-integration/fixtures/semantic/1785598260_derive_profunctor_bifunctor_application_body/Main.snapis excluded by!**/*.snaptests-integration/fixtures/semantic/1785598320_derive_contravariant_given_application_body/Main.snapis excluded by!**/*.snaptests-integration/fixtures/semantic/1785598320_derive_contravariant_triple_application_body/Main.snapis excluded by!**/*.snaptests-integration/fixtures/semantic/1785598320_derive_profunctor_given_application_body/Main.snapis excluded by!**/*.snaptests-integration/fixtures/semantic/1785598320_derive_profunctor_nested_right_body/Main.snapis excluded by!**/*.snaptests-integration/fixtures/semantic/1785598440_derive_contravariant_nullary_constructor_body/Main.snapis excluded by!**/*.snaptests-integration/fixtures/semantic/1785598440_derive_contravariant_open_record_body/Main.snapis excluded by!**/*.snaptests-integration/fixtures/semantic/1785598440_derive_contravariant_phantom_body/Main.snapis excluded by!**/*.snaptests-integration/fixtures/semantic/1785598920_derive_contravariant_bifunctor_application_body/Main.snapis excluded by!**/*.snaptests-integration/fixtures/semantic/1785598920_derive_profunctor_record_body/Main.snapis excluded by!**/*.snaptests-integration/fixtures/semantic/1785603960_derive_bifunctor_mixed_variance_application_body/Main.snapis excluded by!**/*.snaptests-integration/fixtures/semantic/1785603960_derive_functor_contravariant_application_body/Main.snapis excluded by!**/*.snaptests-integration/fixtures/semantic/1785603960_derive_functor_profunctor_application_body/Main.snapis excluded by!**/*.snap
📒 Files selected for processing (35)
compiler-core/checking/src/context.rscompiler-core/checking/src/source/derive/contravariant.rscompiler-core/checking/src/source/derive/generate.rscompiler-core/checking/src/source/derive/generate/contravariant.rscompiler-core/checking/src/source/derive/generate/foldable.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/variance.rstests-integration/fixtures/checking/1785598200_derive_contravariant_captured_application_head/Main.purstests-integration/fixtures/checking/1785598200_derive_contravariant_missing_traversal_instance/Main.purstests-integration/fixtures/checking/1785598200_derive_contravariant_nested_parity_rejection/Main.purstests-integration/fixtures/checking/1785598200_derive_profunctor_bifunctor_priority_rejection/Main.purstests-integration/fixtures/semantic/1785598200_derive_contravariant_empty_data_body/Main.purstests-integration/fixtures/semantic/1785598200_derive_contravariant_function_body/Main.purstests-integration/fixtures/semantic/1785598200_derive_contravariant_functor_application_body/Main.purstests-integration/fixtures/semantic/1785598200_derive_contravariant_mixed_application_body/Main.purstests-integration/fixtures/semantic/1785598200_derive_contravariant_record_body/Main.purstests-integration/fixtures/semantic/1785598200_derive_profunctor_function_body/Main.purstests-integration/fixtures/semantic/1785598200_derive_profunctor_nested_both_body/Main.purstests-integration/fixtures/semantic/1785598200_derive_profunctor_nested_left_body/Main.purstests-integration/fixtures/semantic/1785598200_derive_profunctor_unary_preference_body/Main.purstests-integration/fixtures/semantic/1785598260_derive_profunctor_bifunctor_application_body/Main.purstests-integration/fixtures/semantic/1785598320_derive_contravariant_given_application_body/Main.purstests-integration/fixtures/semantic/1785598320_derive_contravariant_triple_application_body/Main.purstests-integration/fixtures/semantic/1785598320_derive_profunctor_given_application_body/Main.purstests-integration/fixtures/semantic/1785598320_derive_profunctor_nested_right_body/Main.purstests-integration/fixtures/semantic/1785598440_derive_contravariant_nullary_constructor_body/Main.purstests-integration/fixtures/semantic/1785598440_derive_contravariant_open_record_body/Main.purstests-integration/fixtures/semantic/1785598440_derive_contravariant_phantom_body/Main.purstests-integration/fixtures/semantic/1785598920_derive_contravariant_bifunctor_application_body/Main.purstests-integration/fixtures/semantic/1785598920_derive_profunctor_record_body/Main.purstests-integration/fixtures/semantic/1785603960_derive_bifunctor_mixed_variance_application_body/Main.purstests-integration/fixtures/semantic/1785603960_derive_functor_contravariant_application_body/Main.purstests-integration/fixtures/semantic/1785603960_derive_functor_profunctor_application_body/Main.purs
purefunctor
force-pushed
the
derive-contravariant-profunctor
branch
from
August 2, 2026 04:30
ab369de to
511058a
Compare
purefunctor
force-pushed
the
derive-contravariant-profunctor
branch
from
August 2, 2026 04:30
511058a to
35df850
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Add generated instance members for the
ContravariantandProfunctorclass family.The implementation keeps Contravariant/Profunctor emission in an independent generator while extending the shared variance recipe to represent covariant and contravariant application edges. It supports functions, records, unary applications, binary Bifunctor/Profunctor applications, phantom parameters, open records, and available instance constraints.
Functor and Bifunctor derivation can now traverse nested Contravariant and Profunctor edges, recovering covariance across negative boundaries. Foldable and Traversable retain their existing covariant-only traversal policy.
The commits preserve the mixed-variance regressions before applying the Functor/Bifunctor fix, making the generated semantic-tree changes directly reviewable.
Testing
cargo check -p checking --testscargo nextest run -p checkingjust t checkingjust t semanticcargo clippy -p checking --tests -- -D warnings -A clippy::too_many_argumentsjust format