fix(lsp): callHierarchy/incomingCalls returns top-level/script callers (#3093) - #3191
Conversation
#3093) When a call site has no enclosing `sub` (i.e., it is top-level in a script), both the workspace-index path and the open-doc fallback silently dropped the reference. This made `$app->run` in script/real-baseline.pl invisible to incomingCalls for App::run. Fix: synthesize a file-level CallHierarchyItem (kind=File/1, name=basename) for each call site that has no enclosing callable, in both paths: - hierarchy.rs: workspace-index path now uses unwrap_or_else to create the synthetic item instead of wrapping the whole body in if-let-Some. - call_hierarchy_provider/mod.rs: FunctionCall and MethodCall arms now call current_function.cloned().unwrap_or_else(|| <file-level item>) instead of guarding with if-let-Some(from). Also adds uri_basename() helper and fixes a pre-existing double-visit bug in find_incoming_calls: named-sub arm now returns early after visiting children so the bottom visitor doesn't re-visit with the outer (None) context, which would have created spurious file-level callers for calls inside subs. Adds SymbolKind.File (1) mapping to CallHierarchyItem::to_json(). Test: un-ignores scenario_22_call_hierarchy_incoming_to_run_hard_assert and strengthens it to assert the caller URI contains "real-baseline.pl". Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015AGtUiPPvBkWTeN9nm6q6N
Summary by CodeRabbit
WalkthroughAdds a file-level caller fallback for top-level incoming-call matches in both the AST-based and workspace-index call hierarchy paths, maps the new ChangesTop-level caller synthesis for callHierarchy/incomingCalls
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements synthetic file-level callers for top-level call sites that are not enclosed in any named subroutine, preventing them from being silently dropped in incoming call hierarchies. It also enables and updates the corresponding integration test to verify this behavior. The review feedback suggests making the newly introduced uri_basename helper function pub(crate) and reusing it in hierarchy.rs to eliminate code duplication.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| fn uri_basename(uri: &str) -> String { | ||
| uri.rsplit('/').find(|s| !s.is_empty()).unwrap_or(uri).to_string() | ||
| } |
There was a problem hiding this comment.
The uri_basename helper function is currently private to this module, but the exact same logic is duplicated in hierarchy.rs. Making this function pub(crate) allows it to be reused across the crate, improving maintainability and reducing code duplication.
| fn uri_basename(uri: &str) -> String { | |
| uri.rsplit('/').find(|s| !s.is_empty()).unwrap_or(uri).to_string() | |
| } | |
| pub(crate) fn uri_basename(uri: &str) -> String { | |
| uri.rsplit('/').find(|s| !s.is_empty()).unwrap_or(uri).to_string() | |
| } |
| let raw_uri = location.uri.as_str(); | ||
| let basename = raw_uri | ||
| .rsplit('/') | ||
| .find(|s| !s.is_empty()) | ||
| .unwrap_or(raw_uri) | ||
| .to_string(); |
There was a problem hiding this comment.
This block duplicates the URI basename extraction logic defined in call_hierarchy_provider/mod.rs. Reusing the uri_basename helper function here reduces code duplication and ensures consistent behavior.
let raw_uri = location.uri.as_str();
let basename = crate::call_hierarchy_provider::uri_basename(raw_uri);
Independent verification receiptOrchestrator verified the durable artifact (not the builder self-report). External-truth logic review — two seams the test cannot catch, both sound:
Empirical (rebuilt + re-ran in the worktree, real exit codes):
Step 0 confirmed: the top-level Residual / pending: the required CI gates (ripr+ New Gap Gate, Codecov/Patch 95) are the remaining unknowns — the new synthetic-caller branches add struct construction + branches that the static gates may flag; the hierarchy.rs workspace-index closure is integration-covered, not |
|
Droid encountered an error —— View job |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: be37fc838b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let from = self | ||
| .find_workspace_enclosing_callable(&callable_symbols, &location) | ||
| .unwrap_or_else(|| { |
There was a problem hiding this comment.
For top-level refs, this now turns every find_refs result without an enclosing callable into a file caller. In the default workspace path, WorkspaceIndex::find_refs for a qualified target such as App::run also returns bare run references, so an unrelated top-level Other::run()/run() in any indexed script reaches this branch and is reported as an incoming caller even though the package/receiver does not match. Please require an exact qualified match, or otherwise resolve the receiver, before synthesizing the file-level item.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/perl-lsp-rs/src/runtime/language/hierarchy.rs`:
- Around line 633-667: The synthetic file-level callers created in the
incoming-calls path of hierarchy.rs are not being recognized when they
round-trip back through the request parser. Update json_to_call_hierarchy_item()
to map kind 1 to a file item, and add matching file-scoped handling wherever
call-hierarchy requests are resolved so these items don’t get treated as
function/subroutine callers; if that support is not intended yet, keep the
hierarchy builder from emitting reusable file items in the incomingCalls path.
In
`@crates/perl-lsp-ux-tests/tests/ux_scenario_22_crossfile_extended_providers.rs`:
- Around line 598-607: The current incomingCalls assertion in the crossfile
extended providers test only checks that a caller URI contains real-baseline.pl,
which can still pass for a named subroutine inside the same file. Tighten the
check in the test around script_caller so it asserts the synthesized file-level
caller contract by validating from.kind == 1 or from.name == "real-baseline.pl"
in addition to the URI, ensuring the App::run incoming call is attributed to the
top-level script file rather than any arbitrary caller in that file.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2f0b6a00-5da4-4a00-a899-5c27dd0c49ee
📒 Files selected for processing (3)
crates/perl-lsp-rs/src/call_hierarchy_provider/mod.rscrates/perl-lsp-rs/src/runtime/language/hierarchy.rscrates/perl-lsp-ux-tests/tests/ux_scenario_22_crossfile_extended_providers.rs
| // The caller must be the script file — not just any non-empty result. | ||
| // This guards against vacuous passes where an unrelated item happens to appear. | ||
| let script_caller = calls.iter().find(|c| { | ||
| c["from"]["uri"].as_str().map(|u| u.contains("real-baseline.pl")).unwrap_or(false) | ||
| }); | ||
| assert!( | ||
| script_caller.is_some(), | ||
| "incomingCalls for `App::run` must include `real-baseline.pl` as a caller \ | ||
| (top-level `$app->run` call). Got callers: {calls:?}" | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Assert the synthesized file caller, not only the URI.
This still passes if the fixture later gains a named subroutine caller in the same script. Checking from.kind == 1 or from.name == "real-baseline.pl" would pin the file-level caller contract the PR is actually adding.
Suggested tightening
- let script_caller = calls.iter().find(|c| {
- c["from"]["uri"].as_str().map(|u| u.contains("real-baseline.pl")).unwrap_or(false)
- });
+ let script_caller = calls.iter().find(|c| {
+ c["from"]["uri"].as_str().map(|u| u.ends_with("/real-baseline.pl")).unwrap_or(false)
+ && c["from"]["kind"].as_u64() == Some(1)
+ && c["from"]["name"].as_str() == Some("real-baseline.pl")
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // The caller must be the script file — not just any non-empty result. | |
| // This guards against vacuous passes where an unrelated item happens to appear. | |
| let script_caller = calls.iter().find(|c| { | |
| c["from"]["uri"].as_str().map(|u| u.contains("real-baseline.pl")).unwrap_or(false) | |
| }); | |
| assert!( | |
| script_caller.is_some(), | |
| "incomingCalls for `App::run` must include `real-baseline.pl` as a caller \ | |
| (top-level `$app->run` call). Got callers: {calls:?}" | |
| ); | |
| // The caller must be the script file — not just any non-empty result. | |
| // This guards against vacuous passes where an unrelated item happens to appear. | |
| let script_caller = calls.iter().find(|c| { | |
| c["from"]["uri"].as_str().map(|u| u.ends_with("/real-baseline.pl")).unwrap_or(false) | |
| && c["from"]["kind"].as_u64() == Some(1) | |
| && c["from"]["name"].as_str() == Some("real-baseline.pl") | |
| }); | |
| assert!( | |
| script_caller.is_some(), | |
| "incomingCalls for `App::run` must include `real-baseline.pl` as a caller \ | |
| (top-level `$app->run` call). Got callers: {calls:?}" | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@crates/perl-lsp-ux-tests/tests/ux_scenario_22_crossfile_extended_providers.rs`
around lines 598 - 607, The current incomingCalls assertion in the crossfile
extended providers test only checks that a caller URI contains real-baseline.pl,
which can still pass for a named subroutine inside the same file. Tighten the
check in the test around script_caller so it asserts the synthesized file-level
caller contract by validating from.kind == 1 or from.name == "real-baseline.pl"
in addition to the URI, ensuring the App::run incoming call is attributed to the
top-level script file rather than any arbitrary caller in that file.
Codecov/Patch 95: real failure behind a stale "pending"The PR check still renders Cause: the new synthetic-caller lines are exercised only by the integration test ( Remediation in progress (no logic change): extract a shared |
…lib tests (#3093) Extract the inline file-level CallHierarchyItem construction into a `pub(crate) fn synthetic_file_level_caller(uri, range)` helper in `call_hierarchy_provider/mod.rs`. Both the open-document traversal path (FunctionCall / MethodCall arms) and the workspace-index path (hierarchy.rs unwrap_or_else closure) now call the helper, concentrating synthesis logic in one --lib-testable location. Add three --lib unit tests in mod.rs: - test_synthetic_file_level_caller_returns_file_item (direct helper test) - test_incoming_calls_top_level_method_call_synthesizes_file_caller - test_incoming_calls_top_level_function_call_synthesizes_file_caller Add one --lib unit test in hierarchy.rs: - test_incoming_calls_workspace_path_synthesizes_file_level_caller (uses Full workspace index with App->run() at top level of a script to cover the unwrap_or_else closure + seen-map insert, lines 633-645) Fixes Codecov/Patch-95 coverage gap on PR #3191. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Droid finished @EffortlessSteven's task —— View job Phase 2 (validator) — review validatedPR #3191 ( Read full diff, validated both candidate comments against current source, posted both as a single batched review. Results: 2 approved, 0 rejected
Review summary: both candidates identify real defects in the synthetic file-level caller contract that are not exercised by the new test (which only tests a single-package top-level call). Fix-forward on the builder side will need a qualified-key filter in |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/perl-lsp-rs/src/runtime/language/hierarchy.rs`:
- Line 980: Replace the `map_or(false, ...)` predicate in the `find` call within
`hierarchy.rs` with `is_some_and` on the `from.uri` lookup to satisfy clippy’s
`unnecessary_map_or` lint. Keep the existing `find` logic and `script.pl`
containment check, but update the `as_str()` handling to use `is_some_and` so
the `from`/`uri` access remains concise and warning-free.
- Around line 940-988: The test in hierarchy.rs introduces new `.expect()` calls
that will fail the `clippy::expect_used` gate. Update the
`handle_incoming_calls` assertion flow to avoid `expect` by using `assert!`,
`match`, or `ok_or_else` around the `result`, `value`, `calls`, and
`file_caller` checks, while keeping the same verification logic in the
incoming-calls test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 80715f96-15d9-46be-856d-8d6bbe53764f
📒 Files selected for processing (2)
crates/perl-lsp-rs/src/call_hierarchy_provider/mod.rscrates/perl-lsp-rs/src/runtime/language/hierarchy.rs
| server | ||
| .test_index_file_in_building_state(script_uri, script_text) | ||
| .expect("indexing script.pl"); | ||
| // Transition coordinator to Ready so workspace path is taken. | ||
| server.test_simulate_indexing_complete(); | ||
|
|
||
| // Also open as a document so open-doc fallback doesn't add duplicates. | ||
| open_doc(&server, script_uri, script_text); | ||
|
|
||
| // incomingCalls for "App::run" — data.packageName drives workspace_symbol_key. | ||
| let result = server.handle_incoming_calls(Some(json!({ | ||
| "item": { | ||
| "name": "run", | ||
| "kind": 6, | ||
| "uri": "file:///App.pm", | ||
| "range": { | ||
| "start": { "line": 0, "character": 0 }, | ||
| "end": { "line": 2, "character": 1 } | ||
| }, | ||
| "selectionRange": { | ||
| "start": { "line": 1, "character": 4 }, | ||
| "end": { "line": 1, "character": 7 } | ||
| }, | ||
| "data": { | ||
| "packageName": "App", | ||
| "qualifiedName": "App::run" | ||
| } | ||
| } | ||
| }))); | ||
|
|
||
| assert!(result.is_ok(), "handle_incoming_calls must not error: {result:?}"); | ||
| let value = result.expect("already checked"); | ||
| let value = value.expect("handler must return Some value"); | ||
| // handle_incoming_calls returns the calls array directly (not wrapped in {"result":...}) | ||
| let calls = value.as_array().expect("result should be an array"); | ||
|
|
||
| // The reference in script.pl has no enclosing callable, so the workspace | ||
| // path must synthesize a file-level caller with kind=1 (SymbolKind.File). | ||
| let file_caller = calls | ||
| .iter() | ||
| .find(|c| c["from"]["uri"].as_str().map_or(false, |u| u.contains("script.pl"))); | ||
| assert!(file_caller.is_some(), "expected file-level caller from script.pl, got: {calls:?}"); | ||
| let from = &file_caller.expect("already checked")["from"]; | ||
| assert_eq!( | ||
| from["kind"].as_u64(), | ||
| Some(1), | ||
| "file-level caller must have SymbolKind.File=1, got: {from:?}" | ||
| ); | ||
| assert_eq!(from["name"].as_str(), Some("script.pl")); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP 'expect_used|deny\(|warn\(|clippy' --type=toml
fd -t f 'clippy.toml|.clippy.toml'
rg -nP '#!\[(deny|warn)\(clippy::expect_used' -g '*.rs'Repository: EffortlessMetrics/perl-lsp-swarm
Length of output: 265
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '## clippy config and expect_used overrides\n'
git ls-files | rg '(^|/)(clippy\.toml|\.clippy\.toml|Cargo\.toml)$|\.rs$' >/dev/null 2>&1 || true
rg -n --hidden --glob '!**/target/**' --glob '!**/node_modules/**' \
'expect_used|clippy::expect_used|allow\(clippy::expect_used\)|deny\(clippy::expect_used\)|warn\(clippy::expect_used\)|clippy::all|clippy::pedantic|clippy::restriction' \
. || true
printf '\n## target file excerpt around the reviewed lines\n'
sed -n '930,995p' crates/perl-lsp-rs/src/runtime/language/hierarchy.rsRepository: EffortlessMetrics/perl-lsp-swarm
Length of output: 19144
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## files"
fd -H -t f 'clippy.toml|.clippy.toml|Cargo.toml' .
echo
echo "## clippy/expect_used matches"
rg -n --hidden --glob '!**/target/**' --glob '!**/node_modules/**' \
'expect_used|clippy::expect_used|allow\(clippy::expect_used\)|deny\(clippy::expect_used\)|warn\(clippy::expect_used\)|clippy::all|clippy::pedantic|clippy::restriction' \
.
echo
echo "## hierarchy.rs excerpt"
sed -n '930,995p' crates/perl-lsp-rs/src/runtime/language/hierarchy.rsRepository: EffortlessMetrics/perl-lsp-swarm
Length of output: 20544
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## crate root for perl-lsp-rs"
sed -n '1,120p' crates/perl-lsp-rs/src/lib.rs
echo
echo "## any test-only clippy allowances in perl-lsp-rs"
rg -n --hidden --glob '!**/target/**' \
'cfg_attr\(test, allow\(.*expect_used|allow\(clippy::expect_used|allow\(clippy::unwrap_used|allow\(clippy::panic' \
crates/perl-lsp-rs/src crates/perl-lsp-rs/tests crates/perl-lsp-rs/Cargo.tomlRepository: EffortlessMetrics/perl-lsp-swarm
Length of output: 4582
Replace the new .expect() calls in this test. Cargo.toml denies clippy::expect_used, and there’s no local test allowance here, so these uses will trip the clippy gate. Use assert!/match/ok_or_else instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/perl-lsp-rs/src/runtime/language/hierarchy.rs` around lines 940 - 988,
The test in hierarchy.rs introduces new `.expect()` calls that will fail the
`clippy::expect_used` gate. Update the `handle_incoming_calls` assertion flow to
avoid `expect` by using `assert!`, `match`, or `ok_or_else` around the `result`,
`value`, `calls`, and `file_caller` checks, while keeping the same verification
logic in the incoming-calls test.
Source: Coding guidelines
| // path must synthesize a file-level caller with kind=1 (SymbolKind.File). | ||
| let file_caller = calls | ||
| .iter() | ||
| .find(|c| c["from"]["uri"].as_str().map_or(false, |u| u.contains("script.pl"))); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
clippy unnecessary_map_or lint version stabilized
💡 Result:
The Clippy lint unnecessary_map_or was merged into the master branch of the Rust Clippy repository on November 13, 2024 [1]. It was introduced to simplify map_or method calls where a more idiomatic or readable alternative exists [1]. Because Clippy lints are bundled with the Rust toolchain and released periodically alongside rustc, the lint became available to users in the stable Rust releases following its merge in late 2024. It is part of the standard set of Clippy lints and is actively maintained, with ongoing refinements and discussions regarding its behavior and potential future deprecation or splitting into smaller, more specialized lints [2].
Citations:
- 1: New lint:
unnecessary_map_orrust-lang/rust-clippy#11796 - 2: Splitting
unnecessary_map_orinto 2 lints rust-lang/rust-clippy#15999
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== hierarchy.rs around line 980 ==\n'
sed -n '950,1010p' crates/perl-lsp-rs/src/runtime/language/hierarchy.rs | cat -n
printf '\n== clippy / warnings config ==\n'
rg -n --hidden --glob '!target' --glob '!node_modules' 'deny\s*\(\s*warnings\s*\)|unnecessary_map_or|clippy::|warnings' Cargo.toml crates/perl-lsp-rs crates -g 'Cargo.toml' -g '*.rs' | sed -n '1,200p'Repository: EffortlessMetrics/perl-lsp-swarm
Length of output: 22017
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- hierarchy.rs slice ---'
sed -n '970,990p' crates/perl-lsp-rs/src/runtime/language/hierarchy.rs | cat -n
echo
echo '--- lint-related config ---'
rg -n --hidden --glob '!target' --glob '!node_modules' \
'deny\s*\(\s*warnings\s*\)|#!\s*\[\s*deny\s*\(\s*warnings\s*\)\s*\]|clippy::|warnings as errors|deny-warnings|-D warnings' \
Cargo.toml .cargo crates/perl-lsp-rs crates -g 'Cargo.toml' -g '*.rs' -g '*.toml' | sed -n '1,200p'Repository: EffortlessMetrics/perl-lsp-swarm
Length of output: 17141
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== workflow / CI references ==\n'
rg -n --hidden --glob '!target' --glob '!node_modules' \
'agent-clippy|cargo clippy|clippy --workspace|-\s*D warnings|D warnings|profile agent' \
.github Cargo.toml .cargo crates -g '*.yml' -g '*.yaml' -g '*.toml' -g '*.md' -g '*.rs' | sed -n '1,240p'Repository: EffortlessMetrics/perl-lsp-swarm
Length of output: 5128
Use is_some_and here. map_or(false, ...) trips clippy::unnecessary_map_or, and clippy runs with -D warnings in this workspace.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/perl-lsp-rs/src/runtime/language/hierarchy.rs` at line 980, Replace
the `map_or(false, ...)` predicate in the `find` call within `hierarchy.rs` with
`is_some_and` on the `from.uri` lookup to satisfy clippy’s `unnecessary_map_or`
lint. Keep the existing `find` logic and `script.pl` containment check, but
update the `as_str()` handling to use `is_some_and` so the `from`/`uri` access
remains concise and warning-free.
![factory-droid[bot]](https://avatars.githubusercontent.com/in/358017?s=60&v=4){kind=small}
| .find_workspace_enclosing_callable(&callable_symbols, &location) | ||
| .unwrap_or_else(|| { | ||
| // Top-level call site — no enclosing callable in the | ||
| // workspace index. Synthesize a file-level caller so the | ||
| // script appears in incomingCalls instead of being dropped. | ||
| crate::call_hierarchy_provider::synthetic_file_level_caller( | ||
| &location.uri, | ||
| from_range, | ||
| ) |
There was a problem hiding this comment.
[P1] Workspace-index path synthesizes file-level callers for unrelated bare-name matches
find_refs(SymbolKey{pkg:"App", name:"run"}) returns BOTH qualified App::run matches AND bare run matches (see find_references at workspace_index.rs:2211, which also looks up the bare suffix). The new unwrap_or_else synthesizes a file-level caller for every returned location without verifying that the bare reference actually invokes the qualified target. Concrete trigger: a workspace containing script.pl with top-level $app->run AND $other->run will produce a single file-level caller for script.pl whose from_ranges contains a location that calls Other::run, not App::run — the caller is mis-attributed. The pre-fix code silently dropped these (no enclosing sub + if let Some(from) guard), so this PR converts a silent drop into a wrong positive. Fix: filter by qualified name before synthesizing, e.g. only synthesize a file-level caller when the workspace-index recorded the reference under the qualified key for this target.
| crate::call_hierarchy_provider::synthetic_file_level_caller( | ||
| &location.uri, | ||
| from_range, | ||
| ) | ||
| }); |
There was a problem hiding this comment.
[P1] File-kind synthesized callers here do not round-trip through json_to_call_hierarchy_item (lines 806-810)
The synthetic file-level callers produced by the unwrap_or_else above serialize to kind: 1 (SymbolKind.File) via to_json. But json_to_call_hierarchy_item in this file only maps 6 => "method" and defaults everything else (including 1) to "function". If a client later sends one of these file-kind callers back in callHierarchy/incomingCalls or callHierarchy/outgoingCalls to expand it, the server will treat it as a function-kind item whose name = "real-baseline.pl" and search for a subroutine named after the filename — silently returning empty results. Fix: extend the kind match in the deserializer with 1 => "file" so file-kind items round-trip cleanly.
| crate::call_hierarchy_provider::synthetic_file_level_caller( | |
| &location.uri, | |
| from_range, | |
| ) | |
| }); | |
| let kind = match json["kind"].as_u64().unwrap_or(12) { | |
| 1 => "file", | |
| 6 => "method", | |
| _ => "function", | |
| } | |
| .to_string(); |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
1 participant
Diagnosis
incomingCallsforApp::runreturned[]even thoughscript/real-baseline.plcalls$app->runat the top level (not inside asub). Both code paths silently dropped callers with no enclosing callable:hierarchy.rs:632-650):find_workspace_enclosing_callablereturnedNonefor a top-level ref → the ref was dropped insideif let Some(from) = ....call_hierarchy_provider/mod.rs):FunctionCall/MethodCallarms guarded withif let Some(from) = current_function→ top-level sites wherecurrent_function = Noneproduced no output.Step 0 verification: the workspace index DOES record top-level
MethodCallreferences (line 4020 of workspace_index.rs stores bare method names unconditionally). The reference was in the index; the bug was in the consumer, not the recorder.Fix
crates/perl-lsp-rs/src/call_hierarchy_provider/mod.rsuri_basename(uri: &str) -> Stringhelper to extract the filename from a URI.FunctionCallandMethodCallarms: replacedif let Some(from) = current_functionwithcurrent_function.cloned().unwrap_or_else(|| /* synthesize file-level item */)— so top-level call sites produce aCallHierarchyItem { kind: "file", name: basename, uri, ... }.Subroutinearm: addedreturnafter visiting children inside a named sub, preventing a pre-existing double-visit that would have caused spurious file-level callers for calls inside subs.to_json(): added"file" => 1mapping (LSPSymbolKind.File = 1).crates/perl-lsp-rs/src/runtime/language/hierarchy.rsfor location in refsloop: replacedif let Some(from) = find_workspace_enclosing_callable(...)with.unwrap_or_else(|| /* synthesize file-level item */).crates/perl-lsp-ux-tests/tests/ux_scenario_22_crossfile_extended_providers.rsscenario_22_call_hierarchy_incoming_to_run_hard_assert."real-baseline.pl"(non-vacuous guard).Before / After Test Evidence
Before fix (with
#[ignore]removed but no fix): test would FAIL with[]result.After fix (
UX_TEST_EXIT=0):Verification
cargo build -p perl-lsp-rs --bin perl-lsp: BUILD_EXIT=0cargo test -p perl-lsp-rs --lib -- call_hierarchy: 5/5 ok, TEST_EXIT=0cargo clippy -p perl-lsp-rs --locked -- -D warnings -A missing_docs: CLIPPY_EXIT=0cargo fmt --check -p perl-lsp-rs: FMT_RS_EXIT=0cargo fmt --check -p perl-lsp-ux-tests: FMT_UX_EXIT=0scenario_22_call_hierarchy_incoming_to_run_hard_assertPASSED (EXIT=0)🤖 Generated with Claude Code
https://claude.ai/code/session_015AGtUiPPvBkWTeN9nm6q6N