Skip to content

[IDE] [Signature Help] Add basic signature help request to SourceKit #83378

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 23 commits into from
Sep 4, 2025
Merged

[IDE] [Signature Help] Add basic signature help request to SourceKit #83378

merged 23 commits into from
Sep 4, 2025

Conversation

@a7medev
Copy link
Member

@a7medev a7medev commented Jul 28, 2025 •
edited
Loading

Note

This PR depends on #83646, it should be its base PR but GitHub doesn't allow that.
Commits after 6ebbb12 are part of this PR.

Add a basic signaturehelp request to SourceKit that:

  1. Extracts and returns signature label.
  2. Extracts offset and length of each parameter in the signature label.
  3. Returns full documentation for signature (in a future PR, full documentation should be split into signature documentation and parameter documentation).

@a7medev a7medev marked this pull request as draft July 28, 2025 21:26
@a7medev a7medev force-pushed the feat/signature-help-basic branch 4 times, most recently from 7c436a1 to 254f4e4 Compare August 11, 2025 22:11
@a7medev a7medev changed the title [IDE] Add basic signature help request to SourceKit [IDE] [Signature Help] Add basic signature help request to SourceKit Aug 11, 2025
@a7medev a7medev marked this pull request as ready for review August 11, 2025 22:33
@a7medev a7medev force-pushed the feat/signature-help-basic branch 4 times, most recently from 8543c6d to 21977fc Compare August 22, 2025 15:52
@a7medev a7medev mentioned this pull request Aug 22, 2025
Merged
@a7medev a7medev force-pushed the feat/signature-help-basic branch from 10cb4af to aea823a Compare August 26, 2025 21:44
hamishknight
hamishknight reviewed Aug 26, 2025
View reviewed changes
@a7medev a7medev force-pushed the feat/signature-help-basic branch from aea823a to 6b8b3c4 Compare August 28, 2025 16:39
@a7medev a7medev requested a review from hamishknight August 28, 2025 16:39
@a7medev
Copy link
Member Author

a7medev commented Aug 28, 2025

@hamishknight I've addressed your comments (half-addressed this one: #83378 (comment)), can you please recheck? 🙏🏼

@hamishknight
Copy link
Contributor

hamishknight commented Sep 1, 2025

@a7medev Can you rebase this PR to make it a bit easier to re-review now that the other PRs have landed?

a7medev added 12 commits September 1, 2025 14:00
@a7medev
[Test] Split signature help tests for currying
cd96add
@a7medev
[IDE] NFC: Extract createSignatureLabel function & remove unused incl…
026e696 
…udes
@a7medev
[IDE] Output all parameters in signature help
d00565b
@a7medev
[IDE] Use FunctionRefInfo::ApplyLevel for curried functions in Argume…
86b3118 
…ntCompletion

And update signature help to output the signature correctly for a second apply and add tests in signature_help_currying
@a7medev
[IDE] NFC: Forward declare Signature in ArgumentCompletion.h
07d4f23
@a7medev
[IDE] Use raw documentation in signature help
154ca9d
@a7medev
[IDE] Use editor documents infrastructure for signature help
6945dc1
@a7medev
[IDE] Signature help: Keep strings in Allocator instead of SmallString
5f42c37
@a7medev
[IDE] Use keys.signatures instead of keys.members for signature help
b2a61d3
@a7medev
[Test] Add signature help doc comment test
5297c23
@a7medev
[Test] Change signature help tests to use split-file
bd2fdc0
@a7medev
[IDE] Misc enhancements to signature help
aabab5b
@a7medev a7medev force-pushed the feat/signature-help-basic branch from 6b8b3c4 to aabab5b Compare September 1, 2025 11:39
@a7medev
Copy link
Member Author

a7medev commented Sep 1, 2025

@hamishknight Updated. ✅

hamishknight
hamishknight approved these changes Sep 3, 2025
View reviewed changes
Copy link
Contributor

@hamishknight hamishknight left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! LGTM

@a7medev
Copy link
Member Author

a7medev commented Sep 3, 2025

@hamishknight Can you please trigger CI? 🙏🏼

@hamishknight
Copy link
Contributor

hamishknight commented Sep 3, 2025

@swift-ci please smoke test

@hamishknight
Copy link
Contributor

hamishknight commented Sep 3, 2025

@a7medev Looks like a couple of test failures on Windows:

# | Unsupported: 'diff': option -B not recognized

Do we really need to pass -B?

@a7medev
Copy link
Member Author

a7medev commented Sep 3, 2025

@hamishknight Fixed. I was mainly using it to prevent matching the extra newline at the end of the expected output but it wasn't necessary.

@hamishknight
Copy link
Contributor

hamishknight commented Sep 3, 2025

Thanks!

@hamishknight
Copy link
Contributor

hamishknight commented Sep 3, 2025

@swift-ci please smoke test

rintaro
rintaro reviewed Sep 3, 2025
View reviewed changes
Copy link
Member

@rintaro rintaro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am fine with merging this as-is.

But my preference is that tools/SourceKit should only be a facade of lib/IDE.
I.e. all the logic should be in IDE and the primary test should be swift-ide-test.
sourcekitd-test is for testing SourceKit request/response handling/formatting only.

@a7medev
Copy link
Member Author

a7medev commented Sep 3, 2025

@rintaro I agree and we've considered this but we descoped it for now to focus on getting the feature done first. I plan on moving this to lib/IDE if I had time to work on it either during GSoC or after it.

@rintaro
Copy link
Member

rintaro commented Sep 3, 2025

Sounds good to me. Thanks for working on this!

@hamishknight hamishknight merged commit 9d1436c into swiftlang:main Sep 4, 2025
3 checks passed
hamishknight pushed a commit to swiftlang/sourcekit-lsp that referenced this pull request Sep 5, 2025
@a7medev
Add signature help LSP request support (#2250)
a5854f4 
Depends on swiftlang/swift#83378

---

Adds support for the LSP signature help request.

> [!NOTE]
> As of swiftlang/swift#83378, SourceKitD still
doesn't separate parameter documentation from the signature
documentation and thus parameters don't have their own separate
documentation. This should just work once SourceKitD implements this
functionality and we'll only need to modify the tests.
@a7medev a7medev mentioned this pull request Sep 5, 2025
Merged
@a7medev a7medev mentioned this pull request Sep 25, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rintaro rintaro rintaro left review comments

@hamishknight hamishknight hamishknight approved these changes

@bnbarham bnbarham Awaiting requested review from bnbarham bnbarham is a code owner

@hborla hborla Awaiting requested review from hborla

@slavapestov slavapestov Awaiting requested review from slavapestov

@xedin xedin Awaiting requested review from xedin

@tshortli tshortli Awaiting requested review from tshortli

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@a7medev @hamishknight @rintaro