BE-582: HashQL: Change opaque types from invariant to covariant#8835
BE-582: HashQL: Change opaque types from invariant to covariant#8835indietyp wants to merge 2 commits into
Conversation
chore: add new dependency chore: format feat: error module feat: introduce hashql_eval interner chore: checkpoint feat: checkpoint feat: checkpoint chore: remove old value module feat: checkpoint feat: checkpoint feat: checkpoint feat: checkpoint feat: checkpoint chore: checkpoint feat: move entity query into its own modul fix: query request feat: checkpoint (it compiles!) feat: checkpoint feat: checkpoint feat: checkpoint fix: issue around cached thunking feat: covariance for opaque inners fix: cfgattr serde chore: remove graph module
PR SummaryHigh Risk Overview
Tests add Minor: Reviewed by Cursor Bugbot for commit f4fbb1d. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8835 +/- ##
==========================================
+ Coverage 59.13% 59.16% +0.02%
==========================================
Files 1346 1346
Lines 130077 130035 -42
Branches 5883 5884 +1
==========================================
+ Hits 76923 76931 +8
+ Misses 52248 52199 -49
+ Partials 906 905 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR updates HashQL’s type system so opaque types are treated as covariant in their inner representation (while remaining nominally distinct by opaque “family” name). It also fixes an incorrect “top” classification that could collapse unions across opaque boundaries, and updates solver/HIR snapshots and tests to reflect the new behavior.
Changes:
- Make opaque subtyping/constraint collection covariant (propagate constraints through the opaque wrapper).
- Fix
OpaqueType::is_topsoW<Unknown>is not considered the global top type, preserving nominal separation in unions. - Update lattice behavior/tests (notably
meetfor same-name concrete opaques) and refresh HIR lowering UI snapshots to reflect wrapped inference variables.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| libs/@local/hashql/hir/tests/ui/lower/specialization/filter-graph.stdout | Updates expected HIR output to show inference variables wrapped by their opaque type. |
| libs/@local/hashql/hir/tests/ui/lower/specialization/collect-filter-graph.stdout | Updates expected HIR output consistent with covariant opaque display changes. |
| libs/@local/hashql/hir/tests/ui/lower/specialization/collect-custom-collect.stderr | Updates expected diagnostic rendering to include opaque wrappers around inference vars. |
| libs/@local/hashql/hir/tests/ui/lower/checking/minimal-graph.stdout | Refreshes UI snapshot output for wrapped inference variables in minimal graph checking. |
| libs/@local/hashql/hir/tests/ui/lower/checking/filter-graph.stdout | Refreshes UI snapshot output for wrapped inference variables in graph checking. |
| libs/@local/hashql/hir/tests/ui/lower/checking/collect-filter-graph.stdout | Refreshes UI snapshot output for wrapped inference variables in collect+checking path. |
| libs/@local/hashql/core/src/type/visit.rs | Adds #[inline] to Default impl for RecursiveVisitorGuard. |
| libs/@local/hashql/core/src/type/recursion.rs | Adds #[inline] to Default impl for RecursionBoundary. |
| libs/@local/hashql/core/src/type/kind/tests/opaque.rs | Updates and adds tests covering covariance, is_top behavior, and new constraint shapes. |
| libs/@local/hashql/core/src/type/kind/opaque.rs | Implements covariant opaque subtype/constraints, corrects is_top, and updates meet semantics + docs. |
| libs/@local/hashql/core/src/type/inference/solver/tests.rs | Adds regression test for covariant opaque subtyping through unions; adds a module-level clippy expectation. |
| libs/@local/hashql/core/src/type/inference/solver/mod.rs | Adds #[inline] to Default impl for VariableConstraintSatisfiability. |
| libs/@local/hashql/core/src/type/inference/solver/graph.rs | Adds #[inline] to Default impl for Edges. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Merging this PR will improve performance by 19.34%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ⚡ | pattern_match_constant |
180 ns | 150.8 ns | +19.34% |
Tip
Curious why this is faster? Comment @codspeedbot explain why this is faster on this PR, or directly use the CodSpeed MCP with your agent.
Comparing bm/be-582-hashql-make-opaque-types-covariant-in-their-inner (f4fbb1d) with main (232b539)
fc41426 to
f4fbb1d
Compare
Benchmark results
|
| Function | Value | Mean | Flame graphs |
|---|---|---|---|
| resolve_policies_for_actor | user: empty, selectivity: high, policies: 2002 | Flame Graph | |
| resolve_policies_for_actor | user: empty, selectivity: low, policies: 1 | Flame Graph | |
| resolve_policies_for_actor | user: empty, selectivity: medium, policies: 1001 | Flame Graph | |
| resolve_policies_for_actor | user: seeded, selectivity: high, policies: 3314 | Flame Graph | |
| resolve_policies_for_actor | user: seeded, selectivity: low, policies: 1 | Flame Graph | |
| resolve_policies_for_actor | user: seeded, selectivity: medium, policies: 1526 | Flame Graph | |
| resolve_policies_for_actor | user: system, selectivity: high, policies: 2078 | Flame Graph | |
| resolve_policies_for_actor | user: system, selectivity: low, policies: 1 | Flame Graph | |
| resolve_policies_for_actor | user: system, selectivity: medium, policies: 1033 | Flame Graph |
policy_resolution_medium
| Function | Value | Mean | Flame graphs |
|---|---|---|---|
| resolve_policies_for_actor | user: empty, selectivity: high, policies: 102 | Flame Graph | |
| resolve_policies_for_actor | user: empty, selectivity: low, policies: 1 | Flame Graph | |
| resolve_policies_for_actor | user: empty, selectivity: medium, policies: 51 | Flame Graph | |
| resolve_policies_for_actor | user: seeded, selectivity: high, policies: 269 | Flame Graph | |
| resolve_policies_for_actor | user: seeded, selectivity: low, policies: 1 | Flame Graph | |
| resolve_policies_for_actor | user: seeded, selectivity: medium, policies: 107 | Flame Graph | |
| resolve_policies_for_actor | user: system, selectivity: high, policies: 133 | Flame Graph | |
| resolve_policies_for_actor | user: system, selectivity: low, policies: 1 | Flame Graph | |
| resolve_policies_for_actor | user: system, selectivity: medium, policies: 63 | Flame Graph |
policy_resolution_none
| Function | Value | Mean | Flame graphs |
|---|---|---|---|
| resolve_policies_for_actor | user: empty, selectivity: high, policies: 2 | Flame Graph | |
| resolve_policies_for_actor | user: empty, selectivity: low, policies: 1 | Flame Graph | |
| resolve_policies_for_actor | user: empty, selectivity: medium, policies: 1 | Flame Graph | |
| resolve_policies_for_actor | user: system, selectivity: high, policies: 8 | Flame Graph | |
| resolve_policies_for_actor | user: system, selectivity: low, policies: 1 | Flame Graph | |
| resolve_policies_for_actor | user: system, selectivity: medium, policies: 3 | Flame Graph |
policy_resolution_small
| Function | Value | Mean | Flame graphs |
|---|---|---|---|
| resolve_policies_for_actor | user: empty, selectivity: high, policies: 52 | Flame Graph | |
| resolve_policies_for_actor | user: empty, selectivity: low, policies: 1 | Flame Graph | |
| resolve_policies_for_actor | user: empty, selectivity: medium, policies: 25 | Flame Graph | |
| resolve_policies_for_actor | user: seeded, selectivity: high, policies: 94 | Flame Graph | |
| resolve_policies_for_actor | user: seeded, selectivity: low, policies: 1 | Flame Graph | |
| resolve_policies_for_actor | user: seeded, selectivity: medium, policies: 26 | Flame Graph | |
| resolve_policies_for_actor | user: system, selectivity: high, policies: 66 | Flame Graph | |
| resolve_policies_for_actor | user: system, selectivity: low, policies: 1 | Flame Graph | |
| resolve_policies_for_actor | user: system, selectivity: medium, policies: 29 | Flame Graph |
read_scaling_complete
| Function | Value | Mean | Flame graphs |
|---|---|---|---|
| entity_by_id;one_depth | 1 entities | Flame Graph | |
| entity_by_id;one_depth | 10 entities | Flame Graph | |
| entity_by_id;one_depth | 25 entities | Flame Graph | |
| entity_by_id;one_depth | 5 entities | Flame Graph | |
| entity_by_id;one_depth | 50 entities | Flame Graph | |
| entity_by_id;two_depth | 1 entities | Flame Graph | |
| entity_by_id;two_depth | 10 entities | Flame Graph | |
| entity_by_id;two_depth | 25 entities | Flame Graph | |
| entity_by_id;two_depth | 5 entities | Flame Graph | |
| entity_by_id;two_depth | 50 entities | Flame Graph | |
| entity_by_id;zero_depth | 1 entities | Flame Graph | |
| entity_by_id;zero_depth | 10 entities | Flame Graph | |
| entity_by_id;zero_depth | 25 entities | Flame Graph | |
| entity_by_id;zero_depth | 5 entities | Flame Graph | |
| entity_by_id;zero_depth | 50 entities | Flame Graph |
read_scaling_linkless
| Function | Value | Mean | Flame graphs |
|---|---|---|---|
| entity_by_id | 1 entities | Flame Graph | |
| entity_by_id | 10 entities | Flame Graph | |
| entity_by_id | 100 entities | Flame Graph | |
| entity_by_id | 1000 entities | Flame Graph | |
| entity_by_id | 10000 entities | Flame Graph |
representative_read_entity
| Function | Value | Mean | Flame graphs |
|---|---|---|---|
| entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/block/v/1
|
Flame Graph | |
| entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/book/v/1
|
Flame Graph | |
| entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/building/v/1
|
Flame Graph | |
| entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/organization/v/1
|
Flame Graph | |
| entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/page/v/2
|
Flame Graph | |
| entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/person/v/1
|
Flame Graph | |
| entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/playlist/v/1
|
Flame Graph | |
| entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/song/v/1
|
Flame Graph | |
| entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/uk-address/v/1
|
Flame Graph |
representative_read_entity_type
| Function | Value | Mean | Flame graphs |
|---|---|---|---|
| get_entity_type_by_id | Account ID: bf5a9ef5-dc3b-43cf-a291-6210c0321eba
|
Flame Graph |
representative_read_multiple_entities
| Function | Value | Mean | Flame graphs |
|---|---|---|---|
| entity_by_property | traversal_paths=0 | 0 | |
| entity_by_property | traversal_paths=255 | 1,resolve_depths=inherit:1;values:255;properties:255;links:127;link_dests:126;type:true | |
| entity_by_property | traversal_paths=2 | 1,resolve_depths=inherit:0;values:0;properties:0;links:0;link_dests:0;type:false | |
| entity_by_property | traversal_paths=2 | 1,resolve_depths=inherit:0;values:0;properties:0;links:1;link_dests:0;type:true | |
| entity_by_property | traversal_paths=2 | 1,resolve_depths=inherit:0;values:0;properties:2;links:1;link_dests:0;type:true | |
| entity_by_property | traversal_paths=2 | 1,resolve_depths=inherit:0;values:2;properties:2;links:1;link_dests:0;type:true | |
| link_by_source_by_property | traversal_paths=0 | 0 | |
| link_by_source_by_property | traversal_paths=255 | 1,resolve_depths=inherit:1;values:255;properties:255;links:127;link_dests:126;type:true | |
| link_by_source_by_property | traversal_paths=2 | 1,resolve_depths=inherit:0;values:0;properties:0;links:0;link_dests:0;type:false | |
| link_by_source_by_property | traversal_paths=2 | 1,resolve_depths=inherit:0;values:0;properties:0;links:1;link_dests:0;type:true | |
| link_by_source_by_property | traversal_paths=2 | 1,resolve_depths=inherit:0;values:0;properties:2;links:1;link_dests:0;type:true | |
| link_by_source_by_property | traversal_paths=2 | 1,resolve_depths=inherit:0;values:2;properties:2;links:1;link_dests:0;type:true |
scenarios
| Function | Value | Mean | Flame graphs |
|---|---|---|---|
| full_test | query-limited | Flame Graph | |
| full_test | query-unlimited | Flame Graph | |
| linked_queries | query-limited | Flame Graph | |
| linked_queries | query-unlimited | Flame Graph |

🌟 What is the purpose of this PR?
Opaque types in HashQL were previously treated as invariant in their inner representation. This was overly restrictive: because HashQL values are immutable, there is no mutation-based unsoundness argument that would require invariance. This PR changes opaque types to be covariant, meaning
W<A> <: W<B>wheneverA <: B, while still preserving nominal separation between distinct opaque families.This fixes a regression where expressions like
W(A()) as W<A | B>would fail to type-check, because the solver could not propagate constraints through the opaque wrapper when the target type was a union. With covariance, the solver correctly decomposesW<?1> <: W<A | B>into?1 <: A | B, and combined with the lower boundA <: ?1, resolves?1 = A.The
is_topimplementation for opaque types is also corrected:W<Unknown>is the top of theWfiber only, not the global top. Returningtruepreviously causedT | W<Unknown>to incorrectly collapse toUnknown, destroying nominal separation between opaque families.As a consequence of covariance, inference constraints that previously generated
Equals/Unifyconstraints now generateLowerBound/UpperBound/Orderingconstraints, and themeetoperation for same-name concrete opaques now correctly computes the meet of their inner representations rather than requiring identical representations.The snapshot test outputs for HIR lowering are updated to reflect that inference variables inside opaque wrappers now display with their opaque wrapper (e.g.
::graph::types::knowledge::entity::PropertyProvenance(?)instead of bare?), which is a direct consequence of the solver now propagating through the opaque boundary.🔍 What does this change?
OpaqueType::is_subtype_ofto useVariance::Covariantinstead ofVariance::Invariant.OpaqueType::collect_constraintsto emit covariant constraints instead of invariant ones.OpaqueType::is_topto always returnfalse, since an opaque type is never the global top regardless of its inner representation.OpaqueType::meetto handle the concrete same-name case by computing the full lattice meet of the inner representations, rather than requiring structural equality.opaque_covariant_subtype_through_unioncovering theW(A()) as W<A | B>pattern.is_not_topverifying thatW<Unknown>is not the global top.LowerBound/UpperBound/Orderinginstead ofEquals/Unify).OpaqueTypedoc comment with a formal denotational semantics model justifying covariance.🛡 What tests cover this?
opaque_covariant_subtype_through_unionin the inference solver test suite.is_not_topin the opaque type test suite.❓ How to test this?
cargo testinlibs/@local/hashql/core.libs/@local/hashql/hir.