fix(contacts): use fn items for LID mapping extractors so boxed futures compile - #826
Conversation
…es compile Closures returning references tied to their argument are inferred at a concrete lifetime; embedded in is_on_whatsapp/get_user_info's future types, they made any consumer that boxes those futures (async_trait) fail with "implementation of FnOnce is not general enough". Fn items implement Fn for every lifetime by construction. A compile-time regression test reproduces the consumer shape from the report. Fixes #825
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR fixes a lifetime inference failure that prevented ChangesAsync-Trait Closure Lifetime Fix
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
@claude review |
|
Claude finished @jlucaso1's task in 2m 10s —— View job ReviewVerdict: LGTM — surgical, correct fix with a well-targeted compile-time regression guard. Why this fix is rightThe diagnosis in the PR body is precisely the failure mode. Rust infers closures at concrete lifetimes; once the closure type is captured inside the public method's future, every downstream caller that boxes the future ( Behaviorally the three fn items ( Test design
SweepVerified the author's claim that other Suggestions (non-blocking)
Neither is a blocker. Ship it.
|
Benchmark Results67 unchanged benchmark(s)
|
Review follow-up: the next call-site author reads the receiver's signature, not the existing extractors, so the HRTB warning lives there too. Also one-line semantics on each extractor.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e61d3139f8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /// Mapping extractors as fn items, NOT closures. A closure returning | ||
| /// references tied to its argument is inferred at a concrete lifetime, and | ||
| /// because its type is embedded in the public methods' future types, callers | ||
| /// that box those futures (`#[async_trait]`, `Box<dyn Future + Send>`) hit | ||
| /// "implementation of `FnOnce` is not general enough" (issue #825). Fn items |
There was a problem hiding this comment.
AGENTS.md says, “When adding comments to the code, dont be so verbose, also only explain why, not what.” This new doc block explains the implementation mechanics and downstream compiler behavior in detail, including what fn items do, so it violates the local comment guidance and leaves commit/issue-level rationale in source. Please trim it to the minimal why, or move the longer explanation to external documentation.
Useful? React with 👍 / 👎.
1 participant
Problem
is_on_whatsapp()andget_user_info()failed to compile for any consumer that boxes their futures, which is exactly what#[async_trait]does:Reported in #825 with an excellent minimal repro. The
RwLockin the report is incidental: the trigger is theBox<dyn Future + Send>thatasync_traitproduces, which forces the compiler to prove auto traits and higher-ranked bounds for the whole future tree. The real culprit lives in this library: thepersist_lid_mappingscall sites passed closures returning references tied to their argument (|r| (&r.jid, r.lid.as_ref())). Rust infers such closures at a concrete lifetime rather than the higher-rankedfor<'r> Fn(&'r _), and since the closure types are embedded in the public methods' future types, the unprovable obligation leaks to every boxing consumer. None of the user-side workarounds can fix it, since the problem is inside the library's future type.Fix
Replace the three borrowing closures (two in
is_on_whatsapp, one inget_user_info) with fn items, which implementFnfor every lifetime by construction. Zero clones, zero public API change, identical behavior.Swept the rest of the workspace for the same class: the other generic helpers with borrowed-item bounds (
GroupInfo::add_participants,DeviceTopology::record,DeviceRegistryCache::insert,participant_list_hash) are either synchronous (the iterator never lives inside a future) or only ever receive closure-free iterators (slices/Chain), and theFDownloadclosures in appstate return owned values, which generalize fine. The contacts call sites were the only affected surface.Tests
New
tests/async_trait_boxed_future_compat.rs: a compile-time regression guard reproducing the consumer shape from the report (#[async_trait]+RwLock<Option<Arc<Client>>>calling both methods). This class of error only manifests in the downstream crate's context, so the guard IS the compilation. Verified it catches the regression: with the previous closures restored, the guard fails with six "not general enough" errors; with the fn items it compiles and passes.cargo fmt --allcargo clippy --all-targets -- -D warningscargo test -p whatsapp-rust --lib(765 passing)cargo test -p whatsapp-rust --test async_trait_boxed_future_compatBreaking
None.
Fixes #825