fix: feature flag Datagram::into_data

[mxinden]

Datagram::into_data is used in neqo_common::udp only. neqo_common::udp is behind the udp feature. The feature is not part of the crate's default features. Thus a plain cargo check rightly complains with:

warning: method `into_data` is never used
  --> neqo-common/src/datagram.rs:58:19
   |
20 | impl Datagram {
   | ------------- method in this implementation
...
58 |     pub(crate) fn into_data(self) -> Vec<u8> {
   |                   ^^^^^^^^^
   |
   = note: `#[warn(dead_code)]` on by default

This commit hides into_data behind the udp feature as well.

Introduced in #1604. My bad.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.97%. Comparing base (78919a5) to head (2c3de82).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1695   +/-   ##
=======================================
  Coverage   92.97%   92.97%           
=======================================
  Files         120      120           
  Lines       37342    37342           
=======================================
  Hits        34718    34718           
  Misses       2624     2624           

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

[larseggert]

I don't get this error when I run cargo check?

[mxinden]

@larseggert do you run cargo check in the root directory or neqo-common? Can you try the latter?

➜  neqo git:(main) cd neqo-common 
➜  neqo-common git:(main) cargo check
   Compiling syn v2.0.48
   Compiling neqo-common v0.7.1 (/home/mxinden/code/github.com/mozilla/neqo/neqo-common)
   Compiling darling_core v0.20.3
   Compiling serde_derive v1.0.195
   Compiling enum-map-derive v0.17.0
    Checking enum-map v2.7.3
   Compiling darling_macro v0.20.3
   Compiling darling v0.20.3
   Compiling serde_with_macros v3.4.0
    Checking serde v1.0.195
    Checking smallvec v1.11.2
    Checking serde_json v1.0.108
    Checking serde_with v3.4.0
    Checking qlog v0.12.0
warning: method `into_data` is never used
  --> neqo-common/src/datagram.rs:58:19
   |
20 | impl Datagram {
   | ------------- method in this implementation
...
58 |     pub(crate) fn into_data(self) -> Vec<u8> {
   |                   ^^^^^^^^^
   |
   = note: `#[warn(dead_code)]` on by default

warning: `neqo-common` (lib) generated 1 warning
    Finished dev [unoptimized + debuginfo] target(s) in 10.90s
➜  neqo-common git:(main) cargo --version
cargo 1.76.0 (c84b36747 2024-01-18)

[larseggert]

I ran it at the root.

If there are issues with running it in the subdirectories, we should add that to CI.

[mxinden]

A cargo check at the root will compile all crates of the workspace. That includes neqo-client and neqo-server, each activating the udp feature in neqo-common. With the udp feature the Datagram::into_data function is used. Thus a cargo check at the root does not trigger the warning.

I altered the cargo clippy CI step to use cargo-hack's --workspace feature.

modified   .github/workflows/check.yml
@@ -153,7 +153,13 @@ jobs:
         if: success() || failure()
 
       - name: Clippy
-        run: cargo +${{ matrix.rust-toolchain }} clippy --all-targets -- -D warnings || ${{ matrix.rust-toolchain == 'nightly' }}
+        run: |
+          # Use cargo-hack to run clippy on each crate individually with its
+          # respective default features only. Can reveal warnings otherwise
+          # hidden given that a plain cargo clippy combines all features of the
+          # workspace. See e.g. https://github.com/mozilla/neqo/pull/1695.
+          cargo +${{ matrix.rust-toolchain }} install cargo-hack
+          cargo +${{ matrix.rust-toolchain }} hack clippy --all-targets -- -D warnings || ${{ matrix.rust-toolchain == 'nightly' }}

I have pushed this commit in front of the original fix. The first commit (CI change only) now fails as expected. See e.g. https://github.com/mozilla/neqo/actions/runs/8101846886/job/22143006404.

With the second commit (the actual fix) CI is green. (Well one of them failed unrelated on the Fetch NSS and NSPR step.)

@larseggert let me know what you think.

[martinthomson]

Would this warning be generated if we added impl From<Datagram> for Vec<u8> instead?

[mxinden]

Currently Datagram::into_data is pub(crate) as it is only used within neqo-common, more specifically the neqo_common::udp module. Replacing Datagram::into_data with impl From<Datagram> for Vec<u8> would make the functionality pub. All pub crate items are assumed to be used. Thus the warning would be gone, yes.

I suggest not exposing it as pub, as that exposes the internal data representation of Datagram, i.e. exposes that conversion to Vec<u8> is cheap / reasonable. This is not a strong opinion. Happy to go with whatever you prefer.

(Tangentially, we might want to reconsider the data representation of Datagram as part of #1693.)