Ban TODO comments, require FIXME comments (#2511)

This allows us to leave TODO comments for things that should be resolved
before a PR is merged, and have stronger guarantees that these will
actually be resolved. Previously, we've manually created GitHub PR
review comments for TODO comments, which has let some TODOs through by
accident, and is generally a waste of developer time.

gherrit-pr-id: I4b767eb773d725fce0abefbefbe74c24a4b56d90

Author: joshlf

.github/workflows/ci.yml

@@ -774,6 +774,14 @@ jobs:
       - name: Run dependency check
         run: ./ci/check_job_dependencies.sh
 
+  check-todo:
+    runs-on: ubuntu-latest
+    name: Check for todo comments
+    steps:
+      - uses: actions/checkout@eef61447b9ff4aafe5dcd4e0bbf5d482be7e7871 # v4.2.1
+      - name: Run todo check
+        run: ./ci/check_todo.sh
+
   run-git-hooks:
     runs-on: ubuntu-latest
     name: Run Git hooks
@@ -800,7 +808,7 @@ jobs:
       # https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/collaborating-on-repositories-with-code-quality-features/troubleshooting-required-status-checks#handling-skipped-but-required-checks
       if: failure()
       runs-on: ubuntu-latest
-      needs: [build_test, kani, check_be_aarch64, check_avr_artmega, check_fmt, check_readme, check_versions, generate_cache, check-all-toolchains-tested, check-job-dependencies, run-git-hooks]
+      needs: [build_test, kani, check_be_aarch64, check_avr_artmega, check_fmt, check_readme, check_versions, generate_cache, check-all-toolchains-tested, check-job-dependencies, check-todo, run-git-hooks]
       steps:
         - name: Mark the job as failed
           run: exit 1

Cargo.toml

@@ -93,7 +93,7 @@ zerocopy-derive = { version = "=0.8.25-alpha.3", path = "zerocopy-derive" }
 [dev-dependencies]
 # More recent versions of `either` have an MSRV higher than ours.
 either = "=1.13.0"
-# TODO(#381) Remove this dependency once we have our own layout gadgets.
+# FIXME(#381) Remove this dependency once we have our own layout gadgets.
 elain = "0.3.0"
 itertools = "0.11"
 rand = { version = "0.8.5", default-features = false, features = ["small_rng"] }

build.rs

@@ -76,7 +76,7 @@ fn main() {
         for version_cfg in &version_cfgs {
             println!("cargo:rustc-check-cfg=cfg({})", version_cfg.cfg_name);
         }
-        // TODO(https://github.com/rust-lang/rust/issues/124816): Remove these
+        // FIXME(https://github.com/rust-lang/rust/issues/124816): Remove these
         // once they're no longer needed.
         println!("cargo:rustc-check-cfg=cfg(doc_cfg)");
         println!("cargo:rustc-check-cfg=cfg(kani)");

ci/check_todo.sh

@@ -0,0 +1,30 @@
+#!/usr/bin/env bash
+#
+# Copyright 2025 The Fuchsia Authors
+#
+# Licensed under a BSD-style license <LICENSE-BSD>, Apache License, Version 2.0
+# <LICENSE-APACHE or https://www.apache.org/licenses/LICENSE-2.0>, or the MIT
+# license <LICENSE-MIT or https://opensource.org/licenses/MIT>, at your option.
+# This file may not be copied, modified, or distributed except according to
+# those terms.
+
+set -euo pipefail
+
+# This allows us to leave XODO comments in this file and have them still be
+# picked up by this script without having the script itself trigger false
+# positives. The alternative would be to exclude this script entirely, which
+# would mean that we couldn't use XODO comments in this script.
+KEYWORD=$(echo XODO | sed -e 's/X/T/')
+
+# -H: Print filename (default for multiple files/recursive)
+# -n: Print line number
+# -w: Match whole words
+output=$(rg -H -n -w "$KEYWORD" || true)
+
+if [ -n "$output" ]; then
+  echo "Found $KEYWORD markers in the codebase." >&2
+  echo "$KEYWORD is used for tasks that should be done before merging a PR; if you want to leave a message in the codebase, use FIXME." >&2
+  echo "" >&2
+  echo "$output" >&2
+  exit 1
+fi

githooks/pre-push

@@ -18,6 +18,7 @@ echo "Running pre-push git hook: $0"
 ./ci/check_all_toolchains_tested.sh >/dev/null & TOOLCHAINS_PID=$!
 ./ci/check_job_dependencies.sh      >/dev/null & JOB_DEPS_PID=$!
 ./ci/check_readme.sh                >/dev/null & README_PID=$!
+./ci/check_todo.sh                  >/dev/null & XODO_PID=$!
 ./ci/check_versions.sh              >/dev/null & VERSIONS_PID=$!
 
 # `wait <pid>` exits with the same status code as the job it's waiting for.
@@ -30,6 +31,7 @@ wait $FMT_PID
 wait $TOOLCHAINS_PID
 wait $JOB_DEPS_PID
 wait $README_PID
+wait $XODO_PID
 wait $VERSIONS_PID
 
 # Ensure that this script calls all scripts in `ci/*`. This isn't a foolproof

src/byte_slice.rs

@@ -194,15 +194,15 @@ pub unsafe trait IntoByteSliceMut<'a>: IntoByteSlice<'a> + ByteSliceMut {
     fn into_byte_slice_mut(self) -> &'a mut [u8];
 }
 
-// TODO(#429): Add a "SAFETY" comment and remove this `allow`.
+// FIXME(#429): Add a "SAFETY" comment and remove this `allow`.
 #[allow(clippy::undocumented_unsafe_blocks)]
 unsafe impl ByteSlice for &[u8] {}
 
-// TODO(#429): Add a "SAFETY" comment and remove this `allow`.
+// FIXME(#429): Add a "SAFETY" comment and remove this `allow`.
 #[allow(clippy::undocumented_unsafe_blocks)]
 unsafe impl CopyableByteSlice for &[u8] {}
 
-// TODO(#429): Add a "SAFETY" comment and remove this `allow`.
+// FIXME(#429): Add a "SAFETY" comment and remove this `allow`.
 #[allow(clippy::undocumented_unsafe_blocks)]
 unsafe impl CloneableByteSlice for &[u8] {}
 
@@ -229,7 +229,7 @@ unsafe impl<'a> IntoByteSlice<'a> for &'a [u8] {
     }
 }
 
-// TODO(#429): Add a "SAFETY" comment and remove this `allow`.
+// FIXME(#429): Add a "SAFETY" comment and remove this `allow`.
 #[allow(clippy::undocumented_unsafe_blocks)]
 unsafe impl ByteSlice for &mut [u8] {}
 
@@ -254,7 +254,7 @@ unsafe impl SplitByteSlice for &mut [u8] {
         // SAFETY: By contract on caller, `mid` is not greater than
         // `self.len()`.
         //
-        // TODO(#67): Remove this allow. See NumExt for more details.
+        // FIXME(#67): Remove this allow. See NumExt for more details.
         #[allow(unstable_name_collisions)]
         let r_len = unsafe { self.len().unchecked_sub(mid) };
 
@@ -314,7 +314,7 @@ unsafe impl<'a> IntoByteSliceMut<'a> for &'a mut [u8] {
     }
 }
 
-// TODO(#429): Add a "SAFETY" comment and remove this `allow`.
+// FIXME(#429): Add a "SAFETY" comment and remove this `allow`.
 #[allow(clippy::undocumented_unsafe_blocks)]
 unsafe impl ByteSlice for cell::Ref<'_, [u8]> {}
 
@@ -334,7 +334,7 @@ unsafe impl SplitByteSlice for cell::Ref<'_, [u8]> {
     }
 }
 
-// TODO(#429): Add a "SAFETY" comment and remove this `allow`.
+// FIXME(#429): Add a "SAFETY" comment and remove this `allow`.
 #[allow(clippy::undocumented_unsafe_blocks)]
 unsafe impl ByteSlice for cell::RefMut<'_, [u8]> {}
 

src/byteorder.rs

@@ -868,7 +868,7 @@ define_type!(
     []
 );
 
-// TODO(https://github.com/rust-lang/rust/issues/72447): Use the endianness
+// FIXME(https://github.com/rust-lang/rust/issues/72447): Use the endianness
 // conversion methods directly once those are const-stable.
 macro_rules! define_float_conversion {
     ($ty:ty, $bits:ident, $bytes:expr, $mod:ident) => {

src/error.rs

@@ -797,9 +797,9 @@ impl<Src, Dst: ?Sized + Unaligned> From<CastError<Src, Dst>> for SizeError<Src,
 pub type TryCastError<Src, Dst: ?Sized + TryFromBytes> =
     ConvertError<AlignmentError<Src, Dst>, SizeError<Src, Dst>, ValidityError<Src, Dst>>;
 
-// TODO(#1139): Remove the `TryFromBytes` here and in other downstream locations
-// (all the way to `ValidityError`) if we determine it's not necessary for rich
-// validity errors.
+// FIXME(#1139): Remove the `TryFromBytes` here and in other downstream
+// locations (all the way to `ValidityError`) if we determine it's not necessary
+// for rich validity errors.
 impl<Src, Dst: ?Sized + TryFromBytes> TryCastError<Src, Dst> {
     /// Produces the source underlying the failed conversion.
     #[inline]

src/impls.rs

@@ -57,7 +57,7 @@ const _: () = unsafe {
 //
 //     The size of a value is always a multiple of its alignment.
 //
-// TODO(#278): Once we've updated the trait docs to refer to `u8`s rather than
+// FIXME(#278): Once we've updated the trait docs to refer to `u8`s rather than
 // bits or bytes, update this comment, especially the reference to [1].
 const _: () = unsafe {
     unsafe_impl!(u8: Immutable, TryFromBytes, FromZeros, FromBytes, IntoBytes, Unaligned);
@@ -150,7 +150,7 @@ impl_size_eq!(char, Unalign<u32>);
 // Note that we don't `assert_unaligned!(str)` because `assert_unaligned!` uses
 // `align_of`, which only works for `Sized` types.
 //
-// TODO(#429):
+// FIXME(#429):
 // - Add quotes from documentation.
 // - Improve safety proof for `FromZeros` and `IntoBytes`; having the same
 //   layout as `[u8]` isn't sufficient.
@@ -231,7 +231,7 @@ macro_rules! unsafe_impl_try_from_bytes_for_nonzero {
 //   multiple states, so they cannot be 0 bytes, which means that they must be 1
 //   byte. The only valid alignment for a 1-byte type is 1.
 //
-// TODO(#429):
+// FIXME(#429):
 // - Add quotes from documentation.
 // - Add safety comment for `Immutable`. How can we prove that `NonZeroXxx`
 //   doesn't contain any `UnsafeCell`s? It's obviously true, but it's not clear
@@ -245,8 +245,8 @@ macro_rules! unsafe_impl_try_from_bytes_for_nonzero {
 //
 // [2] https://doc.rust-lang.org/1.81.0/std/num/type.NonZeroI8.html
 //
-// TODO(https://github.com/rust-lang/rust/pull/104082): Cite documentation
-// that layout is the same as primitive layout.
+// FIXME(https://github.com/rust-lang/rust/pull/104082): Cite documentation that
+// layout is the same as primitive layout.
 const _: () = unsafe {
     unsafe_impl!(NonZeroU8: Immutable, IntoBytes, Unaligned);
     unsafe_impl!(NonZeroI8: Immutable, IntoBytes, Unaligned);
@@ -288,13 +288,13 @@ const _: () = unsafe {
 //   purpose of those types, it's virtually unthinkable that that would ever
 //   change. The only valid alignment for a 1-byte type is 1.
 //
-// TODO(#429): Add quotes from documentation.
+// FIXME(#429): Add quotes from documentation.
 //
 // [1] https://doc.rust-lang.org/stable/std/num/struct.NonZeroU8.html
 // [2] https://doc.rust-lang.org/stable/std/num/struct.NonZeroI8.html
 //
-// TODO(https://github.com/rust-lang/rust/pull/104082): Cite documentation
-// for layout guarantees.
+// FIXME(https://github.com/rust-lang/rust/pull/104082): Cite documentation for
+// layout guarantees.
 const _: () = unsafe {
     unsafe_impl!(Option<NonZeroU8>: TryFromBytes, FromZeros, FromBytes, IntoBytes, Unaligned);
     unsafe_impl!(Option<NonZeroI8>: TryFromBytes, FromZeros, FromBytes, IntoBytes, Unaligned);
@@ -342,7 +342,7 @@ const _: () = unsafe {
 //   | [`ptr::NonNull<U>`]   | when `U: Sized`                                           |
 //   | `fn`, `extern "C" fn` | always                                                    |
 //
-// TODO(#429), TODO(https://github.com/rust-lang/rust/pull/115333): Cite the
+// FIXME(#429), FIXME(https://github.com/rust-lang/rust/pull/115333): Cite the
 // Stable docs once they're available.
 const _: () = unsafe {
     #[cfg(feature = "alloc")]
@@ -638,7 +638,7 @@ mod atomics {
 
         impl_known_layout!(T => AtomicPtr<T>);
 
-        // TODO(#170): Implement `FromBytes` and `IntoBytes` once we implement
+        // FIXME(#170): Implement `FromBytes` and `IntoBytes` once we implement
         // those traits for `*mut T`.
         impl_for_transmute_from!(T => TryFromBytes for AtomicPtr<T> [UnsafeCell<*mut T>]);
         impl_for_transmute_from!(T => FromZeros for AtomicPtr<T> [UnsafeCell<*mut T>]);
@@ -914,7 +914,7 @@ const _: () = unsafe {
 // `IntoBytes` for raw pointers eventually, but we are holding off until we can
 // figure out how to address those footguns.
 //
-// [1] TODO(https://github.com/rust-lang/rust/pull/116988): Cite the
+// [1] FIXME(https://github.com/rust-lang/rust/pull/116988): Cite the
 // documentation once this PR lands.
 const _: () = unsafe {
     unsafe_impl!(T: ?Sized => Immutable for *const T);
@@ -1474,7 +1474,7 @@ mod tests {
                 }
 
                 <$ty as TryFromBytesTestable>::with_passing_test_cases(|mut val| {
-                    // TODO(#494): These tests only get exercised for types
+                    // FIXME(#494): These tests only get exercised for types
                     // which are `IntoBytes`. Once we implement #494, we should
                     // be able to support non-`IntoBytes` types by zeroing
                     // padding.
@@ -1492,7 +1492,7 @@ mod tests {
 
                     let c = Ptr::from_ref(&*val);
                     let c = c.forget_aligned();
-                    // SAFETY: TODO(#899): This is unsound. `$ty` is not
+                    // SAFETY: FIXME(#899): This is unsound. `$ty` is not
                     // necessarily `IntoBytes`, but that's the corner we've
                     // backed ourselves into by using `Ptr::from_ref`.
                     let c = unsafe { c.assume_initialized() };
@@ -1503,7 +1503,7 @@ mod tests {
 
                     let c = Ptr::from_mut(&mut *val);
                     let c = c.forget_aligned();
-                    // SAFETY: TODO(#899): This is unsound. `$ty` is not
+                    // SAFETY: FIXME(#899): This is unsound. `$ty` is not
                     // necessarily `IntoBytes`, but that's the corner we've
                     // backed ourselves into by using `Ptr::from_ref`.
                     let c = unsafe { c.assume_initialized() };

src/layout.rs

@@ -484,7 +484,7 @@ impl DstLayout {
         // would have failed anyway for runtime reasons (such as a too-small
         // memory region).
         //
-        // TODO(#67): Once our MSRV is 1.65, use let-else:
+        // FIXME(#67): Once our MSRV is 1.65, use let-else:
         // https://blog.rust-lang.org/2022/11/03/Rust-1.65.0.html#let-else-statements
         let size_info = match self.size_info.try_to_nonzero_elem_size() {
             Some(size_info) => size_info,
@@ -544,7 +544,7 @@ impl DstLayout {
                 // Calculate the maximum number of bytes that could be consumed
                 // by the trailing slice.
                 //
-                // TODO(#67): Once our MSRV is 1.65, use let-else:
+                // FIXME(#67): Once our MSRV is 1.65, use let-else:
                 // https://blog.rust-lang.org/2022/11/03/Rust-1.65.0.html#let-else-statements
                 let max_slice_and_padding_bytes = match max_total_bytes.checked_sub(offset) {
                     Some(max) => max,
@@ -608,7 +608,7 @@ impl DstLayout {
     }
 }
 
-// TODO(#67): For some reason, on our MSRV toolchain, this `allow` isn't
+// FIXME(#67): For some reason, on our MSRV toolchain, this `allow` isn't
 // enforced despite having `#![allow(unknown_lints)]` at the crate root, but
 // putting it here works. Once our MSRV is high enough that this bug has been
 // fixed, remove this `allow`.