Pedantic linting #1679

van-sprundel · 2024-10-12T18:07:03Z

nts-pool-ke
ntp-proto
ntpd

Changes for #1658.

There are some opinionated changes here, so feel free to call out anything that's not wanted.
All changes are first ran with cargo clippy --fix -p [project] --all-targets --all-features

I've left out clippy::module_name_repetitions as it doesn't add much here.

Some notable lints from the pedantic group:

Functions that can error/panic now have error docs when relevant.
Big functions (>100 lines) are split into different functions.
- Except test modules, which are marked with #[allow]
ntpd::ctl::NtpCtlOptions now uses the action state instead of bools.
Some match cases became if () { } else { } where relevant.
Some let foo = if ... else became let ... else where relevant.
async and Result<_> are removed on places where it wasn't used and relevant.
const and enum mid-function are moved up at the beginning of the function, or moved up a scope.
ntpd::daemon:;ntp_source::SourceTask::run has been split up into multiple functions.
Conversions that could truncate or lose sign are marked as allowed where relevant.
- For arm and darwin, all truncation is allowed.

This became a pretty big PR in the end. I'm not sure how we could keep the impact minimal. Splitting it into multiple branches based on crates doesn't help much, because the ntp-proto crate has ~1000 lines of changes.

codecov · 2024-10-12T18:08:33Z

Codecov Report

Attention: Patch coverage is 79.67033% with 185 lines in your changes missing coverage. Please review.

Project coverage is 81.60%. Comparing base (419832f) to head (240d6f8).

Files with missing lines	Patch %	Lines
ntpd/src/daemon/ntp_source.rs	43.58%	22 Missing ⚠️
ntp-proto/src/nts_record.rs	92.27%	18 Missing ⚠️
ntpd/src/daemon/system.rs	0.00%	17 Missing ⚠️
ntpd/src/force_sync/mod.rs	0.00%	17 Missing ⚠️
ntp-proto/src/packet/mod.rs	88.70%	14 Missing ⚠️
ntp-proto/src/nts_pool_ke.rs	0.00%	9 Missing ⚠️
ntpd/src/daemon/config/mod.rs	73.52%	9 Missing ⚠️
ntpd/src/force_sync/algorithm.rs	0.00%	9 Missing ⚠️
ntpd/src/metrics/exporter.rs	46.15%	7 Missing ⚠️
nts-pool-ke/src/condcompile/cli.rs	0.00%	6 Missing ⚠️
... and 22 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1679      +/-   ##
==========================================
+ Coverage   81.55%   81.60%   +0.05%     
==========================================
  Files          66       66              
  Lines       20690    20790     +100     
==========================================
+ Hits        16874    16966      +92     
- Misses       3816     3824       +8

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

davidv1992

Thank you for picking this up. The changes make very clear that this was a good idea, even though it will still be some work to fully get these changes to land. Below I have made a number of comments. Note that most of these are notes to ourselves that we need to improve code in some places more and those need not be blockers for landing this in my eyes. I have marked those that do require more attention.

Please also note that given the wide scope of these changes, I am not comfortable merging these only on my opinion, so once this is further along there is going to be a review by a second person.

Again, thank you for doing the legwork on this. By the looks of it, you saved us quite some work. I hope you can bear with us while we make the push to get this in reviewable enough to merge it.

davidv1992 · 2024-10-16T07:37:46Z

ntp-proto/src/identifiers.rs

+    /// # Panics
+    ///
+    /// Will panic if `addr` is not a valid ip address.
+    #[must_use]


Note to self: Hmm, this function will actually never panic, I think the comment is required because of a false positive on the unwrap (which will always succeed since we take 4 bytes in the indexing operation before it).

davidv1992 · 2024-10-16T07:39:46Z

ntp-proto/src/ipfilter.rs

+    #[allow(clippy::cast_possible_truncation)]
+    #[allow(clippy::cast_sign_loss)]


Note to self: We should probably try to move these to more specific statements, given how complex the function actually is.

davidv1992 · 2024-10-16T07:40:40Z

ntp-proto/src/ipfilter.rs

    pub fn is_in(&self, addr: &IpAddr) -> bool {
        match addr {
-            IpAddr::V4(addr) => self.is_in4(addr),
-            IpAddr::V6(addr) => self.is_in6(addr),
+            IpAddr::V4(addr) => self.is_in4(*addr),
+            IpAddr::V6(addr) => self.is_in6(*addr),


Note to self: consider moving the deref up even further.

davidv1992 · 2024-10-16T07:41:22Z

ntp-proto/src/ipfilter.rs

+    /// # Panics
+    ///
+    /// Panics if `addr` has invalid octets.
+    fn is_in4(&self, addr: Ipv4Addr) -> bool {


Note to self: can this panic occur?

davidv1992 · 2024-10-16T07:43:22Z

ntp-proto/src/keyset.rs

@@ -78,12 +81,12 @@ impl KeySetProvider {
    }

    /// Rotate a new key in as primary, forgetting an old one if needed
+    #[allow(clippy::cast_possible_truncation)]


Note to self: consider moving this into the function to avoid allowing this blanketly on the whole.

ntp-proto/src/nts_record.rs

davidv1992 · 2024-10-16T08:06:01Z

ntp-proto/src/packet/extension_fields.rs

@@ -1,3 +1,4 @@
+#![allow(clippy::cast_possible_truncation)]


Note to self: Eliminate here, too general

ntp-proto/src/packet/mod.rs

ntpd/src/ctl.rs

ntpd/src/daemon/ntp_source.rs

davidv1992 · 2024-10-16T08:38:26Z

@rnijveld As a headsup, just so you are aware of this PR, I am going to ask you to review this once it is a bit further along. Most of it is relatively straightforward, and hopefully we can split the parts that aren't into separate PRs. It does highlight a few places to where we should probably take a second look at code, particularly around casts.

van-sprundel · 2024-11-20T15:01:29Z

Sorry for the radio silence, I'll rebase the PR and take a look at the review over the weekend!

Am I safe to assume that all the reviews without "note to self" are something I can refactor?

davidv1992 · 2024-11-28T07:25:27Z

Yes, those are things that need attention before this can move forward.

Add semicolon on non-returning functions Remove redundant else block We already break in the check Put variable in format macro Move consts up in function ignore lint for incomplete function Add error to function docs

Replaced bools with action

One was for string conversion that can be ignored, but there's a lot of casts that might introduce truncation and sign loss, which is now explicitly allowed.

We might eventually need this

This can be rewritten, but this is not important for this issue.

I'm sure there's a prettier way to cast properly.

Breaking for msrv integration

van-sprundel · 2025-01-09T16:09:56Z

While refactoring I did found that test_deny_stops_poll will not always succeed (even on main). Seems like a timing issue. Could it be that the cast to u32 is causing this?

ntpd-rs/ntpd/src/daemon/ntp_source.rs

Lines 541 to 548 in 419832f

    
               let cur = 
        
                   std::time::SystemTime::now().duration_since(std::time::SystemTime::UNIX_EPOCH)?; 
        
               Ok(NtpTimestamp::from_seconds_nanos_since_ntp_era( 
        
                   EPOCH_OFFSET.wrapping_add(cur.as_secs() as u32), 
        
                   cur.subsec_nanos(), 
        
               )) 
        
           }

or something in the test startup that's failing it? my initial changes made it always fail, but now it sometimes fails on main

cargo test test_deny_stops_poll --target x86_64-unknown-linux-gnu --features run_tokio_rustls_tests

van-sprundel force-pushed the pedantic-lint branch from b117a79 to 36e52e4 Compare October 12, 2024 20:48

van-sprundel marked this pull request as ready for review October 15, 2024 20:48

davidv1992 self-assigned this Oct 16, 2024

davidv1992 requested changes Oct 16, 2024

View reviewed changes

van-sprundel added 22 commits January 8, 2025 13:06

Add lint config and fixes nts-pool-ke

d0327f9

Add semicolon on non-returning functions Remove redundant else block We already break in the check Put variable in format macro Move consts up in function ignore lint for incomplete function Add error to function docs

Add lint config in ntpd

0733454

Run clippy fix in ntpd

13eef4e

Fix struct_excessive_bools in ntpd

bbb7b6d

Replaced bools with action

Use type name instead of Default

1105cd6

cargo fmt

ad7078e

Handle conversion of floats

4932e06

One was for string conversion that can be ignored, but there's a lot of casts that might introduce truncation and sign loss, which is now explicitly allowed.

Move struct & enum that are made mid-function

a20cb36

Remove unused Result<_> in function

6fba450

Mark unfinished function with unused_self

3c0efba

We might eventually need this

Use reference and mark funtion as too_many_lines

e7a3906

This can be rewritten, but this is not important for this issue.

Use let else instead of if else

025e03e

Remove async when not used, split run into functions

47b8f7d

assert float check should have a limit

46ea772

Revert clippy cast_lossless

681fbe8

I'm sure there's a prettier way to cast properly.

Allow truncation for darwin target

5e56bef

run cargo fix for ntp-proto

3160bad

Mark truncation, wrap, float_cmp and sign loss with #[allow]

ab7ed1c

Update for loop

2f94665

Update references usages

6df86fe

Use specific defaults

7b6b874

Move function result out of Result<> when redundant

54861e4

van-sprundel added 17 commits January 8, 2025 13:06

Mark debug impl with finish_non_exhaustive for hidden fields

e94b01a

Add should_panic messages

77115e5

Add error and panic docs

c95e170

Update references usage

17810db

Update formatting

cf4a964

Update function to error instead of panic

f91ec40

Remove out of else when redundant

123e854

Use find_map(..) instead of filter_map(..).next()

f1e1d2c

Split big functions

a9ff7ba

Use let .. else instead of let if .. else

8038217

Put v5 imports behind feature flag

f065a6d

Fix formatting

4878655

allow explicit iter loop

bbbb15c

Breaking for msrv integration

Put nts-pool imports behind feature flag

710e3f2

Fix panic message test

ff49a8b

Remove flag behind mutable variable

978b528

Rebase with main

8c44e3f

van-sprundel force-pushed the pedantic-lint branch from 7f66c7b to 8c44e3f Compare January 9, 2025 14:03

van-sprundel added 5 commits January 9, 2025 15:21

Cleanup

29db988

Fix build for rustls vers

1efcbb9

Undo state behind feature

e75d77e

Undo changes for ntp_source

bf8825f

Remove as_bytes for len check

e3cdb00

Bind lifetime to static instead of inner lifetime

240d6f8

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pedantic linting #1679

Pedantic linting #1679

van-sprundel commented Oct 12, 2024 •

edited

Loading

codecov bot commented Oct 12, 2024 •

edited

Loading

davidv1992 left a comment

davidv1992 Oct 16, 2024 •

edited

Loading

davidv1992 Oct 16, 2024 •

edited

Loading

davidv1992 Oct 16, 2024

davidv1992 Oct 16, 2024

davidv1992 Oct 16, 2024

davidv1992 Oct 16, 2024

davidv1992 commented Oct 16, 2024

van-sprundel commented Nov 20, 2024 •

edited

Loading

davidv1992 commented Nov 28, 2024

van-sprundel commented Jan 9, 2025

		#[allow(clippy::cast_possible_truncation)]
		#[allow(clippy::cast_sign_loss)]

Pedantic linting #1679

Are you sure you want to change the base?

Pedantic linting #1679

Conversation

van-sprundel commented Oct 12, 2024 • edited Loading

codecov bot commented Oct 12, 2024 • edited Loading

Codecov Report

davidv1992 left a comment

Choose a reason for hiding this comment

davidv1992 Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

davidv1992 Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

davidv1992 Oct 16, 2024

Choose a reason for hiding this comment

davidv1992 Oct 16, 2024

Choose a reason for hiding this comment

davidv1992 Oct 16, 2024

Choose a reason for hiding this comment

davidv1992 Oct 16, 2024

Choose a reason for hiding this comment

davidv1992 commented Oct 16, 2024

van-sprundel commented Nov 20, 2024 • edited Loading

davidv1992 commented Nov 28, 2024

van-sprundel commented Jan 9, 2025

van-sprundel commented Oct 12, 2024 •

edited

Loading

codecov bot commented Oct 12, 2024 •

edited

Loading

davidv1992 Oct 16, 2024 •

edited

Loading

davidv1992 Oct 16, 2024 •

edited

Loading

van-sprundel commented Nov 20, 2024 •

edited

Loading