Skip to content

[VSCodeSweeper MVP — test only] lsp signature_helper Activity parameter highlight position error#3

Draft
egamma wants to merge 1 commit into
mainfrom
vscodesweeper/fix-246019
Draft

[VSCodeSweeper MVP — test only] lsp signature_helper Activity parameter highlight position error#3
egamma wants to merge 1 commit into
mainfrom
vscodesweeper/fix-246019

Conversation

@egamma

@egamma egamma commented Jul 7, 2026

Copy link
Copy Markdown
Owner

⚠️ VSCodeSweeper MVP — test only. This draft PR was generated by an AI experiment
(vscodesweeper) inside a personal fork. It is
not intended for microsoft/vscode and is not a contribution proposal.

Upstream issue: microsoft#246019

Change summary

Signature help highlights the wrong parameter position when the parameter name string appears multiple times in the signature label.

Validation

Write a unit test in parameterHintsWidget tests that creates a signature where a parameter name (e.g., 'len') also appears in an earlier parameter or the function name, and assert the highlight offsets point to the Nth occurrence matching the activeParameterIndex, not the first.


Branch vscodesweeper/fix-246019 · generated fix · review loop runs separately (codex review).

@egamma egamma added the vscodesweeper-mvp VSCodeSweeper MVP test PR (fork-only) label Jul 7, 2026
@egamma

egamma commented Jul 7, 2026

Copy link
Copy Markdown
Owner Author

🤖 copilot /review — round 1

VSCodeSweeper MVP self-review (fork-only experiment).

● Get commits on branch (shell)
│ cd && git log main...HEAD --oneline
└ 2 lines…

● Get diff stats (shell)
│ cd && git diff main...HEAD --stat
└ 4 lines…

● Get full diff (shell)
│ cd && git diff main...HEAD
└ 201 lines…

No significant issues found in the reviewed changes.

The logic correctly handles:

  • Setting regex.lastIndex with the g flag properly instructs test() to search from that position
  • When test() returns false (no match), lastIndex resets to 0, making idx negative, which fails the idx >= searchStart check and returns [0, 0]
  • The word-boundary regex (\W|^)..(?=\W|$) correctly prevents matching substrings within identifiers
  • The sequential searchStart advancement correctly handles duplicate parameter names by finding each occurrence in order● Code-review(claude-opus-4.6) Review parameter hints fix
    │ Check git status (shell)
    │ │ cd && git --no-pager diff --staged
    │ 2>/dev/null |
    │ │ head -5; git --no-pager diff 2>/dev/null | head -5; git --no-pager log --oneline -3
    │ └ 3 lines…
    └ Read parameterHintsWidget.ts
    │ src/vs/editor/contrib/parameterHints/browser/parameterHintsWidget.ts
    └ L350:396 (46 lines read)

Code Review Summary

Commit: fix #246019: lsp signature_helper Activity parameter highlight position error

What it does: Fixes parameter highlighting in signature help when a parameter name appears as a substring of the function name (e.g., "len" incorrectly highlighted inside "strlen"). The fix computes a searchStart offset from previous parameters so the regex skips earlier occurrences.

Verdict: Looks good ✅ — no bugs or logic errors found.

Minor observations:

  • The searchStart loop is O(n²) for string-labeled parameters, but parameter lists are small in practice — not a concern.
  • If regex.test() returns false for the target parameter, lastIndex resets to 0, making idx negative, which correctly triggers the [0, 0] fallback via the idx >= searchStart guard.
  • Tests are well-structured and cover the key scenarios including the original bug, duplicates, mixed label types, and edge cases.

@egamma

egamma commented Jul 7, 2026

Copy link
Copy Markdown
Owner Author

✅ Round 1: no actionable findings — the review loop converged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

vscodesweeper-mvp VSCodeSweeper MVP test PR (fork-only)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant