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.

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

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

Codecov Report

Attention: Patch coverage is 80.55556% with 203 lines in your changes missing coverage. Please review.

Project coverage is 81.36%. Comparing base (eba70a7) to head (7f66c7b).

Files with missing lines	Patch %	Lines
ntpd/src/daemon/ntp_source.rs	63.97%	58 Missing ⚠️
ntp-proto/src/nts_record.rs	92.24%	18 Missing ⚠️
ntpd/src/daemon/system.rs	0.00%	17 Missing ⚠️
ntpd/src/force_sync/mod.rs	0.00%	16 Missing ⚠️
ntp-proto/src/packet/mod.rs	88.70%	14 Missing ⚠️
ntp-proto/src/keyset.rs	64.28%	10 Missing ⚠️
ntp-proto/src/nts_pool_ke.rs	0.00%	9 Missing ⚠️
ntpd/src/daemon/config/mod.rs	72.72%	9 Missing ⚠️
ntpd/src/force_sync/algorithm.rs	0.00%	9 Missing ⚠️
nts-pool-ke/src/cli.rs	0.00%	6 Missing ⚠️
... and 16 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1679      +/-   ##
==========================================
+ Coverage   81.33%   81.36%   +0.02%     
==========================================
  Files          64       64              
  Lines       19751    19880     +129     
==========================================
+ Hits        16065    16175     +110     
- Misses       3686     3705      +19

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

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.

Box is not an iter

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

Breaking for msrv integration

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.

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

ntp-proto/src/nts_record.rs

+    /// # Errors
+    ///
+    /// Returns error if getting the TLS bool `wants_write` fails.


Doesn't actually error

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

davidv1992 · 2024-10-16T08:09:04Z

ntp-proto/src/packet/mod.rs

-            3 => {
-                let (header, header_size) =
-                    NtpHeaderV3V4::deserialize(data).map_err(|e| e.generalize())?;
-                let mac = if header_size != data.len() {
-                    Some(Mac::deserialize(&data[header_size..]).map_err(|e| e.generalize())?)
-                } else {
-                    None
-                };
-                Ok((
-                    NtpPacket {


Good refactor on this match, but again needs more involved review. Could you please make this a separate PR?

davidv1992 · 2024-10-16T08:18:32Z

ntpd/src/ctl.rs

-
-    /// from the arguments resolve which action should be performed
-    fn resolve_action(&mut self) {
-        if self.help {
-            self.action = NtpCtlAction::Help;
-        } else if self.version {
-            self.action = NtpCtlAction::Version;
-        } else if self.validate {
-            self.action = NtpCtlAction::Validate;
-        } else if self.status {
-            self.action = NtpCtlAction::Status;
-        } else if self.force_sync {
-            self.action = NtpCtlAction::ForceSync;
-        } else {
-            self.action = NtpCtlAction::Help;
-        }
-    }


Needs fix: it is actually quite subtle, but removing this, in combination with the changes above, actually changes behaviour, and in an unwanted way. In particular, it changes behaviour of the -v and -h flags overriding all other actions, which is undesired. Please revert.

davidv1992 · 2024-10-16T08:22:20Z

ntpd/src/daemon/ntp_source.rs

-            let actions = match selected {
-                SelectResult::Recv(result) => {
-                    tracing::debug!("accept packet");
-                    match accept_packet(result, &buf, &self.clock) {
-                        AcceptResult::Accept(packet, recv_timestamp) => {
-                            let send_timestamp = match self.last_send_timestamp {
-                                Some(ts) => ts,
-                                None => {
-                                    debug!(
-                                        "we received a message without having sent one; discarding"


Needs fix: Again, this is likely (part of) a good refactor, but too hard to review well in this context, please split this off into a separate PR.

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?

van-sprundel added 4 commits October 12, 2024 14:31

Add lint config and fixes nts-pool-ke

b898125

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

3f2ae02

Run clippy fix in ntpd

2079282

Fix struct_excessive_bools in ntpd

289801b

Replaced bools with action

van-sprundel added 12 commits October 12, 2024 22:43

Use type name instead of Default

fba3f2b

Add error docs

29ad377

cargo fmt

33f2ad7

Handle conversion of floats

1f7fcf4

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

d56bc77

Remove unused Result<_> in function

9e056d4

Mark unfinished function with unused_self

8593ee7

We might eventually need this

Use reference and mark funtion as too_many_lines

c937f0c

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

Use let else instead of if else

2b5d475

Remove async when not used, split run into functions

90d940b

Revert clippy fix

def05b3

Box is not an iter

assert float check should have a limit

36e52e4

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

van-sprundel added 12 commits October 12, 2024 23:02

Revert clippy cast_lossless

a65658e

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

Allow truncation for darwin target

a4ad207

run cargo fix for ntp-proto

2c146c8

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

5cb97ec

Update for loop

df94865

Update references usages

5ec9a67

Use specific defaults

8320631

Move function result out of Result<> when redundant

f22bd79

Mark debug impl with finish_non_exhaustive for hidden fields

f0f4dde

Add should_panic messages

22ccc51

Add error and panic docs

7a50b46

Update references usage

9bfc9d4

van-sprundel added 13 commits October 13, 2024 12:40

Update formatting

157eb67

Update function to error instead of panic

24ca398

Remove out of else when redundant

55b3dbe

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

e758801

Split big functions

bb7351a

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

2f485bd

Put v5 imports behind feature flag

7ee6ba9

Fix formatting

ac5ef9c

allow explicit iter loop

55502c6

Breaking for msrv integration

Put nts-pool imports behind feature flag

938ee09

Fix panic message test

04d14d5

Revert explicit iter changes

ce1d1c1

Remove flag behind mutable variable

7f66c7b

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

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 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

		#[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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidv1992 commented Oct 16, 2024

van-sprundel commented Nov 20, 2024 • edited Loading

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