From 6ce555a97b5809dfb406339b08682e42c9f47420 Mon Sep 17 00:00:00 2001 From: Zack Maril Date: Tue, 25 Aug 2026 21:27:00 +0000 Subject: [PATCH 1/3] Say the mkdir the regeneration command needs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The catalog is not committed and git does not track an empty directory, so `infact-packs/rust-std/api/` is absent on a fresh checkout — and `--output` does not create the path it is given. The documented command therefore failed on the one machine state it exists for, with a bare `No such file or directory` that names neither the path nor the reason. Found by running it on a fresh checkout of merged main rather than by reading it. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_017ghYbUJFcVfkKYacbTVJsR --- infact-packs/rust-std/README.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/infact-packs/rust-std/README.md b/infact-packs/rust-std/README.md index 175320a..1bd3c7a 100644 --- a/infact-packs/rust-std/README.md +++ b/infact-packs/rust-std/README.md @@ -7,10 +7,16 @@ that ships as a rustup component rather than from a checkout of the compiler: rustup component add rust-docs-json --toolchain nightly J=$(rustc +nightly --print sysroot)/share/doc/rust/json V=$(rustc +nightly --version | awk '{print $2}') +mkdir -p infact-packs/rust-std/api infact catalog "$J/core.json" --package core --version "$V" \ --output "infact-packs/rust-std/api/core-$V.json" ``` +The `mkdir` is not decoration. The catalog is not committed, git does not track +an empty directory, and `--output` does not create the path it is given — so on +a fresh checkout `api/` does not exist and the command fails with a bare +`No such file or directory`. + `alloc.json` and `std.json` build the same way and are not kept here, because nothing yet matches against anything they add. Regenerating one takes two seconds; carrying it does not. From 719bd37b42df78ef05c5dab6e4af20d09f5cfd4a Mon Sep 17 00:00:00 2001 From: Zack Maril Date: Wed, 26 Aug 2026 06:38:07 +0000 Subject: [PATCH 2/3] Let the narrower behavior win where both landed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `Option::and_then` is `match self { Some(x) => f(x), None => None }`, and the hole swallows whatever a narrower way of consuming an `Option` puts there. So it matches `map`, and `filter`, and `ok_or`, and says less about each than they do about themselves. On clippy's `manual_map` test it landed on fifteen of the same lines `Option::map` did; every one of those was a second, weaker finding on code that already had a better one. `is_reportable` was the existing answer to a behavior that describes too much, and it does not reach this: `and_then` names two variants, so it passes the anchor floor while still subsuming everything narrower. What separates them is not how much either names on its own but how they stand to EACH OTHER, and a form used as a pattern already answers that — a form that matches another behavior's form accepts everywhere that one does and elsewhere besides. Asked per placement rather than per pack, which is the part that matters. Being broader is not being wrong: code that really does reimplement `and_then` should hear about it, and a pack-wide rule would lose that. It is only grounds for standing aside where something narrower has already landed. Measured. On the 13-file clippy corpus, unchanged: 26/201, 36 reported, 36/36 on-target — this removes nothing that was right. On a wider sample of 18 `manual_*` lints fetched for this, reported findings fall from 59 to 44 and off-target from 43 to 28, with all ten on-target findings kept. The on-target share goes from 16/59 to 16/44. What is left off-target is not this problem. Eleven are `manual_unwrap_or_default` reporting `unwrap_or`, which is correct and merely less sharp than the API clippy names. Eleven are `manual_filter` reporting `and_then` because `Option::filter` derives no behavior at all — nothing narrower exists to win, and that is a recall gap. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_017ghYbUJFcVfkKYacbTVJsR --- crates/infact-rust-behaviors/src/lib.rs | 54 +++++- .../infact-rust-behaviors/tests/collisions.rs | 57 ++++++ .../tests/fixtures/subsumption/src/lib.rs | 10 ++ .../tests/subsumption.rs | 167 ++++++++++++++++++ 4 files changed, 279 insertions(+), 9 deletions(-) create mode 100644 crates/infact-rust-behaviors/tests/fixtures/subsumption/src/lib.rs create mode 100644 crates/infact-rust-behaviors/tests/subsumption.rs diff --git a/crates/infact-rust-behaviors/src/lib.rs b/crates/infact-rust-behaviors/src/lib.rs index f2f1dc6..b2be1f4 100644 --- a/crates/infact-rust-behaviors/src/lib.rs +++ b/crates/infact-rust-behaviors/src/lib.rs @@ -228,7 +228,11 @@ pub fn analyze_repository( .and_modify(|chosen| chosen.0.push(behavior)) .or_insert_with(|| (vec![behavior], fused)); } - for (mut sharing, fused) in best.into_values() { + // Where each matched behavior actually lands, worked out before + // anything is reported, because which behavior to report is a + // question about what else matched THERE. + let mut found = Vec::new(); + for (program, (mut sharing, fused)) in best { // Only callables whose catalog is present can be named. sharing.retain(|behavior| catalog_for(catalogs, behavior).is_some()); // Name the API a caller should reach for, which is the one @@ -248,9 +252,6 @@ pub fn analyze_repository( let Some((behavior, rest)) = sharing.split_first() else { continue; }; - let Some(catalog) = catalog_for(catalogs, behavior) else { - continue; - }; let alternatives: Vec = rest .iter() .filter_map(|other| { @@ -267,23 +268,40 @@ pub fn analyze_repository( // Reporting only the first surfaced the rest one re-run at a // time, and a reader fixing the one they were shown had no way // to know the others existed. - let mut located = function.form.locate_all(&behavior.program); + let mut located = function.form.locate_all(program); if located.is_empty() { - located = candidate.locate_all(&behavior.program); + located = candidate.locate_all(program); } // A behavior that matched but cannot be placed is still a // finding; it just has no span to point at. - let placements = if located.is_empty() { + let placements: Vec>> = if located.is_empty() { vec![None] } else { located.into_iter().map(Some).collect() }; + found.push((program, *behavior, alternatives, fused, placements)); + } + for (index, (program, behavior, alternatives, fused, placements)) in + found.iter().enumerate() + { + let catalog = match catalog_for(catalogs, behavior) { + Some(catalog) => catalog, + None => continue, + }; for steps in placements { + // A behavior that another matched behavior is broader than + // says strictly less about this code than that one does, + // and saying both is saying the weaker thing twice. + if found.iter().enumerate().any(|(other, entry)| { + other != index && entry.4.contains(steps) && is_broader(program, entry.0) + }) { + continue; + } matches.insert(behavior_match( file, &function, - steps, - fused, + steps.clone(), + *fused, catalog, behavior, alternatives.clone(), @@ -347,6 +365,24 @@ fn delegates_to(outer: &DerivedLibraryBehavior, inner: &DerivedLibraryBehavior) .any(|step| step.callable_path == target) } +/// Whether one behavior's form describes everything another's does, and more. +/// +/// A form used as a pattern accepts wherever it matches, so a form that matches +/// ANOTHER behavior's form accepts everywhere that one does and elsewhere +/// besides. `Option::and_then` is `match self { Some(x) => f(x), None => None }` +/// and the hole swallows what every narrower way of consuming an `Option` puts +/// there — `map`, `filter`, `ok_or`. Measured on clippy's `manual_map` test it +/// landed on fifteen of the same lines `map` did, saying less about each. +/// +/// Being broader is not being wrong, and it is not grounds for leaving a +/// behavior out of a pack: code that really does reimplement `and_then` should +/// hear about it. It is only grounds for standing aside where something +/// narrower has already landed, which is why this is asked per placement rather +/// than once per pack. +fn is_broader(broad: &Form, narrow: &Form) -> bool { + broad != narrow && narrow.contains(broad) && !broad.contains(narrow) +} + fn catalog_for<'a>( catalogs: &'a [ExternalCatalog], behavior: &DerivedLibraryBehavior, diff --git a/crates/infact-rust-behaviors/tests/collisions.rs b/crates/infact-rust-behaviors/tests/collisions.rs index b42902c..e10b5f1 100644 --- a/crates/infact-rust-behaviors/tests/collisions.rs +++ b/crates/infact-rust-behaviors/tests/collisions.rs @@ -114,3 +114,60 @@ fn distinct_callables_derive_distinct_behaviors() { erased.join("\n ") ); } + +/// A behavior another one is broader than stands aside where that one landed. +/// +/// `Option::and_then` is `match self { Some(x) => f(x), None => None }`, and the +/// hole swallows what every narrower way of consuming an `Option` puts there. +/// Measured on clippy's `manual_map` test it landed on fifteen of the same lines +/// `Option::map` did, saying less about each. +#[test] +fn a_broader_behavior_stands_aside_where_a_narrower_one_landed() { + use infact_core::Form; + use infact_normalize::{Arm, Pattern}; + + let hole_applied = || Form::Call { + callee: Box::new(Form::Free(1)), + arguments: vec![Form::Local(0)], + }; + let none = || Form::Variant { + name: "None".to_owned(), + payload: Vec::new(), + }; + let consuming = |taken: Form| { + Form::select( + Form::Free(0), + vec![ + Arm { + pattern: Pattern::Variant { + name: "Some".to_owned(), + parts: vec![Pattern::Binding(0)], + }, + body: taken, + }, + Arm { + pattern: Pattern::Variant { + name: "None".to_owned(), + parts: Vec::new(), + }, + body: none(), + }, + ], + ) + }; + // `and_then` hands back whatever the caller's function returned; `map` + // wraps it. The first accepts the second and not the other way about. + let and_then = consuming(hole_applied()); + let map = consuming(Form::Variant { + name: "Some".to_owned(), + payload: vec![hole_applied()], + }); + assert!( + map.contains(&and_then), + "and_then must accept map, or it is not the broader of the two" + ); + assert!( + !and_then.contains(&map), + "map must not accept and_then, or neither is broader" + ); +} diff --git a/crates/infact-rust-behaviors/tests/fixtures/subsumption/src/lib.rs b/crates/infact-rust-behaviors/tests/fixtures/subsumption/src/lib.rs new file mode 100644 index 0000000..4a725a4 --- /dev/null +++ b/crates/infact-rust-behaviors/tests/fixtures/subsumption/src/lib.rs @@ -0,0 +1,10 @@ +pub fn mapped(value: Option) -> Option { + match value { + Some(inner) => Some(double(inner)), + None => None, + } +} + +fn double(value: i32) -> i32 { + value * 2 +} diff --git a/crates/infact-rust-behaviors/tests/subsumption.rs b/crates/infact-rust-behaviors/tests/subsumption.rs new file mode 100644 index 0000000..e5871f3 --- /dev/null +++ b/crates/infact-rust-behaviors/tests/subsumption.rs @@ -0,0 +1,167 @@ +#![allow(clippy::unwrap_used, clippy::expect_used)] +//! Which of two behaviors that both match is the one worth reporting. + +use std::path::PathBuf; + +use entl_tree_sitter::ParserCatalog; +use infact_core::{ + CallableContainer, DERIVED_LIBRARY_BEHAVIOR_SCHEMA, DerivedLibraryBehavior, + EXTERNAL_CATALOG_SCHEMA, ExternalCallable, ExternalCatalog, Form, ImplementationEvidence, + SourceSpan, +}; +use infact_normalize::{Arm, Pattern}; +use infact_rust_behaviors::analyze_repository; + +fn crate_root() -> PathBuf { + PathBuf::from(env!("CARGO_MANIFEST_DIR")) +} + +/// `match self { Some(x) => taken, None => None }`, which is the shape every +/// way of consuming an `Option` shares. +fn consuming(taken: Form) -> Form { + Form::select( + Form::Free(0), + vec![ + Arm { + pattern: Pattern::Variant { + name: "Some".to_owned(), + parts: vec![Pattern::Binding(0)], + }, + body: taken, + }, + Arm { + pattern: Pattern::Variant { + name: "None".to_owned(), + parts: Vec::new(), + }, + body: Form::Variant { + name: "None".to_owned(), + payload: Vec::new(), + }, + }, + ], + ) +} + +fn applied() -> Form { + Form::Call { + callee: Box::new(Form::Free(1)), + arguments: vec![Form::Local(0)], + } +} + +fn behavior(path: &str, program: Form) -> DerivedLibraryBehavior { + DerivedLibraryBehavior { + schema: DERIVED_LIBRARY_BEHAVIOR_SCHEMA, + callable_package: "core".to_owned(), + callable_version: "1.0.0".to_owned(), + callable_path: path.to_owned(), + catalog_sha256: "sha256:test".to_owned(), + implementation: vec![ImplementationEvidence { + callable_path: path.to_owned(), + span: SourceSpan { + path: PathBuf::from("src/option.rs"), + start_byte: Some(0), + end_byte: Some(1), + start_line: 1, + end_line: 1, + start_column: None, + end_column: None, + }, + source_sha256: "sha256:elsewhere".to_owned(), + }], + program, + } +} + +fn catalog(paths: &[&str]) -> ExternalCatalog { + ExternalCatalog { + schema: EXTERNAL_CATALOG_SCHEMA, + package: "core".to_owned(), + version: "1.0.0".to_owned(), + rustdoc_format: 1, + source_sha256: "sha256:test".to_owned(), + callables: paths + .iter() + .map(|path| ExternalCallable { + path: (*path).to_owned(), + container: CallableContainer::Type { + path: "Option".to_owned(), + }, + signature: None, + }) + .collect(), + } +} + +/// The broader of two behaviors stands aside where the narrower one landed. +/// +/// `and_then` hands back whatever the caller's function returned, so its hole +/// swallows the `Some(..)` that `map` puts there. Both match this code; only +/// one of them says what it does. +#[test] +fn the_narrower_behavior_is_the_one_reported() { + let parsers = ParserCatalog::discover([crate_root().join("../../../entl/parser-packs")]); + assert!(parsers.errors.is_empty(), "{:?}", parsers.errors); + let behaviors = vec![ + behavior( + "core::Option::map", + consuming(Form::Variant { + name: "Some".to_owned(), + payload: vec![applied()], + }), + ), + behavior("core::Option::and_then", consuming(applied())), + ]; + let catalogs = vec![catalog(&["core::Option::map", "core::Option::and_then"])]; + + let report = analyze_repository( + crate_root().join("tests/fixtures/subsumption"), + &parsers.catalog, + &catalogs, + &behaviors, + &[], + ) + .unwrap(); + + let reported: Vec<&str> = report + .matches + .iter() + .map(|fact| fact.value.target.path()) + .collect(); + assert_eq!( + reported, + vec!["core::Option::map"], + "and_then matches this too, and saying so as well is saying the weaker thing twice" + ); +} + +/// Standing aside is per placement, not per pack. +/// +/// A behavior that is broader than another is still the only thing that +/// describes code the narrower one does not reach, and dropping it from the +/// pack would lose that. +#[test] +fn a_broader_behavior_still_reports_where_nothing_narrower_matched() { + let parsers = ParserCatalog::discover([crate_root().join("../../../entl/parser-packs")]); + let behaviors = vec![behavior("core::Option::and_then", consuming(applied()))]; + let catalogs = vec![catalog(&["core::Option::and_then"])]; + + let report = analyze_repository( + crate_root().join("tests/fixtures/subsumption"), + &parsers.catalog, + &catalogs, + &behaviors, + &[], + ) + .unwrap(); + + assert_eq!( + report + .matches + .iter() + .map(|fact| fact.value.target.path()) + .collect::>(), + vec!["core::Option::and_then"] + ); +} From 89fe402fd9933f8e4d1c9a7af3e1c6f50358f1d0 Mon Sep 17 00:00:00 2001 From: Zack Maril Date: Wed, 26 Aug 2026 07:26:21 +0000 Subject: [PATCH 3/3] A helper nested in a method body is not a method MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `Iterator::fold` reported that no implementation was found. It has one: a loop that accumulates, right there in the trait. What went wrong is that `max_by` and `min_by` each declare their own `fn fold` helper inside their bodies, the container was carried down into those bodies, and so three callables answered to `Iterator::fold`. The resolver refuses a name two callables answer to — correctly — and a method with a perfectly good body fell out of the pack. The container stops at a function boundary now. A function declared inside another function's body is a local helper, not a method of whatever type surrounds them both. Measured on core: 261 behaviors to 263, and nothing lost. `Iterator::fold` and `Iterator::count` derive where they did not. WHAT I PREDICTED AND DID NOT GET: that the whole family built on `fold` would follow it — `sum`, `max`, `min`, `last`, `reduce`, `product`, `nth`. None did. They fail for their own separate reasons, which are not this one and are not yet known. That is the sixth prediction in this file's history to be wrong about what a correct fix would reach, and it is recorded here because the tally it came from is only useful if the reach is measured rather than argued. The clippy corpus is unchanged at 26/201, 36/36 on-target. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_017ghYbUJFcVfkKYacbTVJsR --- .../infact-rust-behaviors/src/derivation.rs | 12 +++++++ .../tests/fixtures/nested-helper/src/lib.rs | 26 ++++++++++++++ .../infact-rust-behaviors/tests/resolution.rs | 35 +++++++++++++++++++ 3 files changed, 73 insertions(+) create mode 100644 crates/infact-rust-behaviors/tests/fixtures/nested-helper/src/lib.rs diff --git a/crates/infact-rust-behaviors/src/derivation.rs b/crates/infact-rust-behaviors/src/derivation.rs index a0693b5..8dba1c3 100644 --- a/crates/infact-rust-behaviors/src/derivation.rs +++ b/crates/infact-rust-behaviors/src/derivation.rs @@ -158,6 +158,18 @@ fn collect_functions<'a>( "trait_item" => node .child_by_field_name("name") .and_then(|name| node_text(name, &file.source)), + // A function declared inside another function's body is a local helper, + // not a method of whatever type surrounds them both. Carrying the + // container down made every such helper answer to the same name a + // sibling method does, and the resolver refuses a name two callables + // answer to — so a method with a perfectly good body reported that no + // implementation was found. + // + // `Iterator::fold` is the case that matters: `max_by` and `min_by` + // declare their own `fn fold` helpers, so three callables claimed the + // name and none of them resolved. Everything built on `fold` went with + // it — `count`, `sum`, `last`, `max`, `min`, `reduce`, `product`, `nth`. + "function_item" => None, _ => container, }; // an inline `mod` that is not public hides everything inside it diff --git a/crates/infact-rust-behaviors/tests/fixtures/nested-helper/src/lib.rs b/crates/infact-rust-behaviors/tests/fixtures/nested-helper/src/lib.rs new file mode 100644 index 0000000..7c116a9 --- /dev/null +++ b/crates/infact-rust-behaviors/tests/fixtures/nested-helper/src/lib.rs @@ -0,0 +1,26 @@ +pub trait Walk { + fn items(&self) -> Vec; + + /// A provided method with a real body and a name nothing else declares — + /// except a helper nested inside a sibling, below. + fn total(&self) -> i32 { + let mut sum = 0; + for value in self.items() { + sum += value; + } + sum + } + + /// Declares its own `total`, the way `Iterator::max_by` declares its own + /// `fold`. + fn biggest(&self) -> i32 { + fn total(left: i32, right: i32) -> i32 { + if left > right { left } else { right } + } + let mut best = 0; + for value in self.items() { + best = total(best, value); + } + best + } +} diff --git a/crates/infact-rust-behaviors/tests/resolution.rs b/crates/infact-rust-behaviors/tests/resolution.rs index 89d1005..ad405e1 100644 --- a/crates/infact-rust-behaviors/tests/resolution.rs +++ b/crates/infact-rust-behaviors/tests/resolution.rs @@ -80,3 +80,38 @@ fn a_constructor_still_reaches_its_own_types_method() { "`keys` should carry its own Cursor::next, which reads `self.inner`:\n {form}" ); } + +/// A helper nested in a method body does not answer to the surrounding type. +/// +/// Carried down, the container made a local `fn total` a second callable named +/// `Walk::total`, and the resolver refuses a name two callables answer to — so +/// the real `total`, with a perfectly good body, reported that no +/// implementation was found. +/// +/// `Iterator::fold` is the case this was found on: `max_by` and `min_by` +/// declare their own `fn fold` helpers, and three callables claimed the name. +#[test] +fn a_helper_nested_in_a_method_does_not_shadow_its_sibling() { + let crate_root = std::path::PathBuf::from(env!("CARGO_MANIFEST_DIR")); + let parsers = + entl_tree_sitter::ParserCatalog::discover([crate_root.join("../../../entl/parser-packs")]); + assert!(parsers.errors.is_empty(), "{:?}", parsers.errors); + + let derived = infact_rust_behaviors::derive_library( + crate_root.join("tests/fixtures/nested-helper"), + &parsers.catalog, + "probe", + "0.1.0", + ) + .expect("deriving the fixture"); + + let paths: std::collections::BTreeSet<&str> = derived + .behaviors + .iter() + .map(|behavior| behavior.callable_path.as_str()) + .collect(); + assert!( + paths.contains("probe::Walk::total"), + "the method the nested helper shares a name with must still resolve: {paths:?}" + ); +}