x/tools/gopls: SignatureHelp activeParameter always 0 after v0.18.1

[skewb1k]

After commit 2880aae, SignatureInformation.activeParameter does not have omitempty tag and always marshal. The problem is that its never assigned to anything and always 0, which overrides actual value in SignatureHelp.activeParameter, as stated in spec.

Solution is have same value as in SignatureHelp.activeParameter in SignatureInformation.activeParameter. The fix is minimal—just a one-line change —and I've submitted a PR.

[gopherbot]

Change https://go.dev/cl/683135 mentions this issue: gopls: fix SignatureHelp activeParameter not being passed

[jakebailey]

Having the parameter count in the signature info itself is good, but IIRC that is an optional feature the client may not use, and so having it set at the top-level reponse is also recommended as a fallback. Perhaps this field should be something else to distinguish "not present" from zero?

[jakebailey]

Eh, maybe it doesn't matter in practice since they'll be the same value (no overloads in Go), and therefore clients will always have the right answer.

[findleyr]

@adonovan reverted this behavior in https://go.dev/cl/677936 for SignatureHelp.ActiveParameter. I think it was just an oversight not to do the same for SignatureInformation.

This is an optional field where 0 does not mean 'empty'. It should be a *uint32 and omitempty.

[skewb1k]

Yes, it seems correct that the field should be optional according to the spec. However, I'm still not fully sure whether it should be nil value. It's unclear how SignatureHelp.activeParameter could be known while the corresponding activeParameter of the activeSignature isn't. Maybe if there's always exactly one signature, then the global activeParameter is enough and there's no need to duplicate it inside the array. Still, it feels a bit odd that the presence of a field in an array element depends on the array's length.

That said, with nil value it works and doesn't conflict with the spec, so I think it's fine.