Skip to content

Conversation

@jakebailey
Copy link
Member

@jakebailey jakebailey commented Dec 1, 2025

  • Properly handle the active parameter capability
  • Don't set active parameter when client allows it
  • Check inComment now that we have it
  • Port over backup JS sig help code

Plus, generate all remaining tests that mention sig help. Some new tests with unrelated issues (other LSP features) become skipped.

Fixes #1495

Copilot AI review requested due to automatic review settings December 1, 2025 16:40
Copilot finished reviewing on behalf of jakebailey December 1, 2025 16:41
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR improves signature help functionality by properly handling the active parameter capability, ensuring that activeParameter is placed on SignatureInformation when the client supports it rather than on the top-level SignatureHelp. It also adds comment-in-code checking, ports backup JavaScript signature help code, and generates all remaining signature help tests.

Key Changes:

  • Moved activeParameter from SignatureHelp to SignatureInformation based on client capabilities
  • Added inComment checking to bail out early when user triggers signature help in comments
  • Implemented JavaScript fallback signature help for untyped code
  • Generated comprehensive signature help test suite

Reviewed changes

Copilot reviewed 145 out of 145 changed files in this pull request and generated no comments.

File Description
internal/ls/signaturehelp.go Core implementation of capability-aware active parameter handling and JS fallback
Test baseline files (multiple) Updated baselines reflecting new active parameter placement
Generated test files (multiple) New comprehensive test coverage for signature help scenarios

Copy link
Member

Choose a reason for hiding this comment

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

Why is this one a manual test?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a JS test that previously used removed JSDoc syntax, like function(string, boolean=). The two manual tests are ones I fixed.

TestAutoImportVerbatimTypeOnly1
TestBestCommonTypeObjectLiterals
TestBestCommonTypeObjectLiterals1
TestCalledUnionsOfDissimilarTyeshaveGoodDisplay
Copy link
Member

Choose a reason for hiding this comment

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

I hate when my unions of dissimilar tyes don't haveGoodDisplay

Copy link
Member

Choose a reason for hiding this comment

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

This one is just type-ordering, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, but we loved tests with hardcoded strings (for better or for worse).

@jakebailey jakebailey added this pull request to the merge queue Dec 1, 2025
Merged via the queue into main with commit 00e99d1 Dec 1, 2025
28 checks passed
@jakebailey jakebailey deleted the jabaile/sig-help-improvements branch December 1, 2025 21:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Signature help may not respect capabilities for NoActiveParameterSupport

3 participants