Repository navigation
fix(rust): bind Type::fn() calls only to the named type's methods - #2595
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (5)
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughRust call resolution now uses associated-path qualifiers and recognized literal receiver types to match methods. It avoids falling through to unrelated project methods or same-named module functions. Regression tests cover these cases and preserve first-party resolution paths. ChangesRust associated-call resolution
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~50 minutes Change: Bug fix · Severity of issue fixed: Medium Sequence Diagram(s)sequenceDiagram
participant Caller
participant CallResolver
participant ImportProcessor
participant OwnerRegistry
Caller->>CallResolver: Submit Rust associated call
CallResolver->>ImportProcessor: Read recorded impl self paths
CallResolver->>OwnerRegistry: Find matching owner and trait methods
OwnerRegistry-->>CallResolver: Return matching methods or no match
CallResolver-->>Caller: Resolve matching method or leave call unresolved
Suggested reviewers: Merge Risk: ⚪ Minimal · up to This change stops Rust calls on standard, prelude, derived, or external types from linking to unrelated project methods, while first-party resolution is preserved. No merge-blocking issue was found in the supplied material. The full unit suite was not run locally and CI covers it. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to Owner-aware lookup reduces incorrect connections to unrelated methods. No introduced execution or privilege-escalation path was established. Risk remains low rather than minimal because failure recovery and concurrent reuse are not fully established. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Full details: Out of Scope Changes checkExplanation The merge-base-to-head changes include substantial unrelated work. The incremental diff adds CLI export and report-table changes for issues
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@greptileai please review the latest commit (cc5a1ec). Generated by Claude Code |
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…s own same-named type
|
@greptileai please review the latest commit (ec27207). Generated by Claude Code |
…path in its own module
|
@greptileai please review the latest commit (f4f6691). Generated by Claude Code |
…ne module holds impls of two same-named types
|
@greptileai please review the latest commit (cddfe4c). Generated by Claude Code |
|
@greptileai please review the latest commit (1224a84). Generated by Claude Code |
…helper to keep _resolve_function_call under the complexity limit (#2543)
|
Sonar raised one issue at 1224a84: Fixed in e853eb1. The three stages that run before the caller-independent cache ( Results: this PR's tests plus the call-resolver, call-processor, Rust and Python-local tests gave 859 passed, 3 skipped. ruff and ty are clean. @greptileai please review the latest commit (e853eb1). Generated by Claude Code |
|
This PR is set for a Plus review. TREX isn't supported with Plus or Apex reviews yet, so Greptile reviews this PR at Base while TREX is on. |
|
Main's #2595 decides a Type::fn() whose path head binds an external crate before any probe, reading the manifest's pathless entries as external. A pathless entry renamed onto a workspace member (`base-alias = { version = "0.1", package = "my-base" }`) fetches the crate the repo holds, so `use base_alias::Meter; Meter::new()` lost its edge after the merge. The check now asks rust_head_is_member_dep first, as this branch's other external-head check does. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EbsCWQcSTGQXv3g5sk418m
Summary
Type::f()call whoseTypeis a std/prelude type (String::new(),Vec::new(),String::from(..)), a prelude trait (Default::default(),Clone::clone(&x)), a std path (std::string::String::new(),fmt::Debug::fmt(..)underuse std::fmt) or a manifest-proven external crate (tokio::sync::Mutex::new(..)) now resolves only to a first-party impl block on that very type (impl From<Foo> for u8), and otherwise records no edge. On main the qualifier was dropped and the bare name went to the trie, which bound whichever project type's same-named method sat closest (Frame::new,Frame::from, ...). A new step,_resolve_rust_external_path, runs ahead of the name-based probes and makes that decision.Type::f()/Self::f()on a first-party type whose method the graph cannot see, because a derive generates it (Config::default(), clap'sCli::parse()), now binds only to that type's own methods. Those are an impl block in another module or a trait default the type implements. If none exists, the call gets no edge. The same restriction covers a method call on a literal ("42".parse(), which has typestr). This is the_resolve_rust_named_ownerstep, the last one before the trie. A type alias, a generic parameter and an unplaced external type are unknown rather than known, so they keep the old fallback.Type::f()andrecv.f()never name the caller module's ownfn f. On main,String::new(),HashMap::new()(explicitly imported from std) and"42".parse()bound the file's ownfn new/fn parse.Side effect:
Self::f()on the caller's own type now resolvesexact(washeuristic).The parser fingerprint changes (
parsers/andconstants/are touched), so an existing graph re-parses once on its next sync.Demo
Animated runs
The screenshot and GIF show a real session on the issue's repro crate (
cargo checkpasses), indexed into a fresh Memgraph 3.3. On main,cgr graph callees …src.app.buildlists fiveheuristicedges:String::new()andVec::new()go toFrame.new,String::from("x")toFrame.from, the derivedConfig::default()toFrame.defaultand"42".parse()toFrame.parse. On this branch the same sync re-parses once andbuild()has no CALLS edges. In both runs the genuineFrame::parse("ok")inreal()staysexact.Type of Change
Related Issues
Fixes #2543
Test Plan
make test-paralleloruv run pytest -n auto -m "not integration"): not run locally in full (shared machine); CI runs the full suite on every push.codebase_rag/tests/test_rust_std_assoc_calls.pymake test-integration, requires Docker)Red (fail on main's resolver, pass here):
test_prelude_and_derived_assoc_calls_do_not_bind_project_methods(the issue's repro verbatim)test_derived_parser_parse_does_not_bind_the_frame_parser(mini-redisCli::parse())test_prelude_trait_qualified_calls_do_not_bind_project_methodstest_self_qualified_derived_call_does_not_bind_another_typetest_std_type_calls_do_not_bind_a_same_module_free_functiontest_std_path_qualified_calls_do_not_bind_project_methodstest_external_crate_path_does_not_bind_project_methodstest_prelude_type_does_not_bind_an_out_of_scope_same_named_typeNegative (behaviour that must not change; pass on main and here):
test_first_party_assoc_calls_still_resolve_exact:Frame::new(),use … as F; F::parse()andSelf::new().test_first_party_type_shadowing_a_prelude_name_resolves_locally: a cratestruct String, defined in the module or imported, staysexact.test_impl_elsewhere_trait_default_and_type_alias_keep_their_edges: an impl block in another module, a trait default method and atype Alias = Frameall keep their edges.test_std_type_method_from_a_first_party_impl_still_binds:u8::from(..)still reachesimpl From<Foo> for u8.test_literal_receiver_types/test_non_literal_receiver_has_no_literal_type: a raw identifier (r#type) is not a string literal.Targeted scope run locally (all green):
codebase_rag/tests/test_rust*.py(613 passed, 3 skipped).test_call_resolver,test_call_processor,test_call_processor_integration,test_edge_resolution,test_edge_site_properties,test_value_receiver_class_resolution,test_typed_receiver_lacks_method,test_interface_dispatch_single_impl,test_implements_overrides,test_method_calls_caller_attribution,test_enum_variant_nodes,test_type_inference_iterative,test_polyglot_eval,test_dead_code_eval,test_graph_determinism,test_rs_macro_definition_nodes,test_dynamic_trace_rust,test_cross_file_inherits_implements,test_watch_created_file_calls,test_type_facts,test_same_line_duplicate_qns,test_reingest_new_file_importers,test_structural_relationships,test_generated_docs_are_current,test_parser_fingerprint*.Also:
ruff check,ruff formatandty checkpass. The one remainingruff checkfinding on main is PLW0108 inflow_verdict.py, which PR #2537 fixes.Review follow-up (ec27207)
impllives elsewhere now also requires thatimplto be on a foreign type. Theimplmodule's own binding decides. If nothing there binds the name, the crate that defines a Rust type of that name wins.test_prelude_call_does_not_bind_an_impl_elsewhere_of_the_crates_own_type, covering both theuseand the path-qualifiedimplforms.test_imported_own_type_still_reaches_its_impl_elsewhereandtest_trait_impl_on_the_prelude_type_still_binds.test_rust_*.pypass (631). ruff and ty are clean.Review follow-up (f4f6691)
test_trait_impl_on_a_std_path_reaches_it_beside_the_crates_own_type.u8::fromtests still pass.Sonar follow-up (e853eb1)
_resolve_function_call: 16 → 14. The three caller-scoped stages (receiver shadow, Rust block scope, caller scope) now run in order through one helper,_resolve_caller_scoped_stages, ahead of the caller-independent resolution cache. Behaviour is unchanged.Checklist
make pre-commit)# type: ignore,cast(),Any, orobjecttype hintsReview follow-up (cddfe4c)
Stringimpls collided (Greptile 4172155393): fixed. One module can hold an impl of the crate's ownStringand a trait impl forstd::string::String, and both were judged by the same shared record. Each impl method now also records the self type its own block was written for, and the foreign-type check reads that first. It falls back to the shared record only for trait implementers, and for files that an incremental run does not re-parse.test_std_path_call_skips_the_crates_impl_in_a_module_with_both.test_std_path_call_still_reaches_the_std_impl_in_a_module_with_both. The earlier std-path, prelude and impl-elsewhere tests still pass.🤖 Generated with Claude Code
https://claude.ai/code/session_01QjpNsgSNyL5gsmAFqfuoD6
Generated by Claude Code
Summary by CodeRabbit
Generated by Claude Code