refactor: remove lifetime 'a and replace with 'static
#28
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThis PR removes explicit lifetime parameters from core public traits and types (Action, Filter, Sorter, UI, Source, Launcher, Batcher), replaces per-lifetime bounds with Send/Sync/'static constraints, simplifies PhantomData usage, fixes naming typos (e.g., Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)src/launcher.rs (4)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
🔇 Additional comments (5)
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 |
Summary of ChangesHello @satler-git, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the codebase by eliminating the explicit lifetime parameter Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is a significant refactoring that removes the generic lifetime 'a and replaces it with 'static bounds. This simplifies the overall API and has been applied consistently across multiple files. I appreciate the additional fixes for the 'cusion' -> 'cushion' typo, which improves code clarity. The changes look good, but I've found a couple of minor issues in src/sorter.rs where leftover code from the refactoring has resulted in an unnecessary trait bound and a syntax error.
CodSpeed Performance ReportMerging #28 will not alter performanceComparing Summary
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/launcher/batcher.rs (1)
188-209: Fix borrow-across-await in prepare(): clone input before awaiting.You capture &self.state.input inside async blocks, holding a borrow of self across .await. This is a common compile-time error. Also, Relaxed load suffices after join_all.
let gen_len = self.generators.len(); if self.state.gen_index < gen_len && batch_count > 0 { use std::sync::atomic::{AtomicUsize, Ordering}; let len = AtomicUsize::new(0); + // Avoid borrowing `self` across `.await` + let input = self.state.input.clone(); + let len_ref = &len; let gen_count_to_run = batch_count.min(gen_len - self.state.gen_index); // Iterator<Item = impl Future<Output = Vec<Cushion>>> // でjoin_allでFutureを解決して - let cushions_from_gen = self.generators + let cushions_from_gen = self.generators [self.state.gen_index..(self.state.gen_index + gen_count_to_run)] .iter() - .map(|r#gen| async { - let cushions = r#gen.generate(&self.state.input).await.into_iter(); - // 最終結果で計算が終わったあとの長さにしか興味がないからRelaxedで問題ない - len.fetch_add(cushions.len(), Ordering::Relaxed); - cushions - }); + .map(|gen| { + let input = input.clone(); + async move { + let cushions = gen.generate(&input).await.into_iter(); + // Only final value matters -> Relaxed is fine + len_ref.fetch_add(cushions.len(), Ordering::Relaxed); + cushions + } + }); let cushions_from_gen = futures::future::join_all(cushions_from_gen) .await .into_iter() .flatten(); - v.reserve(len.load(Ordering::SeqCst)); + v.reserve(len.load(Ordering::Relaxed)); for c in cushions_from_gen {Also applies to: 213-215
src/launcher.rs (1)
168-171: Doc default vs. implementation: filter_and.Docs state default=false, but Batcher default is currently true. If you adopt the Batcher fix (set false), this doc is correct; otherwise update the doc.
Do you want me to update the doc comment if you decide to keep default=true?
🧹 Nitpick comments (11)
src/ui.rs (1)
3-9: Add explicit Context bounds to encode invariants.
Batcher<_, Self::Context>and downstream wrappers expect thread-safety. Recommend bounding Context at the trait to avoid scattered bounds on impls.-pub trait UI { - type Context; +pub trait UI { + type Context: Sync + Send + 'static;tests/dummyui.rs (1)
16-17: Prefer non‑owning PhantomData to avoid misleading drop/auto‑trait semantics.
PhantomData<T>models ownership ofT. IfDummyUIdoes not logically ownT, use a non‑owning marker to keep covariance and avoid unintended Send/Drop implications.- _marker: PhantomData<T>, + _marker: PhantomData<fn() -> T>,src/source.rs (2)
3-3: Make'staticexplicit on the trait object to match the new API stance.Given call sites now enforce
'staticon inputs/closures, make the alias explicit for clarity.-pub type Source<T> = Pin<Box<dyn tokio_stream::Stream<Item = T> + Send>>; +pub type Source<T> = Pin<Box<dyn tokio_stream::Stream<Item = T> + Send + 'static>>;
14-21: UseFnMutto matchStreamExt::map’s bound.
maprequiresFnMut. RequiringFnis stricter than needed and blocks common mutable‑closure use cases.-pub fn transform_source<Cushion, SourceContext, F>( +pub fn transform_source<Cushion, SourceContext, F>( source: Source<SourceContext>, f: F, ) -> Source<Cushion> where - SourceContext: 'static, - F: Fn(SourceContext) -> Cushion + Send + 'static, + SourceContext: 'static, + F: FnMut(SourceContext) -> Cushion + Send + 'static, { use tokio_stream::StreamExt as _; Box::pin(source.map(f)) }src/sorter.rs (1)
36-46: Align bounds between struct and impl.Struct requires
SorterContext: Sync, impl requiresSorterContext: Sync + Send. Align these to reduce surprise; also consider non‑owning PhantomData.pub struct SorterWrapper<SorterContext, SorterT, F, Cushion> where - F: Fn(&Cushion) -> SorterContext + Send, - SorterT: Sorter<Context = SorterContext>, - SorterContext: Sync, + F: Fn(&Cushion) -> SorterContext + Send, + SorterT: Sorter<Context = SorterContext>, + SorterContext: Sync + Send, { @@ - _marker: PhantomData<(SorterContext, Cushion)>, + _marker: PhantomData<(fn() -> SorterContext, Cushion)>,src/filter.rs (3)
5-7: Encode Context invariants on the trait.Most wrappers require
Context: Sync + Send. Add it here to centralize constraints.-pub trait Filter: Send { - type Context; +pub trait Filter: Send { + type Context: Sync + Send + 'static;
36-45: Align struct bounds with impl and prefer non‑owning PhantomData.Struct has
FilterContext: Syncwhile the impl needsSync + Send. Align and switch PhantomData form.pub struct FilterWrapper<FilterContext, FilterT, F, Cushion> where - F: Fn(&Cushion) -> FilterContext + Send, - FilterT: Filter<Context = FilterContext>, + F: Fn(&Cushion) -> FilterContext + Send, + FilterT: Filter<Context = FilterContext>, @@ - _marker: PhantomData<(FilterContext, Cushion)>, + _marker: PhantomData<(fn() -> FilterContext, Cushion)>,
62-69: Match struct bounds with impl usage.Make the constructor’s bounds consistent with the impl (
FilterContext: Sync + Send).impl<FilterContext, FilterT, F, Cushion> FilterWrapper<FilterContext, FilterT, F, Cushion> where - F: Fn(&Cushion) -> FilterContext + Send, - FilterT: Filter<Context = FilterContext>, - FilterContext: Sync, - Cushion: Send, + F: Fn(&Cushion) -> FilterContext + Send, + FilterT: Filter<Context = FilterContext>, + FilterContext: Sync + Send, + Cushion: Send, {src/launcher/batcher.rs (1)
379-403: Minor: avoid alias/generic name shadowing in add_raw_ for readability.*FilterT/SorterT are both a type alias and a generic parameter name here. Rename the generics to Flt/Srt/Gn to reduce confusion. No behavior change.
- pub(super) fn add_raw_filter<FilterT>(&mut self, filter: FilterT) + pub(super) fn add_raw_filter<Flt>(&mut self, filter: Flt) where - FilterT: Filter<Context = Cushion> + 'static, + Flt: Filter<Context = Cushion> + 'static, { ... } - pub(super) fn add_raw_sorter<SorterT>(&mut self, sorter: SorterT) + pub(super) fn add_raw_sorter<Srt>(&mut self, sorter: Srt) where - SorterT: Sorter<Context = Cushion> + 'static, + Srt: Sorter<Context = Cushion> + 'static, { ... } - pub(super) fn add_raw_generator<GenT>(&mut self, generator: GenT) + pub(super) fn add_raw_generator<Gn>(&mut self, generator: Gn) where - GenT: Generator<Item = Cushion> + Sync + Send + 'static, + Gn: Generator<Item = Cushion> + Sync + Send + 'static, { ... }src/action.rs (1)
37-47: Use PhantomData<fn(&Cushion)> to reflect reference-only dependency.Current PhantomData models ownership; fn(&Cushion) models a borrow and improves variance semantics. Same idea applies to ClosureAction’s PhantomData.
- _marker: PhantomData<Cushion>, + _marker: PhantomData<fn(&Cushion)>,Optionally:
-pub struct ClosureAction<Context, F>(F, PhantomData<Context>) +pub struct ClosureAction<Context, F>(F, PhantomData<fn(&Context)>)src/launcher.rs (1)
126-133: Align set_ui bound with stored type.set_ui requires Fn + Send + Sync + 'static, but Batcher’s CushionToUIF previously stored only +Send. After applying the Batcher alias update (+Sync+'static), these are consistent. If you choose not to change Batcher, drop +Sync here.
No code diff here if you applied the Batcher alias change. Otherwise:
- F: Fn(&Cushion) -> UIContext + Send + Sync + 'static, + F: Fn(&Cushion) -> UIContext + Send + 'static,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/action.rs(3 hunks)src/filter.rs(3 hunks)src/launcher.rs(3 hunks)src/launcher/batcher.rs(15 hunks)src/sorter.rs(3 hunks)src/source.rs(1 hunks)src/ui.rs(1 hunks)tests/dummyui.rs(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
src/ui.rs (2)
src/launcher.rs (1)
run(152-166)tests/dummyui.rs (1)
run(39-65)
tests/dummyui.rs (2)
src/launcher.rs (1)
run(152-166)src/ui.rs (1)
run(6-9)
src/sorter.rs (4)
src/action.rs (2)
new(20-22)new(69-76)src/filter.rs (2)
new(19-21)new(69-76)tests/dummyui.rs (1)
new(24-29)src/generator.rs (2)
new(24-26)new(79-85)
src/action.rs (4)
src/filter.rs (2)
new(19-21)new(69-76)src/sorter.rs (2)
new(19-21)new(70-76)tests/dummyui.rs (1)
new(24-29)src/generator.rs (2)
new(24-26)new(79-85)
src/filter.rs (4)
src/action.rs (2)
new(20-22)new(69-76)src/sorter.rs (2)
new(19-21)new(70-76)tests/dummyui.rs (1)
new(24-29)src/generator.rs (2)
new(24-26)new(79-85)
src/launcher/batcher.rs (1)
src/launcher.rs (5)
add_raw_source(66-69)add_raw_filter(80-86)add_raw_sorter(97-103)add_raw_generator(144-150)default(31-37)
src/launcher.rs (4)
src/launcher/batcher.rs (4)
add_raw_source(380-382)add_raw_filter(384-389)add_raw_sorter(391-396)add_raw_generator(398-403)src/source.rs (1)
transform_source(14-25)tests/dummyui.rs (2)
new(24-29)run(39-65)src/ui.rs (1)
run(6-9)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: bench (ubuntu-latest)
- GitHub Check: review (ubuntu-latest)
🔇 Additional comments (5)
src/ui.rs (1)
6-9: No issues found—UI trait is used only generically.The verification confirms the original review comment's concern is accurate but not problematic. The
UItrait uses RPITIT, which does make it non-object-safe, but ripgrep found zero instances ofdyn UIorBox<dyn UI>in the codebase. Instead,UIis used exclusively as a generic type parameter (e.g.,Launcher<Cushion, UIT, UIContext> where UIT: UI<Context = UIContext>), which is fully compatible with RPITIT. The code is correct as-is.tests/dummyui.rs (1)
32-43: LGTM on lifetime removal andcompute_cushionfix verified.Verification confirms:
- Misspelling
compute_cusionsuccessfully removed from codebase- Correct spelling
compute_cushionin use (tests/dummyui.rs:61, src/launcher/batcher.rs:126)- Bounds
T: Sync + Send, F: Fn(&T) + Synccorrectly ensure asyncrunreturnsimpl Future + Sendsatisfying trait requirementssrc/source.rs (1)
5-12: The review comment is incorrect—no breaking changes identified.The verification found that all current call sites to
from_iterpass iterators that are already'static(ranges like0..COUNT, ownedVec, etc.). The'staticbound is semantically necessary for the return typeSource<T>when it's aBox<Pin<...>>without an explicit lifetime parameter—by default, trait objects in boxes implicitly require'staticbounds unless the type carries a lifetime. Since there are no call sites passing non-'staticiterators, removing the bound would only be beneficial if the API were redesigned to carry a lifetime (e.g.,Source<'a, T>), which is not the case here. The stated concern about breaking existing code does not apply to the current codebase.Likely an incorrect or invalid review comment.
src/action.rs (1)
4-8: Lifetimes removal looks solid; bounds are sensible.Action is Send; ClosureAction enforces F: Send and Context: Sync; impl requires Context: Sync+Send. Good.
If desired, I can add From for ClosureAction to ease construction.
Also applies to: 10-23, 25-35
src/launcher.rs (1)
58-69: API surface is coherent post-lifetime removal.add_* wrappers and raw variants have consistent 'static bounds and match the new non-lifetime traits. Good cleanup.
Consider adding small examples in docs showing typical transformer closures now that lifetimes are gone.
Also applies to: 80-87, 88-96, 97-103, 105-115, 117-124, 135-143, 144-151
Summary by CodeRabbit
Refactor
Bug Fixes