Add support for client-side regex validators in interactive forms - #6108

Does the validation allow for both integer and floating point values? I'm guessing the answer is 'yes' and the server is responsible for rejecting kinds of numbers that it can't handle. (Which is fine, just checking my understanding.)

The current implementation does allow either (this is the same before and after this PR, my changes in number validation here are mostly for consistency - although I see I didn't make it completely consistent, so I will tidy this up a bit more tomorrow).

We could add a validator kind for numbers for whole numbers similar to how we added a regex validator for strings here (we could also just add a flag on the number field, but maybe the validator would keep things a bit more general).

I would suggest we do add this if either of us start using numeric fields, because it's a nicer experience to have them validation client-side than only at the "end" of the form, even if it is just one input.

I agree that we don't need to add them until we have a use case.

I updated the test in b593384 to include a decimal number so this behaviour is clearer.

I do wonder if this behaviour is obvious though - I suspect it just fell out of using JS/TS here - @h9jiang do you have an opinion on this? If you're not already using number (or you assumed it was integers), we could "fix" that now. Otherwise we can keep as-is and decide on how to restrict to whole numbers when someone first uses it.

is this a & logic or a | logic if multiple validator exist. I assumed it is "and".

Correct, I've added this to the comment:

Answers must pass all (supported) validators to be considered valid.

is that possible to move the block up so all the type decl are together on top of the file.

Done!

The LSP has a quite strict rule of regex engine

see https://microsoft.github.io/language-server-protocol/specifications/lsp/3.18/specification/#regExp

I assume the regex pattern here will adhere to the client capability. It is not necessary to reference the regex client capabilities here (but it would be more clear if we can mention that).

Oh, I was unaware of this capability and had assumed LSP was fixed. I've updated this comment to:

/**
 * The regex pattern to validate the input.
 *
 * The server must only provide regular expressions for the engine supported by the client
 * in the `general.regularExpressions` client capability. If the server cannot provide an
 * appropriate regular expression it should not provide the regex validator.
 */

Does this make sense to you?

Also FYI, I didn't realise this capability was new in LSP 3.18, so this does mean you'll need to be on the latest LSP client for the server to be passed this. I think it's probably better to require this than to make servers assume the client knows ES regex if not provided. Dart-Code is already on the latest LSP client.

I see, in that case, I need to also upgrade the client to lsp 3.18, but the gopls latest release v0.23.0 is using 3.17 so I will make sure to upgrade the vscode-go to 3.18 after the current vscode-go release so it can work with the next gopls release v0.24.0.

Does this make sense to you?

Thanks. Yes this make sense to me.

do we allow multiple validator of the same kind? I assumed yes...

Yep, I've added:

Multiple validators of the same kind are allowed.

I understand the purpose of the field but I do find it confusing in combination with the message below.

The message below says, if the input does not match the pattern. But if the matchIsValid == false, then message should be positive. if the matchIsValid == true, then message should be negative.

Good catch - I wrote the comment on message before I added matchIsValid. I've updated the comment on message now to:

The message to show if the answer is not valid according to pattern and matchIsValid.

Hongxiang asked me to take a look. This CL seems to be mostly a Dart reflection of the agreed protocol changes, so I'm not qualified to say anything intelligent about the code. About the protocol however:

Regular expressions vary between implementations. (Even RE2, used by Go, varies across languages since the original C++ implementation is defined to operate on a byte stream wheres the Go and Java ports operation on a sequence of Unicode code points, so the "any byte" operator has no meaning.)

The LSP of course acknowledges this in its RegularExpressionsClientCapabilities feature, but it's not clear to me how that feature can realistically work, as it expects both peers (client and server) to check the regular expression engine in use and either support it, emulate it, or (more likely) have a go at at it with the wrong engine, or just refuse to do anything. Given that most (client, server) pairs have a language barrier, it seems almost doomed not to work.

I don't think our protocol changes are really making the LSP regexp situation worse than it already is, but that's my one concern with our use of regexp validation.

@adonovan thanks for taking a look!

The validation here is by design entirely optional - it's only for improving the user experience and if either the server or the client dropped the validation, nothing bad should happen (since the server would do the normal server-side validation when the whole form is submitted).

So, my intention was that a server should only provide regular expression validators if it can do so in the format supported by the client (in RegularExpressionsClientCapabilities). For the VS Code LSP client, that's ES2020. If the client uses a different regex engine, then the server should either provide the regex patterns in a compatible form, or not provide them at all. (if this isn't clear from the code, I should definitely make this clearer).

It'd be nice if there was a better story for this, but I think it will be difficult to solve more generally (without implemented some very specific rules in the spec, or some parallel to regex) - I'm definitely open to ideas though :-)

This CL seems to be mostly a Dart reflection of the agreed protocol changes, so I'm not qualified to say anything intelligent about the code.

btw, I believed the goal was to share the form.ts file, so I think all changes here are potentially relevant for Go. It's not intended that anything here is Dart-specific (although my motivation is to support this in the Dart server).

Agreed, I don't think there's much we can do; I just think it is wildly unrealistic for clients and servers to do anything other than execute their own dialect of regular expressions and hope that most server-provided expressions fall in the least-common denominator subdialect.

the goal was to share the form.ts file, so I think all changes here are potentially relevant for Go

Yeah, I understand that our tools will share and execute parts of the code, I just don't know enough TS to review them competently.

I just think it is wildly unrealistic for clients and servers to do anything other than execute their own dialect of regular expressions and hope that most server-provided expressions fall in the least-common denominator subdialect.

I'll just point out that it isn't a requirement that the client and server use the same regular expression, only that they use equivalent regular expressions. It isn't even a requirement that the server use a regular expression at all, though in most cases that will be the easiest way to keep the client and server consistent with each other.

I also think it's best for servers to not return these regex validators if the clients regex engine is not one that is likely to be compatible the the expressions provided. In the Dart server we're only going to return these optional validators if the client uses the ECMAScript regex engine, because that's what they're written/tested for. This means clients that use other regex engines will fall back to the original server-validation only.

If there are significant LSP clients that are not using ECMAScript, nothing prevents the server from returning a different set of regular expressions for that engine for those clients (or if the expressions happen to be compatible, the same one).

@bwilkerson perhaps it's worth capturing the regexp engine from client capabilities in analytics?

That makes sense to me. There are probably other capabilities that it would be useful to capture as well. We should talk off-line. Let me know when you're available.