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)
- 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.
- 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.
- 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.
Filed against this repository for tracking, because the upstream
perfectionistrepository is not in scope for me to write to. Please forward or mirror as appropriate.Summary
perfectionist::macro_argument_bindingflags arguments todebug_assert!,debug_assert_eq!,debug_assert_ne!, and third-party look-alikes such asassert_cmp::debug_assert_op!anddebug_assert_op_expr!. Applying the suggested fix — bind the expression to aletand pass the binding — silently breaks the debug-only contract of the entiredebug_assert_*family:In the rewritten form,
a_dot_field <= b_dot_fieldruns in release builds too, because theletis a normal statement that is no longer guarded bycfg(debug_assertions). The whole point ofdebug_assert_*is that the work happens only in debug builds.Observed on
0.0.0-rc.9, runningcargo dylintagainst this repository'sclaude/setup-dylint-perfectionist-VPKWSbranch. 13 of 25 flagged sites in this repository fall in this category, every one of them a side-effect-free comparison of locals: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 —insertruns 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 ofdebug_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)
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.dylint.tomlknobmacros_to_skip) so third-partydebug_assert_*analogues (assert-cmp'sdebug_assert_op!/debug_assert_op_expr!,static_assertions::const_assert!etc.) can be added per-project.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 callsdebug_assert_op!/debug_assert_op_expr!. Tracking issue: this issue, #414.