Netstack Team's Rust Patterns

This document enumerates patterns that the netstack team has found to produce:

  • Code that is more resilient to behavior change at a distance.
  • Code that is more easily reviewed with less "working memory".

These guidelines are considered additive to the Rust API rubric.

The patterns suggested here provide centralized guidance and knowledge. Contributions of all types - edits, additions, removals, etc. - are encouraged.

Rough edges encountered during code authoring or reviewing under the proposed guidelines are expected to make their way back into this document, so we can codify alternative patterns when needed.

Avoid unused results

This is mostly machine-enforced since https://fxrev.dev/510442 via rustc's unused-results lint. See the documentation for an explanation of the reasoning.

When discarding results, encode the types of ignored fields, so it becomes part of the contract.

When discarding a result prefer to use a named variable prefixed with _ when the semantic meaning is not immediately clear from the type, and it won't affect drop semantics.

The added information serves as a prompt for both author and reviewer:

  • Is there an invariant that should be checked using the return?
  • Should this function return a value at all?

Note: Historically the team has used the form let () = on assignments as a statement that no information is being discarded. That practice is still accepted, but it is no longer necessary with the introduction of the lint.

Examples

Use the prompts

// Bad. The dropped type is unknown without knowing the signature of write.
let _ = writer.write(payload);
let _n = writer.write(payload);

// Okay. The dropped type is locally visible and enforced. Absence of invariant
// enforcement such as a check for partial writes is obvious and can be flagged
// in code review.
let _: usize = writer.write(payload);
let _n: usize = writer.write(payload);

// Good.
let n = writer.write(payload);
assert_eq!(n, payload.len());

Adopted dropping formats

// Encode the type of the ignored return value.
let _: Foo = foo::do_work(foo, bar, baz);

// When destructuring tuples, type the fields not in use.
let (foo, _) : (_, Bar) = foo::foo_and_bar();

// When destructuring named structs, no annotations are necessary, the field's
// type is encoded by the struct.
let Foo{ fi, fo: _ } =  foo::do_work(foo, bar, baz);

// Encode the most specific type possible.
let _: Option<_> = foo.maybe_get_trait_impl(); // Can't name opaque type.
let _: Option<usize> = foo.maybe_get_len(); // Can name concrete type.

// Use best judgement for unwieldy concrete types.
// If the type expands to formatted code that would span >= 5 lines of type
// annotation, use best judgement to cut off the signature.
let _: futures::future::Ready<Result<Buffer<_>>, anyhow::Error> = y();
// Prefer to specify if only a couple of levels of nesting and < 5 lines.
let _: Result<
    Result<(bool, usize), fidl_fuchsia_net_interfaces_admin::AddressRemovalError>,
    fidl::Error,
> = x();

Defeating patterns

Be mindful of defeating patterns:

// Bad, this is a drop that does not encode the type.
std::mem::drop(foo());
// Prefer instead:
let _: Foo = foo();

Tests

Declaration

Name tests after what they're testing without the test_ prefix. That's the adopted pattern in the Rust standard library.

If the test name is not sufficient to encode the objective of the test, add a short non doc comment before it or at the top of the function's body explaining what this test is exercising. We use non-doc comments because we expect the target audience to be readers of the code, and not of the public API.

Tests should always be in a module named tests or one of its descendants. Crates that contain only integration tests do not need a tests module, e.g. network/tests/integration, network/tests/fidl.

Example:

// Tests Controller::do_work error returns.
#[test]
fn controller_do_work_errors() { /* ... */ }

Test support functions should be in a module named testutil. If the module is meant for use in-crate only, it should be declared pub(crate) and #[cfg(test)]. This module should not contain any #[test] functions. If tests are needed for the functionality in the testutil module, a sub-module called tests should be created (i.e., testutil::tests).

Prefer panics

Do not use Rust's support for tests which return Result; such tests do not automatically emit backtraces, relying on the errors themselves to carry a backtrace. Test failures that don't emit backtraces are typically much harder to interpret. At the time of writing, the backtrace feature in Rust is unstable and disabled in the Fuchsia Rust build configuration, but even if enabled not all errors contain a backtrace; best to panic to avoid relying on external factors for backtraces.

Imports

The following rules apply to styling imports.

Employ one use statement per crate or direct child module. Combine children of those into the same use statement.

use child_mod::{Foo, Bar};
use futures::{
    stream::{self, futures_unordered::FuturesUnordered},
    Future, Stream,
};

Always alias imported-but-not-referenced traits to _. It avoids cluttering the namespace and informs the reader that the identifier is not referenced.

use futures::FutureExt as _;

Avoid bringing symbols from other crates into the scope. Especially if things all have similar names. Exceptions apply for widely used, self-explanatory std types like HashMap, HashSet, etc.

Importing symbols from the same crate is always allowed, including to follow the pattern of declaring crate-local Error and Result types.

Some crates rely heavily on external types. If usage is expected to be ubiquitous throughout the crate, it's acceptable to import those types as a means to reduce verbosity, as long as it doesn't introduce ambiguity.

// DON'T. Parsing this module quickly grows out of hand since it's hard to make
// sense where types are coming from.
mod confusing_mod {
    use packet_formats::IpAddress;
    use crate::IpAddr;
    use fidl_fuchsia_net::Ipv4Address;
}

// DO. Import only types from this crate.
mod happy_mod {
    use crate::IpAddress;

    fn foo() -> packet_formats::IpAddress { /* .. */ }
    fn bar() -> IpAddress { /* .. */ }
    fn baz() -> fidl_fuchsia_net::Ipv4Address { /* .. */ }
}

Some well-known crates have adopted aliases or alias formation rules. Those are:

  • fuchsia_async may be aliased to fasync.
  • fidl_fuchsia_* prefixes may be aliased to f*, e.g.:
    • use fidl_fuchsia_net_interfaces_admin as fnet_interfaces_admin;
    • use fidl_fuchsia_net_routes as fnet_routes;

Importing * is strongly discouraged. Tests modules that import from super are acceptable exceptions; it is assumed that a test module will use most if not all symbols declared in its parent.

Importing types in function bodies is always allowed, since it's easy to reason locally about where the types come from.

fn do_stuff() {
    use some::deeply::nested::{Foo, bar::Bar};
    let x = Foo::new();
    let y = Bar::new();
    /* ... */
}

Prefer exhaustive matches

Match exhaustively whenever possible, avoiding catch-all patterns. Matching exhaustively provides more local context during reviews, acts as a prompt to revisit when the enumeration is updated, and requires more explicit forms to drop information.

Some patterns implicitly defeat exhaustive matches. We should use them with care:

  • Avoid if let patterns, since that's effectively a non-exhaustive match. We carve out the exception to allow binding to the Some value of an Option<T>, since it's a well known type and the None variant doesn't carry any information. Note that if T is an enum, if let Some(Foo::Variant) is not a valid use of the exception, since it sidesteps an exhaustive match on T.
  • Avoid methods on enum receivers that indicate variants without matching, e.g.:
    • is_foo(&self) -> bool enum methods like Result::is_ok or Option::is_none.
    • foo(self) -> Option<T> enum methods like Result::ok, which serve as single-variant extraction helpers that are easy to miss in review.

Rust provides the non_exhaustive attribute which defeats the intent of matching exhaustively, and flexible FIDL types are annotated with that attribute. When dealing with such types, attempting to match exhaustively is prone to becoming stale - the type can evolve without breaking the code - and should, thus, be avoided.

// Don't attempt to match exhaustively, exhaustive enumeration is prone to
// becoming stale and misleading future readers if `Foo` takes more variants.
fn bad_flexible_match(foo: fidl_foo::FlexibleFoo) {
    match foo {
        fidl_foo::FlexibleFoo::Bar => { /* ... */ },
        foo @ fidl_foo::FlexibleFoo::Baz |
        foo => panic!("unexpected foo {:?}", foo)
    }
}

// Use the catch-all pattern instead when the type is non_exhaustive.
fn good_flexible_match(foo: fidl_foo::FlexibleFoo) {
    match foo {
        fidl_foo::FlexibleFoo::Bar => { /* ... */ },
        foo => panic!("unexpected foo {:?}", foo)
    }
}

// Note that if the type was not marked non_exhaustive we'd prefer to match
// exhaustively.
fn strict_match(foo: fidl_foo::StrictFoo) {
    match foo {
        fidl_foo::StrictFoo::Bar => { /* ... */ },
        foo @ fidl_foo::StrictFoo::Baz |
        foo @ fidl_foo::StrictFoo::Boo => panic!("unexpected foo {:?}", foo)
    }
}

TODO(https://github.com/rust-lang/rust/issues/89554): Revisit non_exhaustive guidance once non_exhaustive_omitted_patterns lint is stabilized.

Avoid default type parameters

Rust supports defining parameterized types with defaulted type parameters. This can be convenient for certain parameters, e.g. ones that are only used to override behavior in tests. For example:

// This can be used as `Foo<X>` or `Foo<X, Y>`.
struct Foo<A, B = u32>(A, B);

// Blanket impl for all possible `Foo`s.
impl<A, B> MyTrait for Foo<A, B> { /* ... */ }

The problem with defaulting type parameters is that it can easily lead to incomplete blanket implementations. Suppose Foo is extended with another defaulted type parameter:

// Now `Foo<A> = Foo<A, u32> = Foo<A, u32, ()>.
struct Foo<A, B = u32, C = ()>(A, B, C);

The impl MyTrait block still works unmodified, so there's no signal from the compiler that the coverage is incomplete: it only covers Foo<A, B, ()> instead of all possible Foo<A, B, C>s. Avoiding defaulted type parameters puts the onus on the author to make sure any impls, blanket or otherwise, cover the correct set of types.

Logging

Logs are a critical debugging resource for developers, but unfortunately, there are several pitfalls to logging that reduce their value and signal. Log too frequently, or at too high of a severity, and the logs become noisy, diluting the actually important debugging signal for a given problem. Conversely, logging too infrequently, or with too low of a severity, means that the debugging signal might not make it into the logs to begin with. Striking a balance is essential for cultivating useful logs.

Netstack3 makes use of the tracing crate for logging, which exposes several log levels. Before diving into the logging guidance for the Netstack3 component, it's important to known a few default configurations for log severity:

  • The minimum log severity (i.e. log levels below this value are suppressed) is set to info in production settings, and debug when using the debug netstack component (e.g. in tests). This can be configured lower by developers when running Netstack3 locally.
  • The test maximum log severity (i.e. log levels above this value cause an otherwise passing test to be reported as having failed) is set to warn by default. Individual tests suites can configure this threshold up or down.

Now, when should you use each log level?

  • error: Reserved for severe failures with a high probability of manifesting in user-visible problems. Examples:
    • Removing an interface because the underlying port unexpectedly closed.
    • Observing FIDL errors other than PEER_CLOSED.
  • warn: Events or failures that are not so severe as to justify error, but that may still manifest in user-visible problems or problems in other components. Examples:
    • Returning successfully from APIs that are unimplemented.
    • Rejecting API requests that had incorrect semantics (e.g. invalid args).
    • Observing that the underlying port's physical status changes to offline.
  • info: Notable Events, and cheap debugging signals. Examples
    • Changes to the network state: i.e. adding/removing addresses, routes, or interfaces.
    • Interesting network events (e.g. Duplicate Address Detection fails).
  • debug: Events and debugging signals that are relevant only to Netstack developers, or are too noisy to justify putting on production devices.
    • Control events from the network, such as Router Advertisements.
    • Incoming API calls (e.g. socket operations or FIDL methods), as well as the result returned to the caller.
    • Starting/stopping of periodic tasks, such as the Neighbor Table garbage collector.
  • trace: Debugging signals that are prohibitively expensive outside of toy environments (i.e. unit tests). Examples
    • Extremely frequent messages such as messages tied to sending/receiving datapath packets, or messages tied to high frequency timers firing.

The intention is for instances of error and trace in our code to be exceptionally rare, and for instances of warn to be uncommon. The majority of log instances should be either info or debug.

Consider debug to be the default log level choice. Before deciding to promote a message to info, ask yourself what the frequency of the message is expected to be, and what value this message will provide when triaging a field issue. Before deciding to promote a message to warn, ask yourself how likely this is to represent an issue to another team (i.e. would it be appropriate for another team to assign you a bug because they saw this warning in the logs?).

Process for changes to this page

All are invited and welcome to propose changes to the patterns adopted by the Netstack team. Proposed patterns will be accepted or rejected by the team after a process of consensus building through discussion, falling back to a go/no-go simple majority vote.

Follow the steps below to propose a change.

  1. Author and publish a CL changing this page.
  2. [optional] Socialize with a small group and iterate.
  3. Request review from the entire team through email and chat. Non-Googlers can reach out through discussion groups.
  4. Iterate on the CL based on review comments and offline sessions. Remember to publish outcomes of offline sessions back to the CL.
  5. Team members may express support +1, opposition -1, or indifference. Indifference is voiced through a single comment thread on Gerrit where members state indifference. That thread is to be kept unresolved until the CL merges. Team members may change their vote at any time.
  6. Proposals will be in review for at most 2 weeks. A last call announcement is sent at the end of the first week. The timeline may be short-circuited if the entire team has cast their vote.
  7. When consensus can't be achieved, the team will tally the votes and make a decision to go ahead or not using a simple majority.

Things to keep in mind:

  • Authors and leads will shepherd changes through this process.
  • Be respectful of others; address points on their merits alone.
  • Avoid long comments; express disagreement with supporting arguments succinctly.
  • Back-and-forth on the CL is discouraged. Fallback to breakout video or in-person sessions (public, preferably) and encode the consensus back into the comment thread.
  • Controversial points can be dropped and addressed in a follow-up proposal if necessary.
  • Indifference votes are used to measure the perceived benefit of encoding some patterns. A strong indifference signal is interpreted as a hint that the point being discussed does not merit encoding as a pattern.