Add RefactorTriggerReason

[jessetrinity]

Adds RefactorTriggerReason proposed in #35096. Partially covers cases for refactor discoverability #37895.

This change allows the user to send the code action command (ctrl + . in VS and VSCode) to request available refactors at a cursor location (empty span) rather than having to select the entire span of the code to be refactored. The intent is to make refactors more discoverable by allowing a user to "ask" if a refactor is available, rather than having to know in advance that a refactor is applicable to a particular span.

In the future we may want to offer more refactors when the code action command is sent while non-empty span is selected. This change enables that behavior but intentionally does not implement it.

[typescript-bot]

Thanks for the PR! It looks like you've changed the TSServer protocol in some way. Please ensure that any changes here don't break consumers of the current TSServer API. For some extra review, we'll ping @sheetalkamat, @amcasey, @mjbvz, @minestarks for you. Feel free to loop in other consumers/maintainers if necessary

[amcasey]

I like the idea but have some thoughts on various stylistic points.

[amcasey]

The simplest version of this is a boolean, isUserInvoked. I can see why we might want to be able to generalize that in the future though, so moving to an enum, { Invoked, Speculative }, seems reasonable - it gives us the option of addressing more nuances in the future. I'm not sure why we'd go more abstract than that though - it seems unlikely we'll need both an enum and an additional payload.

[jessetrinity]

Switching to an enum

export enum RefactorTriggerReason {
    Implicit = "implicit",
    Invoked = "invoked",
}

seems fine to me. Curious what you think the value of preemptively adding a Speculative option would be.

@mjbvz is there potential that VSCode might want to send info in addition to the triggerReason, such as a request for a specific "refactorKind"?

[amcasey]

My Speculative is your Implicit. I'm fine with your name.

[mjbvz]

Server protocol changes look good to me

[amcasey]

LGTM modulo some minor naming comments.

I didn't review the expanded ranges carefully (let me know if that's something you're worried about) but the new tests don't obviously cover the newly supported regions.

[amcasey]

I find this a little confusing since the default elsewhere is implicit.

[amcasey]

I can see the convenience of having this be called userRequested in all the different refactorings, but, personally, I think it would be clearer to give it a name that's specific to each function you've added it to. For example, in this case, the parameter could be called something like considerFunctionBodies - that would be more meaningful both within the function and at the call-sites that aren't part of a flow that can be either implicit or user-requested. As a bonus, it would also address my comment above, that this parameter defaults to explicit, whereas the ones in the protocol default to implicit.

[amcasey]

Unfortunately, JS doesn't have named arguments. Instead, when we're passing a boolean, we tend to give the parameter name as a /*comment*/ before the argument (unless the boolean already has a descriptive name, of course).

[jessetrinity]

It wasn't my intention to pass a named argument, but to pass whatever triggerReason === "invoked" evaluates to (false if triggerReason is "implicit" or undefined. Did I do that incorrectly?