# Coding conventions

## Formatting

**TL;DR:** Run [`cargo xtask fmt`](./scripts.md#cargo-xtask) to catch (and fix)
formatting issues.

### `rustfmt`

(enforced using [`cargo xtask fmt rustfmt`](./scripts.md#cargo-xtask))

OpenVMM source must be formatted using
[rustfmt](https://github.com/rust-lang/rustfmt), which automatically and
mechanically applies standard formatting to all the code. This eliminates time
spent discussing or reviewing stylistic issues in pull requests.

The CI will run `rustfmt --check` to enforce consistent formatting, and will
fail if it notices any discrepancies.

Unfortunately, `rustfmt` isn't infinitely customizable, and there are several
rules that are must be manually enforced:

- All lines must end with LF, not CRLF.
- Top-level `use` imports should be non-nested.

> NOTE: Some of these manually-enforced conventions were introduced late into
> OpenVMM's development, and there may be chunks of the codebase that do not
> adhere to these conventions.
>
> If you're working in a file and notice that it isn't following a certain
> convention, please take a moment to fix it!

Assuming you've followed the guide and set up `rust-analyzer` to
[format on save](../ide_setup.md#enabling-format-on-save), you should rarely have
to think about formatting in `.rs` files.

### Project-specific formatting rules

Aside from using ecosystem-standard `rustfmt` formatting, OpenVMM also includes a
handful of (automatically enforced) project-specific formatting rules.

#### House Rules

(enforced using [`cargo xtask fmt house-rules`](./scripts.md#cargo-xtask))

These are a few minor bits of code-style that are specific to OpenVMM:

- Enforcing the standard Microsoft copyright header at the top of each file
- Enforcing single-trailing-newlines in code
- Ensuring crate names do not include `-` in their name (use `_` instead!)

#### Unused `Cargo.toml` Dependencies

(enforced using [`cargo xtask fmt unused-deps`](./scripts.md#cargo-xtask))

We have an in-repo fork of
[`cargo-machete`](https://github.com/bnjbvr/cargo-machete) that ensures
`Cargo.toml` files only include dependencies that are actually being used.

Avoiding unused dependencies makes it easier to reason about what a crate is
doing just by looking at its dependencies, and also helps cut-down on
incremental compile times.

#### Formatting `Cargo.toml`

**Not yet enforced by `cargo xtask fmt`! Coming Soon™️**

When defining dependencies in `Cargo.toml` files, please organize dependencies
into the following groups:

- crate-specific "subcrates"
- crates under vm/
- crates under vm/vmcore/
- crates under support/
- external dependencies

The rationale here is that crates should be grouped according to how related to
how _widely applicable_ they are. i.e: crates from crates.io and support are
widely applicable outside of OpenVMM, whereas crates under vmcore/ only make
sense within the context of OpenVMM, and crate-specific subcrates are - by
definition - only applicable to the crate they are being imported from.

Additionally, we make use of the
[workspace dependencies](https://doc.rust-lang.org/nightly/cargo/reference/workspaces.html#the-dependencies-table)
feature to ensure that all our dependencies stay in sync. This requires defining
dependencies in your crate's `Cargo.toml` file and in the project's root `Cargo.toml`.

So, for example:

```toml
[package]
name = "hvlite"

[dependencies]
# crate-specific subcrates
hvlite_core.workspace = true
...

# /vmcore
vmcore.workspace = true
...

# /vm/devices
firmware_uefi_custom_vars.workspace = true
storvsp.workspace = true
...

# /support
guid.workspace = true
inspect.workspace = true
inspect_proto.workspace = true
...

# external dependencies
anyhow.workspace = true
cfg-if.workspace = true
clap.workspace = true
...
```

## Linting (`clippy`)

OpenVMM uses [`cargo clippy`](https://github.com/rust-lang/rust-clippy) to
supplement rustc's built-in lints.

Assuming you've followed the guide and set up `rust-analyzer` to
[use clippy](../ide_setup.md#enabling-clippy), you should see clippy lints
appear inline when working on Rust code.

The CI runs `cargo clippy` on every crate in the repo prior to building the
project, and will fast-fail if it catches any warnings / errors.

### Suppressing Lints

In general, lints should be fixed by modifying the code to satisfy the lint.
However, there are cases where a lint may need to be `allow`'d inline.

In these cases, you _must_ provide a inline comment providing reasonable
justification for the suppressed lint.

e.g:

```rust
// x86_64-unknown-linux-musl targets have a different type defn for
// `libc::cmsghdr`, hence why these lints are being suppressed.
#[allow(clippy::needless_update, clippy::useless_conversion)]
libc::cmsghdr {
    cmsg_level: libc::SOL_SOCKET,
    cmsg_type: libc::SCM_RIGHTS,
    cmsg_len: (size_of::<libc::cmsghdr>() + size_of_val(fds))
        .try_into()
        .unwrap(),
    ..std::mem::zeroed()
}
```

### OpenVMM's Project-Specific `clippy` Configuration

We stick fairly close to the default set of rustc / clippy lints, though there
are some default lints that we've decided to disabled project wide, and other
non-default lints which we've explicitly opted into.

See
[`.cargo/config.toml`](https://github.com/microsoft/openvmm?path=/.cargo/config.toml)
for a list of globally enabled/disabled lints, along with justification as to
why certain lints have been enabled/disabled.

## Unsafe Code

> _Preface:_ When possible, try to avoid introducing new `unsafe` code!
>
> Before rolling your own `unsafe` code, check to see if a safe abstraction
> already exists, either in-tree, on crates.io\*, or in the standard library.
>
> \*subject to an unsafe-code audit

Rather than synthesizing our own unsafe code conventions, we follow the
guidelines outlined in the following two resources:

- [The Rust `unsafe` docs](https://doc.rust-lang.org/std/keyword.unsafe.html)
- [The Fuchsia "Unsafe code in Rust" Guidelines](https://fuchsia.googlesource.com/fuchsia/+/refs/heads/main/docs/development/languages/rust/unsafe.md)

In a nutshell:

- `unsafe` functions are required to include `/// # Safety` documentation
  describing any preconditions the caller must uphold when calling the function.
- `unsafe {}` blocks are required to include a `// SAFETY:` comment describing
  how the preconditions for calling the `unsafe` function(s) within the block
  are being satisfied.
- `allow(unsafe_code)` annotations are required to include an `// UNSAFETY:`
  comment justifying why the code in question needs to use `unsafe`. This
  annotation must be placed at the module or crate level.

These requirements are enforced by CI, and will cause the build to fail if
required documentation is missing.

Editing a file containing unsafe code will trigger CI to automatically add the
OpenVMM Unsafe Approvers group to your PR. This is to ensure that all unsafe code
is audited for correctness by area experts.

## Additional special considerations

The following rules are unique to the OpenVMM codebase, and may be unfamiliar to
devs coming from other Rust open-source projects.

### Uses of `cfg(target_arch = ...)` must be justified

**TL;DR:** Unless you're working on something that's *genuinely* tied to the
host's CPU architecture, you almost certainly want to use `cfg(guest_arch =
...)` instead.

OpenVMM is a multi-architecture VMM framework, capable of running both x86 and
Aarch64 guests (and potentially others in the future).

Often times, the host architecture and the guest architecture will match,
thereby enabling the use of hardware-accelerated CPU virtualization backends
(e.g: KVM on Linux, WHP in Windows, etc...). That said, it's probable that at
some point in the future, OpenVMM will also support *mismatched* guest/host
architectures via an emulated CPU virtualization backend (akin to what QEMU
does).

Having an emulated CPU backend would enable
OpenVMM to support such useful scenarios as:

- running Arch64 guests on x86 machines
- running x86 guests on ARM
- running something exotic (e.g: RISC-V) on x86/ARM machines
  - ...assuming we had the bandwidth to implement + maintain something like that
- running OpenVMM on systems without hardware-accelerated CPU virtualization enabled

With these scenarios in mind, it would be short-sighted to rely entirely on
`cfg(target_arch = ...)` to gate guest-facing arch-specific functionality, as it
would inexorably tie the guest's arch to the host's arch, making any future
initiatives to pry the two apart significantly more difficult!

As such, the `OpenVMM` repo includes infrastructure to specify a custom,
OpenVMM-specific `cfg(guest_arch = ...)` cfg parameter.

By default, `cfg(guest_arch = ...)` will act the same way as `cfg(target_arch =
...)`,  but it can be swapped to a different architecture by setting the
`HVLITE_GUEST_TARGET` env var at compile-time.

There are very few reasons to use `cfg(target_arch = ...)` within the OpenVMM
repo, and to enforce this rule, we have an in-house `xtask fmt house-rules`
check that lints each use of `cfg(target_arch = ...)` to include a
"justification" for why it's being used.

e.g: `cfg(target_arch = ...)` would be applicable when feature-gating a CPU
intrinsic (such as CPUID, or a SIMD instruction), or when implementing a
`*-sys` crate where the underlying C API/ABI varies between architectures.

...otherwise, use `cfg(guest_arch = ...)`!

### Avoid `Default` when using `zerocopy::FromZeroes`

The rule:

- A type can `derive(Default)` **XOR** `derive(FromZeroes)`.
- A type that is `FromZeroes` can also `impl Default`, but it must be a
  conscious, explicit choice, with justification (read: inline comment) as to
  why that particular default value was chosen.

The why:

- The all-zero type is often not a semantically valid `Default` value for a type
- There are plenty of types that don't have a `Default` value, but _do_ have a
  valid all-zero repr

#### Additional context

As per the Rust docs for [`Default::default`](https://doc.rust-lang.org/std/default/trait.Default.html#tymethod.default):

> `fn default() -> Self`
>
> Returns the “default value” for a type.
>
> Default values are often some kind of initial value, identity value, or anything else that may make sense as a default.

Notably, **default should _not_ be used as some shorthand to "zero initialize"
values!** For most non-trivial structs, the all-zero representation is _not_ a
semantically valid `Default`!

This is true in many contexts... but one that's particularly relevant in OpenVMM
is that of FFI via C-style APIs and ABIs.

In C, it's very common for types to undergo multi-stage instantiation, where
they are initially allocated as all-zero, and then get "filled in" by some
secondary init code. Notably: it's quite rare for that initial "all-zero" struct
to be a valid instance of the type!

##### Example: C FFI

For example, a common pattern in C libraries might look something like:

```c
struct Handle {
    uint16_t opaque_handle;
}

struct Handle handle = {0};
init_handle(&handle);

update_handle(handle, options);
do_thing(handle);
```

In this example: it would be an error to invoke `update_handle` or `do_thing`
with an all-zero handle, as the type hadn't finished being fully initialized.

If we wanted to use this library from Rust, a "naive" approach would be to do
something like:

```rust
#[repr(C)]
#[derive(Default)]
struct Handle {
    opaque_handle: u16,
}

let mut handle = Handle::default(); // BAD!
unsafe { init_handle(&handle) };

unsafe { update_handle(handle, options) };
unsafe { do_thing(handle) };
```

While this _technically_ works... using `Default` here is kinda bogus!

After all - `Handle::default()` doesn't actually call `init_handle`, which means
the value returned by `default()` doesn't match the "promise" of the trait!
Namely: the returned value is not semantically valid yet!

In many other Rust codebase, this "overloading" of `Default` to represent both
semantically valid value _and_ all-zero values (in FFI) is par for the course,
as once structs get more complicated, having a `derive` that is able to fully
init a "uninitialized" struct in-memory is quite handy...

In OpenVMM, we don't do this. Instead, we use a separate trait to init all-zero
structs.

**In OpenVMM, we use `FromZeroes` and `FromZeroes::new_zeroed()` to work with types
that have valid all-zero representations, _without_ implying that those types
also have valid all-zero _default_ values!**

So, for the example above:

```rust
#[repr(C)]
#[derive(zerocopy::FromZeroes)]
struct Handle {
    opaque_handle: u16
}

let mut handle = Handle::new_zeroed(); // GOOD!
unsafe { init_handle(&handle) };
```

Now, it's impossible for code elsewhere to obtain a `Handle` via
`Handle::default`, and mistakenly forget to invoke `init_handle` on it.

...but if it so happens that we _do_ want a `Default` impl for `Handle`, we can
do so by _manually_ implementing `derive(Default)` ourselves:

```rust
// Default + FromZeroes: `default` returns fully initialized handle
impl Default for Handle {
    fn default() -> Handle {
        let mut handle = Handle::new_zeroed();
        unsafe { init_handle(&handle) };
        handle
    }
}
```

### Avoid Requiring `Debug` on Traits

**TL;DR:** Don't do this:

```rust
trait MyTrait: std::fmt::Debug
```

Implementations of the standard library's `Debug` trait can be surprisingly large,
and the final binary size of OpenHCL and related binaries is a major concern
for us. Unused implementations of this trait are usually removed during the
optimization process (like all dead code), making this a non-issue. However when
traits, and more specifically trait objects, are involved, the compiler has a
much more difficult time proving that implementations are unused. This can
result in large amounts of functionally dead code ending up in the final
binaries.

If you need to implement `Debug` for a struct containing such a trait object, you
will need to do so manually, so that you can skip over that field.

Moreover, it's usually not good form to leave tracing statements that log a
struct's `Debug` representation in production. Prefer tracing just the fields
you're interested in, and/or connecting objects to the `Inspect` graph.

### Crate Naming

**Crates must be named with underscores, not dashes and underscores used in
folder names.**

- Bad: `my-cool-crate`
- Good: `my_cool_crate`

Rust does not allow dashes in imports, with any dashes getting replaced with
underscores when used in the code. Avoiding dashes altogether makes it easier to
grep for crate names, and makes things more consistent across the repo.

This convention is enforced by CI

**Do not name crates with the words "base, util, common" or other terms that are
overly general.**

For example, consider a crate that provides a common data structure used by
multiple devices:

- Bad: `devices_common`
- Good: `range_map`

Libraries that contain the following eventually become a mishmash of unrelated
functionality that is located there for convenience. This
[blog post](https://dave.cheney.net/2019/01/08/avoid-package-names-like-base-util-or-common)
goes more in-depth as to why.

Instead, name things based on what they logically provide, like functionality or
data types.
