From 6915e1c57e62873615111dd9ea9b12c60f742aec Mon Sep 17 00:00:00 2001 From: Jade Ellis Date: Tue, 1 Jul 2025 23:30:13 +0100 Subject: [PATCH 1/6] docs: Note policy on large formatting diffs --- CONTRIBUTING.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 1c091183..cf140ea1 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -21,6 +21,8 @@ comment saying why. Do not write inefficient code for the sake of satisfying lints. If a lint is wrong and provides a more inefficient solution or suggestion, allow the lint and mention that in a comment. +If there is a large formatting change across unrelated files, make a separate commit so that it can be added to the `.git-blame-ignore-revs` file. + ### Pre-commit Checks Continuwuity uses pre-commit hooks to enforce various coding standards and catch common issues before they're committed. These checks include: From 7e0c0216039471a671feaacb3c49dc87954f2790 Mon Sep 17 00:00:00 2001 From: Jade Ellis Date: Tue, 1 Jul 2025 23:32:09 +0100 Subject: [PATCH 2/6] docs: Note python requirements --- CONTRIBUTING.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index cf140ea1..106a349e 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -38,6 +38,10 @@ You can run these checks locally by installing [prefligit](https://github.com/j1 ```bash +# Requires UV: +# Mac/linux: curl -LsSf https://astral.sh/uv/install.sh | sh +# Windows: powershell -ExecutionPolicy ByPass -c "irm https://astral.sh/uv/install.ps1 | iex" + # Install prefligit using cargo-binstall cargo binstall prefligit @@ -50,6 +54,8 @@ prefligit --all-files Alternatively, you can use [pre-commit](https://pre-commit.com/): ```bash +# Requires python + # Install pre-commit pip install pre-commit From 1aa891c0537603513ae185ad54fab4b017e07cfb Mon Sep 17 00:00:00 2001 From: Jade Ellis Date: Tue, 1 Jul 2025 23:57:24 +0100 Subject: [PATCH 3/6] refactor: Add with_lock traits --- src/core/utils/mod.rs | 1 + src/core/utils/with_lock.rs | 65 +++++++++++++++++++++++++++++++++++++ 2 files changed, 66 insertions(+) create mode 100644 src/core/utils/with_lock.rs diff --git a/src/core/utils/mod.rs b/src/core/utils/mod.rs index 54404e4c..329e2ea2 100644 --- a/src/core/utils/mod.rs +++ b/src/core/utils/mod.rs @@ -19,6 +19,7 @@ pub mod sys; #[cfg(test)] mod tests; pub mod time; +pub mod with_lock; pub use ::conduwuit_macros::implement; pub use ::ctor::{ctor, dtor}; diff --git a/src/core/utils/with_lock.rs b/src/core/utils/with_lock.rs new file mode 100644 index 00000000..76f014d1 --- /dev/null +++ b/src/core/utils/with_lock.rs @@ -0,0 +1,65 @@ +//! Traits for explicitly scoping the lifetime of locks. + +use std::sync::{Arc, Mutex}; + +pub trait WithLock { + /// Acquires a lock and executes the given closure with the locked data. + fn with_lock(&self, f: F) + where + F: FnMut(&mut T); +} + +impl WithLock for Mutex { + fn with_lock(&self, mut f: F) + where + F: FnMut(&mut T), + { + // The locking and unlocking logic is hidden inside this function. + let mut data_guard = self.lock().unwrap(); + f(&mut data_guard); + // Lock is released here when `data_guard` goes out of scope. + } +} + +impl WithLock for Arc> { + fn with_lock(&self, mut f: F) + where + F: FnMut(&mut T), + { + // The locking and unlocking logic is hidden inside this function. + let mut data_guard = self.lock().unwrap(); + f(&mut data_guard); + // Lock is released here when `data_guard` goes out of scope. + } +} + +pub trait WithLockAsync { + /// Acquires a lock and executes the given closure with the locked data. + fn with_lock(&self, f: F) -> impl Future + where + F: FnMut(&mut T); +} + +impl WithLockAsync for futures::lock::Mutex { + async fn with_lock(&self, mut f: F) + where + F: FnMut(&mut T), + { + // The locking and unlocking logic is hidden inside this function. + let mut data_guard = self.lock().await; + f(&mut data_guard); + // Lock is released here when `data_guard` goes out of scope. + } +} + +impl WithLockAsync for Arc> { + async fn with_lock(&self, mut f: F) + where + F: FnMut(&mut T), + { + // The locking and unlocking logic is hidden inside this function. + let mut data_guard = self.lock().await; + f(&mut data_guard); + // Lock is released here when `data_guard` goes out of scope. + } +} From 81d013dd610b69d978433a2f1d3ba1c2de0b4c01 Mon Sep 17 00:00:00 2001 From: Jade Ellis Date: Wed, 2 Jul 2025 00:44:28 +0100 Subject: [PATCH 4/6] docs: Add code style guide --- CONTRIBUTING.md | 55 +----- docs/SUMMARY.md | 1 + docs/development.md | 2 +- docs/development/code_style.md | 331 +++++++++++++++++++++++++++++++++ 4 files changed, 342 insertions(+), 47 deletions(-) create mode 100644 docs/development/code_style.md diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 106a349e..d6a3342b 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -1,27 +1,15 @@ # Contributing guide This page is about contributing to Continuwuity. The -[development](./development.md) page may be of interest for you as well. +[development](./development.md) and [code style guide](./development/code_style.md) pages may be of interest for you as well. If you would like to work on an [issue][issues] that is not assigned, preferably ask in the Matrix room first at [#continuwuity:continuwuity.org][continuwuity-matrix], and comment on it. -### Linting and Formatting +### Code Style -It is mandatory all your changes satisfy the lints (clippy, rustc, rustdoc, etc) -and your code is formatted via the **nightly** rustfmt (`cargo +nightly fmt`). A lot of the -`rustfmt.toml` features depend on nightly toolchain. It would be ideal if they -weren't nightly-exclusive features, but they currently still are. CI's rustfmt -uses nightly. - -If you need to allow a lint, please make sure it's either obvious as to why -(e.g. clippy saying redundant clone but it's actually required) or it has a -comment saying why. Do not write inefficient code for the sake of satisfying -lints. If a lint is wrong and provides a more inefficient solution or -suggestion, allow the lint and mention that in a comment. - -If there is a large formatting change across unrelated files, make a separate commit so that it can be added to the `.git-blame-ignore-revs` file. +Please review and follow the [code style guide](./development/code_style.md) for formatting, linting, naming conventions, and other code standards. ### Pre-commit Checks @@ -66,7 +54,7 @@ pre-commit install pre-commit run --all-files ``` -These same checks are run in CI via the prefligit-checks workflow to ensure consistency. +These same checks are run in CI via the prefligit-checks workflow to ensure consistency. These must pass before the PR is merged. ### Running tests locally @@ -115,37 +103,13 @@ To build the documentation locally: The output of the mdbook generation is in `public/`. You can open the HTML files directly in your browser without needing a web server. -### Inclusivity and Diversity - -All **MUST** code and write with inclusivity and diversity in mind. See the -[following page by Google on writing inclusive code and -documentation](https://developers.google.com/style/inclusive-documentation). - -This **EXPLICITLY** forbids usage of terms like "blacklist"/"whitelist" and -"master"/"slave", [forbids gender-specific words and -phrases](https://developers.google.com/style/pronouns#gender-neutral-pronouns), -forbids ableist language like "sanity-check", "cripple", or "insane", and -forbids culture-specific language (e.g. US-only holidays or cultures). - -No exceptions are allowed. Dependencies that may use these terms are allowed but -[do not replicate the name in your functions or -variables](https://developers.google.com/style/inclusive-documentation#write-around). - -In addition to language, write and code with the user experience in mind. This -is software that intends to be used by everyone, so make it easy and comfortable -for everyone to use. 🏳️‍⚧️ - -### Variable, comment, function, etc standards - -Rust's default style and standards with regards to [function names, variable -names, comments](https://rust-lang.github.io/api-guidelines/naming.html), etc -applies here. ### Commit Messages Continuwuity follows the [Conventional Commits](https://www.conventionalcommits.org/) specification for commit messages. This provides a standardized format that makes the commit history more readable and enables automated tools to generate changelogs. The basic structure is: + ``` [(optional scope)]: @@ -186,11 +150,10 @@ of it, especially when the CI completed successfully and everything so it Before submitting a pull request, please ensure: 1. Your code passes all CI checks (formatting, linting, typo detection, etc.) -2. Your commit messages follow the conventional commits format -3. Tests are added for new functionality -4. Documentation is updated if needed - - +2. Your code follows the [code style guide](./development/code_style.md) +3. Your commit messages follow the conventional commits format +4. Tests are added for new functionality +5. Documentation is updated if needed Direct all PRs/MRs to the `main` branch. diff --git a/docs/SUMMARY.md b/docs/SUMMARY.md index af729003..acdc2dcc 100644 --- a/docs/SUMMARY.md +++ b/docs/SUMMARY.md @@ -17,6 +17,7 @@ - [Troubleshooting](troubleshooting.md) - [Development](development.md) - [Contributing](contributing.md) + - [Code Style Guide](development/code_style.md) - [Testing](development/testing.md) - [Hot Reloading ("Live" Development)](development/hot_reload.md) - [Community (and Guidelines)](community.md) diff --git a/docs/development.md b/docs/development.md index 68b963c8..dd587da4 100644 --- a/docs/development.md +++ b/docs/development.md @@ -2,7 +2,7 @@ Information about developing the project. If you are only interested in using it, you can safely ignore this page. If you plan on contributing, see the -[contributor's guide](./contributing.md). +[contributor's guide](./contributing.md) and [code style guide](./development/code_style.md). ## Continuwuity project layout diff --git a/docs/development/code_style.md b/docs/development/code_style.md new file mode 100644 index 00000000..6051c787 --- /dev/null +++ b/docs/development/code_style.md @@ -0,0 +1,331 @@ +# Code Style Guide + +This guide outlines the coding standards and best practices for Continuwuity development. These guidelines help avoid bugs and maintain code consistency, readability, and quality across the project. + +These guidelines apply to new code on a best-effort basis. When modifying existing code, follow existing patterns in the immediate area you're changing and then gradually improve code style when making substantial changes. + +## General Principles + +- **Clarity over cleverness**: Write code that is easy to understand and maintain +- **Consistency**: Pragmatically follow existing patterns in the codebase, rather than adding new dependencies. +- **Safety**: Prefer safe, explicit code over unsafe code with implicit requirements +- **Performance**: Consider performance implications, but not at the expense of correctness or maintainability + +## Formatting and Linting + +All code must satisfy lints (clippy, rustc, rustdoc, etc) and be formatted using **nightly** rustfmt (`cargo +nightly fmt`). Many of the `rustfmt.toml` features depend on the nightly toolchain. + +If you need to allow a lint, ensure it's either obvious why (e.g. clippy saying redundant clone but it's actually required) or add a comment explaining the reason. Do not write inefficient code just to satisfy lints. If a lint is wrong and provides a less efficient solution, allow the lint and mention that in a comment. + +If making large formatting changes across unrelated files, create a separate commit so it can be added to the `.git-blame-ignore-revs` file. + +## Rust-Specific Guidelines + +### Naming Conventions + +Follow standard Rust naming conventions as outlined in the [Rust API Guidelines](https://rust-lang.github.io/api-guidelines/naming.html): + +- Use `snake_case` for functions, variables, and modules +- Use `PascalCase` for types, traits, and enum variants +- Use `SCREAMING_SNAKE_CASE` for constants and statics +- Use descriptive names that clearly indicate purpose + +```rs +// Good +fn process_user_request(user_id: &UserId) -> Result { ... } + +const MAX_RETRY_ATTEMPTS: usize = 3; + +struct UserSession { + session_id: String, + created_at: SystemTime, +} + +// Avoid +fn proc_reqw(id: &str) -> Result { ... } +``` + +### Error Handling + +- Use `Result` for operations that can fail +- Prefer specific error types over generic ones +- Use `?` operator for error propagation +- Provide meaningful error messages +- If needed, create or use an error enum. + +```rs +// Good +fn parse_server_name(input: &str) -> Result { + ServerName::parse(input) + .map_err(|_| InvalidServerNameError::new(input)) +} + +// Avoid +fn parse_server_name(input: &str) -> Result> { + Ok(ServerName::parse(input).unwrap()) +} +``` + +### Option Handling + +- Prefer explicit `Option` handling over unwrapping +- Use combinators like `map`, `and_then`, `unwrap_or_else` when appropriate + +```rs +// Good +let display_name = user.display_name + .as_ref() + .map(|name| name.trim()) + .filter(|name| !name.is_empty()) + .unwrap_or(&user.localpart); + +// Avoid +let display_name = if user.display_name.is_some() { + user.display_name.as_ref().unwrap() +} else { + &user.localpart +}; +``` + +## Logging Guidelines + +### Structured Logging + +**Always use structured logging instead of string interpolation.** This improves log parsing, filtering, and observability. + +```rs +// Good - structured parameters +debug!( + room_id = %room_id, + user_id = %user_id, + event_type = ?event.event_type(), + "Processing room event" +); + +info!( + server_name = %server_name, + response_time_ms = response_time.as_millis(), + "Federation request completed successfully" +); + +// Avoid - string interpolation +debug!("Processing room event for {room_id} from {user_id}"); +info!("Federation request to {server_name} took {response_time:?}"); +``` + +### Log Levels + +Use appropriate log levels: + +- `error!`: Unrecoverable errors that affect functionality +- `warn!`: Potentially problematic situations that don't stop execution +- `info!`: General information about application flow +- `debug!`: Detailed information for debugging +- `trace!`: Very detailed information, typically only useful during development + +Keep in mind the frequency that the log will be reached, and the relevancy to a server operator. + +```rs +// Good +error!( + error = %err, + room_id = %room_id, + "Failed to send event to room" +); + +warn!( + server_name = %server_name, + attempt = retry_count, + "Federation request failed, retrying" +); + +info!( + user_id = %user_id, + "User registered successfully" +); + +debug!( + event_id = %event_id, + auth_events = ?auth_event_ids, + "Validating event authorization" +); +``` + +### Sensitive Information + +Never log sensitive information such as: +- Access tokens +- Passwords +- Private keys +- Personal user data (unless specifically needed for debugging) + +```rs +// Good +debug!( + user_id = %user_id, + session_id = %session_id, + "Processing authenticated request" +); + +// Avoid +debug!( + user_id = %user_id, + access_token = %access_token, + "Processing authenticated request" +); +``` + +## Lock Management + +### Explicit Lock Scopes + +**Always use closure guards instead of implicitly dropped guards.** This makes lock scopes explicit and helps prevent deadlocks. + +Use the `WithLock` trait from `continuwuity-core::utils::with_lock`: + +```rs +use continuwuity_core::utils::with_lock::WithLock; + +// Good - explicit closure guard +shared_data.with_lock(|data| { + data.counter += 1; + data.last_updated = SystemTime::now(); + // Lock is explicitly released here +}); + +// Avoid - implicit guard +{ + let mut data = shared_data.lock().unwrap(); + data.counter += 1; + data.last_updated = SystemTime::now(); + // Lock released when guard goes out of scope - less explicit +} +``` + +For async contexts, use the async variant: + +```rs +use continuwuity_core::utils::with_lock::WithLockAsync; + +// Good - async closure guard +async_shared_data.with_lock(|data| { + data.process_async_update(); +}).await; +``` + +### Lock Ordering + +When acquiring multiple locks, always acquire them in a consistent order to prevent deadlocks: + +```rs +// Good - consistent ordering (e.g., by memory address or logical hierarchy) +let locks = [&lock_a, &lock_b, &lock_c]; +locks.sort_by_key(|lock| lock as *const _ as usize); + +for lock in locks { + lock.with_lock(|data| { + // Process data + }); +} + +// Avoid - inconsistent ordering that can cause deadlocks +lock_b.with_lock(|data_b| { + lock_a.with_lock(|data_a| { + // Deadlock risk if another thread acquires in A->B order + }); +}); +``` + +## Documentation + +### Code Comments + +- Reference related documentation or parts of the specification +- When a task has multiple ways of being acheved, explain your reasoning for your decision +- Update comments when code changes + +```rs +/// Processes a federation request with automatic retries and backoff. +/// +/// Implements exponential backoff to handle temporary +/// network issues and server overload gracefully. +pub async fn send_federation_request( + destination: &ServerName, + request: FederationRequest, +) -> Result { + // Retry with exponential backoff because federation can be flaky + // due to network issues or temporary server overload + let mut retry_delay = Duration::from_millis(100); + + for attempt in 1..=MAX_RETRIES { + match try_send_request(destination, &request).await { + Ok(response) => return Ok(response), + Err(err) if err.is_retriable() && attempt < MAX_RETRIES => { + warn!( + destination = %destination, + attempt = attempt, + error = %err, + retry_delay_ms = retry_delay.as_millis(), + "Federation request failed, retrying" + ); + + tokio::time::sleep(retry_delay).await; + retry_delay *= 2; // Exponential backoff + } + Err(err) => return Err(err), + } + } + + unreachable!("Loop should have returned or failed by now") +} +``` + +### Async Patterns + +- Use `async`/`await` appropriately +- Avoid blocking operations in async contexts +- Consider using `tokio::task::spawn_blocking` for CPU-intensive work + +```rs +// Good - non-blocking async operation +pub async fn fetch_user_profile( + &self, + user_id: &UserId, +) -> Result { + let profile = self.db + .get_user_profile(user_id) + .await?; + + Ok(profile) +} + +// Good - CPU-intensive work moved to blocking thread +pub async fn generate_thumbnail( + &self, + image_data: Vec, +) -> Result, Error> { + tokio::task::spawn_blocking(move || { + image::generate_thumbnail(image_data) + }) + .await + .map_err(|_| Error::TaskJoinError)? +} +``` + +## Inclusivity and Diversity Guidelines + +All code and documentation must be written with inclusivity and diversity in mind. This ensures our software is welcoming and accessible to all users and contributors. Follow the [Google guide on writing inclusive code and documentation](https://developers.google.com/style/inclusive-documentation) for comprehensive guidance. + +The following types of language are explicitly forbidden in all code, comments, documentation, and commit messages: + +**Ableist language:** Avoid terms like "sanity check", "crazy", "insane", "cripple", or "blind to". Use alternatives like "validation", "unexpected", "disable", or "unaware of". + +**Socially-charged technical terms:** Replace overly divisive terminology with neutral alternatives: +- "whitelist/blacklist" → "allowlist/denylist" or "permitted/blocked" +- "master/slave" → "primary/replica", "controller/worker", or "parent/child" + +When working with external dependencies that use non-inclusive terminology, avoid propagating them in your own APIs and variable names. + +Use diverse examples in documentation that avoid culturally-specific references, assumptions about user demographics, or unnecessarily gendered language. Design with accessibility and inclusivity in mind by providing clear error messages and considering diverse user needs. + +This software is intended to be used by everyone regardless of background, identity, or ability. Write code and documentation that reflects this commitment to inclusivity. From be7c3348155bc5e3bd2c77f03695a9fdb7d13f48 Mon Sep 17 00:00:00 2001 From: Jade Ellis Date: Wed, 2 Jul 2025 18:26:18 +0100 Subject: [PATCH 5/6] docs: Add link to UV docs --- CONTRIBUTING.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index d6a3342b..51609d5f 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -26,7 +26,7 @@ You can run these checks locally by installing [prefligit](https://github.com/j1 ```bash -# Requires UV: +# Requires UV: https://docs.astral.sh/uv/getting-started/installation/ # Mac/linux: curl -LsSf https://astral.sh/uv/install.sh | sh # Windows: powershell -ExecutionPolicy ByPass -c "irm https://astral.sh/uv/install.ps1 | iex" From 823c73f480d602d1cf4b46d2515c7fb98881dfd8 Mon Sep 17 00:00:00 2001 From: Jade Ellis Date: Wed, 2 Jul 2025 18:34:55 +0100 Subject: [PATCH 6/6] docs: Fix code examples in style guide --- docs/development/code_style.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/development/code_style.md b/docs/development/code_style.md index 6051c787..7fa9d6bb 100644 --- a/docs/development/code_style.md +++ b/docs/development/code_style.md @@ -181,10 +181,10 @@ debug!( **Always use closure guards instead of implicitly dropped guards.** This makes lock scopes explicit and helps prevent deadlocks. -Use the `WithLock` trait from `continuwuity-core::utils::with_lock`: +Use the `WithLock` trait from `core::utils::with_lock`: ```rs -use continuwuity_core::utils::with_lock::WithLock; +use conduwuit::utils::with_lock::WithLock; // Good - explicit closure guard shared_data.with_lock(|data| { @@ -205,7 +205,7 @@ shared_data.with_lock(|data| { For async contexts, use the async variant: ```rs -use continuwuity_core::utils::with_lock::WithLockAsync; +use conduwuit::utils::with_lock::WithLockAsync; // Good - async closure guard async_shared_data.with_lock(|data| {