feat!: enforce #[non_exhaustive] policy via clippy lints - #299
feat!: enforce #[non_exhaustive] policy via clippy lints#299crowecawcaw wants to merge 1 commit into
Conversation
279be48 to
0eab8d2
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
2565761 to
021c145
Compare
Set clippy::exhaustive_enums and clippy::exhaustive_structs to `deny` as workspace lints so the SemVer growth question is an explicit, reviewed decision for every public type, and settle the existing surface against a documented policy (specs/non-exhaustive-policy.md). The level is `deny`, not `warn`: an undecided public type is a hard error under a plain `cargo clippy`, so it fails locally while the type is being written rather than relying on CI's `-D warnings`. openjd-cli (a binary) and openjd-for-js (publish = false) are exempt at the crate level. The rule: types that mirror a numbered OpenJD spec section grow by extension RFC and are gated by the decode-time extension allowlist, so they are marked #[non_exhaustive] (76 types) — e.g. template::JobTemplate, HostRequirements, the parameter-definition/UserInterface families, plus PathFormat, HostContext, and the template growth-axis enums. Types that represent a decidable concept, are caller-constructed configuration, or are the instantiated job:: model the sessions runtime constructs and exhaustively matches (with no allowlist in front of them) stay closed with an #[expect(..., reason = "...")] recording why (80 sites). Key distinction, documented in the policy: template::* (deserialize-time input a consumer reads) is non_exhaustive; job::* (instantiated model the runtime executes) stays closed, so a new field is a compile error on the runner rather than a silently ignored `_` arm — the same rule already applied to the runtime state machines (ActionState, SessionState, ...). Adds PathMappingRule::new and CallerLimits builder methods as the supported construction path for the two non_exhaustive types that callers legitimately build by hand, and migrates call sites. openjd-cli (a binary) and openjd-for-js (publish = false) are exempted from the lint at the crate level. BREAKING CHANGE: many public enums and structs in openjd-expr, openjd-model, openjd-sessions, and openjd-snapshots are now #[non_exhaustive]. Downstream Rust consumers must add wildcard match arms and use `..` patterns / the provided constructors instead of struct literals. No functional behavior change. Signed-off-by: Stephen Crowe <6042774+crowecawcaw@users.noreply.github.com>
021c145 to
9fc65c7
Compare
mwiebe
left a comment
mwiebe
left a comment
There was a problem hiding this comment.
I'm not convinced that a blanket policy change like this is a good idea. When selecting exhaustive vs non-exhaustive before, I made individual judgements about it, and I think careful audits of those choices would be better than broad policy modification.
| /// Path format (POSIX, Windows, or URI). | ||
| #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, Serialize, Deserialize)] | ||
| #[serde(rename_all = "UPPERCASE")] | ||
| #[non_exhaustive] |
There was a problem hiding this comment.
I left this exhaustive because supporting a new path format seems worth doing a breaking change for.
| /// <https://github.com/OpenJobDescription/openjd-specifications/wiki/How-Jobs-Are-Run#path-mapping> | ||
| #[derive(Debug, Clone, Serialize, Deserialize)] | ||
| #[serde(deny_unknown_fields)] | ||
| #[non_exhaustive] |
There was a problem hiding this comment.
We have OpenJobDescription/openjd-specifications#47 suggesting to add the destination path format here, which seems like a good idea to me. But, that would be a breaking change so I don't think non_exhaustive is right for this.
| /// the previous split between `FunctionLibrary::with_host_context` and | ||
| /// `FunctionLibrary::with_unresolved_host_context`. | ||
| #[derive(Debug, Clone, Default)] | ||
| #[non_exhaustive] |
There was a problem hiding this comment.
Why shouldn't this one be exhaustive?
| PathFormat::Posix | PathFormat::Uri => '/', | ||
| // `PathFormat` is `#[non_exhaustive]`; POSIX-style `/` is the correct | ||
| // default for any future non-Windows path flavor. | ||
| _ => '/', |
There was a problem hiding this comment.
This looks worse to me than the way it was before, where adding a path format was a breaking change.
| /// §5 Action | ||
| #[derive(Debug, Clone, Deserialize)] | ||
| #[serde(rename_all = "camelCase", deny_unknown_fields)] | ||
| #[non_exhaustive] |
There was a problem hiding this comment.
I think the openjd model objects would be better served by being exhaustive so we get breaking changes when we adjust them.
There was a problem hiding this comment.
The high level idea was that changes to the OJD model are already gated behind the extensions list, so library consumers can control what they handle with extensions instead of the library version. If we leave these as exhaustive, every new extension will be a breaking change and will require consumers to update their code.
I guess it's a bit of an open question. The extensions list suggests extension support is intended to optional and non-breaking, but I could also see the case for breaking changes as a feature, so consumers don't accidentally drop functionality in a default case in a match.
There was a problem hiding this comment.
This spec contains the crux of the PR, curious about your thoughts on it. The individual changes flow out of it: https://github.com/crowecawcaw/openjd-rs/blob/9fc65c701f3a38c071acfa0cb7ae6100b3891a4a/specs/non-exhaustive-policy.md
|
The intent of this PR was that every field must be intentionally assigned to either exhaustive or non-exhaustive, never defaulted, so we don't forget to choose a stance and ship the default wrongly. The enforcement mechanism is requiring either |
2 participants
What was the problem/requirement? (What/Why)
Some public interfaces should have
#[non_exhaustive]applied so expanding the interfaces in the future doesn't force a breaking change. But not all interfaces have it. Also some interfaces should be exhaustive.What was the solution? (How)
non_exhaustivewith a lint rule. Every public interface must either benon_exhaustiveor must have a justification about why it's not.Recommend reading the spec before reading the diff: https://github.com/crowecawcaw/openjd-rs/blob/279be48524ad2b13a9eb54c8ff15c5ab9f0dab0f/specs/non-exhaustive-policy.md
What is the impact of this change?
non_exhaustiveapplied consistently across the interfaces.How was this change tested?
Coverlay
Was this change documented?
Yes
Is this a breaking change?
Yes
Does this change impact security?
No
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.