Skip to content

Plan: Add the whitaker_support_macros proc-macro crate (1.3.1) - #352

Draft
leynos wants to merge 3 commits into
harden-lint-configfrom
1-3-1-add-whitaker-support-macros-proc-macro-crate
Draft

Plan: Add the whitaker_support_macros proc-macro crate (1.3.1)#352
leynos wants to merge 3 commits into
harden-lint-configfrom
1-3-1-add-whitaker-support-macros-proc-macro-crate

Conversation

@leynos

@leynos leynos commented Aug 21, 2026

Copy link
Copy Markdown
Owner

Summary

ExecPlan for roadmap item 1.3.1 — the crates/whitaker_support_macros proc-macro crate implementing #[dylint_expect(lib, lints, reason)] per ADR 002.

📄 Plan: docs/execplans/1-3-1-add-whitaker-support-macros-proc-macro-crate.md

Status: BLOCKED. The plan's prototyping milestone was run during planning rather than deferred, and it falsified a premise of ADR 002. A proposed architecture deviation (D-9) is recorded with three options and a recommendation; no implementation may begin until one is accepted.

What the prototype found

ADR 002's mandated four-attribute expansion does not suppress unexpected_cfgs. That diagnostic is resolved during cfg-expansion, before the annotated item's own lint levels are in scope, so a sibling #[allow(unexpected_cfgs)] arrives too late. An attribute macro cannot place the allow in an enclosing scope without wrapping the item and changing its semantics.

Verified end-to-end with a real proc-macro = true crate emitting the ADR's exact attributes, consumed by a crate carrying this workspace's lint policy — it still warns at the call-site.

Configuration Result
Bare cfg_attr gate, no allows only unexpected_cfgs
Same + check-cfg = ['cfg(dylint_lib, values(any()))'] zero diagnostics, exit 0
ADR 002's full expansion via a real proc macro still warns
allow(unexpected_cfgs) on enclosing module / as inner attribute suppresses correctly
cfg active + misspelt lint unknown_lints fires — the typo safety net

Two of the remaining three mandated attributes suppress diagnostics the gated form never emits. The third, allow(unknown_lints), removes the only signal that catches a misspelt lint name — turning every typo into a silent no-op. ADR 002 §Options considered rejects Option D partly on these grounds, so that rationale needs correcting too.

Decision required (D-9)

  • (a) Implement ADR 002 verbatim — ships a macro known not to achieve warning-freedom.
  • (b) Recommended. Amend ADR 002; the macro emits the cfg_attr gate alone, Whitaker adds one check-cfg entry and documents it as the consumer prerequisite. Preserves typo detection.
  • (c) Supersede ADR 002 with Option D plus a dylint_expect_shape lint that validates call-sites against loaded libraries and registered lint names — closing all three silent-no-op routes no macro can. Removes 1.3.1–1.3.4 as written.

The plan is written for (b) and ready to execute on acceptance.

Design review

A six-lens expert panel reviewed the first draft. Resulting changes:

  • Cut the Verus sidecar, the permutation property, the BDD feature file, and googletest/pretty_assertions. The argument-key alphabet has three symbols, so by pigeonhole the 121-case enumeration is a total decision procedure — the proof would have restated a decidable property, which AGENTS.md forbids. make verus also runs in no CI workflow, and no proof file in verus/ has ever been modified.
  • Flattened the module layout after a misread of clippy::self_named_module_files; renamed grammar to keys, which is all it ever validated.
  • Fixed ArgShapeError to carry position so diagnostics can be anchored; newtypes now hold LitStr so spans survive and LibraryName gains a real invariant.
  • Dropped syn's full feature before it leaks into the published manifest and onto every downstream build graph.
  • Reordered the release step: the new crate publishes last and every publish is guarded, so it cannot strand whitaker-installer in an all-or-nothing set -euxo pipefail block.
  • Corrected whitaker_lints throughout — no such library exists; the real names are whitaker_suite and the per-lint crates.
  • Replaced INV-DIAG-1's non-vacuity control, which was itself vacuous (trybuild embeds the fixture path, so .stderr files always differ).
  • Deferred the guide narratives to 1.3.4, which owns them.

Follow-up work identified

  • A dylint_expect_shape lint (proposed 1.3.5) — the only mechanism that can close the wrong-lib and misspelt-lint silent no-ops.
  • crates/whitaker_test_macros emits #[cfg_attr(clippy, expect(clippy::allow_attributes, ...))], which the current toolchain rejects with clippy::unnecessary_clippy_cfg.

Verification

make markdownlint and make nixie pass. No code changes in this PR.

References

🤖 Generated with Claude Code

Summary by Sourcery

Approve and document the revised plan for adding conditional Dylint expectation macros after validating the original expansion and identifying remaining integration risks.

Enhancements:

  • Document and approve the amended architecture for the planned whitaker_support_macros proc-macro, reducing the expansion to a cfg-gated Dylint expectation and requiring a consumer check-cfg entry.
  • Record prototype findings, testing and design-review decisions, including diagnostic limitations, suite compatibility risks, validation scope, dependency choices, and release considerations.

Documentation:

  • Add the roadmap 1.3.1 ExecPlan and update ADR 002 with the accepted design, risks, constraints, and follow-up decisions.

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, we are unable to review this pull request

The GitHub API does not allow us to fetch diffs exceeding 300 files, and this pull request has 429

@coderabbitai

coderabbitai Bot commented Aug 21, 2026

Copy link
Copy Markdown

Warning

Your free Security trial is over. An organization admin can activate billing to continue.

@coderabbitai

coderabbitai Bot commented Aug 21, 2026

Copy link
Copy Markdown

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 52593fa6-ae79-4c42-82fd-4c2c59432308

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Comment @coderabbitai help to get the list of available commands.

codescene-access[bot]

This comment was marked as outdated.

@leynos

leynos commented Aug 21, 2026

Copy link
Copy Markdown
Owner Author

Update: D-9 accepted as option (b); R-1 discharged by spike

Plan status is now APPROVED. ADR 002 has been amended and moved to Accepted.

ADR 002 amendments

  • §Status → Accepted, 2026-08-21, with the reasoning summarised.
  • §Decision outcome → the expansion is now the cfg_attr gate alone, with the check-cfg entry documented as the mechanism rather than a convenience.
  • §Options considered, Option D → rejection rationale corrected. Both original premises were false: the gated form emits neither unknown_lints nor clippy::allow_attributes.
  • Table 1 → two rows corrected. "Works in downstream crates without extra config" was scored High for Options A/B/C and Low for D; it is in fact Low for every option, since none can suppress unexpected_cfgs without a consuming-manifest entry. That row was the decisive argument for C over D, so Option C is now retained on ergonomics and having one place to add validation — not on avoiding config.
  • §Known risks, §Outstanding decisions, §Decision drivers, §Technical requirements → updated.
  • whitaker_lintswhitaker_suite throughout.

R-1 discharged: #[expect(...)] does work

A spike (since discarded, as requested) compiled a fixture under a real Dylint session. Against an individual no_std_fs_operations library built from current source, item-level #[allow], item-level #[expect], module-level #![allow], and the cfg_attr-gated #[expect] all suppressed correctly, with no spurious unfulfilled-expectation warnings. Axiom A-4 is established.

R-1b: the spike found something larger

The aggregated whitaker_suite library ignores lint-level attributes entirely. Controlled experiment — identical fixture, identical source revision, identical toolchain, only the loaded library differing:

########## LIBRARY: no_std_fs_operations ##########
error: LINT FIRED
 --> src/lib.rs:2:46          <- unannotated control only

########## LIBRARY: whitaker_suite ##########
error: LINT FIRED
 --> src/lib.rs:2:46          <- control
error: LINT FIRED
 --> src/lib.rs:5:52          <- #[allow(no_std_fs_operations)]  IGNORED
error: LINT FIRED
 --> src/lib.rs:8:53          <- #[expect(no_std_fs_operations)] IGNORED
warning: this lint expectation is unfulfilled
 --> src/lib.rs:7:10          <- spurious

Since whitaker_suite is what whitaker --all, make lint-whitaker, and every installed consumer load, no attribute-based suppression works in practice today. docs/users-guide.md:647 states the opposite.

Ruled out by controlled comparison: staleness (reproduced with a suite rebuilt from source), lint-identity mismatch (SUITE_LINT_DECLS uses the same &'static Lint statics the passes emit with), and cfg_attr interaction (plain #[allow] fails too). Remaining suspect is the declare_combined_late_lint_pass! aggregation in suite/src/driver.rs; not root-caused.

This does not block 1.3.1 — every obligation in the plan is token-level. It does gate ADR 002 migration phase 3, and is tracked separately (D-13).

make markdownlint and make nixie pass.

codescene-access[bot]

This comment was marked as outdated.

leynos added 3 commits August 21, 2026 19:11
Draft the execution plan for roadmap item 1.3.1: the
`crates/whitaker_support_macros` proc-macro crate implementing
`#[dylint_expect(lib = ..., lints(...), reason = ...)]` per ADR 002.

The plan decomposes the work so the one genuinely general property --
argument-key well-formedness and order-independence -- lands on a pure
function over a three-symbol alphabet, provable in Verus and
exhaustively enumerable in a test, while token-tree behaviour is
covered by parameterized, snapshot, property, and trybuild fixtures.

Scope was extended by user direction to include crates.io publish
wiring, the ADR 002 status change to Accepted, and a Dylint suite
design cross-reference.

An opening prototyping milestone answers the empirical questions the
design rests on -- notably whether `#[expect(...)]` works at all for
Dylint-registered lints -- before any production code is written.
Run the prototyping milestone during planning rather than deferring it,
and record what it found. ADR 002's mandated expansion does not suppress
`unexpected_cfgs`: that diagnostic resolves during cfg-expansion, before
the annotated item's own lint levels are in scope, so a sibling
`#[allow]` arrives too late. Verified end-to-end with a real proc macro
emitting the ADR's exact four attributes.

Two of the remaining three mandated attributes suppress diagnostics the
gated form never emits, and the third removes the only signal that
catches a misspelt lint name. Record the deviation as D-9 with three
options and a recommendation, and set the plan to BLOCKED pending
explicit acceptance.

Fold in a six-lens design review. Cut the Verus sidecar, the permutation
property, the BDD feature file, and two new assertion crates: the
argument-key alphabet has three symbols, so by pigeonhole the 121-case
enumeration is a total decision procedure and the proof would have
restated a decidable property. Flatten the module layout after a misread
of `self_named_module_files`; rename `grammar` to `keys` because it never
covered arity; carry position in `ArgShapeError` so diagnostics can be
anchored; hold `LitStr` in the newtypes so spans survive and the library
name gains a real invariant; drop `syn`'s `full` feature before it leaks
to downstream consumers; publish the new crate last and guard the block;
correct the non-existent `whitaker_lints` library name throughout;
replace INV-DIAG-1's non-vacuity control, which was itself vacuous.
Amend ADR 002 per the accepted deviation D-9(b) and move it to Accepted.
The macro expands to the `cfg_attr` gate alone; the `check-cfg` manifest
entry is documented as the mechanism rather than a convenience, because a
sibling `#[allow(unexpected_cfgs)]` provably cannot suppress a diagnostic
resolved during cfg-expansion.

Correct Option D's rejection rationale, which rested on two false
premises: the gated form emits neither `unknown_lints` nor
`clippy::allow_attributes`. Correct two rows of Table 1 accordingly --
"works in downstream crates without extra config" was the decisive
argument for Option C over Option D and is in fact Low for every option.
Replace the non-existent `whitaker_lints` library name with
`whitaker_suite`.

Discharge R-1 with a spike, since discarded. `#[expect(...)]` does work
for Dylint-registered Whitaker lints: against an individual lint library
built from current source, item-level allow, item-level expect,
module-level allow and the gated expect all suppress correctly with no
spurious unfulfilled-expectation warnings.

Record R-1b, which the spike surfaced. The aggregated `whitaker_suite`
library ignores lint-level attributes entirely and emits a spurious
`unfulfilled_lint_expectations` warning for every expect, while
individual libraries built from the same commit behave correctly.
Staleness, lint-identity mismatch and cfg_attr interaction were each
ruled out by controlled comparison. Since the suite is what every
installed consumer loads, no attribute-based suppression works in
practice today. This gates ADR 002 migration phase 3, not 1.3.1 delivery,
whose obligations are all token-level.

Move the plan to APPROVED.
@leynos
leynos force-pushed the 1-3-1-add-whitaker-support-macros-proc-macro-crate branch from fcbe353 to 114e6bf Compare August 21, 2026 17:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant