Skip to content

Match recorded violations in strict packs (Part A) #43

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
iMacTia wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Match recorded violations in strict packs (Part A) #43

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 25 additions & 4 deletions src/packs/checker.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,20 @@ pub struct ViolationIdentifier {
pub referencing_pack_name: String,
pub defining_pack_name: String,
}

impl ViolationIdentifier {
/// `strict` describes how a violation should be treated, not which violation
/// it is, and `package_todo.yml` has nowhere to record it, so recorded
/// violations are always rebuilt with `strict: false`. Compare through this
/// so a violation in a strict pack can still match its recorded entry.
pub fn recorded_key(&self) -> Self {

@dduugg dduugg Aug 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Design note, non-blocking, and fine to defer to a follow-up.

Consider moving strict off ViolationIdentifier and onto Violation instead of normalizing at comparison time. Your comment here already says why: strict describes how a violation should be treated, not which violation it is. The doc comment just below at checker.rs:55-64 sets the same rule for source_location, that the identifier defines sameness for comparison against package_todo.yml, "which doesn't store line/column." strict isn't stored there either.

The change is mechanical. Every reader of .identifier.strict (json.rs:56,90; csv.rs:12,53; package_todo.rs:144) already has a full &Violation, and build_strict_violation_message never reads the field. Constructors are pack.rs:195, which is where #41 starts and which then stops having to invent strict: false, plus pack_checker.rs:180 and four test constructors. You'd get all three comparison sites back to plain contains(&v.identifier), #41 becomes impossible to express instead of something a future call site has to remember to guard, and the extra allocations go away.

One alternative to skip: excluding strict from a manual PartialEq/Hash. Violation's derived Eq/Hash delegate to the identifier, and get_all_violations dedupes into a HashSet<Violation>, so making strict: true equal strict: false lets an insert keep the wrong flag, which build_strict_mode_violations then filters on.

recorded_key() is correct as written. This is about where the field lives, not about a bug.

@iMacTia iMacTia Aug 19, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Agreed on the reasoning, and the part I find most persuasive is that it makes #41 impossible to express instead of something a future call site has to remember to guard. Not taking it here though, for two reasons.

It is a wider diff than the fix. On this branch .identifier.strict is read at checker.rs:238 and :328, csv.rs:12 and :53, json.rs:56 and :90, and package_todo.rs:144, and it is set at pack.rs:195, pack_checker.rs:180, and the test builders in common_test.rs, pack.rs and text.rs. All mechanical, as you say, but Part A is the half meant to merge and release on its own, and this would put a refactor in front of it.

The second reason is that #45 does not thin those readers out, it thickens two of them. write_violations_to_disk now reads .identifier.strict together with a recorded_key() lookup, and the update summary at checker.rs:346 does the same. The count of readers is unchanged, there is just more logic in the two that matter. So doing the move now means either redoing it after #45 lands or colliding with the exact lines #45 changes, in a PR you are mid-review on.

My preference is a follow-up once both are in. Happy to write it up as an issue with your reasoning so it does not get buried in a merged PR, if you would rather have it tracked than take my word that I will get to it.

Sent with Claude Code

Self {
strict: false,
..self.clone()
}
}
}

/// A violation combines an identifier with display metadata.
///
/// `source_location` is intentionally separate from `ViolationIdentifier` because:
Expand Down Expand Up @@ -142,7 +156,10 @@ impl<'a> CheckAllBuilder<'a> {
self.found_violations
.violations
.iter()
.filter(|v| !recorded_violations.contains(&v.identifier))
.filter(|v| {
!recorded_violations
.contains(&v.identifier.recorded_key())
})
.collect()
};
reportable_violations
Expand All @@ -152,11 +169,11 @@ impl<'a> CheckAllBuilder<'a> {
&mut self,
recorded_violations: &'a HashSet<ViolationIdentifier>,
) -> anyhow::Result<Vec<&'a ViolationIdentifier>> {
let found_violation_identifiers: HashSet<&ViolationIdentifier> = self
let found_violation_identifiers: HashSet<ViolationIdentifier> = self

@dduugg dduugg Aug 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Minor: this moves from HashSet<&ViolationIdentifier> to an owned HashSet<ViolationIdentifier>, so it now clones 4 Strings per found violation rather than copying a pointer. Small next to parsing 15.6k files, so fine to leave.

If you want the cheaper version, a borrowed key tuple that excludes strict avoids the allocations entirely. Moving strict onto Violation (see my note on recorded_key) would also let this go back to borrowing.

@iMacTia iMacTia Aug 19, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Leaving it, on your own numbers. You measured this on #45 and came down the same way: 5.87ms and 5.65MiB for the owned set against 4.21ms and 0.28MiB for a borrowed key at 20k violations, linear in violations rather than in files, and noise next to parsing the tree.

The borrowed RecordedKey<'a> is the version worth having, and it falls out of moving strict onto Violation instead of standing on its own, so I would rather do both in one follow-up than half of it here.

Sent with Claude Code

.found_violations
.violations
.par_iter()
.map(|v| &v.identifier)
.map(|v| v.identifier.recorded_key())
.collect();
let relative_files = self
.found_violations
Expand Down Expand Up @@ -196,9 +213,13 @@ impl<'a> CheckAllBuilder<'a> {
Ok(stale_violations)
}

/// `found_violation_identifiers` is keyed by [`ViolationIdentifier::recorded_key`].
/// `todo_violation_identifier` needs no such normalization: it comes from
/// `pack_set.all_violations`, which rebuilds every recorded violation with
/// `strict: false` already, so it is its own recorded key.
fn is_stale_violation(
relative_files: &HashSet<&str>,
found_violation_identifiers: &HashSet<&ViolationIdentifier>,
found_violation_identifiers: &HashSet<ViolationIdentifier>,
todo_violation_identifier: &ViolationIdentifier,
) -> bool {
let violation_path_exists =
Expand Down
13 changes: 12 additions & 1 deletion tests/check_test.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -321,6 +321,10 @@ fn test_check_without_stale_violations() -> Result<(), Box<dyn Error>> {

#[test]
fn test_check_with_strict_mode() -> Result<(), Box<dyn Error>> {
// The violation here IS recorded in packs/foo/package_todo.yml, so it has to
// match its recorded entry: reported neither as a new violation nor as a
// stale todo. Strict mode still fails the run, which is what keeps this at
// exit 1, so the two strict messages are the whole of the output.
cargo_bin_cmd!("pks")
.arg("--project-root")
.arg("tests/fixtures/uses_strict_mode")
Expand All @@ -332,7 +336,14 @@ fn test_check_with_strict_mode() -> Result<(), Box<dyn Error>> {
))
.stdout(predicate::str::contains(
"packs/foo cannot have dependency violations on packs/bar because strict mode is enabled for dependency violations in the enforcing pack's package.yml file",
));
))
.stdout(
predicate::str::contains(
"There were stale violations found, please run `packs update`",
)
.not(),
)
.stdout(predicate::str::contains("violation(s) detected:").not());

common::teardown();
Ok(())
Expand Down