This document enumerates patterns that the netstack team has found to produce:
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.
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:
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.
// 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());
// 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();
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();
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 called tests or one of its descendents. 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).
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.
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.fuchsia_zircon may be aliased to zx.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(); /* ... */ }
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, 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.Things to keep in mind: