Skip to content

Commit 64eb774

Browse files
scottgerringscottgerring
committed
address review comments
1 parent 110f824 commit 64eb774

19 files changed

Lines changed: 547 additions & 114 deletions

File tree

‎.github/workflows/pr.yaml‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,9 @@ jobs:
1818
- run: curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- -y
1919
- run: . $HOME/.cargo/env && rustup update
2020
- name: Install cargo-nextest
21-
run: . $HOME/.cargo/env && cargo install cargo-nextest --version 0.9.145 --locked
21+
run: |
22+
. $HOME/.cargo/env
23+
curl -LsSf https://get.nexte.st/0.9.145/linux | tar zxf - -C "${CARGO_HOME:-$HOME/.cargo}/bin"
2224
- run: . $HOME/.cargo/env && cargo build --release --all
2325
- name: Run tests
2426
run: . $HOME/.cargo/env && scripts/test.sh

‎cargo_pup_lint_impl/src/lib.rs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,3 +24,4 @@ pub use architecture_lint_rule::ArchitectureLintRule;
2424
pub use helpers::architecture_lint_collection::ArchitectureLintCollection;
2525
pub use helpers::architecture_lint_runner::ArchitectureLintRunner;
2626
pub use helpers::architecture_lint_runner::Mode;
27+
pub use lints::configuration_factory::LintConfigurationFactory;

‎cargo_pup_lint_impl/src/lints/module_lint/lint.rs‎

Lines changed: 217 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -20,13 +20,15 @@ fn evaluate_module_rule_with(
2020
rule: &ModuleRule,
2121
evaluate_leaf: &impl Fn(&ModuleRule) -> Option<bool>,
2222
) -> Option<bool> {
23+
// If either operand doesn't apply to this item kind, the whole composite
24+
// rule is inapplicable rather than silently collapsing to whichever
25+
// operand happened to apply.
2326
let combine =
2427
|left: Option<bool>, right: Option<bool>, operation: fn(bool, bool) -> bool| match (
2528
left, right,
2629
) {
2730
(Some(left), Some(right)) => Some(operation(left, right)),
28-
(Some(result), None) | (None, Some(result)) => Some(result),
29-
(None, None) => None,
31+
_ => None,
3032
};
3133

3234
match rule {
@@ -47,6 +49,53 @@ fn evaluate_module_rule_with(
4749
}
4850
}
4951

52+
// Walks a composite rule that evaluated to `Some(false)`, collecting the
53+
// individual leaves responsible along with the boolean value each one was
54+
// expected to produce. Mirrors the struct-lint equivalent so And/Or/Not
55+
// failures report distinct, leaf-specific diagnostics instead of one generic
56+
// message. Only ever called once the root rule has evaluated to `Some`, so
57+
// every leaf reached here is guaranteed to evaluate to `Some` too (a `None`
58+
// leaf would have made the whole composite `None`, not `Some(false)`).
59+
fn collect_module_rule_violations<'a>(
60+
rule: &'a ModuleRule,
61+
expected: bool,
62+
evaluate_leaf: &impl Fn(&ModuleRule) -> Option<bool>,
63+
violations: &mut Vec<(&'a ModuleRule, bool)>,
64+
) {
65+
let actual = evaluate_module_rule_with(rule, evaluate_leaf).expect(
66+
"collect_module_rule_violations is only invoked once the root rule evaluates to Some",
67+
);
68+
if actual == expected {
69+
return;
70+
}
71+
72+
match rule {
73+
ModuleRule::And(left, right) if expected => {
74+
collect_module_rule_violations(left, true, evaluate_leaf, violations);
75+
collect_module_rule_violations(right, true, evaluate_leaf, violations);
76+
}
77+
ModuleRule::And(left, _right) => {
78+
// Both operands are true when !(left && right) fails, but only
79+
// one needs to become false to satisfy the negation - report a
80+
// single candidate rather than presenting both as required fixes.
81+
collect_module_rule_violations(left, false, evaluate_leaf, violations);
82+
}
83+
ModuleRule::Or(left, right) if expected => {
84+
collect_module_rule_violations(left, true, evaluate_leaf, violations);
85+
collect_module_rule_violations(right, true, evaluate_leaf, violations);
86+
}
87+
ModuleRule::Or(left, right) => {
88+
collect_module_rule_violations(left, false, evaluate_leaf, violations);
89+
collect_module_rule_violations(right, false, evaluate_leaf, violations);
90+
}
91+
ModuleRule::Not(inner) => {
92+
collect_module_rule_violations(inner, !expected, evaluate_leaf, violations);
93+
}
94+
leaf => violations.push((leaf, expected)),
95+
}
96+
}
97+
98+
#[cfg(test)]
5099
fn module_rule_severity(rule: &ModuleRule) -> Severity {
51100
match rule {
52101
ModuleRule::MustBeNamed(_, severity)
@@ -334,6 +383,162 @@ impl ModuleLint {
334383
}
335384
}
336385

386+
// Renders a leaf-specific diagnostic for one violation collected by
387+
// `collect_module_rule_violations`, reusing the same lint declarations
388+
// and message styles as the direct (non-logical) rule branches below.
389+
fn render_module_rule_violation(
390+
&self,
391+
ctx: &LateContext<'_>,
392+
item: &Item<'_>,
393+
rule: &ModuleRule,
394+
expected: bool,
395+
) {
396+
match rule {
397+
ModuleRule::MustBeNamed(pattern, severity)
398+
| ModuleRule::MustNotBeNamed(pattern, severity) => {
399+
let must_match = matches!(rule, ModuleRule::MustBeNamed(..)) == expected;
400+
let item_name = ctx
401+
.tcx
402+
.item_name(item.owner_id.def_id.to_def_id())
403+
.to_string();
404+
if must_match {
405+
span_lint_and_help(
406+
ctx,
407+
MODULE_MUST_BE_NAMED::get_by_severity(*severity),
408+
self.name().as_str(),
409+
item.span,
410+
format!("Module must match pattern '{pattern}', found '{item_name}'"),
411+
None,
412+
format!("Rename this module to match the pattern '{pattern}'"),
413+
);
414+
} else {
415+
span_lint_and_help(
416+
ctx,
417+
MODULE_MUST_NOT_BE_NAMED::get_by_severity(*severity),
418+
self.name().as_str(),
419+
item.span,
420+
format!("Module must not match pattern '{pattern}'"),
421+
None,
422+
"Choose a name that doesn't match this pattern",
423+
);
424+
}
425+
}
426+
ModuleRule::MustNotBeEmpty(severity) => {
427+
if expected {
428+
span_lint_and_help(
429+
ctx,
430+
MODULE_MUST_NOT_BE_EMPTY::get_by_severity(*severity),
431+
self.name().as_str(),
432+
item.span,
433+
"Module must not be empty",
434+
None,
435+
"Add content to this module or remove it",
436+
);
437+
} else {
438+
span_lint_and_help(
439+
ctx,
440+
MODULE_MUST_BE_EMPTY::get_by_severity(*severity),
441+
self.name().as_str(),
442+
item.span,
443+
"Module must be empty",
444+
None,
445+
"Remove content from this module",
446+
);
447+
}
448+
}
449+
ModuleRule::MustBeEmpty(severity) => {
450+
if expected {
451+
span_lint_and_help(
452+
ctx,
453+
MODULE_MUST_BE_EMPTY::get_by_severity(*severity),
454+
self.name().as_str(),
455+
item.span,
456+
"Module must be empty",
457+
None,
458+
"Remove disallowed items from this module",
459+
);
460+
} else {
461+
span_lint_and_help(
462+
ctx,
463+
MODULE_MUST_NOT_BE_EMPTY::get_by_severity(*severity),
464+
self.name().as_str(),
465+
item.span,
466+
"Module must not be empty",
467+
None,
468+
"Add content to this module",
469+
);
470+
}
471+
}
472+
ModuleRule::MustHaveEmptyModFile(severity) => {
473+
let message = if expected {
474+
"Module's mod.rs file must be empty (only allowed to re-export other modules)"
475+
} else {
476+
"Module's mod.rs file must not be empty"
477+
};
478+
span_lint_and_help(
479+
ctx,
480+
MODULE_MUST_HAVE_EMPTY_MOD_FILE::get_by_severity(*severity),
481+
self.name().as_str(),
482+
item.span,
483+
message,
484+
None,
485+
"Remove disallowed items from the mod.rs file or move them to a submodule",
486+
);
487+
}
488+
ModuleRule::RestrictImports { severity, .. } => {
489+
let message = if expected {
490+
"Use of this module is not permitted by the configured import restrictions"
491+
} else {
492+
"Use of this module is required to be denied by the configured import restrictions"
493+
};
494+
span_lint_and_help(
495+
ctx,
496+
MODULE_RESTRICT_IMPORTS::get_by_severity(*severity),
497+
self.name().as_str(),
498+
item.span,
499+
message,
500+
None,
501+
"Adjust this import to satisfy the configured restrictions",
502+
);
503+
}
504+
ModuleRule::NoWildcardImports(severity) => {
505+
let message = if expected {
506+
"Wildcard imports are not allowed"
507+
} else {
508+
"Wildcard imports are required here"
509+
};
510+
span_lint_and_help(
511+
ctx,
512+
MODULE_WILDCARD_IMPORT::get_by_severity(*severity),
513+
self.name().as_str(),
514+
item.span,
515+
message,
516+
None,
517+
"Import specific items instead of using a wildcard",
518+
);
519+
}
520+
ModuleRule::DeniedItems { severity, .. } => {
521+
let message = if expected {
522+
"This item type is not allowed in this module"
523+
} else {
524+
"This item type is required to be denied in this module"
525+
};
526+
span_lint_and_help(
527+
ctx,
528+
MODULE_DENIED_ITEMS::get_by_severity(*severity),
529+
self.name().as_str(),
530+
item.span,
531+
message,
532+
None,
533+
"Consider moving this item to a different module",
534+
);
535+
}
536+
ModuleRule::And(_, _) | ModuleRule::Or(_, _) | ModuleRule::Not(_) => {
537+
unreachable!("composite rules are not leaves")
538+
}
539+
}
540+
}
541+
337542
// Helper function to check for disallowed items in a module and call the callback when found
338543
fn check_for_disallowed_items<C>(
339544
&self,
@@ -435,14 +640,6 @@ declare_variable_severity_lint!(
435640
"Module's mod.rs file must be empty (only allowed to re-export other modules)"
436641
);
437642

438-
declare_variable_severity_lint!(
439-
pub,
440-
MODULE_LOGICAL_RULE,
441-
MODULE_LOGICAL_RULE_LINT_DENY,
442-
MODULE_LOGICAL_RULE_LINT_WARN,
443-
"Module must satisfy its configured logical rule"
444-
);
445-
446643
impl_lint_pass!(ModuleLint => [
447644
MODULE_MUST_BE_NAMED_LINT_DENY, MODULE_MUST_BE_NAMED_LINT_WARN,
448645
MODULE_MUST_NOT_BE_NAMED_LINT_DENY, MODULE_MUST_NOT_BE_NAMED_LINT_WARN,
@@ -451,8 +648,7 @@ impl_lint_pass!(ModuleLint => [
451648
MODULE_MUST_HAVE_EMPTY_MOD_FILE_LINT_DENY, MODULE_MUST_HAVE_EMPTY_MOD_FILE_LINT_WARN,
452649
MODULE_RESTRICT_IMPORTS_LINT_DENY, MODULE_RESTRICT_IMPORTS_LINT_WARN,
453650
MODULE_WILDCARD_IMPORT_LINT_DENY, MODULE_WILDCARD_IMPORT_LINT_WARN,
454-
MODULE_DENIED_ITEMS_LINT_DENY, MODULE_DENIED_ITEMS_LINT_WARN,
455-
MODULE_LOGICAL_RULE_LINT_DENY, MODULE_LOGICAL_RULE_LINT_WARN
651+
MODULE_DENIED_ITEMS_LINT_DENY, MODULE_DENIED_ITEMS_LINT_WARN
456652
]);
457653

458654
impl ArchitectureLintRule for ModuleLint {
@@ -730,16 +926,14 @@ impl<'tcx> LateLintPass<'tcx> for ModuleLint {
730926
}
731927
}
732928
ModuleRule::And(_, _) | ModuleRule::Or(_, _) | ModuleRule::Not(_) => {
929+
let evaluate_leaf =
930+
|leaf: &ModuleRule| self.evaluate_module_rule_leaf(ctx, item, leaf);
733931
if self.evaluate_module_rule(ctx, item, rule) == Some(false) {
734-
span_lint_and_help(
735-
ctx,
736-
MODULE_LOGICAL_RULE::get_by_severity(module_rule_severity(rule)),
737-
self.name().as_str(),
738-
item.span,
739-
"Item does not satisfy the configured logical module rule",
740-
None,
741-
"Satisfy at least the required combination of module constraints",
742-
);
932+
let mut violations = Vec::new();
933+
collect_module_rule_violations(rule, true, &evaluate_leaf, &mut violations);
934+
for (leaf, expected) in violations {
935+
self.render_module_rule_violation(ctx, item, leaf, expected);
936+
}
743937
}
744938
}
745939
}
@@ -790,13 +984,13 @@ mod tests {
790984
}
791985

792986
#[test]
793-
fn ignores_inapplicable_leaves_without_hiding_applicable_results() {
987+
fn inapplicable_leaf_makes_composite_inapplicable() {
794988
let rule = ModuleRule::And(
795989
Box::new(named("passes", Severity::Warn)),
796990
Box::new(ModuleRule::MustNotBeEmpty(Severity::Warn)),
797991
);
798992

799-
assert_eq!(evaluate_module_rule_with(&rule, &leaf_value), Some(true));
993+
assert_eq!(evaluate_module_rule_with(&rule, &leaf_value), None);
800994
}
801995

802996
#[test]

0 commit comments

Comments
 (0)