Skip to content

Commit 5010cf3

Browse files
committed
Report removed guarantees in review
1 parent 1aab94d commit 5010cf3

3 files changed

Lines changed: 86 additions & 0 deletions

File tree

src/diagnostic.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,7 @@ pub mod code {
5151
pub const REVIEW_TYPE_FIELDS_CHANGED: &str = "RSR010";
5252
pub const REVIEW_BOUNDARY_CHANGED: &str = "RSR011";
5353
pub const REVIEW_UNSAFE_NATIVE_ADDED: &str = "RSR012";
54+
pub const REVIEW_GUARANTEE_REMOVED: &str = "RSR013";
5455
}
5556

5657
#[derive(Debug, Clone, PartialEq, Eq)]

src/review.rs

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,7 @@ pub enum ReviewRisk {
4747
Effect,
4848
Boundary,
4949
Unsafe,
50+
Guarantee,
5051
}
5152

5253
impl ReviewRisk {
@@ -58,6 +59,7 @@ impl ReviewRisk {
5859
Self::Effect => "effect",
5960
Self::Boundary => "boundary",
6061
Self::Unsafe => "unsafe",
62+
Self::Guarantee => "guarantee",
6163
}
6264
}
6365
}
@@ -238,6 +240,10 @@ fn review_fixes(code: &str) -> Vec<ReviewFix> {
238240
"review_unsafe_native_boundary",
239241
"Review the new unsafe or native boundary and require explicit justification.",
240242
),
243+
code::REVIEW_GUARANTEE_REMOVED => (
244+
"review_removed_guarantee",
245+
"Review callers that relied on the removed runtime guarantee.",
246+
),
241247
_ => ("review_change", "Review this source-level contract change."),
242248
};
243249
vec![ReviewFix {
@@ -414,6 +420,26 @@ fn compare_function(old: &FunctionSig, new: &FunctionSig, findings: &mut Vec<Rev
414420
Some(effects_contract(&new_unsafe_native)),
415421
));
416422
}
423+
let old_guarantees = guarantee_effects(&old.effects);
424+
let new_guarantees = guarantee_effects(&new.effects);
425+
let removed_guarantees: BTreeSet<_> = old_guarantees
426+
.difference(&new_guarantees)
427+
.cloned()
428+
.collect();
429+
if !removed_guarantees.is_empty() {
430+
findings.push(review_finding(
431+
code::REVIEW_GUARANTEE_REMOVED,
432+
ReviewRisk::Guarantee,
433+
format!(
434+
"function `{}` removed guarantee(s): {}.",
435+
old.name,
436+
effects_contract(&removed_guarantees)
437+
),
438+
paired_spans(&old.span, &new.span, "old guarantees", "new guarantees"),
439+
Some(effects_contract(&old_guarantees)),
440+
Some(effects_contract(&new_guarantees)),
441+
));
442+
}
417443
if old.boundary != new.boundary {
418444
findings.push(review_finding(
419445
code::REVIEW_BOUNDARY_CHANGED,
@@ -552,6 +578,19 @@ fn unsafe_native_effects(effects: &BTreeSet<String>) -> BTreeSet<String> {
552578
.collect()
553579
}
554580

581+
fn guarantee_effects(effects: &BTreeSet<String>) -> BTreeSet<String> {
582+
effects
583+
.iter()
584+
.filter(|effect| {
585+
matches!(
586+
effect.as_str(),
587+
"no_panic" | "noalloc" | "no_block" | "pure"
588+
)
589+
})
590+
.cloned()
591+
.collect()
592+
}
593+
555594
fn type_contract(ty: &TypeSig) -> String {
556595
format!(
557596
"{} {} {{ {} }}",

tests/checker.rs

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -269,6 +269,52 @@ fn checksum(data: read Bytes) -> UInt64
269269
}));
270270
}
271271

272+
#[test]
273+
fn review_reports_removed_guarantees() {
274+
let old_source = r#"
275+
mode: managed
276+
277+
fn checksum(data: read Bytes) -> UInt64
278+
effects(noalloc, no_panic, pure)
279+
{
280+
Bytes.checksum(data: read data)
281+
}
282+
"#;
283+
let new_source = r#"
284+
mode: managed
285+
286+
fn checksum(data: read Bytes) -> UInt64
287+
effects(no_panic)
288+
{
289+
Bytes.checksum(data: read data)
290+
}
291+
"#;
292+
293+
let findings = review_sources("old.rss", old_source, "new.rss", new_source);
294+
let guarantee = findings
295+
.iter()
296+
.find(|finding| finding.code == "RSR013")
297+
.expect("expected removed guarantee finding");
298+
299+
assert_eq!(guarantee.risk, ReviewRisk::Guarantee);
300+
assert_eq!(guarantee.before.as_deref(), Some("no_panic, noalloc, pure"));
301+
assert_eq!(guarantee.after.as_deref(), Some("no_panic"));
302+
assert!(
303+
guarantee
304+
.summary
305+
.contains("removed guarantee(s): noalloc, pure")
306+
);
307+
assert!(format_review_human(&findings).contains("RSR013[guarantee]:"));
308+
309+
let json = format_review_json(&findings);
310+
let value: Value = serde_json::from_str(&json).expect("review JSON should parse");
311+
assert!(value.as_array().is_some_and(|items| {
312+
items
313+
.iter()
314+
.any(|item| item["code"] == "RSR013" && item["risk"] == "guarantee")
315+
}));
316+
}
317+
272318
#[test]
273319
fn review_reports_type_layout_changes() {
274320
let old_source = r#"

0 commit comments

Comments
 (0)