Refactor/clippy warnings #18

theotchlx · 2025-11-27T23:06:21Z

This PR addresses cargo clippy --all-features warnings.

theotchlx · 2025-12-06T17:54:08Z

One warning given by clippy is about type complexity.
It is interesting to abstract complex types with typedefs, to lower the cognitive load where needed.

Example (src/pathfinding/hybrid_parenting.rs:66):

struct HybridParentingWorkArea<NM: NodeManager, CM: ContactManager> {
    /// The bundle associated with this work area.
    pub bundle: Bundle,
    /// The source route stage, representing the starting point for routing.
    pub source: Rc<RefCell<RouteStage<NM, CM>>>,
    /// A sorted list of node IDs to be excluded from routing paths.
    pub excluded_nodes_sorted: Vec<NodeID>,
    /// A vector containing vectors of route stages, grouped by destination.
    /// Each inner vector represents possible routes to a specific destination,
    /// sorted in order of preference.
    pub by_destination: Vec<Vec<Rc<RefCell<RouteStage<NM, CM>>>>>,
}

We can replace with:

struct HybridParentingWorkArea<NM: NodeManager, CM: ContactManager> {
   ...
    pub by_destination: Vec<Vec<SharedRouteStage<NM, CM>>>,
}

pub type SharedRouteStage<NM: NodeManager, CM: ContactManager> = Rc<RefCell<RouteStage<NM, CM>>>;

However, we are warned about a current limitation of the type checker:

warning: bounds on generic parameters in type aliases are not enforced
  --> src/pathfinding/hybrid_parenting.rs:79:31
   |
79 | pub type SharedRouteStage<NM: NodeManager, CM: ContactManager> = Rc<RefCell<RouteStage<NM, CM>>>;
   |                               ^^^^^^^^^^^      ^^^^^^^^^^^^^^ will not be checked at usage sites of the type alias
   |
   = note: this is a known limitation of the type checker that may be lifted in a future edition.
           see issue #112792 <https://github.com/rust-lang/rust/issues/112792> for more information
   = note: `#[warn(type_alias_bounds)]` on by default
help: remove these bounds
   |
79 - pub type SharedRouteStage<NM: NodeManager, CM: ContactManager> = Rc<RefCell<RouteStage<NM, CM>>>;
79 + pub type SharedRouteStage<NM, CM> = Rc<RefCell<RouteStage<NM, CM>>>;

So, I won't simplify the types if it means that the type checker won't work (plus it generates this other warning).

theotchlx · 2025-12-06T18:58:51Z

There's also tons of macros that reference using crate:: ... instead of $crate:: ....

E.g.:

warning: `crate` references the macro call's crate
   --> src/pathfinding/limiting_contact/mod.rs:100:17  
    |                                                                                                                                                                         
100 |             NM: crate::node_manager::NodeManager,
    |                 ^^^^^ help: to reference the macro definition's crate, use: `$crate`                                 
    |                                      
    = help: for further information visit https://rust-lang.github.io/rust-clippy/rust-1.91.0/index.html#crate_in_macro_def

i.e. "crate refers to the macro call’s crate, whereas $crate refers to the macro definition’s crate. Rarely is the former intended. See: https://doc.rust-lang.org/reference/macros-by-example.html#hygiene"

I'm not sure if the A-SABR macros actually make use of this, so I won't touch it as it could modify the code behaviour.

Implement Default for types with a new() method.

theotchlx · 2025-12-06T20:08:33Z

This only fixes lints that afaik don't have any side effects on the business logic. Ready for review.

olivier-dj · 2025-12-06T20:16:27Z

One warning given by clippy is about type complexity. It is interesting to abstract complex types with typedefs, to lower the cognitive load where needed.

Example (src/pathfinding/hybrid_parenting.rs:66):

struct HybridParentingWorkArea<NM: NodeManager, CM: ContactManager> {
    /// The bundle associated with this work area.
    pub bundle: Bundle,
    /// The source route stage, representing the starting point for routing.
    pub source: Rc<RefCell<RouteStage<NM, CM>>>,
    /// A sorted list of node IDs to be excluded from routing paths.
    pub excluded_nodes_sorted: Vec<NodeID>,
    /// A vector containing vectors of route stages, grouped by destination.
    /// Each inner vector represents possible routes to a specific destination,
    /// sorted in order of preference.
    pub by_destination: Vec<Vec<Rc<RefCell<RouteStage<NM, CM>>>>>,
}

We can replace with:

struct HybridParentingWorkArea<NM: NodeManager, CM: ContactManager> {
   ...
    pub by_destination: Vec<Vec<SharedRouteStage<NM, CM>>>,
}

pub type SharedRouteStage<NM: NodeManager, CM: ContactManager> = Rc<RefCell<RouteStage<NM, CM>>>;

However, we are warned about a current limitation of the type checker:

warning: bounds on generic parameters in type aliases are not enforced
  --> src/pathfinding/hybrid_parenting.rs:79:31
   |
79 | pub type SharedRouteStage<NM: NodeManager, CM: ContactManager> = Rc<RefCell<RouteStage<NM, CM>>>;
   |                               ^^^^^^^^^^^      ^^^^^^^^^^^^^^ will not be checked at usage sites of the type alias
   |
   = note: this is a known limitation of the type checker that may be lifted in a future edition.
           see issue #112792 <https://github.com/rust-lang/rust/issues/112792> for more information
   = note: `#[warn(type_alias_bounds)]` on by default
help: remove these bounds
   |
79 - pub type SharedRouteStage<NM: NodeManager, CM: ContactManager> = Rc<RefCell<RouteStage<NM, CM>>>;
79 + pub type SharedRouteStage<NM, CM> = Rc<RefCell<RouteStage<NM, CM>>>;

So, I won't simplify the types if it means that the type checker won't work (plus it generates this other warning).

One warning given by clippy is about type complexity. It is interesting to abstract complex types with typedefs, to lower the cognitive load where needed.

Example (src/pathfinding/hybrid_parenting.rs:66):

struct HybridParentingWorkArea<NM: NodeManager, CM: ContactManager> {
    /// The bundle associated with this work area.
    pub bundle: Bundle,
    /// The source route stage, representing the starting point for routing.
    pub source: Rc<RefCell<RouteStage<NM, CM>>>,
    /// A sorted list of node IDs to be excluded from routing paths.
    pub excluded_nodes_sorted: Vec<NodeID>,
    /// A vector containing vectors of route stages, grouped by destination.
    /// Each inner vector represents possible routes to a specific destination,
    /// sorted in order of preference.
    pub by_destination: Vec<Vec<Rc<RefCell<RouteStage<NM, CM>>>>>,
}

We can replace with:

struct HybridParentingWorkArea<NM: NodeManager, CM: ContactManager> {
   ...
    pub by_destination: Vec<Vec<SharedRouteStage<NM, CM>>>,
}

pub type SharedRouteStage<NM: NodeManager, CM: ContactManager> = Rc<RefCell<RouteStage<NM, CM>>>;

However, we are warned about a current limitation of the type checker:

warning: bounds on generic parameters in type aliases are not enforced
  --> src/pathfinding/hybrid_parenting.rs:79:31
   |
79 | pub type SharedRouteStage<NM: NodeManager, CM: ContactManager> = Rc<RefCell<RouteStage<NM, CM>>>;
   |                               ^^^^^^^^^^^      ^^^^^^^^^^^^^^ will not be checked at usage sites of the type alias
   |
   = note: this is a known limitation of the type checker that may be lifted in a future edition.
           see issue #112792 <https://github.com/rust-lang/rust/issues/112792> for more information
   = note: `#[warn(type_alias_bounds)]` on by default
help: remove these bounds
   |
79 - pub type SharedRouteStage<NM: NodeManager, CM: ContactManager> = Rc<RefCell<RouteStage<NM, CM>>>;
79 + pub type SharedRouteStage<NM, CM> = Rc<RefCell<RouteStage<NM, CM>>>;

So, I won't simplify the types if it means that the type checker won't work (plus it generates this other warning).

Yes because I don't think the type annotation is required here (you can see the original code doesn't have the T: trait annotation for the rd refcell declaration, so just:

pub type SharedRouteStage<NM, CM> = Rc<RefCell<RouteStage<NM, CM>>>;

seems valid to me, and any type (e.g. a struct) that uses this alias will enforce the trait requirement, e.g. :

struct HybridParentingWorkArea<NM: NodeManager, CM: ContactManager> {
   ...
    pub by_destination: Vec<Vec<SharedRouteStage<NM, CM>>>,  // CM: ContactManager enforced by struct def
}

olivier-dj · 2025-12-06T20:20:50Z

src/parsing.rs

    /// A hashmap that stores the coercion functions with their associated markers.
    map: HashMap<&'a str, T>,
 }
+impl<'a, T> Default for Dispatcher<'a, T> {


used somewhere?

Clippy recommends implementing Default for types that have a new() method, because "The user might expect to be able to use Default as the type can be constructed without arguments."

Since RoutingTable is part of the public API of a-sabr, it would be used by users of a-sabr to fill all or some of the struct fields automatically with Default::default() for convenience.

Reference: https://rust-lang.github.io/rust-clippy/rust-1.91.0/index.html#new_without_default

However if this is not particularly convenient or important at the moment, just tell me to and I'll remove the commit.

olivier-dj · 2025-12-06T20:21:23Z

src/route_storage/table.rs

    _phantom_distance: PhantomData<D>,
 }

+impl<NM: NodeManager, CM: ContactManager, D: Distance<NM, CM>> Default for RoutingTable<NM, CM, D> {


theotchlx · 2025-12-07T10:55:24Z

Thanks for the review, I'll add type aliases

theotchlx · 2025-12-24T19:57:51Z

Here's a question I have:

src/parsing.rs defines the following types:

pub type ContactMarkerMap<'a> = Dispatcher<'a, ContactDispatcher>;
pub type NodeMarkerMap<'a> = Dispatcher<'a, NodeDispatcher>;
pub type ContactDispatcher = fn(&mut dyn Lexer) -> ParsingState<Box<dyn ContactManager>>;
pub type NodeDispatcher = fn(&mut dyn Lexer) -> ParsingState<Box<dyn NodeManager>>;

[Node,Contact]MarkerMap are used throughout the code.
However, there is throughout the code another pattern, that is static rather than dynamic:

E.g.: (src/utils/mod.rs)

...
    node_marker_map: Option<&Dispatcher<'_, fn(&mut dyn Lexer) -> ParsingState<NM>>>,
...

These appear in multiple places in the code, and sometime give out "type complexity" warnings from Clippy.

To address this, I was thinking of defining the following public types in src/parsing.rs:

pub type StaticNodeMarkerMap<'a, NM> = Dispatcher<'a, StaticNodeDispatcher<NM>>;
pub type StaticNodeDispatcher<NM> = fn(&mut dyn Lexer) -> ParsingState<NM>;

And similar for Contact[*]. Do you think that's a good idea? Is the naming OK?

theotchlx · 2025-12-25T02:32:34Z

#18 (comment)

Since the code is the same for both, I could do a generic:

pub type StaticMarkerMap<'a, M> = Dispatcher<'a, StaticDispatcher<M>>;
pub type StaticDispatcher<M> = fn(&mut dyn Lexer) -> ParsingState<M>;

M stands for manager (contact or node)

E.g., in src/contact_plan/from_asabr_lexer.rs:

    pub fn parse<
        NM: NodeManager + DispatchParser<NM> + Parser<NM>,
        CM: ContactManager + DispatchParser<CM> + Parser<CM>,
    >(
        lexer: &mut dyn Lexer,
        node_marker_map: Option<&Dispatcher<fn(&mut dyn Lexer) -> ParsingState<NM>>>,
        contact_marker_map: Option<&Dispatcher<fn(&mut dyn Lexer) -> ParsingState<CM>>>,
    ) -> Result<(Vec<Node<NM>>, Vec<Contact<NM, CM>>), String> {
        ...
    }

Becomes:

    pub fn parse<
        NM: NodeManager + DispatchParser<NM> + Parser<NM>,
        CM: ContactManager + DispatchParser<CM> + Parser<CM>,
    >(
        lexer: &mut dyn Lexer,
        node_marker_map: Option<&StaticMarkerMap<NM>>,
        contact_marker_map: Option<&StaticMarkerMap<CM>>,
    ) -> Result<(Vec<Node<NM>>, Vec<Contact<NM, CM>>), String> {
        ...
    }

theotchlx · 2025-12-26T02:02:25Z

I have added a few commits with some typedefs (public and private) and a doc comment correction.
Details and motives are in the commit messages.

Clippy warned about type complexity regarding Rc<RefCell<RouteStage>>>. This commit fixes some of these specific warnings. This commit introduces a new public type `SharedRouteStage`: `pub type SharedRouteStage<NM, CM> = Rc<RefCell<RouteStage<NM, CM>>>;` And replaces the code where it felt it improved readability / cognitive load.

Clippy warned about type complexity where the following, or similar, appears: `Option<&Dispatcher<fn(&mut dyn Lexer) -> ParsingState<T>>>,` Which raises the warnings. This commit replaces it with: `Option<&StaticMarkerMap<T>>,` This commit defines the following in src/parsing.rs, alongside similar definitions: `pub type StaticMarkerMap<'a, M> = Dispatcher<'a, StaticDispatcher<M>>;` `pub type StaticDispatcher<M> = fn(&mut dyn Lexer) -> ParsingState<M>;` The naming was chosen as similar type definitions exist, but with dyn trait instead of generics.

The types in the code did not match those described in that doc comment. This commit attemps to fix this doc comment.

Clippy warned about type complexity where the following, or similar, appears: `pub first_hops: HashMap<usize, (Rc<RefCell<Contact<NM, CM>>>, Vec<SharedRouteStage<NM, CM>>)>,` Which raises the warnings. This commit reduces it to: `pub first_hops: HashMap<usize, FirstHopsVec<NM, CM>>,` By introducing the following *private* types in `src/parsing.rs`, alongside similar definitions: ``` type FirstHopsVec<NM, CM> = ( Rc<RefCell<Contact<NM, CM>>>, Vec<Rc<RefCell<RouteStage<NM, CM>>>>, ); type FirstHop<NM, CM> = ( Rc<RefCell<Contact<NM, CM>>>, Rc<RefCell<RouteStage<NM, CM>>>, ); ``` The naming was chosen from the naming of attributes that used those types. To not mistake the plural form and to reflect the inner type, its naming ends with 'Vec'. The two new types are private because I did not find a use for them outside the routing module.

Clippy warns about the complexity of such types: `Result<(Vec<Node<NM>>, Vec<Contact<NM, CM>>), String>` This commit reduces it to: `Result<ContactPlan<NM, NM, CM>, String>` By defining a new `ContactPlan` type in `contact_plan/mod.rs`: `type ContactPlan<NNM, CNM, CCM> = (Vec<Node<NNM>>, Vec<Contact<CNM, CCM>>);` The name was inspired from `exercises/0-ion-tvgutil-parsing/README.md:104`

This commit introduces a new *public* type: `type SharedPathFindingOutput<NM, CM> = Rc<RefCell<PathFindingOutput<NM, CM>>>;` for convenience, and to lower cognitive load where clippy deems necessary. This commit only applies this type in a single place, one where clippy generates a warning.

theotchlx · 2025-12-26T20:03:12Z

I ran some benchmarks to make sure everything was OK.

cargo bench --bench spsn_benchmark -- --sample-size=400 --measurement-time=40 --warm-up-time=5 --save-baseline=main on main
followed by
cargo bench --bench spsn_benchmark -- --sample-size=400 --measurement-time=40 --warm-up-time=5 --baseline=main --verbose on the HEAD of this branch.

I ran this last one three times, and the results seem to suggest that performance was not or minimally impacted?
It's unclear as the result do sometime alternate between "No change in performance detected / within noise threshold", "regression" or "improved" for the same test. Maybe some parameter should be adjusted, or I should use Criterion differently ? (I tried doing it on a 'quiet' computer)

E.g. with VolCgrNodeParenting:

Benchmark 2:

Benchmarking Routers/VolCgrNodeParenting
Benchmarking Routers/VolCgrNodeParenting: Warming up for 5.0000 s
Benchmarking Routers/VolCgrNodeParenting: Collecting 400 samples in estimated 54.676 s (1200 iterations)
Benchmarking Routers/VolCgrNodeParenting: Analyzing
Routers/VolCgrNodeParenting
                        time:   [3.1406 ms 3.1672 ms 3.1942 ms]
                        change: [-11.056% -10.255% -9.3507%] (p = 0.00 < 0.05)
                        Performance has improved.
mean   [3.1406 ms 3.1942 ms] std. dev.      [258.14 µs 287.27 µs]
median [3.0062 ms 3.1941 ms] med. abs. dev. [185.67 µs 408.23 µs]

Benchmark 3:

Benchmarking Routers/VolCgrNodeParenting
Benchmarking Routers/VolCgrNodeParenting: Warming up for 5.0000 s
Benchmarking Routers/VolCgrNodeParenting: Collecting 400 samples in estimated 55.010 s (1200 iterations)
Benchmarking Routers/VolCgrNodeParenting: Analyzing
Routers/VolCgrNodeParenting
                        time:   [3.6594 ms 3.6690 ms 3.6793 ms]
                        change: [+3.3534% +3.9637% +4.5464%] (p = 0.00 < 0.05)
                        Performance has regressed.
Found 9 outliers among 400 measurements (2.25%)
  5 (1.25%) high mild
  4 (1.00%) high severe
mean   [3.6594 ms 3.6793 ms] std. dev.      [83.620 µs 119.79 µs]
median [3.6373 ms 3.6614 ms] med. abs. dev. [81.740 µs 99.625 µs]

I also ran the following on main and on this branch, and both ran fine with seemingly no difference between the two:

cargo run --example bundle_processing --features node_proc
cargo run --example contact_plans
cargo run --example dijkstra_accuracy --features contact_work_area
cargo run --example eto_management --all-features
cargo run --example satellite_constellation --features node_tx

theotchlx added 3 commits November 27, 2025 23:58

refactor: fix clippy uninlined_format_args

867cd83

refactor: fix clippy needless_return

8610e5b

refactor: fix clippy redundant_field_names

e76c292

refactor: fix clippy new_without_default

05e61eb

Implement Default for types with a new() method.

theotchlx marked this pull request as ready for review December 6, 2025 20:07

olivier-dj reviewed Dec 6, 2025

View reviewed changes

theotchlx force-pushed the refactor/clippy-warnings branch from face6e0 to 58e76ff Compare December 24, 2025 19:42

theotchlx force-pushed the refactor/clippy-warnings branch 2 times, most recently from d2ae65d to 1608560 Compare December 26, 2025 02:02

theotchlx added 7 commits December 26, 2025 17:00

docs(routing): fix incorrect doc comment

6da5023

The types in the code did not match those described in that doc comment. This commit attemps to fix this doc comment.

style: fix formatting & whitespace

1246f94

theotchlx force-pushed the refactor/clippy-warnings branch from a432a69 to 1246f94 Compare December 26, 2025 16:10

theotchlx mentioned this pull request Dec 29, 2025

refactor: use guard pattern on Option #19

Open

Refactor/clippy warnings #18

Are you sure you want to change the base?

Refactor/clippy warnings #18

Uh oh!

Conversation

theotchlx commented Nov 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theotchlx commented Dec 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theotchlx commented Dec 6, 2025

Uh oh!

theotchlx commented Dec 6, 2025

Uh oh!

olivier-dj commented Dec 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

olivier-dj Dec 6, 2025

Choose a reason for hiding this comment

Uh oh!

theotchlx Dec 7, 2025

Choose a reason for hiding this comment

Uh oh!

olivier-dj Dec 6, 2025

Choose a reason for hiding this comment

Uh oh!

theotchlx Dec 7, 2025

Choose a reason for hiding this comment

Uh oh!

theotchlx commented Dec 7, 2025

Uh oh!

theotchlx commented Dec 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theotchlx commented Dec 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theotchlx commented Dec 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theotchlx commented Dec 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

theotchlx commented Nov 27, 2025 •

edited

Loading

theotchlx commented Dec 6, 2025 •

edited

Loading

olivier-dj commented Dec 6, 2025 •

edited

Loading

theotchlx commented Dec 24, 2025 •

edited

Loading

theotchlx commented Dec 25, 2025 •

edited

Loading

theotchlx commented Dec 26, 2025 •

edited

Loading

theotchlx commented Dec 26, 2025 •

edited

Loading