From 853428f273d49ea011b87f645bb5f4b6c2438190 Mon Sep 17 00:00:00 2001 From: Zack Maril Date: Tue, 25 Aug 2026 11:19:50 +0000 Subject: [PATCH 1/6] Recognize an all-different check, and say what replacing it costs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit No library checks distinctness by comparing every pair. `itertools::all_unique` reaches the answer through a hash set, so derivation produces a hash-set form and the quadratic loop it exists to replace matches nothing. The shape has to be named directly, the way the strum recognizers name a shape a derive macro would have produced — but over the normalized form rather than the syntax tree, which is what lets one recognizer cover spellings that share no syntax. The shape: a walk over each pair of one sequence that leaves with a constant on finding an equal pair, in a body yielding the opposite constant otherwise. It refuses a relation other than equality (`a < b` over every pair is a sortedness check), a walk that escapes with a computed value, and two arms naming one constant. `Condition` joins the match fact. A match says the code computes what the API computes; it does not say the two are interchangeable, and here they are not. Reaching the answer through a hash set needs `Eq + Hash` where the loop needs only `PartialEq` — not pedantry: `f64` is `PartialEq` and not `Eq`, two `NaN`s are unequal to each other, so the loop calls them distinct and the set cannot be built. It allocates where the loop does not, it need not run the comparison at all where the loop runs it quadratically, and at four elements the loop wins. Where the gap is visible instead of merely possible, the recognizer refuses: a `const fn` has no allocator, and a reader should not be handed a finding they must then reject. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_017ghYbUJFcVfkKYacbTVJsR --- crates/infact-core/src/lib.rs | 63 +++++ crates/infact-rust-behaviors/src/idioms.rs | 314 +++++++++++++++++++++ crates/infact-rust-behaviors/src/lib.rs | 78 +++++ crates/infact-rust-normalize/src/lib.rs | 19 ++ crates/infact-ts-behaviors/src/lib.rs | 1 + 5 files changed, 475 insertions(+) create mode 100644 crates/infact-rust-behaviors/src/idioms.rs diff --git a/crates/infact-core/src/lib.rs b/crates/infact-core/src/lib.rs index 273a127..92f0f83 100644 --- a/crates/infact-core/src/lib.rs +++ b/crates/infact-core/src/lib.rs @@ -397,6 +397,69 @@ pub struct LibraryBehaviorMatch { /// something else is interleaved with it. #[serde(default)] pub fused: bool, + /// What has to hold for the swap to be sound, that this cannot check. + /// + /// A match says the code computes what the API computes. It does not say + /// the two are interchangeable here, and for some behaviors they are not: + /// the API may need a stronger bound on the element type, or allocate where + /// the code does not, or reach the same answer by a different route that a + /// caller could tell apart. None of that is in the syntax. + /// + /// Reporting the gap is what makes this a recommendation rather than a + /// lint. Where the gap IS visible — a `const fn` that cannot allocate at + /// all — the recognizer refuses instead, because a condition a reader must + /// check is worse than a finding they never see. + #[serde(default, skip_serializing_if = "Vec::is_empty")] + pub conditions: Vec, +} + +/// Something a recommendation depends on that the syntax does not settle. +#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash, Serialize, Deserialize)] +#[serde(rename_all = "kebab-case")] +pub enum Condition { + /// The API needs a bound on the element type that the code does not. + /// + /// A pairwise `==` needs only `PartialEq`; reaching the same answer through + /// a hash set needs `Eq + Hash`. The difference is not pedantic: `f64` is + /// `PartialEq` and not `Eq`, and two `NaN`s are unequal to each other, so + /// the loop calls them distinct and the set cannot be built at all. + ElementBound { + requires: String, + code_requires: String, + }, + /// The API allocates where the code does not. + Allocates, + /// The API reaches the answer without making the comparisons the code makes. + /// + /// An operator with an observable effect — one that logs, counts, or panics + /// — runs a quadratic number of times here and need not run at all there. + ComparisonObservable, + /// The code is cheaper at the sizes it is actually called with. + /// + /// A quadratic scan of four elements beats allocating a hash set. Which one + /// this is depends on the caller, and nothing in the callee says. + SmallInputsFavourTheCode, +} + +impl std::fmt::Display for Condition { + fn fmt(&self, formatter: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + Self::ElementBound { + requires, + code_requires, + } => write!( + formatter, + "the element type must be {requires}; the code needs only {code_requires}" + ), + Self::Allocates => formatter.write_str("the API allocates and the code does not"), + Self::ComparisonObservable => formatter.write_str( + "a comparison with an observable effect runs here and need not run there", + ), + Self::SmallInputsFavourTheCode => { + formatter.write_str("the code is faster at small sizes") + } + } + } } #[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash, Serialize, Deserialize)] diff --git a/crates/infact-rust-behaviors/src/idioms.rs b/crates/infact-rust-behaviors/src/idioms.rs new file mode 100644 index 0000000..ad2ee43 --- /dev/null +++ b/crates/infact-rust-behaviors/src/idioms.rs @@ -0,0 +1,314 @@ +//! Recognizing an algorithm written out, and what it would take to replace it. +//! +//! Derivation matches repository code against a form derived from a library's +//! own implementation, which works whenever the library wrote the thing the way +//! a caller would. Sometimes it did not. No library checks that a collection's +//! elements are distinct by comparing every pair; `itertools::all_unique` +//! reaches the same answer through a hash set, so its derived form is a hash +//! set and the quadratic loop it exists to replace matches nothing. +//! +//! What is left is a shape that has to be named directly. That is what the +//! strum recognizers already do — `enum_shapes` states a shape a derive macro +//! would have produced and counts what a query cannot. This states its shapes +//! over the normalized form instead of the syntax tree, which is what lets one +//! recognizer cover spellings that share no syntax. +//! +//! The recognizers here are deliberately narrow. A false positive costs more +//! than a miss: a recommendation that is wrong one time in ten is a +//! recommendation that gets switched off, and every shape below refuses +//! anything it cannot account for. + +use infact_core::{Condition, Form, Pattern}; + +/// Why a candidate yielded no recommendation. +/// +/// Separate from [`infact_behaviors::Refusal`], which says why a *library* +/// callable yielded no behavior. These say why code that looked like an idiom +/// is not one, or is one that must not be recommended against, and they are +/// worth counting apart: the first bounds what the recognizer can see and the +/// second bounds what it is willing to say. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum IdiomRefusal { + /// The form does not have the shape at all. + NotThisShape, + /// The walk decides something, but not by comparing the pair it is given. + /// + /// A pairwise walk that tests a relation other than equality is asking a + /// different question: `a < b` over every pair is a sortedness check, and + /// recommending a distinctness API for it would be wrong. + DecidesSomethingElse, + /// The walk computes rather than decides. + /// + /// Escaping with a value built from the elements means the pairs are being + /// used for their content, not merely for whether a duplicate exists. + EscapesWithAValue, + /// The code cannot call an allocating API. + /// + /// A `const fn` has no allocator. This is the one condition below that is + /// visible in the syntax, so it is refused rather than reported: a reader + /// should not be handed a recommendation they must then reject. + CannotAllocate, +} + +/// An algorithm recognized in written-out form. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum Idiom { + /// Deciding that no two elements of one sequence are equal. + AllDifferent, +} + +impl Idiom { + /// The API this recommends, as a path into the package that offers it. + pub const fn callable_path(&self) -> (&'static str, &'static str) { + match self { + Self::AllDifferent => ("itertools", "itertools::Itertools::all_unique"), + } + } + + /// What has to hold for the recommendation to be sound. + /// + /// Fixed per idiom rather than per finding, because these are properties of + /// the two implementations rather than of the code that was matched. They + /// are stated in full even when several will usually be satisfied: a reader + /// who can dismiss three of them in a second is better served than one who + /// is not told about the fourth. + pub fn conditions(&self) -> Vec { + match self { + Self::AllDifferent => vec![ + Condition::ElementBound { + requires: "Eq + Hash".to_owned(), + code_requires: "PartialEq".to_owned(), + }, + Condition::Allocates, + Condition::ComparisonObservable, + Condition::SmallInputsFavourTheCode, + ], + } + } +} + +/// Whether a function may allocate. +/// +/// The recognizer is handed this rather than reading it, because whether a +/// language has such a context at all is a frontend question. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct Context { + pub can_allocate: bool, +} + +/// Recognize an all-different check in a normalized function body. +/// +/// The shape is a walk over each pair of one sequence that leaves with one +/// constant on finding an equal pair, in a body that yields the opposite +/// constant otherwise. `Pairwise` is what makes this one shape rather than +/// several: the index and iterator spellings of the walk have already met by +/// the time this runs. +/// +/// Returns the sequence the check is over, which is what a caller would put the +/// recommended call on. +pub fn all_different(form: &Form, context: Context) -> Result<&Form, IdiomRefusal> { + let (sequence, escaped, otherwise) = decisive_pairwise(form)?; + // Two arms of one decision that name the same constant decide nothing. + if escaped == otherwise { + return Err(IdiomRefusal::NotThisShape); + } + if !context.can_allocate { + return Err(IdiomRefusal::CannotAllocate); + } + Ok(sequence) +} + +/// A pairwise walk that leaves with a constant, and the constant after it. +/// +/// The walk and the value the body ends with are one claim: a walk that escapes +/// with `false` inside a body ending in `true` decides a question about every +/// pair, and the same walk followed by anything else is doing something this +/// cannot name. +fn decisive_pairwise(form: &Form) -> Result<(&Form, &str, &str), IdiomRefusal> { + // Why the nearest thing to the shape was not it. A walk over pairs that + // decides the wrong question is worth saying so about; not finding a walk + // at all is the uninformative answer, so anything else outranks it. + let mut refusal = IdiomRefusal::NotThisShape; + if let Form::Sequence(steps) = form { + // The walk need not be the first step: a function may bind or check + // things before it. It must be immediately before the value, because + // anything between them could change what is returned. + for pair in steps.windows(2) { + let [walk, Form::Constant(otherwise)] = pair else { + continue; + }; + match escaping_pairwise(walk) { + Ok((sequence, escaped)) => return Ok((sequence, escaped, otherwise)), + Err(IdiomRefusal::NotThisShape) => {} + Err(specific) => refusal = specific, + } + } + } + for child in form.children() { + match decisive_pairwise(child) { + Ok(found) => return Ok(found), + Err(IdiomRefusal::NotThisShape) => {} + Err(specific) => refusal = specific, + } + } + Err(refusal) +} + +/// A walk over pairs that leaves as soon as two are equal. +fn escaping_pairwise(form: &Form) -> Result<(&Form, &str), IdiomRefusal> { + let Form::Pairwise { + sequence, + left, + right, + body, + } = form + else { + return Err(IdiomRefusal::NotThisShape); + }; + let (Pattern::Binding(left), Pattern::Binding(right)) = (left.as_ref(), right.as_ref()) else { + return Err(IdiomRefusal::NotThisShape); + }; + // A test with an `else` is choosing between two things to do, and only one + // of them is being described here. + let Form::Branch { + condition, + consequence, + alternative: None, + } = body.as_ref() + else { + return Err(IdiomRefusal::NotThisShape); + }; + if !compares_the_pair(condition, *left, *right) { + return Err(IdiomRefusal::DecidesSomethingElse); + } + let Form::Return(escaped) = consequence.as_ref() else { + return Err(IdiomRefusal::NotThisShape); + }; + let Form::Constant(escaped) = escaped.as_ref() else { + return Err(IdiomRefusal::EscapesWithAValue); + }; + Ok((sequence.as_ref(), escaped)) +} + +/// Whether a test asks whether the two elements of a pair are equal. +/// +/// Equality only. `<` over every pair is a sortedness check and `!=` is the +/// opposite question, and both would be told to use a distinctness API by a +/// test that merely looked for a comparison. +fn compares_the_pair(condition: &Form, left: u32, right: u32) -> bool { + let Form::Binary { + operator, + left: first, + right: second, + } = condition + else { + return false; + }; + if operator != "==" { + return false; + } + let named = |form: &Form, index: u32| *form == Form::Local(index); + (named(first, left) && named(second, right)) || (named(first, right) && named(second, left)) +} + +#[cfg(test)] +mod tests { + use super::*; + + fn allowed() -> Context { + Context { can_allocate: true } + } + + fn pairwise(body: Form) -> Form { + Form::Sequence(vec![ + Form::Pairwise { + sequence: Box::new(Form::Free(0)), + left: Box::new(Pattern::Binding(0)), + right: Box::new(Pattern::Binding(1)), + body: Box::new(body), + }, + Form::Constant("true".to_owned()), + ]) + } + + fn escaping(condition: Form, escaped: Form) -> Form { + Form::Branch { + condition: Box::new(condition), + consequence: Box::new(Form::Return(Box::new(escaped))), + alternative: None, + } + } + + fn equal_pair() -> Form { + Form::Binary { + operator: "==".to_owned(), + left: Box::new(Form::Local(0)), + right: Box::new(Form::Local(1)), + } + } + + #[test] + fn a_pairwise_equality_check_is_all_different() { + let form = pairwise(escaping(equal_pair(), Form::Constant("false".to_owned()))); + assert_eq!(all_different(&form, allowed()), Ok(&Form::Free(0))); + } + + /// A relation other than equality asks a different question. + #[test] + fn a_pairwise_ordering_check_is_not_all_different() { + let ordered = Form::Binary { + operator: "<".to_owned(), + left: Box::new(Form::Local(0)), + right: Box::new(Form::Local(1)), + }; + let form = pairwise(escaping(ordered, Form::Constant("false".to_owned()))); + assert_eq!( + all_different(&form, allowed()), + Err(IdiomRefusal::DecidesSomethingElse) + ); + } + + /// Leaving with a computed value means the pairs are being used, not counted. + #[test] + fn a_walk_that_escapes_with_a_value_is_refused() { + let form = pairwise(escaping(equal_pair(), Form::Local(0))); + assert_eq!( + all_different(&form, allowed()), + Err(IdiomRefusal::EscapesWithAValue) + ); + } + + /// Both arms naming one constant decides nothing. + #[test] + fn a_walk_that_yields_what_it_escapes_with_is_refused() { + let form = pairwise(escaping(equal_pair(), Form::Constant("true".to_owned()))); + assert_eq!( + all_different(&form, allowed()), + Err(IdiomRefusal::NotThisShape) + ); + } + + /// A context with no allocator is told nothing rather than told wrongly. + #[test] + fn a_context_that_cannot_allocate_is_refused() { + let form = pairwise(escaping(equal_pair(), Form::Constant("false".to_owned()))); + let context = Context { + can_allocate: false, + }; + assert_eq!( + all_different(&form, context), + Err(IdiomRefusal::CannotAllocate) + ); + } + + /// Every recommendation states what it depends on. + #[test] + fn the_recommendation_carries_its_conditions() { + let conditions = Idiom::AllDifferent.conditions(); + assert!(conditions.contains(&Condition::Allocates)); + assert!(conditions.iter().any(|condition| matches!( + condition, + Condition::ElementBound { requires, .. } if requires == "Eq + Hash" + ))); + } +} diff --git a/crates/infact-rust-behaviors/src/lib.rs b/crates/infact-rust-behaviors/src/lib.rs index 27467b2..94d1e61 100644 --- a/crates/infact-rust-behaviors/src/lib.rs +++ b/crates/infact-rust-behaviors/src/lib.rs @@ -1,6 +1,7 @@ //! Facts matching Rust code behavior to external library APIs. mod derivation; +mod idioms; mod macro_derivation; mod pack; @@ -21,6 +22,7 @@ use tree_sitter::Node; pub use derivation::{ DerivedLibrary, derive_behavior, derive_catalog, derive_library, is_comparable, is_reportable, }; +pub use idioms::{Context, Idiom, IdiomRefusal, all_different}; pub use macro_derivation::{MacroDerivationRequest, derive_macro_behavior}; pub use pack::{ BuiltLibraryPack, LibraryPackRequest, behavior_file_name, build_library_pack, registry_sources, @@ -199,6 +201,10 @@ pub fn analyze_repository( // is put in the same one. Locating stays on the form as written, // because that is what the spans were taken from. let candidate = function.form.simplify(); + // An idiom is recognized directly rather than derived, because no + // library writes the thing it replaces. Same fact, same evidence, + // different route to the shape. + collect_idiom_matches(file, &function, &candidate, catalogs, &mut matches)?; // Behaviors that share a form are indistinguishable here by // construction, so all of them are kept and reported together. let mut best: BTreeMap<&Form, (Vec<&&DerivedLibraryBehavior>, bool)> = BTreeMap::new(); @@ -409,6 +415,7 @@ fn behavior_match( alternatives, span, fused, + conditions: Vec::new(), }, derivation: Derivation { analyzer: "rust.library-behaviors".to_owned(), @@ -735,6 +742,76 @@ fn collect_enum_macro_matches( Ok(()) } +/// Report the algorithms recognized in one normalized function. +/// +/// An idiom names the API it recommends by path, so it can only be reported +/// when a catalog for that package is loaded — the same rule derived behaviors +/// follow, and for the same reason: naming a version that was never read would +/// be inventing provenance. +fn collect_idiom_matches( + file: &ParsedFile, + function: &infact_rust_normalize::NormalizedFunction, + candidate: &Form, + catalogs: &[ExternalCatalog], + output: &mut BTreeSet>, +) -> Result<()> { + let context = idioms::Context { + can_allocate: !function.is_const, + }; + if idioms::all_different(candidate, context).is_err() { + return Ok(()); + } + let idiom = idioms::Idiom::AllDifferent; + let (package, path) = idiom.callable_path(); + let Some(catalog) = catalogs.iter().find(|catalog| { + catalog.package == package + && catalog + .callables + .iter() + .any(|callable| callable.path == path) + }) else { + return Ok(()); + }; + output.insert(Fact { + value: LibraryBehaviorMatch { + target: LibraryTarget::Callable { + package: catalog.package.clone(), + version: catalog.version.clone(), + path: path.to_owned(), + catalog_sha256: catalog.source_sha256.clone(), + }, + // The recognizer names one API, so there is nothing to choose + // between; what is uncertain about the recommendation is in the + // conditions rather than in which callable it points at. + alternatives: Vec::new(), + fused: false, + span: SourceSpan { + path: file.path.clone(), + start_byte: Some(function.start_byte), + end_byte: Some(function.end_byte), + start_line: function.start_line, + end_line: function.end_line, + start_column: None, + end_column: None, + }, + conditions: idiom.conditions(), + }, + derivation: Derivation { + analyzer: "rust.idioms".to_owned(), + analyzer_version: env!("CARGO_PKG_VERSION").to_owned(), + inputs: vec![InputEvidence { + path: file.path.clone(), + content_sha256: file.provenance.source_sha256.clone(), + parser_id: file.provenance.parser_id.clone(), + parser_version: file.provenance.parser_version.clone(), + grammar_sha256: file.provenance.grammar_sha256.clone(), + queries_sha256: file.provenance.queries_sha256.clone(), + }], + }, + }); + Ok(()) +} + fn mapping_is_exhaustive(mappings: &BTreeMap, variants: &[String]) -> bool { mappings.keys().cloned().collect::>() == variants.iter().cloned().collect::>() @@ -783,6 +860,7 @@ fn macro_behavior_match( // a derive names exactly one macro alternatives: Vec::new(), fused: false, + conditions: Vec::new(), span: SourceSpan { path: file.path.clone(), start_byte: Some(start_byte), diff --git a/crates/infact-rust-normalize/src/lib.rs b/crates/infact-rust-normalize/src/lib.rs index cde8d29..b96f43d 100644 --- a/crates/infact-rust-normalize/src/lib.rs +++ b/crates/infact-rust-normalize/src/lib.rs @@ -100,6 +100,13 @@ pub struct NormalizedFunction { /// steps of `form` when `form` is a sequence. This is what lets a match be /// reported against statements rather than the whole function. pub statements: Vec, + /// Whether the body runs at compile time, where there is no allocator. + /// + /// Not part of the form: what a function computes is the same whether or + /// not it is `const`. It is recorded beside it because a recommendation to + /// reach for a collection is wrong here however right the match is, and a + /// reader should not be shown a finding they must then reject. + pub is_const: bool, } /// The source extent of one statement. @@ -1064,6 +1071,17 @@ pub fn normalize_body(body: Node<'_>, source: &[u8]) -> Form { normalizer.block(body) } +/// Whether a function is declared `const`. +/// +/// The modifier is an anonymous token before the `fn`, so it is read off the +/// children rather than a field. +fn is_const_function(node: Node<'_>, source: &[u8]) -> bool { + let mut cursor = node.walk(); + node.children(&mut cursor) + .take_while(|child| child.kind() != "fn") + .any(|child| !child.is_named() && text(child, source) == "const") +} + fn collect_functions<'a>(node: Node<'a>, output: &mut Vec>) { if node.kind() == "function_item" { output.push(node); @@ -1098,6 +1116,7 @@ pub fn normalize_file(file: &ParsedFile) -> Vec { .child_by_field_name("body") .map(statement_spans) .unwrap_or_default(), + is_const: is_const_function(node, &file.source), }) }) .collect() diff --git a/crates/infact-ts-behaviors/src/lib.rs b/crates/infact-ts-behaviors/src/lib.rs index ce280b5..6f3355d 100644 --- a/crates/infact-ts-behaviors/src/lib.rs +++ b/crates/infact-ts-behaviors/src/lib.rs @@ -355,6 +355,7 @@ fn behavior_match( alternatives, span, fused, + conditions: Vec::new(), }, derivation: Derivation { analyzer: "typescript.library-behaviors".to_owned(), From ac743887dd2022903f58d0ba74025e175c0d2e3e Mon Sep 17 00:00:00 2001 From: Zack Maril Date: Tue, 25 Aug 2026 11:56:00 +0000 Subject: [PATCH 2/6] Measure on CodeNet, and let the corpus correct the shape MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Nine findings across 127,210 Rust files, every one verified by hand as a hand-rolled distinctness check. Zero across 3,650 files of production Rust, where the idiom is written with a HashSet already. The first run found zero, and the two things that were wrong are both in the code now. The bound. `whole_index_span` demanded `0..v.len()`, and pairwise loops in the corpus are bound by a bare variable six to one. The sequence is therefore read off the body — whatever it indexes at both positions, and it must be exactly one thing — and the bound decides the extent: `0..v.len()` walks `v`, and `0..n` walks `v[..n]`, which is the form the frontend already produces for that slice written out. The prefix is recorded rather than glossed, because a walk bounded by something other than the length does not cover the sequence, and saying it did would recommend an API over elements that were never read. An outer loop that stops one short of the inner one is the same walk and is now a row in the table. The reaction. The recognizer wanted `return false` inside a body yielding `true`; what the corpus writes is `println!("no"); return;` and `ans = false`. So the test is no longer the shape of the escape but what it depends on: a reaction that does not read either element records only that a duplicate exists, which is exactly what the recommended API answers. What the code then does with that answer is the caller's business. This is what keeps the refusals honest rather than loosening them. `continue` is the commonest thing a pairwise equality test does — six times more common than every accepted spelling combined — and it skips duplicate pairs inside a larger computation. `count += 1` counts them. Neither records only existence, and neither is accepted. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_017ghYbUJFcVfkKYacbTVJsR --- crates/infact-normalize/src/lib.rs | 46 +++++ crates/infact-normalize/src/simplify.rs | 116 +++++++---- crates/infact-rust-behaviors/src/idioms.rs | 189 +++++++++++++----- .../infact-rust-normalize/tests/normalize.rs | 134 +++++++++++++ 4 files changed, 400 insertions(+), 85 deletions(-) diff --git a/crates/infact-normalize/src/lib.rs b/crates/infact-normalize/src/lib.rs index b6feb1b..ee99b7c 100644 --- a/crates/infact-normalize/src/lib.rs +++ b/crates/infact-normalize/src/lib.rs @@ -1025,6 +1025,52 @@ impl Form { && self.is_comparable() } + /// The one sequence a body indexes at every named position. + /// + /// A loop bound is often a variable rather than the sequence's own length — + /// `for i in 0..n` far more often than `for i in 0..v.len()` — so the span + /// does not always say what is being walked. The body does: whatever it + /// reads at those positions is the sequence, and it has to be exactly one + /// of them, or the loop is walking positions into two things at once and is + /// not a walk over either. + fn sole_indexed_sequence(&self, positions: &[u32]) -> Option<&Self> { + let mut sequence = None; + let mut seen = Vec::new(); + self.collect_indexed(positions, &mut sequence, &mut seen)?; + positions + .iter() + .all(|position| seen.contains(position)) + .then_some(sequence)? + } + + /// Gather the sequence indexed at each position, failing on disagreement. + fn collect_indexed<'a>( + &'a self, + positions: &[u32], + sequence: &mut Option<&'a Self>, + seen: &mut Vec, + ) -> Option<()> { + if let Self::Index { + sequence: indexed, + position, + } = self + && let Self::Local(index) = position.as_ref() + && positions.contains(index) + { + if sequence.is_some_and(|found| found != indexed.as_ref()) { + return None; + } + *sequence = Some(indexed.as_ref()); + if !seen.contains(index) { + seen.push(*index); + } + } + for child in self.children() { + child.collect_indexed(positions, sequence, seen)?; + } + Some(()) + } + /// Whether a body reads a sequence only by indexing it at named positions. /// /// This is the licence to forget the index. `for i in 0..v.len()` visits diff --git a/crates/infact-normalize/src/simplify.rs b/crates/infact-normalize/src/simplify.rs index 16bc5a1..42700e1 100644 --- a/crates/infact-normalize/src/simplify.rs +++ b/crates/infact-normalize/src/simplify.rs @@ -423,14 +423,16 @@ impl Form { let Pattern::Binding(index) = item.as_ref() else { return None; }; - let source = whole_index_span(sequence)?; - if !body.indexed_only(source, &[*index]) || body.writes_indexed(source) { + let bound = span_from_zero(sequence)?; + let positions = [*index]; + let source = body.sole_indexed_sequence(&positions)?; + if !body.indexed_only(source, &positions) || body.writes_indexed(source) { return None; } Some(Self::Traverse { - sequence: Box::new(source.clone()), + sequence: Box::new(walked_sequence(bound, source)), item: item.clone(), - body: Box::new(body.with_indexed_elements(source, &[*index])), + body: Box::new(body.with_indexed_elements(source, &positions)), direction: *direction, }) } @@ -485,19 +487,18 @@ impl Form { second: &Pattern, body: &Self, ) -> Option { - let source = whole_index_span(outer)?; let (Pattern::Binding(left), Pattern::Binding(right)) = (first, second) else { return None; }; - if !covers_each_pair_once(inner, *left, source) { - return None; - } + let bound = span_from_zero(outer)?; + let extent = pairwise_extent(bound, inner, *left)?; let positions = [*left, *right]; + let source = body.sole_indexed_sequence(&positions)?; if !body.indexed_only(source, &positions) || body.writes_indexed(source) { return None; } Some(Self::Pairwise { - sequence: Box::new(source.clone()), + sequence: Box::new(walked_sequence(extent, source)), left: Box::new(first.clone()), right: Box::new(second.clone()), body: Box::new(body.with_indexed_elements(source, &positions)), @@ -809,12 +810,11 @@ fn unfoldable(bindings: &[(u32, Form)]) -> Vec<(u32, Form)> { } } -/// The sequence whose whole index range a span walks. +/// The bound of a span that counts up from zero. /// -/// `0..v.len()` reaches every position of `v` and nothing else. An inclusive -/// bound reaches one position past the end, which is a different walk and in -/// Rust a panicking one, so it is not this. -fn whole_index_span(form: &Form) -> Option<&Form> { +/// An inclusive bound reaches one position past the end, which is a different +/// walk and in Rust a panicking one, so it is not this. +fn span_from_zero(form: &Form) -> Option<&Form> { let Form::Span { start, end, @@ -823,43 +823,81 @@ fn whole_index_span(form: &Form) -> Option<&Form> { else { return None; }; - if *inclusive || **start != Form::Number("0".to_owned()) { - return None; - } - let Form::Method { + (!*inclusive && **start == Form::Number("0".to_owned())).then(|| end.as_ref()) +} + +/// The sequence a loop counting to `bound` actually walks. +/// +/// `0..v.len()` walks `v` itself. `0..n` walks its first `n` elements, which is +/// the slice `v[..n]` — and that is the form the frontend already produces for +/// that slice written out, so a loop over a prefix and a slice of the same +/// extent agree. Recording it this way is what keeps the form honest: a walk +/// bounded by something other than the length does not cover the sequence, and +/// saying it did would recommend an API over elements that were never read. +/// +/// Measured, this is most of the corpus rather than an edge: across CodeNet's +/// Rust submissions, pairwise loops bound by a bare variable outnumber those +/// bound by `len()` six to one. +fn walked_sequence(bound: &Form, sequence: &Form) -> Form { + if let Form::Method { name, receiver, arguments, - } = end.as_ref() - else { - return None; - }; - (name == "len" && arguments.is_empty()).then(|| receiver.as_ref()) + } = bound + && name == "len" + && arguments.is_empty() + && receiver.as_ref() == sequence + { + return sequence.clone(); + } + Form::Index { + sequence: Box::new(sequence.clone()), + position: Box::new(Form::Span { + start: Box::new(Form::Number("0".to_owned())), + end: Box::new(bound.clone()), + inclusive: false, + }), + } } -/// Whether an inner span visits each pair of a sequence exactly once. +/// How far a pair of nested spans reads, when they reach each pair once. +/// +/// Two inner spans do it, and they are the two triangles of the index square: +/// `i + 1 .. bound` takes every position after the outer one, and `0 .. i` +/// every position before it. Either way each unordered pair is reached once. /// -/// Two spans do it, and they are the two triangles of the index square: -/// `i + 1 .. len` takes every position after the outer one, and `0 .. i` every -/// position before it. Either way each unordered pair is reached once. A table -/// rather than arithmetic — a bound this cannot read is a bound this refuses. -fn covers_each_pair_once(span: &Form, outer: u32, sequence: &Form) -> bool { +/// The upper triangle admits an outer loop that stops one short. `0..n - 1` +/// with `i + 1..n` reaches exactly the pairs `0..n` with `i + 1..n` does, +/// because the last position has nothing above it to pair with — and it is +/// what a third of the checks measured in the corpus were written as. The +/// extent is then the inner bound rather than the outer, because the inner +/// bound is the one that says how far the sequence is actually read. +/// +/// A table rather than arithmetic: a bound this cannot read is one it refuses. +fn pairwise_extent<'a>(outer_bound: &'a Form, inner: &'a Form, outer: u32) -> Option<&'a Form> { let Form::Span { start, end, inclusive: false, - } = span + } = inner else { - return false; + return None; }; - let above = is_successor_of(start, outer) - && matches!(whole_index_span(&Form::Span { - start: Box::new(Form::Number("0".to_owned())), - end: end.clone(), - inclusive: false, - }), Some(walked) if walked == sequence); - let below = **start == Form::Number("0".to_owned()) && **end == Form::Local(outer); - above || below + if is_successor_of(start, outer) { + let matches_outer = end.as_ref() == outer_bound || is_predecessor_of(outer_bound, end); + return matches_outer.then(|| end.as_ref()); + } + // The lower triangle allows no such slack: an outer loop that stopped one + // short would never pair the last position with anything. + (**start == Form::Number("0".to_owned()) && **end == Form::Local(outer)).then_some(outer_bound) +} + +/// Whether a form is one less than another. +fn is_predecessor_of(form: &Form, of: &Form) -> bool { + matches!(form, Form::Binary { operator, left, right } + if operator == "-" + && left.as_ref() == of + && **right == Form::Number("1".to_owned())) } /// Whether a form is one more than a named position. diff --git a/crates/infact-rust-behaviors/src/idioms.rs b/crates/infact-rust-behaviors/src/idioms.rs index ad2ee43..10aa57d 100644 --- a/crates/infact-rust-behaviors/src/idioms.rs +++ b/crates/infact-rust-behaviors/src/idioms.rs @@ -98,55 +98,40 @@ pub struct Context { /// Recognize an all-different check in a normalized function body. /// -/// The shape is a walk over each pair of one sequence that leaves with one -/// constant on finding an equal pair, in a body that yields the opposite -/// constant otherwise. `Pairwise` is what makes this one shape rather than -/// several: the index and iterator spellings of the walk have already met by -/// the time this runs. +/// The shape is a walk over each pair of one sequence that, on finding two +/// equal elements, does something that does not depend on WHICH pair it found. +/// That is the whole claim: a walk like that computes exactly whether a +/// duplicate exists, which is what the recommended API answers. What the code +/// then does with the answer — return it, print it, set a flag — is the +/// caller's business and does not change what the loop computed. +/// +/// `Pairwise` is what makes this one shape rather than several: the index and +/// iterator spellings of the walk have already met by the time this runs. /// /// Returns the sequence the check is over, which is what a caller would put the /// recommended call on. pub fn all_different(form: &Form, context: Context) -> Result<&Form, IdiomRefusal> { - let (sequence, escaped, otherwise) = decisive_pairwise(form)?; - // Two arms of one decision that name the same constant decide nothing. - if escaped == otherwise { - return Err(IdiomRefusal::NotThisShape); - } + let sequence = deciding_pairwise(form)?; if !context.can_allocate { return Err(IdiomRefusal::CannotAllocate); } Ok(sequence) } -/// A pairwise walk that leaves with a constant, and the constant after it. -/// -/// The walk and the value the body ends with are one claim: a walk that escapes -/// with `false` inside a body ending in `true` decides a question about every -/// pair, and the same walk followed by anything else is doing something this -/// cannot name. -fn decisive_pairwise(form: &Form) -> Result<(&Form, &str, &str), IdiomRefusal> { +/// Search a form for a pairwise walk that decides distinctness. +fn deciding_pairwise(form: &Form) -> Result<&Form, IdiomRefusal> { // Why the nearest thing to the shape was not it. A walk over pairs that // decides the wrong question is worth saying so about; not finding a walk // at all is the uninformative answer, so anything else outranks it. let mut refusal = IdiomRefusal::NotThisShape; - if let Form::Sequence(steps) = form { - // The walk need not be the first step: a function may bind or check - // things before it. It must be immediately before the value, because - // anything between them could change what is returned. - for pair in steps.windows(2) { - let [walk, Form::Constant(otherwise)] = pair else { - continue; - }; - match escaping_pairwise(walk) { - Ok((sequence, escaped)) => return Ok((sequence, escaped, otherwise)), - Err(IdiomRefusal::NotThisShape) => {} - Err(specific) => refusal = specific, - } - } + match distinctness_walk(form) { + Ok(sequence) => return Ok(sequence), + Err(IdiomRefusal::NotThisShape) => {} + Err(specific) => refusal = specific, } for child in form.children() { - match decisive_pairwise(child) { - Ok(found) => return Ok(found), + match deciding_pairwise(child) { + Ok(sequence) => return Ok(sequence), Err(IdiomRefusal::NotThisShape) => {} Err(specific) => refusal = specific, } @@ -154,8 +139,9 @@ fn decisive_pairwise(form: &Form) -> Result<(&Form, &str, &str), IdiomRefusal> { Err(refusal) } -/// A walk over pairs that leaves as soon as two are equal. -fn escaping_pairwise(form: &Form) -> Result<(&Form, &str), IdiomRefusal> { +/// A walk over pairs whose only reaction to an equal pair is to record that one +/// exists. +fn distinctness_walk(form: &Form) -> Result<&Form, IdiomRefusal> { let Form::Pairwise { sequence, left, @@ -181,13 +167,58 @@ fn escaping_pairwise(form: &Form) -> Result<(&Form, &str), IdiomRefusal> { if !compares_the_pair(condition, *left, *right) { return Err(IdiomRefusal::DecidesSomethingElse); } - let Form::Return(escaped) = consequence.as_ref() else { - return Err(IdiomRefusal::NotThisShape); - }; - let Form::Constant(escaped) = escaped.as_ref() else { + // Reading either element means the pair is being used for its content, and + // an API that answers only whether a duplicate exists cannot supply that. + if consequence.references_local(*left) || consequence.references_local(*right) { return Err(IdiomRefusal::EscapesWithAValue); - }; - Ok((sequence.as_ref(), escaped)) + } + if !records_that_one_exists(consequence) { + return Err(IdiomRefusal::NotThisShape); + } + Ok(sequence.as_ref()) +} + +/// Whether a reaction to an equal pair records only that one was found. +/// +/// Two spellings, and between them they are how the check is written. Leaving +/// the function ends the walk, so nothing after it runs and the duplicate has +/// decided the outcome. Assigning a constant to a name from outside sets the +/// flag that is read afterwards. +/// +/// `continue` and `break` are neither, and refusing them is most of what keeps +/// this honest: measured across CodeNet, `continue` is the commonest thing a +/// pairwise equality test does by a factor of six, and it is skipping duplicate +/// pairs inside a larger computation rather than testing for them. `count += 1` +/// is refused for the same reason — it counts duplicates, and knowing one +/// exists does not give you how many. +fn records_that_one_exists(consequence: &Form) -> bool { + match consequence { + Form::Return(_) => true, + // A constant is a flag. A value built from what is already there is an + // accumulation, which is a different question about the same pairs. + Form::Assign { + operator, + target, + value, + } => { + operator == "=" + && matches!(target.as_ref(), Form::Local(_) | Form::Free(_)) + && matches!(value.as_ref(), Form::Constant(_)) + } + // `println!("no"); return;` is one reaction written as two steps, and + // it is the spelling the corpus actually uses. Only the last step may + // leave, because a `return` reached in the middle would make the steps + // after it dead rather than part of the reaction. + Form::Sequence(steps) => steps.split_last().is_some_and(|(last, rest)| { + records_that_one_exists(last) && !rest.iter().any(leaves_the_walk) + }), + _ => false, + } +} + +/// Whether a step can end the walk from somewhere other than its end. +fn leaves_the_walk(form: &Form) -> bool { + matches!(form, Form::Return(_)) || form.children().into_iter().any(leaves_the_walk) } /// Whether a test asks whether the two elements of a pair are equal. @@ -268,9 +299,12 @@ mod tests { ); } - /// Leaving with a computed value means the pairs are being used, not counted. + /// Leaving with one of the elements means the pair is being used. + /// + /// Knowing that a duplicate exists does not tell you what it was, so an API + /// that answers only the first question cannot stand in here. #[test] - fn a_walk_that_escapes_with_a_value_is_refused() { + fn a_walk_that_escapes_with_an_element_is_refused() { let form = pairwise(escaping(equal_pair(), Form::Local(0))); assert_eq!( all_different(&form, allowed()), @@ -278,16 +312,79 @@ mod tests { ); } - /// Both arms naming one constant decides nothing. + /// Skipping a duplicate pair is not testing for one. + /// + /// The commonest thing a pairwise equality test does in real code, by a + /// wide margin, and it belongs to a larger computation rather than being + /// one. + #[test] + fn a_walk_that_continues_past_a_duplicate_is_refused() { + let form = pairwise(Form::Branch { + condition: Box::new(equal_pair()), + consequence: Box::new(Form::Opaque { + kind: "continue_expression".to_owned(), + parts: Vec::new(), + }), + alternative: None, + }); + assert_eq!( + all_different(&form, allowed()), + Err(IdiomRefusal::NotThisShape) + ); + } + + /// Counting duplicates asks how many, not whether. #[test] - fn a_walk_that_yields_what_it_escapes_with_is_refused() { - let form = pairwise(escaping(equal_pair(), Form::Constant("true".to_owned()))); + fn a_walk_that_counts_duplicates_is_refused() { + let form = pairwise(Form::Branch { + condition: Box::new(equal_pair()), + consequence: Box::new(Form::Assign { + operator: "+=".to_owned(), + target: Box::new(Form::Local(2)), + value: Box::new(Form::Number("1".to_owned())), + }), + alternative: None, + }); assert_eq!( all_different(&form, allowed()), Err(IdiomRefusal::NotThisShape) ); } + /// Setting a flag is the other way the check is written. + #[test] + fn a_walk_that_sets_a_flag_is_all_different() { + let form = pairwise(Form::Branch { + condition: Box::new(equal_pair()), + consequence: Box::new(Form::Assign { + operator: "=".to_owned(), + target: Box::new(Form::Local(2)), + value: Box::new(Form::Constant("false".to_owned())), + }), + alternative: None, + }); + assert_eq!(all_different(&form, allowed()), Ok(&Form::Free(0))); + } + + /// Reporting and leaving is one reaction written as two steps. + /// + /// This is the spelling CodeNet's submissions actually use. + #[test] + fn a_walk_that_reports_then_leaves_is_all_different() { + let form = pairwise(Form::Branch { + condition: Box::new(equal_pair()), + consequence: Box::new(Form::Sequence(vec![ + Form::Opaque { + kind: "macro:println".to_owned(), + parts: vec![Form::Constant("\"no\"".to_owned())], + }, + Form::Return(Box::new(Form::Literal)), + ])), + alternative: None, + }); + assert_eq!(all_different(&form, allowed()), Ok(&Form::Free(0))); + } + /// A context with no allocator is told nothing rather than told wrongly. #[test] fn a_context_that_cannot_allocate_is_refused() { diff --git a/crates/infact-rust-normalize/tests/normalize.rs b/crates/infact-rust-normalize/tests/normalize.rs index db46fe7..89aa93f 100644 --- a/crates/infact-rust-normalize/tests/normalize.rs +++ b/crates/infact-rust-normalize/tests/normalize.rs @@ -486,3 +486,137 @@ fn a_subtraction_chain_has_one_spelling() { ); assert_eq!(first, second); } + +/// A loop bounded by a variable walks a prefix, and the form says so. +/// +/// Measured on CodeNet, pairwise loops bound by a bare variable outnumber those +/// bound by `len()` six to one, so this is the common case rather than an edge. +/// The prefix is recorded because `0..n` does not reach past `n`, and claiming +/// it covered the sequence would recommend an API over elements never read. +#[test] +fn a_loop_bounded_by_a_variable_walks_a_prefix() { + let bounded = behavior_of( + "fn f(values: &[i32], n: usize) -> bool { + for i in 0..n { + for j in i + 1..n { + if values[i] == values[j] { return false; } + } + } + true + }", + "f", + ); + assert!(bounded.contains("(pairwise (index"), "{bounded}"); + let whole = behavior_of( + "fn f(values: &[i32]) -> bool { + for i in 0..values.len() { + for j in i + 1..values.len() { + if values[i] == values[j] { return false; } + } + } + true + }", + "f", + ); + assert!(whole.contains("(pairwise f"), "{whole}"); + assert_ne!(bounded, whole); +} + +/// Two loops bounded by different things are not a walk over pairs. +#[test] +fn loops_with_disagreeing_bounds_are_not_a_pairwise_walk() { + let form = behavior_of( + "fn f(values: &[i32], n: usize, m: usize) -> bool { + for i in 0..n { + for j in i + 1..m { + if values[i] == values[j] { return false; } + } + } + true + }", + "f", + ); + assert!(!form.contains("(pairwise"), "{form}"); +} + +/// Indexing two different sequences is a walk over neither. +#[test] +fn indexing_two_sequences_is_not_a_pairwise_walk() { + let form = behavior_of( + "fn f(left: &[i32], right: &[i32], n: usize) -> bool { + for i in 0..n { + for j in i + 1..n { + if left[i] == right[j] { return false; } + } + } + true + }", + "f", + ); + assert!(!form.contains("(pairwise"), "{form}"); +} + +/// An outer loop that stops one short reaches the same pairs. +/// +/// `0..n - 1` with `i + 1..n` pairs everything `0..n` would: the last position +/// has nothing above it. A third of the checks measured in CodeNet are written +/// this way. +#[test] +fn an_outer_loop_that_stops_one_short_is_still_a_pairwise_walk() { + let form = behavior_of( + "fn f(values: &[i32], n: usize) -> bool { + for i in 0..n - 1 { + for j in i + 1..n { + if values[i] == values[j] { return false; } + } + } + true + }", + "f", + ); + assert!(form.contains("(pairwise"), "{form}"); + // The extent is how far the sequence is read, which is the inner bound. + let whole = behavior_of( + "fn f(values: &[i32], n: usize) -> bool { + for i in 0..n { + for j in i + 1..n { + if values[i] == values[j] { return false; } + } + } + true + }", + "f", + ); + assert_eq!(form, whole); +} + +/// A lower-triangle inner loop gets no such slack. +/// +/// `0..n - 1` with `0..i` never pairs the last position with anything, so it is +/// not a walk over the pairs of `n` elements. +#[test] +fn a_short_outer_loop_with_a_lower_triangle_walks_a_shorter_prefix() { + let short = behavior_of( + "fn f(values: &[i32], n: usize) -> bool { + for i in 0..n - 1 { + for j in 0..i { + if values[i] == values[j] { return false; } + } + } + true + }", + "f", + ); + let whole = behavior_of( + "fn f(values: &[i32], n: usize) -> bool { + for i in 0..n { + for j in 0..i { + if values[i] == values[j] { return false; } + } + } + true + }", + "f", + ); + assert_ne!(short, whole); +} From 5319a4d9ab173f3f14593003a9ba3799e15d7bbe Mon Sep 17 00:00:00 2001 From: Zack Maril Date: Tue, 25 Aug 2026 12:05:17 +0000 Subject: [PATCH 3/6] Read the bound off the catalog rather than from memory MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The recognizer named `Eq + Hash` as what `all_unique` requires. That was right, and it was a claim about a version of a library that nothing had checked — which is the claim a reader is least placed to verify and exactly what the catalog exists to settle. It is now read from the shipped signature, where itertools 0.15.0 states `Self::Item: Eq + Hash` itself. The same signature now gates the match. AGENTS.md requires a matcher to verify the callable it relies on, and the idiom recognizer named a path without ever looking at what stood behind it. A path is not a promise: a catalog is generated data and an API can change between versions. So the recommendation is made only when the callable still takes a receiver and still returns `bool` — still answers the question the shape decides — and a catalog with no signature at all cannot be checked and is not used. `NORMALIZED_FORM_SCHEMA` goes to 2. Behavior packs are gated by their own envelope version and still deserialize, and both the round-trip test and a scan of the shipped programs agree none of them contain the constructs the new laws touch, so nothing needs regenerating. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_017ghYbUJFcVfkKYacbTVJsR --- crates/infact-normalize/src/lib.rs | 2 +- crates/infact-rust-behaviors/src/idioms.rs | 126 +++++++++++++++++++-- crates/infact-rust-behaviors/src/lib.rs | 21 ++-- 3 files changed, 128 insertions(+), 21 deletions(-) diff --git a/crates/infact-normalize/src/lib.rs b/crates/infact-normalize/src/lib.rs index ee99b7c..760d9cf 100644 --- a/crates/infact-normalize/src/lib.rs +++ b/crates/infact-normalize/src/lib.rs @@ -21,7 +21,7 @@ use std::fmt::{self, Display, Formatter}; use serde::{Deserialize, Serialize}; -pub const NORMALIZED_FORM_SCHEMA: u32 = 1; +pub const NORMALIZED_FORM_SCHEMA: u32 = 2; /// The deepest a form may nest and still describe an operation. /// diff --git a/crates/infact-rust-behaviors/src/idioms.rs b/crates/infact-rust-behaviors/src/idioms.rs index 10aa57d..b8e6c73 100644 --- a/crates/infact-rust-behaviors/src/idioms.rs +++ b/crates/infact-rust-behaviors/src/idioms.rs @@ -18,7 +18,7 @@ //! recommendation that gets switched off, and every shape below refuses //! anything it cannot account for. -use infact_core::{Condition, Form, Pattern}; +use infact_core::{Condition, ExternalBound, ExternalCallable, ExternalType, Form, Pattern}; /// Why a candidate yielded no recommendation. /// @@ -65,6 +65,17 @@ impl Idiom { } } + /// What the code needs of its elements to compute this at all. + /// + /// The other half of an [`Condition::ElementBound`] comes from the catalog. + /// This half is a property of the written-out shape: comparing two elements + /// with `==` is all a pairwise distinctness check asks of them. + const fn element_bound_of_the_code(&self) -> &'static str { + match self { + Self::AllDifferent => "PartialEq", + } + } + /// What has to hold for the recommendation to be sound. /// /// Fixed per idiom rather than per finding, because these are properties of @@ -72,21 +83,70 @@ impl Idiom { /// are stated in full even when several will usually be satisfied: a reader /// who can dismiss three of them in a second is better served than one who /// is not told about the fourth. - pub fn conditions(&self) -> Vec { + /// + /// The element bound is read off the catalog rather than written here. + /// Naming a bound from memory would be a claim about a version of a library + /// that nothing checked, and it is exactly the claim a reader is least able + /// to verify. + pub fn conditions(&self, callable: &ExternalCallable) -> Vec { + let mut conditions = Vec::new(); + if let Some(requires) = element_bound(callable) { + conditions.push(Condition::ElementBound { + requires, + code_requires: self.element_bound_of_the_code().to_owned(), + }); + } match self { - Self::AllDifferent => vec![ - Condition::ElementBound { - requires: "Eq + Hash".to_owned(), - code_requires: "PartialEq".to_owned(), - }, + Self::AllDifferent => conditions.extend([ Condition::Allocates, Condition::ComparisonObservable, Condition::SmallInputsFavourTheCode, - ], + ]), } + conditions } } +/// Whether a catalogued callable answers a yes-or-no question about a sequence. +/// +/// The recognizer names one API by path, and a path is not a promise: a catalog +/// is generated data, and the callable behind a path can change between +/// versions. Checking that it still takes the receiver and still returns `bool` +/// is what keeps a recommendation from being made against a signature nobody +/// read. +pub fn answers_a_predicate(callable: &ExternalCallable) -> bool { + let Some(signature) = &callable.signature else { + return false; + }; + let returns_bool = matches!( + &signature.output, + Some(ExternalType::Primitive { name }) if name == "bool" + ); + returns_bool && signature.inputs.iter().any(|input| input.name == "self") +} + +/// The bounds a callable puts on the elements it walks. +/// +/// A requirement whose subject is the iterator's own `Item` is a requirement on +/// the elements; everything else constrains the iterator or its lifetimes and +/// is not what a caller has to check about their data. +fn element_bound(callable: &ExternalCallable) -> Option { + let signature = callable.signature.as_ref()?; + let bounds = signature + .requirements + .iter() + .filter(|requirement| { + matches!(&requirement.subject, ExternalType::Associated { name, .. } if name == "Item") + }) + .flat_map(|requirement| &requirement.bounds) + .filter_map(|bound| match bound { + ExternalBound::Trait { path } => Some(path.rsplit("::").next().unwrap_or(path)), + ExternalBound::Lifetime { .. } => None, + }) + .collect::>(); + (!bounds.is_empty()).then(|| bounds.join(" + ")) +} + /// Whether a function may allocate. /// /// The recognizer is handed this rather than reading it, because whether a @@ -398,14 +458,56 @@ mod tests { ); } - /// Every recommendation states what it depends on. + /// The element bound is read off the catalog, not written from memory. #[test] - fn the_recommendation_carries_its_conditions() { - let conditions = Idiom::AllDifferent.conditions(); + fn the_recommendation_carries_the_catalogs_own_bound() { + let conditions = Idiom::AllDifferent.conditions(&all_unique()); assert!(conditions.contains(&Condition::Allocates)); assert!(conditions.iter().any(|condition| matches!( condition, - Condition::ElementBound { requires, .. } if requires == "Eq + Hash" + Condition::ElementBound { requires, code_requires } + if requires == "Eq + Hash" && code_requires == "PartialEq" ))); } + + /// A callable that no longer answers yes or no is not this API any more. + #[test] + fn a_callable_that_does_not_return_bool_is_not_a_predicate() { + assert!(answers_a_predicate(&all_unique())); + let mut changed = all_unique(); + if let Some(signature) = changed.signature.as_mut() { + signature.output = Some(ExternalType::Primitive { + name: "usize".to_owned(), + }); + } + assert!(!answers_a_predicate(&changed)); + } + + /// A catalog with no signature at all cannot be verified, so it is not used. + #[test] + fn a_callable_with_no_signature_is_not_a_predicate() { + let mut unknown = all_unique(); + unknown.signature = None; + assert!(!answers_a_predicate(&unknown)); + assert!( + Idiom::AllDifferent + .conditions(&unknown) + .iter() + .all(|condition| !matches!(condition, Condition::ElementBound { .. })) + ); + } + + /// The catalog entry the recognizer points at, as it is shipped. + fn all_unique() -> ExternalCallable { + let catalog = std::path::Path::new(env!("CARGO_MANIFEST_DIR")) + .join("../../infact-packs/rust-itertools/api/itertools-0.15.0.json"); + let catalog: infact_core::ExternalCatalog = + serde_json::from_slice(&std::fs::read(catalog).expect("itertools catalog")) + .expect("parsing the catalog"); + catalog + .callables + .into_iter() + .find(|callable| callable.path == Idiom::AllDifferent.callable_path().1) + .expect("all_unique in the catalog") + } } diff --git a/crates/infact-rust-behaviors/src/lib.rs b/crates/infact-rust-behaviors/src/lib.rs index 94d1e61..5f1dd65 100644 --- a/crates/infact-rust-behaviors/src/lib.rs +++ b/crates/infact-rust-behaviors/src/lib.rs @@ -763,13 +763,18 @@ fn collect_idiom_matches( } let idiom = idioms::Idiom::AllDifferent; let (package, path) = idiom.callable_path(); - let Some(catalog) = catalogs.iter().find(|catalog| { - catalog.package == package - && catalog - .callables - .iter() - .any(|callable| callable.path == path) - }) else { + // The callable has to be present AND still answer the question the idiom + // decides. A catalog is generated data and a path is not a promise: the + // signature is what says the API still does this. + let found = catalogs.iter().find_map(|catalog| { + let callable = catalog + .callables + .iter() + .find(|callable| callable.path == path)?; + (catalog.package == package && idioms::answers_a_predicate(callable)) + .then_some((catalog, callable)) + }); + let Some((catalog, callable)) = found else { return Ok(()); }; output.insert(Fact { @@ -794,7 +799,7 @@ fn collect_idiom_matches( start_column: None, end_column: None, }, - conditions: idiom.conditions(), + conditions: idiom.conditions(callable), }, derivation: Derivation { analyzer: "rust.idioms".to_owned(), From 60f626d7dbbabef00180475a9c5da4ccd5c1b607 Mon Sep 17 00:00:00 2001 From: Zack Maril Date: Tue, 25 Aug 2026 12:50:15 +0000 Subject: [PATCH 4/6] Use the problem id as a label, and follow what it says is missing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeNet groups submissions by problem, so a problem that is ABOUT distinctness labels its submissions for free: every accepted one contains the check, and whatever is not flagged is a spelling not handled. Three problems answered to the nine findings — 330 Rust submissions between them. Most of them are not the target. 181 use a set and 44 sort and dedup: already idiomatic, and correctly silent. Eighteen hand-roll a nested loop, which is the population that should be flagged. Eleven now are, up from nine. Two spellings the label turned up. A reaction written as several steps ending in `break` rather than `return` — `ans = "No"; break;` — records that a duplicate exists just as much as returning does, so the test is now that one step records and no step does anything unaccounted for, rather than that the last step is a particular thing. And the square loop. `for i in 0..n { for j in 0..n { if i != j { .. } } }` was refused with the reasoning that the guard is written over indices the rewrite forgets. That was the wrong conclusion from a right observation: the guard has to be CONSUMED, not carried, and removing it is sound because what it excludes is what the resulting form excludes anyway. So `Coverage` arrives after all, with the two inhabitants that earned it — each pair once, or each pair both ways round. The distinction is real and kept: a decision that does not care how often it sees a pair gets the same answer from either, and a count gets double. Every reaction this recognizer accepts is idempotent, which is why it may take both, and it says so where it takes them. A span may now start anywhere, so `1..k` walks `v[1..k]` exactly as `0..n` walks `v[..n]`. This gained nothing measured — the submissions it was aimed at turn out to compare `S[i]` against `S[j].to_string()`, and equating `a` with `f(b)` is not something to do — but the asymmetry it removes was arbitrary, and the lower triangle now has to start where its outer loop did. Still zero false positives: 127,210 CodeNet files and 3,650 of production Rust, every finding read by hand. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_017ghYbUJFcVfkKYacbTVJsR --- crates/infact-core/src/lib.rs | 2 +- crates/infact-normalize/src/lib.rs | 46 +++- crates/infact-normalize/src/renaming.rs | 2 + crates/infact-normalize/src/simplify.rs | 210 ++++++++++++++---- crates/infact-rust-behaviors/src/idioms.rs | 95 +++++++- .../infact-rust-normalize/examples/lower.rs | 10 +- .../infact-rust-normalize/tests/normalize.rs | 140 +++++++++++- 7 files changed, 439 insertions(+), 66 deletions(-) diff --git a/crates/infact-core/src/lib.rs b/crates/infact-core/src/lib.rs index 92f0f83..d877797 100644 --- a/crates/infact-core/src/lib.rs +++ b/crates/infact-core/src/lib.rs @@ -2,7 +2,7 @@ use std::path::PathBuf; -pub use infact_normalize::{Form, Pattern}; +pub use infact_normalize::{Coverage, Form, Pattern}; use serde::{Deserialize, Serialize}; use strum::IntoStaticStr; diff --git a/crates/infact-normalize/src/lib.rs b/crates/infact-normalize/src/lib.rs index 760d9cf..0bc398b 100644 --- a/crates/infact-normalize/src/lib.rs +++ b/crates/infact-normalize/src/lib.rs @@ -215,16 +215,22 @@ pub enum Form { /// special case: it is a normalization that lets a written-out loop compare /// against a library API that exists. /// - /// Only the distinct-pairs walk reduces to this. Walking adjacent pairs is - /// `windows(2)` and a different coverage; walking every ordered pair - /// including an element with itself is a third. Both stay two traversals - /// until something needs them, because a coverage field with one inhabitant - /// says nothing and a wrong one would claim a walk the code does not make. + /// Walking adjacent pairs is `windows(2)` and is not this: it is a third + /// coverage, and it stays two traversals until something needs it. Pairwise { sequence: Box
, left: Box, right: Box, body: Box, + /// Which of the pairs the walk actually reaches. + /// + /// Written out, the two are a triangular nested loop and a square one + /// with the diagonal guarded away, and both are common. They reach the + /// same pairs and differ in how often, which is behavior: a decision + /// that does not care how many times it sees a pair gets the same + /// answer from either, and a count gets double. Recording it is what + /// lets a reader of the form tell which they have. + coverage: Coverage, }, /// Producing a new sequence by transforming each element. Transform { @@ -340,6 +346,25 @@ pub enum Form { }, } +/// How often a pairwise walk reaches each pair. +#[derive( + Debug, Clone, Copy, Default, PartialEq, Eq, PartialOrd, Ord, Hash, Serialize, Deserialize, +)] +#[serde(rename_all = "kebab-case")] +pub enum Coverage { + /// Each unordered pair once: `for i in 0..n { for j in i + 1..n { .. } }`, + /// which is what `itertools::tuple_combinations` offers. + #[default] + Once, + /// Each unordered pair both ways round, and no element with itself. + /// + /// A square nested loop over one sequence with an `i != j` guard. Every + /// pair is visited twice, in both orders, so this says strictly less than + /// [`Coverage::Once`] about anything order- or count-sensitive and exactly + /// as much about anything that is neither. + BothWays, +} + /// Which way a walk runs. /// /// Only a walk that can stop early is changed by this: searching from the front @@ -558,11 +583,13 @@ impl Form { left, right, body, + coverage, } => Self::Pairwise { sequence: apply(sequence), left: left.clone(), right: right.clone(), body: apply(body), + coverage: *coverage, }, Self::Transform { sequence, @@ -1228,7 +1255,14 @@ impl Display for Form { left, right, body, - } => write!(formatter, "(pairwise {sequence} {left} {right} {body})"), + coverage, + } => { + let kind = match coverage { + Coverage::Once => "pairwise", + Coverage::BothWays => "pairwise-both-ways", + }; + write!(formatter, "({kind} {sequence} {left} {right} {body})") + } Self::Accumulate { sequence, initial, diff --git a/crates/infact-normalize/src/renaming.rs b/crates/infact-normalize/src/renaming.rs index c1b90c6..e8bf5a6 100644 --- a/crates/infact-normalize/src/renaming.rs +++ b/crates/infact-normalize/src/renaming.rs @@ -100,6 +100,7 @@ impl Renaming { left, right, body, + coverage, } => { let sequence = self.boxed(sequence); let left = Box::new(self.pattern(left)); @@ -109,6 +110,7 @@ impl Renaming { left, right, body: self.boxed(body), + coverage: *coverage, } } Form::Sift { diff --git a/crates/infact-normalize/src/simplify.rs b/crates/infact-normalize/src/simplify.rs index 42700e1..37d6709 100644 --- a/crates/infact-normalize/src/simplify.rs +++ b/crates/infact-normalize/src/simplify.rs @@ -22,7 +22,7 @@ use std::cell::Cell; -use crate::{Direction, Form, Pattern}; +use crate::{Coverage, Direction, Form, Pattern}; /// How many times to sweep before giving up. /// @@ -423,14 +423,14 @@ impl Form { let Pattern::Binding(index) = item.as_ref() else { return None; }; - let bound = span_from_zero(sequence)?; + let (start, end) = counting_span(sequence)?; let positions = [*index]; let source = body.sole_indexed_sequence(&positions)?; if !body.indexed_only(source, &positions) || body.writes_indexed(source) { return None; } Some(Self::Traverse { - sequence: Box::new(walked_sequence(bound, source)), + sequence: Box::new(walked_sequence(start, end, source)), item: item.clone(), body: Box::new(body.with_indexed_elements(source, &positions)), direction: *direction, @@ -475,9 +475,52 @@ impl Form { return None; }; self.as_indexed_pairwise(outer, first, inner, second, inner_body) + .or_else(|| Self::as_guarded_pairwise(outer, first, inner, second, inner_body)) .or_else(|| Self::as_enumerated_pairwise(outer, first, inner, second, inner_body)) } + /// The square spelling: two loops over the whole range, minus the diagonal. + /// + /// `for i in 0..n { for j in 0..n { if i != j { .. } } }` reaches each pair + /// twice rather than once, which is why it is [`Coverage::BothWays`] rather + /// than the same thing as a triangular loop. + /// + /// The guard has to be consumed rather than left in the body, and that is + /// the whole difficulty: it is the one place the positions are compared to + /// each other instead of used to index, so a body that still contained it + /// could never satisfy the test that the positions are only ever indices. + /// Removing it is sound precisely because what it excludes — an element + /// paired with itself — is what the resulting form already excludes. + fn as_guarded_pairwise( + outer: &Self, + first: &Pattern, + inner: &Self, + second: &Pattern, + body: &Self, + ) -> Option { + let (Pattern::Binding(left), Pattern::Binding(right)) = (first, second) else { + return None; + }; + // Both loops must walk the same positions, or the square is not square. + let (start, end) = counting_span(outer)?; + if counting_span(inner)? != (start, end) { + return None; + } + let body = without_diagonal_guard(body, *left, *right)?; + let positions = [*left, *right]; + let source = body.sole_indexed_sequence(&positions)?; + if !body.indexed_only(source, &positions) || body.writes_indexed(source) { + return None; + } + Some(Self::Pairwise { + sequence: Box::new(walked_sequence(start, end, source)), + left: Box::new(first.clone()), + right: Box::new(second.clone()), + body: Box::new(body.with_indexed_elements(source, &positions)), + coverage: Coverage::BothWays, + }) + } + /// The index spelling: two spans over one sequence's positions. fn as_indexed_pairwise( &self, @@ -490,18 +533,19 @@ impl Form { let (Pattern::Binding(left), Pattern::Binding(right)) = (first, second) else { return None; }; - let bound = span_from_zero(outer)?; - let extent = pairwise_extent(bound, inner, *left)?; + let (start, end) = counting_span(outer)?; + let end = pairwise_extent(start, end, inner, *left)?; let positions = [*left, *right]; let source = body.sole_indexed_sequence(&positions)?; if !body.indexed_only(source, &positions) || body.writes_indexed(source) { return None; } Some(Self::Pairwise { - sequence: Box::new(walked_sequence(extent, source)), + sequence: Box::new(walked_sequence(start, end, source)), left: Box::new(first.clone()), right: Box::new(second.clone()), body: Box::new(body.with_indexed_elements(source, &positions)), + coverage: Coverage::Once, }) } @@ -533,6 +577,7 @@ impl Form { left: Box::new(element.clone()), right: Box::new(second.clone()), body: Box::new(body.clone()), + coverage: Coverage::Once, }) } @@ -810,11 +855,11 @@ fn unfoldable(bindings: &[(u32, Form)]) -> Vec<(u32, Form)> { } } -/// The bound of a span that counts up from zero. +/// The two ends of a span a loop counts through. /// /// An inclusive bound reaches one position past the end, which is a different /// walk and in Rust a panicking one, so it is not this. -fn span_from_zero(form: &Form) -> Option<&Form> { +fn counting_span(form: &Form) -> Option<(&Form, &Form)> { let Form::Span { start, end, @@ -823,38 +868,34 @@ fn span_from_zero(form: &Form) -> Option<&Form> { else { return None; }; - (!*inclusive && **start == Form::Number("0".to_owned())).then(|| end.as_ref()) + (!*inclusive).then(|| (start.as_ref(), end.as_ref())) } /// The sequence a loop counting to `bound` actually walks. /// -/// `0..v.len()` walks `v` itself. `0..n` walks its first `n` elements, which is -/// the slice `v[..n]` — and that is the form the frontend already produces for -/// that slice written out, so a loop over a prefix and a slice of the same -/// extent agree. Recording it this way is what keeps the form honest: a walk -/// bounded by something other than the length does not cover the sequence, and -/// saying it did would recommend an API over elements that were never read. +/// `0..v.len()` walks `v` itself. Anything else walks a part of it: `0..n` is +/// the slice `v[..n]` and `1..n` is `v[1..n]` — and that is the form the +/// frontend already produces for those slices written out, so a loop over a +/// range and a slice of the same extent agree. Recording it this way is what +/// keeps the form honest: a walk bounded by something other than the length +/// does not cover the sequence, and saying it did would recommend an API over +/// elements that were never read. /// /// Measured, this is most of the corpus rather than an edge: across CodeNet's /// Rust submissions, pairwise loops bound by a bare variable outnumber those /// bound by `len()` six to one. -fn walked_sequence(bound: &Form, sequence: &Form) -> Form { - if let Form::Method { - name, - receiver, - arguments, - } = bound - && name == "len" - && arguments.is_empty() - && receiver.as_ref() == sequence - { +fn walked_sequence(start: &Form, end: &Form, sequence: &Form) -> Form { + let from_the_beginning = *start == Form::Number("0".to_owned()); + let to_the_end = matches!(end, Form::Method { name, receiver, arguments } + if name == "len" && arguments.is_empty() && receiver.as_ref() == sequence); + if from_the_beginning && to_the_end { return sequence.clone(); } Form::Index { sequence: Box::new(sequence.clone()), position: Box::new(Form::Span { - start: Box::new(Form::Number("0".to_owned())), - end: Box::new(bound.clone()), + start: Box::new(start.clone()), + end: Box::new(end.clone()), inclusive: false, }), } @@ -874,22 +915,109 @@ fn walked_sequence(bound: &Form, sequence: &Form) -> Form { /// bound is the one that says how far the sequence is actually read. /// /// A table rather than arithmetic: a bound this cannot read is one it refuses. -fn pairwise_extent<'a>(outer_bound: &'a Form, inner: &'a Form, outer: u32) -> Option<&'a Form> { - let Form::Span { - start, - end, - inclusive: false, - } = inner - else { - return None; - }; - if is_successor_of(start, outer) { - let matches_outer = end.as_ref() == outer_bound || is_predecessor_of(outer_bound, end); - return matches_outer.then(|| end.as_ref()); +fn pairwise_extent<'a>( + outer_start: &Form, + outer_end: &'a Form, + inner: &'a Form, + outer: u32, +) -> Option<&'a Form> { + let (inner_start, inner_end) = counting_span(inner)?; + if is_successor_of(inner_start, outer) { + let matches_outer = inner_end == outer_end || is_predecessor_of(outer_end, inner_end); + return matches_outer.then_some(inner_end); } // The lower triangle allows no such slack: an outer loop that stopped one - // short would never pair the last position with anything. - (**start == Form::Number("0".to_owned()) && **end == Form::Local(outer)).then_some(outer_bound) + // short would never pair the last position with anything. Its inner loop + // must also start where the outer one did, or the pairs below that point + // go unvisited. + (inner_start == outer_start && *inner_end == Form::Local(outer)).then_some(outer_end) +} + +/// A square loop's body with its `i != j` guard taken out. +/// +/// Three spellings, and the corpus writes all three: the guard as a conjunct of +/// the test that follows it, the guard as the whole body wrapping the work, and +/// the guard as an early `continue`. Returns `None` when there is no guard at +/// all — an unguarded square loop pairs elements with themselves, so every +/// element equals something and the walk decides nothing. +fn without_diagonal_guard(body: &Form, left: u32, right: u32) -> Option { + match body { + // `if i != j && a[i] == a[j] { .. }` + Form::Branch { + condition, + consequence, + alternative, + } => { + if let Form::Binary { + operator, + left: first, + right: second, + } = condition.as_ref() + && operator == "&&" + { + let remaining = if excludes_the_diagonal(first, left, right) { + second + } else if excludes_the_diagonal(second, left, right) { + first + } else { + return None; + }; + return Some(Form::Branch { + condition: remaining.clone(), + consequence: consequence.clone(), + alternative: alternative.clone(), + }); + } + // `if i != j { .. }` wrapping the work, with nothing to do when the + // positions are equal. + (excludes_the_diagonal(condition, left, right) && alternative.is_none()) + .then(|| consequence.as_ref().clone()) + } + // `if i == j { continue; }` before the work. + Form::Sequence(steps) => { + let (guard, rest) = steps.split_first()?; + let Form::Branch { + condition, + consequence, + alternative: None, + } = guard + else { + return None; + }; + if !matches!(consequence.as_ref(), Form::Opaque { kind, .. } if kind == "continue_expression") + { + return None; + } + let Form::Binary { + operator, + left: first, + right: second, + } = condition.as_ref() + else { + return None; + }; + if operator != "==" || !names_both_positions(first, second, left, right) { + return None; + } + match rest { + [only] => Some(only.clone()), + _ => Some(Form::Sequence(rest.to_vec())), + } + } + _ => None, + } +} + +/// Whether a test is exactly `i != j` over the two loop positions. +fn excludes_the_diagonal(form: &Form, left: u32, right: u32) -> bool { + matches!(form, Form::Binary { operator, left: first, right: second } + if operator == "!=" && names_both_positions(first, second, left, right)) +} + +/// Whether two forms are the two loop positions, either way round. +fn names_both_positions(first: &Form, second: &Form, left: u32, right: u32) -> bool { + (*first == Form::Local(left) && *second == Form::Local(right)) + || (*first == Form::Local(right) && *second == Form::Local(left)) } /// Whether a form is one less than another. diff --git a/crates/infact-rust-behaviors/src/idioms.rs b/crates/infact-rust-behaviors/src/idioms.rs index b8e6c73..811eff8 100644 --- a/crates/infact-rust-behaviors/src/idioms.rs +++ b/crates/infact-rust-behaviors/src/idioms.rs @@ -202,11 +202,18 @@ fn deciding_pairwise(form: &Form) -> Result<&Form, IdiomRefusal> { /// A walk over pairs whose only reaction to an equal pair is to record that one /// exists. fn distinctness_walk(form: &Form) -> Result<&Form, IdiomRefusal> { + // Either coverage will do, and that is a claim worth making explicitly: + // a square guarded loop reaches each pair twice, and every reaction this + // accepts is idempotent — returning twice is returning, and setting a flag + // to the same constant twice is setting it. The reaction that is NOT + // idempotent is counting, and counting is refused below for its own + // reasons, so nothing here depends on which spelling was written. let Form::Pairwise { sequence, left, right, body, + coverage: _, } = form else { return Err(IdiomRefusal::NotThisShape); @@ -265,20 +272,38 @@ fn records_that_one_exists(consequence: &Form) -> bool { && matches!(target.as_ref(), Form::Local(_) | Form::Free(_)) && matches!(value.as_ref(), Form::Constant(_)) } - // `println!("no"); return;` is one reaction written as two steps, and - // it is the spelling the corpus actually uses. Only the last step may - // leave, because a `return` reached in the middle would make the steps - // after it dead rather than part of the reaction. - Form::Sequence(steps) => steps.split_last().is_some_and(|(last, rest)| { - records_that_one_exists(last) && !rest.iter().any(leaves_the_walk) - }), + // One reaction is routinely written as several steps: `println!("no"); + // return;` and `ans = "No"; break;` are both in the corpus. It is still + // only recording that a duplicate exists as long as one step does that + // and no step does anything else — every other step must be reporting + // or leaving, because those change nothing about the answer. + Form::Sequence(steps) => { + steps.iter().any(records_that_one_exists) && steps.iter().all(is_inert_beside_recording) + } _ => false, } } -/// Whether a step can end the walk from somewhere other than its end. -fn leaves_the_walk(form: &Form) -> bool { - matches!(form, Form::Return(_)) || form.children().into_iter().any(leaves_the_walk) +/// Whether a step changes nothing about whether a duplicate was found. +/// +/// Leaving a loop early is an optimization on a walk whose answer is already +/// settled, and reporting is how the answer gets out. Anything else — another +/// assignment, a call, a push — is work this recognizer has not accounted for, +/// and a reaction it cannot account for is one it must not summarize. +/// +/// Any macro counts as reporting, which is looser than it sounds and looser +/// than it reads. The corpus spells the report `println!`, `write!`, `p!` and +/// `echo!` — the last two being the submitter's own — so a list of known names +/// would be a list of one corpus's habits. What bounds the damage is that the +/// caller has already established the reaction cannot mention either element, +/// so a macro here cannot carry the pair out; the residual risk is a macro with +/// an unrelated effect, which makes the finding fused rather than wrong. +fn is_inert_beside_recording(step: &Form) -> bool { + records_that_one_exists(step) + || matches!(step, Form::Opaque { kind, .. } + if kind == "break_expression" + || kind == "continue_expression" + || kind.starts_with("macro:")) } /// Whether a test asks whether the two elements of a pair are equal. @@ -317,6 +342,7 @@ mod tests { left: Box::new(Pattern::Binding(0)), right: Box::new(Pattern::Binding(1)), body: Box::new(body), + coverage: infact_core::Coverage::Once, }, Form::Constant("true".to_owned()), ]) @@ -426,6 +452,55 @@ mod tests { assert_eq!(all_different(&form, allowed()), Ok(&Form::Free(0))); } + /// Recording and leaving the inner loop is one reaction written as two. + /// + /// `break` settles nothing by itself, but a walk whose flag is already set + /// has nothing left to learn from the rest of the row. + #[test] + fn a_walk_that_records_then_breaks_is_all_different() { + let form = pairwise(Form::Branch { + condition: Box::new(equal_pair()), + consequence: Box::new(Form::Sequence(vec![ + Form::Assign { + operator: "=".to_owned(), + target: Box::new(Form::Local(2)), + value: Box::new(Form::Constant("\"No\"".to_owned())), + }, + Form::Opaque { + kind: "break_expression".to_owned(), + parts: Vec::new(), + }, + ])), + alternative: None, + }); + assert_eq!(all_different(&form, allowed()), Ok(&Form::Free(0))); + } + + /// A reaction that also does unaccounted work is refused. + #[test] + fn a_walk_that_does_more_than_record_is_refused() { + let form = pairwise(Form::Branch { + condition: Box::new(equal_pair()), + consequence: Box::new(Form::Sequence(vec![ + Form::Assign { + operator: "=".to_owned(), + target: Box::new(Form::Local(2)), + value: Box::new(Form::Constant("false".to_owned())), + }, + Form::Method { + name: "push".to_owned(), + receiver: Box::new(Form::Local(3)), + arguments: vec![Form::Number("1".to_owned())], + }, + ])), + alternative: None, + }); + assert_eq!( + all_different(&form, allowed()), + Err(IdiomRefusal::NotThisShape) + ); + } + /// Reporting and leaving is one reaction written as two steps. /// /// This is the spelling CodeNet's submissions actually use. diff --git a/crates/infact-rust-normalize/examples/lower.rs b/crates/infact-rust-normalize/examples/lower.rs index 2434c35..f8c7ca1 100644 --- a/crates/infact-rust-normalize/examples/lower.rs +++ b/crates/infact-rust-normalize/examples/lower.rs @@ -15,7 +15,7 @@ use std::path::PathBuf; use std::sync::Arc; use entl_tree_sitter::{ParserPack, ParserRuntime}; -use infact_normalize::{Direction, Form, Pattern}; +use infact_normalize::{Coverage, Direction, Form, Pattern}; use infact_rust_normalize::normalize_file; /// A place the form did not keep enough to emit, by what was missing. @@ -156,10 +156,16 @@ fn lower(form: &Form, level: usize, guesses: &Guesses, names: &Names) -> String left, right, body, + coverage, } => { guesses.note("Pairwise: the loop bounds that produced the pairs are not recorded"); + let call = match coverage { + Coverage::Once => "tuple_combinations()", + // Each pair both ways round is what `permutations(2)` yields. + Coverage::BothWays => "permutations(2)", + }; format!( - "for ({}, {}) in {}.tuple_combinations() {{\n{}{}\n{}}}", + "for ({}, {}) in {}.{call} {{\n{}{}\n{}}}", pattern(left, names), pattern(right, names), sub(sequence), diff --git a/crates/infact-rust-normalize/tests/normalize.rs b/crates/infact-rust-normalize/tests/normalize.rs index 89aa93f..e618032 100644 --- a/crates/infact-rust-normalize/tests/normalize.rs +++ b/crates/infact-rust-normalize/tests/normalize.rs @@ -433,13 +433,16 @@ fn the_lower_triangle_is_also_a_pairwise_walk() { assert!(form.contains("(pairwise"), "{form}"); } -/// A nested loop over the whole sequence twice is not a walk over pairs. +/// A guarded square loop over `len()` is a walk over pairs, both ways round. /// -/// It visits each ordered pair and an element with itself, and the `i != j` -/// that usually accompanies it is written over indices this rewrite would -/// forget. Refusing is the honest outcome. +/// This used to be refused on the grounds that the `i != j` is written over +/// indices the rewrite forgets. That was the wrong conclusion from a right +/// observation: the guard has to be CONSUMED rather than carried, and removing +/// it is sound because what it excludes is what the resulting form excludes +/// anyway. Measured against CodeNet submissions to problems that are about +/// distinctness, this spelling is a fifth of the hand-rolled ones. #[test] -fn a_full_nested_loop_is_not_a_pairwise_walk() { +fn a_guarded_square_loop_over_a_length_is_a_pairwise_walk() { let form = behavior_of( "fn f(values: &[i32]) -> bool { for i in 0..values.len() { @@ -451,7 +454,7 @@ fn a_full_nested_loop_is_not_a_pairwise_walk() { }", "f", ); - assert!(!form.contains("(pairwise"), "{form}"); + assert!(form.contains("(pairwise-both-ways"), "{form}"); } /// A nested loop that touches the sequence itself is not a walk over pairs. @@ -620,3 +623,128 @@ fn a_short_outer_loop_with_a_lower_triangle_walks_a_shorter_prefix() { ); assert_ne!(short, whole); } + +/// A square loop with the diagonal guarded away is a walk over pairs. +/// +/// It reaches each pair both ways round rather than once, which the form +/// records, because a decision that does not care gets the same answer and a +/// count gets double. +#[test] +fn a_guarded_square_loop_is_a_pairwise_walk() { + for guard in [ + "if i != j && values[i] == values[j] { return false; }", + "if i != j { if values[i] == values[j] { return false; } }", + "if i == j { continue; } if values[i] == values[j] { return false; }", + ] { + let form = behavior_of( + &format!( + "fn f(values: &[i32], n: usize) -> bool {{ + for i in 0..n {{ for j in 0..n {{ {guard} }} }} + true + }}" + ), + "f", + ); + assert!(form.contains("(pairwise-both-ways"), "{guard}: {form}"); + } +} + +/// An unguarded square loop pairs elements with themselves and decides nothing. +#[test] +fn an_unguarded_square_loop_is_not_a_pairwise_walk() { + let form = behavior_of( + "fn f(values: &[i32], n: usize) -> bool { + for i in 0..n { for j in 0..n { if values[i] == values[j] { return false; } } } + true + }", + "f", + ); + assert!(!form.contains("(pairwise"), "{form}"); +} + +/// Visiting each pair twice is not the same form as visiting it once. +#[test] +fn the_two_coverages_are_different_forms() { + let square = behavior_of( + "fn f(values: &[i32], n: usize) -> bool { + for i in 0..n { for j in 0..n { if i != j && values[i] == values[j] { return false; } } } + true + }", + "f", + ); + let triangle = behavior_of( + "fn f(values: &[i32], n: usize) -> bool { + for i in 0..n { for j in i + 1..n { if values[i] == values[j] { return false; } } } + true + }", + "f", + ); + assert_ne!(square, triangle); +} + +/// A square loop guarded on something other than the diagonal is not this. +#[test] +fn a_square_loop_guarded_on_something_else_is_not_a_pairwise_walk() { + let form = behavior_of( + "fn f(values: &[i32], n: usize) -> bool { + for i in 0..n { for j in 0..n { if i < j && values[i] == values[j] { return false; } } } + true + }", + "f", + ); + assert!(!form.contains("(pairwise"), "{form}"); +} + +/// A loop over an interior range walks that slice, not the whole sequence. +/// +/// `1..k` reads neither the first element nor anything from `k` on, so the +/// extent is `v[1..k]` — the same form the frontend gives that slice written +/// out. A prefix and an interior range are the same case and neither is the +/// whole sequence. +#[test] +fn a_loop_over_an_interior_range_walks_a_slice() { + let interior = behavior_of( + "fn f(values: &[i32], k: usize) -> bool { + for i in 1..k { + for j in i + 1..k { + if values[i] == values[j] { return false; } + } + } + true + }", + "f", + ); + assert!(interior.contains("(pairwise (index"), "{interior}"); + let whole = behavior_of( + "fn f(values: &[i32]) -> bool { + for i in 0..values.len() { + for j in i + 1..values.len() { + if values[i] == values[j] { return false; } + } + } + true + }", + "f", + ); + assert_ne!(interior, whole); +} + +/// A lower-triangle inner loop must start where the outer one did. +/// +/// `for i in 1..k { for j in 0..i }` reaches pairs involving position 0, which +/// the outer loop never visits, so the walk is not over the slice `1..k`. +#[test] +fn a_lower_triangle_that_reaches_below_the_start_is_refused() { + let form = behavior_of( + "fn f(values: &[i32], k: usize) -> bool { + for i in 1..k { + for j in 0..i { + if values[i] == values[j] { return false; } + } + } + true + }", + "f", + ); + assert!(!form.contains("(pairwise"), "{form}"); +} From 90232d0d2b027d7f3878ac0e9b874021e6d75579 Mon Sep 17 00:00:00 2001 From: Zack Maril Date: Tue, 25 Aug 2026 13:19:24 +0000 Subject: [PATCH 5/6] Teach the unifier about walks over pairs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `Pairwise` appeared nowhere in matching.rs. Adding a variant without an arm does not fail to compile — the fallthrough answers `false` — so the form silently matched nothing, and any behavior ever written over it would have gone quiet with no error to explain why. The arm compares coverage as well, because seeing each pair once and seeing it both ways round reach the same pairs a different number of times, and a pattern that counts would get double from the wrong one. The body admits fusion the way a traversal's does. Nothing observable changes today: no derived library behavior contains a walk over pairs, because no library writes one. The hole was in the layer that will be asked first when one does. Measured while establishing this: across the three labelled problems, 14 functions reduce to a `Pairwise` and 11 are reported. The three that are not are the same submission thrice, comparing `S[i]` against `S[j].to_string()`, which is a correct refusal. So the recognizer turns away nothing it should take, and every remaining miss is upstream, in whether the form becomes a `Pairwise` at all. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_017ghYbUJFcVfkKYacbTVJsR --- crates/infact-normalize/src/lib.rs | 38 +++++++++++++++++++++++++ crates/infact-normalize/src/matching.rs | 26 +++++++++++++++++ 2 files changed, 64 insertions(+) diff --git a/crates/infact-normalize/src/lib.rs b/crates/infact-normalize/src/lib.rs index 0bc398b..2c72aa9 100644 --- a/crates/infact-normalize/src/lib.rs +++ b/crates/infact-normalize/src/lib.rs @@ -1461,6 +1461,44 @@ mod tests { } } + fn pairwise(coverage: Coverage) -> Form { + Form::Pairwise { + sequence: Box::new(Form::Free(0)), + left: Box::new(Pattern::Binding(0)), + right: Box::new(Pattern::Binding(1)), + body: Box::new(Form::Binary { + operator: "==".to_owned(), + left: Box::new(Form::Local(0)), + right: Box::new(Form::Local(1)), + }), + coverage, + } + } + + /// A walk over pairs takes part in matching like every other form. + /// + /// Adding a variant without teaching the unifier about it does not fail to + /// compile: the fallthrough answers `false`, so the form silently matches + /// nothing and every behavior written over it goes quiet. + #[test] + fn a_walk_over_pairs_matches_itself() { + assert!(pairwise(Coverage::Once).contains(&pairwise(Coverage::Once))); + assert!( + Form::Sequence(vec![Form::Literal, pairwise(Coverage::Once)]) + .contains(&pairwise(Coverage::Once)) + ); + } + + /// Seeing each pair once is not seeing it both ways round. + /// + /// The two reach the same pairs and differ in how often, which is behavior + /// for anything that counts. + #[test] + fn the_two_coverages_do_not_match_each_other() { + assert!(!pairwise(Coverage::Once).contains(&pairwise(Coverage::BothWays))); + assert!(!pairwise(Coverage::BothWays).contains(&pairwise(Coverage::Once))); + } + /// Code that does a thing four times has four findings. /// /// Reporting only the first meant a reader who fixed what they were shown diff --git a/crates/infact-normalize/src/matching.rs b/crates/infact-normalize/src/matching.rs index 6d96aa3..2e5c46c 100644 --- a/crates/infact-normalize/src/matching.rs +++ b/crates/infact-normalize/src/matching.rs @@ -443,6 +443,32 @@ impl Bindings { && (self.form(subject_body, pattern_body) || self.fused_body(subject_body, pattern_body)) } + ( + Form::Pairwise { + sequence: subject_sequence, + left: subject_left, + right: subject_right, + body: subject_body, + coverage: subject_coverage, + }, + Form::Pairwise { + sequence: pattern_sequence, + left: pattern_left, + right: pattern_right, + body: pattern_body, + coverage: pattern_coverage, + }, + ) => { + // Seeing each pair once and seeing it both ways round are + // different walks, and a pattern that counts would get double + // from the wrong one. + subject_coverage == pattern_coverage + && self.form(subject_sequence, pattern_sequence) + && self.pattern(subject_left, pattern_left) + && self.pattern(subject_right, pattern_right) + && (self.form(subject_body, pattern_body) + || self.fused_body(subject_body, pattern_body)) + } ( Form::Accumulate { sequence: subject_sequence, From 83b770f4bf5fcc19da242276c1170811d311d7d4 Mon Sep 17 00:00:00 2001 From: Zack Maril Date: Tue, 25 Aug 2026 13:52:11 +0000 Subject: [PATCH 6/6] Recognize the idiom with the matcher instead of beside it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The recognizer walked the form by hand: find a Pairwise, confirm its body is a Branch, pull out the condition, pull out the consequence. All four are things unification already does, and doing them again beside it is how a matcher comes to be reimplemented next to itself — with its own idea of what counts as the same shape, free to drift. What was missing was not a way to match but a way to keep what matching found. `Bindings` already works out what every hole and every name stood for, because a hole has to mean the same thing each time it appears, and then threw it all away at the door. `Form::resolve_all` hands it back as `Resolved`, so a recognizer with something to say about a PART of what matched can say it about the piece the matcher found rather than one a second traversal went looking for. The all-different shape is now a `Form` — a pairwise walk whose body is a branch with no else — with the test and the reaction left as holes. The holes are wide on purpose: every judgement about them stays in the recognizer, which is what lets it say WHICH one was wrong, where a pattern narrow enough to reject on its own could only ever answer "no". Every refusal reason survives. Three things the idiom path had copied now come from where they belong. Evidence and span construction were written out once per emitter until there were three. And placement: findings pointed at whole functions because the idiom path never called `locate_all`. It does now, and four of the eleven findings narrowed from the enclosing function to the loop itself — s117010675 from lines 3-22 to 13-20, which is exactly the nested loop. Same eleven findings, better aimed. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_017ghYbUJFcVfkKYacbTVJsR --- crates/infact-core/src/lib.rs | 2 +- crates/infact-normalize/src/lib.rs | 29 ++++ crates/infact-normalize/src/matching.rs | 43 ++++++ crates/infact-rust-behaviors/src/idioms.rs | 160 +++++++++++++-------- crates/infact-rust-behaviors/src/lib.rs | 144 +++++++++++-------- 5 files changed, 256 insertions(+), 122 deletions(-) diff --git a/crates/infact-core/src/lib.rs b/crates/infact-core/src/lib.rs index d877797..24e54ce 100644 --- a/crates/infact-core/src/lib.rs +++ b/crates/infact-core/src/lib.rs @@ -2,7 +2,7 @@ use std::path::PathBuf; -pub use infact_normalize::{Coverage, Form, Pattern}; +pub use infact_normalize::{Coverage, Form, Pattern, Resolved}; use serde::{Deserialize, Serialize}; use strum::IntoStaticStr; diff --git a/crates/infact-normalize/src/lib.rs b/crates/infact-normalize/src/lib.rs index 2c72aa9..e34cae1 100644 --- a/crates/infact-normalize/src/lib.rs +++ b/crates/infact-normalize/src/lib.rs @@ -15,6 +15,7 @@ mod renaming; mod simplify; use matching::Bindings; +pub use matching::Resolved; use renaming::Renaming; use std::fmt::{self, Display, Formatter}; @@ -783,6 +784,34 @@ impl Form { renaming.form(self) } + /// Every place `pattern` matches, with what its roles stood for. + /// + /// The companion to [`Form::contains`], which answers whether a pattern is + /// here and discards what it found. A recognizer that has to say something + /// about a *part* of what matched — this hole must not mention that name — + /// needs the parts, and working them out by walking the subject again is + /// how a matcher comes to be reimplemented beside itself. + /// + /// Matches at nodes, not at runs of statements: a pattern spread over + /// several steps has no single node to have matched, so [`Form::locate_all`] + /// is what places those. + #[must_use] + pub fn resolve_all(&self, pattern: &Self) -> Vec { + let mut found = Vec::new(); + self.resolve_into(pattern, &mut found); + found + } + + fn resolve_into(&self, pattern: &Self, found: &mut Vec) { + let mut bindings = Bindings::default(); + if bindings.form(self, pattern) { + found.push(bindings.resolved()); + } + for child in self.children() { + child.resolve_into(pattern, found); + } + } + /// Whether this form contains `pattern` anywhere within it. /// /// Repository code rarely consists of nothing but the behavior in question, diff --git a/crates/infact-normalize/src/matching.rs b/crates/infact-normalize/src/matching.rs index 2e5c46c..1a20db5 100644 --- a/crates/infact-normalize/src/matching.rs +++ b/crates/infact-normalize/src/matching.rs @@ -59,6 +59,41 @@ fn interrupts_iteration(form: &Form) -> bool { } } +/// What a pattern's roles stood for where it matched. +/// +/// Matching already works this out — a hole has to mean the same thing every +/// time it appears, so it is recorded — and used to throw it away at the door, +/// leaving callers who needed a part of what matched to walk the subject again +/// by hand looking for the thing the matcher had just found. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct Resolved { + holes: Vec<(u32, Form)>, + locals: Vec<(u32, u32)>, +} + +impl Resolved { + /// What the pattern's hole stood for. + #[must_use] + pub fn hole(&self, index: u32) -> Option<&Form> { + self.holes + .iter() + .find(|(bound, _)| *bound == index) + .map(|(_, form)| form) + } + + /// Which of the subject's names the pattern's name lined up with. + /// + /// A pattern names the roles it binds by its own numbering; this is how to + /// ask a question about the subject, whose numbering is its own. + #[must_use] + pub fn local(&self, index: u32) -> Option { + self.locals + .iter() + .find(|(bound, _)| *bound == index) + .map(|(_, subject)| *subject) + } +} + impl Bindings { /// A fresh set of bindings, told whether to accept work done alongside the /// pattern rather than only the pattern itself. @@ -69,6 +104,14 @@ impl Bindings { } } + /// What this match worked out, for a caller that needs the parts. + pub(crate) fn resolved(self) -> Resolved { + Resolved { + holes: self.holes, + locals: self.locals, + } + } + /// Match the remaining steps, stepping over statements that leave the /// behavior alone. pub(crate) fn follow(&mut self, haystack: &[Form], steps: &[Form]) -> bool { diff --git a/crates/infact-rust-behaviors/src/idioms.rs b/crates/infact-rust-behaviors/src/idioms.rs index 811eff8..64e0797 100644 --- a/crates/infact-rust-behaviors/src/idioms.rs +++ b/crates/infact-rust-behaviors/src/idioms.rs @@ -18,7 +18,9 @@ //! recommendation that gets switched off, and every shape below refuses //! anything it cannot account for. -use infact_core::{Condition, ExternalBound, ExternalCallable, ExternalType, Form, Pattern}; +use infact_core::{ + Condition, Coverage, ExternalBound, ExternalCallable, ExternalType, Form, Pattern, Resolved, +}; /// Why a candidate yielded no recommendation. /// @@ -156,6 +158,48 @@ pub struct Context { pub can_allocate: bool, } +/// The shape of a pairwise decision, as a pattern to be matched. +/// +/// Written as a `Form` and unified by the same machinery a derived library +/// behavior goes through, rather than as a walk over the subject by hand. What +/// that buys is not brevity but the parts: unification records what each hole +/// stood for, so the conditions below are stated about the pieces it found +/// instead of re-found by a second traversal that can disagree with the first. +/// +/// The holes are deliberately wide. Hole 1 is the test and hole 2 the reaction, +/// and admitting anything there is what leaves every judgement about them to +/// [`all_different`], which can then say WHICH of them was wrong. A pattern +/// narrow enough to reject on its own would only ever answer "no". +fn pairwise_decision(coverage: Coverage) -> Form { + Form::Pairwise { + sequence: Box::new(Form::Free(0)), + left: Box::new(Pattern::Binding(0)), + right: Box::new(Pattern::Binding(1)), + body: Box::new(Form::Branch { + condition: Box::new(Form::Free(1)), + consequence: Box::new(Form::Free(2)), + // A test with an `else` is choosing between two things to do, and + // only one of them is being described here. + alternative: None, + }), + coverage, + } +} + +/// A recognized algorithm, and what a caller needs to act on it. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct Recognized { + /// The sequence the check is over, which is what the recommended call goes + /// on. + pub sequence: Form, + /// The shape as it was matched, for locating it among the statements. + /// + /// The recognizer's pattern leaves the sequence a hole and admits either + /// coverage, so on its own it would locate the first walk in the function + /// rather than the one recognized. Both are filled back in here. + pub shape: Form, +} + /// Recognize an all-different check in a normalized function body. /// /// The shape is a walk over each pair of one sequence that, on finding two @@ -165,84 +209,68 @@ pub struct Context { /// then does with the answer — return it, print it, set a flag — is the /// caller's business and does not change what the loop computed. /// -/// `Pairwise` is what makes this one shape rather than several: the index and -/// iterator spellings of the walk have already met by the time this runs. +/// Either coverage will do, and that is worth saying explicitly: a square +/// guarded loop reaches each pair twice, and every reaction accepted below is +/// idempotent — returning twice is returning, setting a flag to the same +/// constant twice is setting it. The reaction that is NOT idempotent is +/// counting, and counting is refused for its own reasons, so nothing here +/// depends on which spelling was written. /// /// Returns the sequence the check is over, which is what a caller would put the /// recommended call on. -pub fn all_different(form: &Form, context: Context) -> Result<&Form, IdiomRefusal> { - let sequence = deciding_pairwise(form)?; - if !context.can_allocate { - return Err(IdiomRefusal::CannotAllocate); - } - Ok(sequence) -} - -/// Search a form for a pairwise walk that decides distinctness. -fn deciding_pairwise(form: &Form) -> Result<&Form, IdiomRefusal> { - // Why the nearest thing to the shape was not it. A walk over pairs that - // decides the wrong question is worth saying so about; not finding a walk - // at all is the uninformative answer, so anything else outranks it. +pub fn all_different(form: &Form, context: Context) -> Result { + // Why the nearest thing to the shape was not it. Finding no walk at all is + // the uninformative answer, so anything else outranks it. let mut refusal = IdiomRefusal::NotThisShape; - match distinctness_walk(form) { - Ok(sequence) => return Ok(sequence), - Err(IdiomRefusal::NotThisShape) => {} - Err(specific) => refusal = specific, - } - for child in form.children() { - match deciding_pairwise(child) { - Ok(sequence) => return Ok(sequence), - Err(IdiomRefusal::NotThisShape) => {} - Err(specific) => refusal = specific, + for coverage in [Coverage::Once, Coverage::BothWays] { + let pattern = pairwise_decision(coverage); + for resolved in form.resolve_all(&pattern) { + match decides_distinctness(&resolved) { + Ok(sequence) => { + if !context.can_allocate { + return Err(IdiomRefusal::CannotAllocate); + } + let mut shape = pattern.clone(); + if let Form::Pairwise { sequence, .. } = &mut shape { + **sequence = resolved.hole(0).cloned().unwrap_or(Form::Literal); + } + return Ok(Recognized { sequence, shape }); + } + Err(IdiomRefusal::NotThisShape) => {} + Err(specific) => refusal = specific, + } } } Err(refusal) } -/// A walk over pairs whose only reaction to an equal pair is to record that one -/// exists. -fn distinctness_walk(form: &Form) -> Result<&Form, IdiomRefusal> { - // Either coverage will do, and that is a claim worth making explicitly: - // a square guarded loop reaches each pair twice, and every reaction this - // accepts is idempotent — returning twice is returning, and setting a flag - // to the same constant twice is setting it. The reaction that is NOT - // idempotent is counting, and counting is refused below for its own - // reasons, so nothing here depends on which spelling was written. - let Form::Pairwise { - sequence, - left, - right, - body, - coverage: _, - } = form +/// Whether one match of the pairwise shape decides distinctness. +/// +/// Every question here is asked of a piece the matcher handed over, and the two +/// names are the subject's own — a pattern numbers its roles by its own +/// counting, and asking whether the reaction mentions the pattern's `Local(0)` +/// would be asking about the wrong function's names. +fn decides_distinctness(resolved: &Resolved) -> Result { + let (Some(sequence), Some(condition), Some(consequence)) = + (resolved.hole(0), resolved.hole(1), resolved.hole(2)) else { return Err(IdiomRefusal::NotThisShape); }; - let (Pattern::Binding(left), Pattern::Binding(right)) = (left.as_ref(), right.as_ref()) else { + let (Some(left), Some(right)) = (resolved.local(0), resolved.local(1)) else { return Err(IdiomRefusal::NotThisShape); }; - // A test with an `else` is choosing between two things to do, and only one - // of them is being described here. - let Form::Branch { - condition, - consequence, - alternative: None, - } = body.as_ref() - else { - return Err(IdiomRefusal::NotThisShape); - }; - if !compares_the_pair(condition, *left, *right) { + if !compares_the_pair(condition, left, right) { return Err(IdiomRefusal::DecidesSomethingElse); } // Reading either element means the pair is being used for its content, and // an API that answers only whether a duplicate exists cannot supply that. - if consequence.references_local(*left) || consequence.references_local(*right) { + if consequence.references_local(left) || consequence.references_local(right) { return Err(IdiomRefusal::EscapesWithAValue); } if !records_that_one_exists(consequence) { return Err(IdiomRefusal::NotThisShape); } - Ok(sequence.as_ref()) + Ok(sequence.clone()) } /// Whether a reaction to an equal pair records only that one was found. @@ -367,7 +395,10 @@ mod tests { #[test] fn a_pairwise_equality_check_is_all_different() { let form = pairwise(escaping(equal_pair(), Form::Constant("false".to_owned()))); - assert_eq!(all_different(&form, allowed()), Ok(&Form::Free(0))); + assert_eq!( + all_different(&form, allowed()).map(|found| found.sequence), + Ok(Form::Free(0)) + ); } /// A relation other than equality asks a different question. @@ -449,7 +480,10 @@ mod tests { }), alternative: None, }); - assert_eq!(all_different(&form, allowed()), Ok(&Form::Free(0))); + assert_eq!( + all_different(&form, allowed()).map(|found| found.sequence), + Ok(Form::Free(0)) + ); } /// Recording and leaving the inner loop is one reaction written as two. @@ -473,7 +507,10 @@ mod tests { ])), alternative: None, }); - assert_eq!(all_different(&form, allowed()), Ok(&Form::Free(0))); + assert_eq!( + all_different(&form, allowed()).map(|found| found.sequence), + Ok(Form::Free(0)) + ); } /// A reaction that also does unaccounted work is refused. @@ -517,7 +554,10 @@ mod tests { ])), alternative: None, }); - assert_eq!(all_different(&form, allowed()), Ok(&Form::Free(0))); + assert_eq!( + all_different(&form, allowed()).map(|found| found.sequence), + Ok(Form::Free(0)) + ); } /// A context with no allocator is told nothing rather than told wrongly. diff --git a/crates/infact-rust-behaviors/src/lib.rs b/crates/infact-rust-behaviors/src/lib.rs index 5f1dd65..d3bf874 100644 --- a/crates/infact-rust-behaviors/src/lib.rs +++ b/crates/infact-rust-behaviors/src/lib.rs @@ -22,7 +22,7 @@ use tree_sitter::Node; pub use derivation::{ DerivedLibrary, derive_behavior, derive_catalog, derive_library, is_comparable, is_reportable, }; -pub use idioms::{Context, Idiom, IdiomRefusal, all_different}; +pub use idioms::{Context, Idiom, IdiomRefusal, Recognized, all_different}; pub use macro_derivation::{MacroDerivationRequest, derive_macro_behavior}; pub use pack::{ BuiltLibraryPack, LibraryPackRequest, behavior_file_name, build_library_pack, registry_sources, @@ -366,22 +366,18 @@ fn is_plainer(candidate: &str, current: &str) -> bool { (candidate.len(), candidate) < (current.len(), current) } -/// Report a match at the statements that carry it. +/// The statements a match occupies, or the whole function when it has no run. /// /// A behavior usually occupies a run of consecutive statements inside a larger /// function, and naming the function alone leaves a reader to find it again. /// When the run cannot be located — the behavior matched somewhere nested, or /// the body is a single expression — the function is the honest answer. -fn behavior_match( +fn located_span( file: &ParsedFile, function: &infact_rust_normalize::NormalizedFunction, located: Option>, - fused: bool, - catalog: &ExternalCatalog, - behavior: &DerivedLibraryBehavior, - alternatives: Vec, -) -> Result> { - let span = located +) -> SourceSpan { + located .and_then(|steps| { let first = function.statements.get(steps.start)?; let last = function.statements.get(steps.end.checked_sub(1)?)?; @@ -403,7 +399,39 @@ fn behavior_match( end_line: function.end_line, start_column: None, end_column: None, - }); + }) +} + +/// What was read to reach a finding, named by the analyzer that reached it. +/// +/// Every fact this crate emits carries the same evidence about the same file, +/// and it was written out once per emitter until the third emitter made the +/// pattern hard to miss. +fn derivation_of(file: &ParsedFile, analyzer: &str) -> Derivation { + Derivation { + analyzer: analyzer.to_owned(), + analyzer_version: env!("CARGO_PKG_VERSION").to_owned(), + inputs: vec![InputEvidence { + path: file.path.clone(), + content_sha256: file.provenance.source_sha256.clone(), + parser_id: file.provenance.parser_id.clone(), + parser_version: file.provenance.parser_version.clone(), + grammar_sha256: file.provenance.grammar_sha256.clone(), + queries_sha256: file.provenance.queries_sha256.clone(), + }], + } +} + +/// Report a match at the statements that carry it. +fn behavior_match( + file: &ParsedFile, + function: &infact_rust_normalize::NormalizedFunction, + located: Option>, + fused: bool, + catalog: &ExternalCatalog, + behavior: &DerivedLibraryBehavior, + alternatives: Vec, +) -> Result> { Ok(Fact { value: LibraryBehaviorMatch { target: LibraryTarget::Callable { @@ -413,22 +441,14 @@ fn behavior_match( catalog_sha256: catalog.source_sha256.clone(), }, alternatives, - span, + span: located_span(file, function, located), fused, + // A derived behavior is the library's own implementation, so + // matching it says the code IS what the API does. Nothing further + // has to hold for the swap. conditions: Vec::new(), }, - derivation: Derivation { - analyzer: "rust.library-behaviors".to_owned(), - analyzer_version: env!("CARGO_PKG_VERSION").to_owned(), - inputs: vec![InputEvidence { - path: file.path.clone(), - content_sha256: file.provenance.source_sha256.clone(), - parser_id: file.provenance.parser_id.clone(), - parser_version: file.provenance.parser_version.clone(), - grammar_sha256: file.provenance.grammar_sha256.clone(), - queries_sha256: file.provenance.queries_sha256.clone(), - }], - }, + derivation: derivation_of(file, "rust.library-behaviors"), }) } @@ -748,6 +768,12 @@ fn collect_enum_macro_matches( /// when a catalog for that package is loaded — the same rule derived behaviors /// follow, and for the same reason: naming a version that was never read would /// be inventing provenance. +/// +/// What separates this from the derived-behavior loop above is only where the +/// shape comes from: there it is normalized out of a library's own source, here +/// it is written down, because no library writes the thing it replaces. +/// Everything after the shape — matching it, placing it, naming the target, +/// carrying the evidence — is the same code. fn collect_idiom_matches( file: &ParsedFile, function: &infact_rust_normalize::NormalizedFunction, @@ -758,9 +784,9 @@ fn collect_idiom_matches( let context = idioms::Context { can_allocate: !function.is_const, }; - if idioms::all_different(candidate, context).is_err() { + let Ok(walked) = idioms::all_different(candidate, context) else { return Ok(()); - } + }; let idiom = idioms::Idiom::AllDifferent; let (package, path) = idiom.callable_path(); // The callable has to be present AND still answer the question the idiom @@ -777,43 +803,39 @@ fn collect_idiom_matches( let Some((catalog, callable)) = found else { return Ok(()); }; - output.insert(Fact { - value: LibraryBehaviorMatch { - target: LibraryTarget::Callable { - package: catalog.package.clone(), - version: catalog.version.clone(), - path: path.to_owned(), - catalog_sha256: catalog.source_sha256.clone(), - }, - // The recognizer names one API, so there is nothing to choose - // between; what is uncertain about the recommendation is in the - // conditions rather than in which callable it points at. - alternatives: Vec::new(), - fused: false, - span: SourceSpan { - path: file.path.clone(), - start_byte: Some(function.start_byte), - end_byte: Some(function.end_byte), - start_line: function.start_line, - end_line: function.end_line, - start_column: None, - end_column: None, + // Point at the statements the walk occupies rather than the whole function, + // by the route every other finding is placed by. The shape that matched is + // rebuilt from what the recognizer resolved, so what gets located is what + // was actually recognized. + let mut located = function.form.locate_all(&walked.shape); + if located.is_empty() { + located = candidate.locate_all(&walked.shape); + } + let placements = if located.is_empty() { + vec![None] + } else { + located.into_iter().map(Some).collect() + }; + for steps in placements { + output.insert(Fact { + value: LibraryBehaviorMatch { + target: LibraryTarget::Callable { + package: catalog.package.clone(), + version: catalog.version.clone(), + path: path.to_owned(), + catalog_sha256: catalog.source_sha256.clone(), + }, + // The recognizer names one API, so there is nothing to choose + // between; what is uncertain about the recommendation is in the + // conditions rather than in which callable it points at. + alternatives: Vec::new(), + fused: false, + span: located_span(file, function, steps), + conditions: idiom.conditions(callable), }, - conditions: idiom.conditions(callable), - }, - derivation: Derivation { - analyzer: "rust.idioms".to_owned(), - analyzer_version: env!("CARGO_PKG_VERSION").to_owned(), - inputs: vec![InputEvidence { - path: file.path.clone(), - content_sha256: file.provenance.source_sha256.clone(), - parser_id: file.provenance.parser_id.clone(), - parser_version: file.provenance.parser_version.clone(), - grammar_sha256: file.provenance.grammar_sha256.clone(), - queries_sha256: file.provenance.queries_sha256.clone(), - }], - }, - }); + derivation: derivation_of(file, "rust.idioms"), + }); + } Ok(()) }