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/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/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/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/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:?}" + ); +} 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"] + ); +} 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.