1# Coding Style Guide 2 3## Philosophy 4 5The following is high level guidance for producing contributions to crosvm. 6 7- Prefer mechanism to policy. 8- Use existing protocols when they are adequate, such as virtio. 9- Prefer security over code re-use and speed of development. 10- Only the version of Rust in use by the ChromeOS toolchain is supported. This is ordinarily the 11 stable version of Rust, but can be behind a version for a few weeks. 12- Avoid distribution specific code. 13 14## Style guidelines 15 16### Prefer single responsibility functions 17 18Functions should have a single responsibility. This helps keep functions short and readable. We 19prefer this because functions with multiple responsibilities are hard to follow, often suffer from 20extensive indentation (very short effective line length), and are trickier to test. 21 22When you encounter large/complex functions or are about to add complexity, consider split them into 23multiple functions. Useful patterns that can help with this include splitting enums into sub-enums, 24or broader refactoring to split unrelated responsibilities from each other. 25 26### Avoid large argument lists 27 28When a function exceeds roughly 6 parameters, this is usually a signal that we should be creating a 29struct to handle the parameters. More than 6 arguments tends to make call sites unwieldy & hard to 30read. It could also be a hint that the function has too many responsibilities and should be split 31up. 32 33### Avoid extensive indentation 34 35Sometimes indentation becomes excessive in functions and severely limits the usable line length. 36Even with editor support, it can be tricky to tell which code is associated with which block. 37Classic examples of this are function calls that pass lambdas, where the call site is nested inside 38multiple matches or conditionals. In these cases, try to remove indentation by creating helpers to 39reset the indentation level, but be thoughtful about whether this makes the situation worse by 40creating an onion (too many layers / an overly deep stack). 41 42### Unsafe code: minimize code under `unsafe` 43 44Every line of unsafe code can cause memory safety issues. As such, we want to minimize code under 45`unsafe`. Often times we want to have an `unsafe` function because the caller must satisfy safety 46conditions, but we only have one or two actual `unsafe` lines in the function, along with many safe 47lines. In these situations, mark the function `unsafe`, but apply 48[`#[deny(unsafe_op_in_unsafe_fn)]`](https://doc.rust-lang.org/rustc/lints/listing/allowed-by-default.html#unsafe-op-in-unsafe-fn). 49This requires us to explicitly mark the `unsafe` code inside as `unsafe` rather than allowing any 50line in the function to be unsafe. 51 52### Unsafe code: write standard safety statements 53 54Rust tooling expects documentation for `unsafe` code and functions to follow the stdlib's 55[guidelines](https://std-dev-guide.rust-lang.org/policy/safety-comments.html). Notably, use 56`// SAFETY:` for `unsafe` blocks, and always have a `# Safety` section for `unsafe` functions in 57their doc comment. This helps us comply with 58[`undocumented_unsafe_blocks`](https://rust-lang.github.io/rust-clippy/master/#/undocumented_unsafe_blocks), 59which will eventually be turned on. 60 61Note that not all existing code follows this pattern. `// Safe because` comments are still common in 62the codebase, and should be migrated to the new pattern as they are encountered. 63 64### Formatting 65 66To format all code, crosvm defers to `rustfmt`. In addition, the code adheres to the following 67rules: 68 69Each `use` statement should import a single item, as produced by `rustfmt` with 70[`imports_granularity=item`]. Do not use braces to import multiple items. 71 72The `use` statements for each module should be grouped into blocks separated by whitespace in the 73order produced by `rustfmt` with [`group_imports=StdExternalCrate`] and sorted alphabetically: 74 751. `std` 761. third-party + crosvm crates 771. `crate` + `super` 78 79The import formatting options of `rustfmt` are currently unstable, so these are not enforced 80automatically. If a nightly Rust toolchain is present, it is possible to automatically reformat the 81code to match these guidelines by running `tools/fmt --nightly`. 82 83crosvm uses the [remain](https://github.com/dtolnay/remain) crate to keep error enums sorted, along 84with the `#[sorted]` attribute to keep their corresponding match statements in the same order. 85 86### Unit test code 87 88Unit tests and other highly-specific tests (which may include some small, but not all, integration 89tests) should be written differently than how non-test code is written. Tests prevent regressions 90from being committed, show how APIs can be used, and help with understanding bugs in code. That 91means tests must be clear both now and in the future to a developer with low familiarity of the code 92under test. They should be understandable by reading from top to bottom without referencing any 93other code. Towards these goals, tests should: 94 95- To a reasonable extent, be structured as Arrange-Act-Assert. 96- Test the minimum number of behaviors in a single test. Make separate tests for separate behavior. 97- Avoid helper methods that send critical inputs or assert outputs within the helper itself. It 98 should be easy to read a test and determine the critical inputs/outputs without digging through 99 helper methods. Setup common to many tests is fine to factor out, but lean toward duplicating code 100 if it aids readability. 101- Avoid branching statements like conditionals and loops (which can make debugging more difficult). 102- Document the reason constants were chosen in the test, including if they were picked arbitrarily 103 such that in the future, changing the value is okay. (This can be done with constant variable 104 names, which is ideal if the value is used more than once, or in a comment.) 105- Name tests to describe what is being tested and the expected outcome, for example 106 `test_foo_invalid_bar_returns_baz`. 107 108Less-specific tests, such as most integration tests and system tests, are more likely to require 109obfuscating work behind helper methods. It is still good to strive for clarity and ease of debugging 110in those tests, but they do not need to follow these guidelines. 111 112## Handling technical debt 113 114During development, we don't always have cycles or expertise available to fix problematic patterns 115or overly complex code. In these situations where we find an existing problem, or are tacking on 116code to a problematic area, we should document the problem in a bug and add it to the 117[Code Health](https://issuetracker.google.com/hotlists/4285957) hotlist. This is where maintainers 118look to determine what debt most needs attention. The bug should cover: 119 120- Which style guidance is being violated. 121- What the impact is (readability, easy to introduce bugs, hard to test, etc) 122- Any recommendations for a fix. 123 124[`group_imports=stdexternalcrate`]: https://rust-lang.github.io/rustfmt/?version=v1.5.1&search=#group_imports 125[`imports_granularity=item`]: https://rust-lang.github.io/rustfmt/?version=v1.5.1&search=#imports_granularity 126