Add support for new self-describing/parameterised refactors

[DanTup]

Some initial support for new server refactors:

Sep-15-2022.17-48-40.mp4

Some things outstanding:

• 1. needs integration tests
• 2. currently assumes we want to use saveDialog for filePaths (may we want open? is there a more general API in VS Code we can use?)
• 3. we currently hard-code the button text to "Move" that should probably be provided by the server
• 4. Parameter.defaultValue is currently String but since the experimental capability is intended to control refactors that have parameters without default values, it seems like this should likely be String??
• 5. only filePath is currently implemented
• 6. we currently assume any "literal" code action (not a bare command) with a single argument that has an "arguments" field that is a list, and also a data.parameters that is a list, is one of these refactors. Do we need something more explicit?
• 7. the save dialog has a filter { "Dart Files": ["dart"] }, does this need to come from the server?
• 8. URIs vs paths
• 9. Typed defaultValue/values instead of strings?
• 10. How to add new field types (eg. "openUri") in a non-breaking way (eg. if server knows the client supports this functionality, it can send fields with no defaults, but the client might not know how to provide a value if it's a new type)
• 11. Tests for specific parameter kinds (ultimately we won't be adding integration tests here for every refactor, but we need coverage of every parameter kind)

@bwilkerson FYI.. once I've resolved the items above I'll likely merge this (it's behind the same dart.experimentalNewRefactors flag server is using) to make it easier to work on the server side (at least while no additional client changes are required). Some of the things above may require related server changes though.

I'm treating this experimental flag as for development only, so don't plan to worry about compatibility until we're ready to remove that.

[bwilkerson]

This is fantastic! I'm very excited to see this happening.

When you get to where you need server changes, I'm happy for either you or I to make them, whichever of us has more time.

[DanTup]

@bwilkerson just going through some of the things above:

2. currently assumes we want to use saveDialog for filePaths (may we want open? is there a more general API in VS Code we can use?)

It seems VS Code doesn't have any "generic" file dialog, it has to explicitly be an Open or Save one (which are delegated to the OS by default, although users can also opt-in to a simpler text-based entry in a command-palette based picker). If it's possible we might ever want the user to select a file for reading from (an "open file dialog") perhaps we should use "saveFilePath" instead of "filePath"?

3. we currently hard-code the button text to "Move" that should probably be provided by the server

This text is shown on the button in the bottom right here:

Should we add some field (actionText?) to the parameter? (we don't currently have any type-specific options on here, but I suspect it may be beneficial)?

4. Parameter.defaultValue is currently String but since the experimental capability is intended to control refactors that have parameters without default values, it seems like this should likely be String??

Does this seem like the right choice? (I can mark it as String? now in this extension and update the server later, unless you beat me to it).

7. the save dialog has a filter { "Dart Files": ["dart"] }, does this need to come from the server?

Similar to (3), we could add an additional field to the parameter (we could also make a simplified version, or even read the extension from defaultValue and fabricate it like "${PascalCase(extension)} Files": [extension], but it wouldn't be very flexible.

[bwilkerson]

I have more thoughts than answers, but hopefully some of this will be helpful.

If it's possible we might ever want the user to select a file for reading from (an "open file dialog") perhaps we should use "saveFilePath" instead of "filePath"?

It seems unlikely that we'll ever need to open a file for reading, but given the low cost of guarding against it, I agree we should use 'saveFilePath' (and potentially 'openFilePath' in the future).

Should we add some field (actionText?) to the parameter? (we don't currently have any type-specific options on here, but I suspect it may be beneficial)?

I can definitely see the possibility of wanting a different label on the button for other refactorings, so it seems reasonable to want to pass the label to the client. We have a 'label' field that appears to not be used in this case, but I don't know that other clients might not want it to label something. For example, I suspect that IntelliJ would implement this kind of a parameter as a text field with a button beside it that would open the system dialog and put the selected file's path in the text field. I had imagined the 'label' to be a label for the text field. It's hard to know, though what other clients might want or whether there will even be other clients.

If we add 'actionText' or 'actionLabel', we should consider changing 'label' to be more specific, such as 'parameterLabel'. Not sure it's necessary, but worth considering.

Does this seem like the right choice? (I can mark it as String? now in this extension and update the server later, unless you beat me to it).

I think I was assuming that we'd pass back an empty string, but allowing null as a clear signal that there is no default would probably be better. Yes, that seems fine to me.

Similar to (3), we could add an additional field to the parameter (we could also make a simplified version, or even read the extension from defaultValue and fabricate it like "${PascalCase(extension)} Files": [extension], but it wouldn't be very flexible.

We wouldn't be able to extract it from the defaultValue if there is no default value. That might be fine, because we probably don't want to filter the list if there's no default value, but we'll have to handle that case.

I'm not sure whether we'll ever want to filter for other extensions (though I suppose I could imagine refactorings around analysis options files). Again, the guiding principle I'd apply is that if it isn't too high a cost to do so it would be better to keep it possible. We do want to also consider the future cost of adding support in new clients. I don't know whether passing the data over is a hinderance or a help from that perspective, so I don't know how to weight that factor.

[DanTup]

It seems unlikely that we'll ever need to open a file for reading, but given the low cost of guarding against it, I agree we should use 'saveFilePath' (and potentially 'openFilePath' in the future).

SGTM!

We have a 'label' field that appears to not be used in this case, but I don't know that other clients might not want it to label something.

I was actually using it here for "title", though the comment on that says (which is why it doesn't show up here):

**
 * Dialog title.
 *
 * This parameter might be ignored, as not all operating systems display a title on save dialogs
 * (for example, macOS).
 */

I had imagined the 'label' to be a label for the text field. It's hard to know, though what other clients might want or whether there will even be other clients.

I guess using it as title probably wasn't right here then. I think your suggestion makes sense, though maybe we should also have a title? For ex.:

actionLabel: String // for saveFilePath / openFilePath. Eg: "Move"/"Save"/etc.
parameterLabel: String // short name for field if rendered next to an input box. Eg: "Target file"/"Name"/etc.
title: String // Slightly more verbose text. Eg.: "Select a file to move to" / "Enter a name for the new class"

I think I was assuming that we'd pass back an empty string, but allowing null as a clear signal that there is no default would probably be better. Yes, that seems fine to me.

Ah, empty string would work. Although I did mean to ask about using strings for all the values - would it be better if we typed them based on the type of the field? (eg. tickboxes would be real true/false instead of string versions). If we did that, null (or being omitted / undefined) may make more sense. If we stick with strings, maybe empty.

I'm not sure whether we'll ever want to filter for other extensions (though I suppose I could imagine refactorings around analysis options files). Again, the guiding principle I'd apply is that if it isn't too high a cost to do so it would be better to keep it possible. We do want to also consider the future cost of adding support in new clients. I don't know whether passing the data over is a hinderance or a help from that perspective, so I don't know how to weight that factor.

For something like this (and actionLabel, etc.) I don't think it's critical a client can use it (they'd just not filter the dialog), but it's probably nice to have if they can. I think the cost of adding it is very low and if we do want to use it in the future, it avoids additional changes in the client. If it's possible other LSP users want something similar, having support for this might make it easier for them to reuse the same protocol as us (which could be a step towards having something more standard).

On that note - how would you feel about making filePath (/saveFilePath) actually fileUri (/saveUri)? This is a little more consistent with LSP and doesn't restrict things to file:// URIs (even though in the Dart server that's all we'd support).

[bwilkerson]

... would it be better if we typed them based on the type of the field?

Probably so, yes. In which case null makes more sense as the "no value" marker because we can use that consistently.

I think the cost of adding it is very low and if we do want to use it in the future, it avoids additional changes in the client.

I find that to be convincing. Let's add it.

... how would you feel about making filePath (/saveFilePath) actually fileUri (/saveUri)?

I'm good with being more compatible with the existing spec, both in terms of possibly getting this adopted at some future date as well as from the perspective of making it easier for more clients to support it.

[DanTup]

How to add new field types (eg. "openUri") in a non-breaking way (eg. if server knows the client supports this functionality, it can send fields with no defaults, but the client might not know how to provide a value if it's a new type)

@bwilkerson currently we have a bool experimental.commandParameterSupport that a client can pass to indicate that it supports prompting the user (and server will not produce any refactors that require input because they don't have default values). I wonder if we should expand it to instead list the kinds of fields that the client supports:

experimental: {
  commandParameterSupport: {
    supportedKinds: ["saveUri"]
  }
}

This will allow the server to react to different versions of the client. For example if we shipped Dart-Code supporting only saveUri and then added a new refactor to server that needs a string (that has no default), the server doesn't know whether the client supports prompting for that string (only that it supports prompting for "something").

Inside refactors, checks would then be based on the kinds (that they're using that don't have defaults) instead of just a bool:

bool isAvailable() => supportsFileCreation && _memberToMove != null && 
    // This refactor has a string parameter with no default, so it can only be produced if the
    // client is able to prompt for a string.
    clientSupportsPromptingForKind("string");

[bwilkerson]

I wonder if we should expand it to instead list the kinds of fields that the client supports:

Yes. I like that idea. I think it improves the compatibility story and is very much in keeping with the spirit of LSP.

[DanTup]

@bwilkerson there are only two unticked items above:

1. only saveUri is implemented
   This seems fine since we don't have anything else on server yet
2. A question about whether we need explicit detection of these code actions
   I no longer think we do, the current mechanism is likely specific enough (it needs a single argument object with a field called arguments that is an array, as well as parameters in its data field that is also an array with the same length). Additionally, if we accidentally rewrote an unrelated CodeAction through the command, it should essentially behave as a no-op and just call the original command as-is when it fails to prompt for any fields

So my feeling is we should merge this (and ensure any future server changes are compatible with this). WDYT?

The server still has a flag ("dart.previewNewRefactors") that enables the new refactors so nothing will show up yet, but control of that is then entirely in the servers hands.

[bwilkerson]

A couple of questions to double check my understanding.

Is the flag dart.previewNewRefactors controlled by the client? If so (and maybe in either case) we'll need something on the server side to allow us to control which refactorings are produced in production and which are only produced for testing so that we can test new refactorings during their development.

I think the worst case scenario would be where we decided we needed to make an incompatible change to the protocol. In that case we'd need to put something in place so that the server never produced the new command structure when the client only supported the old command structure. Is that the extent of what we'd need to do?

If so, I'm guessing that the standard way that happens in LSP is through the client and server capabilities. Is there anything we might want to do to the protocol (before merging these changes) to make it easier to make changes in the future?

[DanTup]

(turns out the flag is called experimentalNewRefactors not previewNewRefactors)

Is the flag dart.previewNewRefactors controlled by the client?

It comes from the client, but it's not coded specifically in Dart-Code. That is, you can open your VS Code settings and add "dart.experimentalNewRefactors": true and the server will receive that and enable the refactors. However, since this is not declared by Dart-Code, VS code will warn that the setting is unknown (we can ofc declare it in the extension, but if we don't expect users to use it and it's just for our own personal testing, it's not worth doing).

If so (and maybe in either case) we'll need something on the server side to allow us to control which refactorings are produced in production and which are only produced for testing so that we can test new refactorings during their development.

We can certainly use this flag (or another flag, or a flag per refactor) for testing like this. We can invent any settings we want in the server (by just adding them to the client config class like this) that we can then add to our own VS Code user settings without needing to involve the VS Code extension.

I think the worst case scenario would be where we decided we needed to make an incompatible change to the protocol. In that case we'd need to put something in place so that the server never produced the new command structure when the client only supported the old command structure. Is that the extent of what we'd need to do?

If so, I'm guessing that the standard way that happens in LSP is through the client and server capabilities. Is there anything we might want to do to the protocol (before merging these changes) to make it easier to make changes in the future?

Yep, I would suggest we extend the dartCodeAction object we added to the client capabilities to allow a client to advertise that it supports the new format (it's already an object so we can add fields other than commandParameterSupport or additional fields inside commandParameterSupport alongside supportedKinds).

We can use server capabilities if we need to inform the client what we support too, although in most cases I suspect we wouldn't need to. For example we could assume clients sending supportedKinds and not newFeatureFlag as wanting the original behaviour, and clients sending both as wanting the new behaviour. We don't necessarily need to tell them in our response which we will send, although if there are reasons clients might want to know (before they actually get the CodeActions) we could include something there.

I can't think of anything we'd need to do that we couldn't do in a backwards-compatible way - I think between some of the changes above and using objects for the capabilities likely gives us all the flexibility we'll need.

[bwilkerson]

Then merging this PR sounds good to me.

[DanTup]

I've published a new pre-release version of the Dart extension, although it seems to be taking a while to be approved. Once it's up (the version will be 3.51.20221010 - the latter part being YYYYMMDD) if you install it (see image below for how to switch to pre-release if you haven't already) if you add "dart.experimentalNewRefactors": true to your VS Code user settings (press F1 to open palette, then run Preferences: Open User Settings (JSON)) and use a recent analyzer, the new refactor should show up for.

Let me know if you spot any issues (and if/when you have an opinion whether we should remove move-top-level-to-file from under that flag).