[ty] Fix signature help for ParamSpec-specialized class calls by charliermarsh · Pull Request #24399 · astral-sh/ruff (original) (raw)
Summary
Given A[int,](), signature help was failing to combine the display items with the semantics of the signature. This PR refactors such that we emit structured display data for the IDE crate to consume. So ty_ide can just consume a pre-built Vec<CallSignatureParameter> directly.
Closes astral-sh/ty#3214.
Typing conformance results
No changes detected ✅
Current numbers
The percentage of diagnostics emitted that were expected errors held steady at 87.72%. The percentage of expected errors that received a diagnostic held steady at 82.85%. The number of fully passing files held steady at 74/132.
Memory usage report
Memory usage unchanged ✅
ecosystem-analyzer results
| Lint rule | Added | Removed | Changed |
|---|---|---|---|
| invalid-await | 40 | 0 | 0 |
| invalid-return-type | 1 | 0 | 0 |
| Total | 41 | 0 | 0 |
Changes in flaky projects detected. Raw diff output excludes flaky projects; see the HTML report for details.
Full report with detailed diff (timing results)
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've some questions but that's mainly from my limited understanding of how signature help works but otherwise it looks good. I tried it out on a few other examples locally as well including Concatenate, none of them panics.
| .skip_auto_import() |
|---|
| .build() |
| .contains("sentinel") |
| .not_contains("**P"); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm unable to follow this test case, was the server providing **P (as oppose to just P) for the completion?
I'm not able to reproduce this on the playground by trying to trigger the completion at the cursor position as described in this test case.
| assert_snapshot!(test.signature_help_render(), @" |
|---|
| ============== active signature ============= |
| [**P]() -> A[(int, /)] |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to catchup with my understanding, this is what the server displays when type variables are involved? Like, it shows the type variable and what it specializes into if there is a specialization?
Out of curiosity of trying, Pylance seems to skip showing signature help in this scenarios where the cursor is providing the arguments for the generic type variables. No action item here, just something that I noticed.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I believe that for ordinary callable parameters we do show specialized types.
| if current_arg_index < details.parameter_label_offsets.len() { |
|---|
| if current_arg_index < details.parameters.len() { |
| Some(current_arg_index) |
| } else if details.parameters.last().is_some_and(|parameter |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: it might be useful to update the comment above to include why this condition exists here.
Is it just a fallback to use the variadic parameter in case the length exceeds the argument index? Is that ok that regardless of whether the argument is positional or keyword, it maps to last parameter? And, just to confirm, the len - 1 is because the active_parameter is the index (0-based)?
Comment on lines +707 to +712
| let start = usize::from(range.start()); |
|---|
| let end = usize::from(range.end()); |
| let Some(label) = display_details |
| .label |
| .get(start..end) |
| .map(ToString::to_string) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
| let start = usize::from(range.start()); |
|---|
| let end = usize::from(range.end()); |
| let Some(label) = display_details |
| .label |
| .get(start..end) |
| .map(ToString::to_string) |
| let Some(label) = display_details |
| .label |
| .get(range.to_std_range()) |
| .map(ToString::to_string) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})