Skip to content

perfectionist::macro_argument_binding's suggested fix breaks the debug-only contract of the debug_assert_* family #415

@KSXGitHub

Description

@KSXGitHub

Filed against this repository for tracking, because the upstream perfectionist repository is not in scope for me to write to. Please forward or mirror as appropriate.

Summary

perfectionist::macro_argument_binding flags arguments to debug_assert!, debug_assert_eq!, debug_assert_ne!, and third-party look-alikes such as assert_cmp::debug_assert_op! and debug_assert_op_expr!. Applying the suggested fix — bind the expression to a let and pass the binding — silently breaks the debug-only contract of the entire debug_assert_* family:

let computed = a_dot_field <= b_dot_field;
debug_assert_op!(computed);

versus

debug_assert_op!(a_dot_field <= b_dot_field);

In the rewritten form, a_dot_field <= b_dot_field runs in release builds too, because the let is a normal statement that is no longer guarded by cfg(debug_assertions). The whole point of debug_assert_* is that the work happens only in debug builds.

Observed on 0.0.0-rc.9, running cargo dylint against this repository's claude/setup-dylint-perfectionist-VPKWS branch. 13 of 25 flagged sites in this repository fall in this category, every one of them a side-effect-free comparison of locals:

error: non-trivial expression passed directly to a macro
  --> src/data_tree/hardlink.rs:37:30
   |
37 |             debug_assert_op!(number_of_links > 1);
   |                              ^^^^^^^^^^^^^^^^^^^

error: non-trivial expression passed directly to a macro
  --> src/visualizer/methods/bar_table.rs:47:30
   |
47 |             debug_assert_op!(lv0_value <= lv1_value);
   |                              ^^^^^^^^^^^^^^^^^^^^^^
…

Why I think this is a real upstream issue, not a workflow nit

The lint's own docstring picks debug_assert_eq!(map.insert(key, value), None, "duplicate") as the canonical motivating bug. That is genuinely dangerous when the argument has side effects — insert runs only in debug. But the proposed fix only resolves the symptom when the argument has side effects you wanted to run unconditionally. For the much more common case of debug_assert_op!(a <= b) over local reads, the proposed fix is actively wrong: it converts a debug-only invariant check into a release-time cost, in exchange for nothing.

The lint cannot prove purity in general, but it can recognise the receiving macro. Every receiver in this category lives in a small, well-known list.

Suggested resolutions (pick any subset)

  1. Built-in suppression list for the debug_assert_* family. At minimum: debug_assert, debug_assert_eq, debug_assert_ne (std). Treat these as never-flagged — the lint's "fix" is wrong for every single call to them, regardless of argument purity.
  2. A configurable allowlist (dylint.toml knob macros_to_skip) so third-party debug_assert_* analogues (assert-cmp's debug_assert_op! / debug_assert_op_expr!, static_assertions::const_assert! etc.) can be added per-project.
  3. Document the trade-off prominently in the rule docstring if neither 1 nor 2 is acceptable, so users at least know that suppressing this rule on debug_assert_* is the standard answer rather than a workaround.

Workaround applied here

Function-scope #[cfg_attr(dylint_lib = "perfectionist", expect(perfectionist::macro_argument_binding, reason = "…"))] on every function that calls debug_assert_op! / debug_assert_op_expr!. Tracking issue: this issue, #414.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions