[PATCH 18/19] Documentation: rust: discuss `#[expect(...)]` in the guidelines

Miguel Ojeda posted 19 patches 1 year, 3 months ago
[PATCH 18/19] Documentation: rust: discuss `#[expect(...)]` in the guidelines
Posted by Miguel Ojeda 1 year, 3 months ago
Discuss `#[expect(...)]` in the Lints sections of the coding guidelines
document, which is an upcoming feature in Rust 1.81.0, and explain that
it is generally to be preferred over `allow` unless there is a reason
not to use it (e.g. conditional compilation being involved).

Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
---
 Documentation/rust/coding-guidelines.rst | 110 +++++++++++++++++++++++
 1 file changed, 110 insertions(+)

diff --git a/Documentation/rust/coding-guidelines.rst b/Documentation/rust/coding-guidelines.rst
index 185d3b51117d..5d64bc69acae 100644
--- a/Documentation/rust/coding-guidelines.rst
+++ b/Documentation/rust/coding-guidelines.rst
@@ -256,3 +256,113 @@ By that virtue, it makes it possible to comfortably enable more diagnostics by
 default (i.e. outside ``W=`` levels). In particular, those that may have some
 false positives but that are otherwise quite useful to keep enabled to catch
 potential mistakes.
+
+On top of that, Rust provides the ``expect`` attribute which takes this further.
+It makes the compiler warn if the warning was not produced. For instance, the
+following will ensure that, when ``f()`` is called somewhere, we will have to
+remove the attribute:
+
+.. code-block:: rust
+
+	#[expect(dead_code)]
+	fn f() {}
+
+If we do not, we get a warning from the compiler::
+
+	warning: this lint expectation is unfulfilled
+	 --> x.rs:3:10
+	  |
+	3 | #[expect(dead_code)]
+	  |          ^^^^^^^^^
+	  |
+	  = note: `#[warn(unfulfilled_lint_expectations)]` on by default
+
+This means that ``expect``\ s do not get forgotten when they are not needed, which
+may happen in several situations, e.g.:
+
+- Temporary attributes added while developing.
+
+- Improvements in lints in the compiler, Clippy or custom tools which may
+  remove a false positive.
+
+- When the lint is not needed anymore because it was expected that it would be
+  removed at some point, such as the ``dead_code`` example above.
+
+It also increases the visibility of the remaining ``allow``\ s and reduces the
+chance of misapplying one.
+
+Thus prefer ``except`` over ``allow`` unless:
+
+- The lint attribute is intended to be temporary, e.g. while developing.
+
+- Conditional compilation triggers the warning in some cases but not others.
+
+  If there are only a few cases where the warning triggers (or does not
+  trigger) compared to the total number of cases, then one may consider using
+  a conditional ``expect`` (i.e. ``cfg_attr(..., expect(...))``). Otherwise,
+  it is likely simpler to just use ``allow``.
+
+- Inside macros, when the different invocations may create expanded code that
+  triggers the warning in some cases but not in others.
+
+- When code may trigger a warning for some architectures but not others, such
+  as an ``as`` cast to a C FFI type.
+
+As a more developed example, consider for instance this program:
+
+.. code-block:: rust
+
+	fn g() {}
+
+	fn main() {
+	    #[cfg(CONFIG_X)]
+	    g();
+	}
+
+Here, function ``g()`` is dead code if ``CONFIG_X`` is not set. Can we use
+``expect`` here?
+
+.. code-block:: rust
+
+	#[expect(dead_code)]
+	fn g() {}
+
+	fn main() {
+	    #[cfg(CONFIG_X)]
+	    g();
+	}
+
+This would emit a lint if ``CONFIG_X`` is set, since it is not dead code in that
+configuration. Therefore, in cases like this, we cannot use ``expect`` as-is.
+
+A simple possibility is using ``allow``:
+
+.. code-block:: rust
+
+	#[allow(dead_code)]
+	fn g() {}
+
+	fn main() {
+	    #[cfg(CONFIG_X)]
+	    g();
+	}
+
+An alternative would be using a conditional ``expect``:
+
+.. code-block:: rust
+
+	#[cfg_attr(not(CONFIG_X), expect(dead_code))]
+	fn g() {}
+
+	fn main() {
+	    #[cfg(CONFIG_X)]
+	    g();
+	}
+
+This would ensure that, if someone introduces another call to ``g()`` somewhere
+(e.g. unconditionally), then it would be spotted that it is not dead code
+anymore. However, the ``cfg_attr`` is more complex than a simple ``allow``.
+
+Therefore, it is likely that it is not worth using conditional ``expect``\ s when
+more than one or two configurations are involved or when the lint may be
+triggered due to non-local changes (such as ``dead_code``).
-- 
2.46.0
Re: [PATCH 18/19] Documentation: rust: discuss `#[expect(...)]` in the guidelines
Posted by Trevor Gross 1 year, 2 months ago
On Wed, Sep 4, 2024 at 4:45 PM Miguel Ojeda <ojeda@kernel.org> wrote:
>
> Discuss `#[expect(...)]` in the Lints sections of the coding guidelines
> document, which is an upcoming feature in Rust 1.81.0, and explain that
> it is generally to be preferred over `allow` unless there is a reason
> not to use it (e.g. conditional compilation being involved).
>
> Signed-off-by: Miguel Ojeda <ojeda@kernel.org>

Would it be good to mention that a reason can be specified with
`reason = "..."`? I don't think we use this anywhere yet.
Re: [PATCH 18/19] Documentation: rust: discuss `#[expect(...)]` in the guidelines
Posted by Miguel Ojeda 1 year, 2 months ago
On Sun, Sep 29, 2024 at 7:11 AM Trevor Gross <tmgross@umich.edu> wrote:
>
> Would it be good to mention that a reason can be specified with
> `reason = "..."`? I don't think we use this anywhere yet.

I mainly wanted to introduce things "slowly" :)

But I would be happy if we consistently use `reason`, I think it is a
good idea to document those, and perhaps we can get to the point where
we go for `clippy::allow_attributes_without_reason` too.

Relatedly, I also thought about `clippy::allow_attributes`, but due to
conditional compilation and the other reasons mentioned in the docs
added in the series, it is likely not something we can easily do.

Another tangent: I find the `reason` syntax a bit too "busy". I think
in some cases we may just want to write things as if they were "normal
comments" (and multiline and so on). Highlighting in particular ways
could help perhaps. It would have been nice to have something like `//
ALLOW` on top of the attribute, similar to `// SAFETY`, and have the
compiler understand that directly like Clippy nowadays does for `//
SAFETY`, but I guess there are downsides.

Cheers,
Miguel