Rewrite fluid match pattern reconstruction

[StachuDotNet]

Reconstruction (what happens when you copy/paste some Dark code) of Exprs has been generally solid in Fluid, but the implementation for reconstructing match patterns was hacky, and did not support reconstructing only some of a nested pattern (i.e. if a Tuple pattern or Constructor pattern was partially highlighted when cutting/copying, the whole thing would be reconstructed - even the unselected parts).

This is how it used to operate:

and here's with the fix

---

This broke down as follows:

• in FluidTokenization.res, organize the code to have all of the actual 'tokenization' be in one area visually.
• extend tokenization MP inputs (without needing to pass in the wrapping match expr)
• rename FluidTokenization.tokensForEditor to .tokenizeForDebugger
  previously, we had tokensForEditor and tokenizeForEditor - I found this pretty confusing.
• (goal 1) rewrite implementation to match how we reconstruct exprs (no longer trying to parse directly from the output tokens)
• (goal 2) support reconstructing just part of a nested match pattern

TODOs:

• more tests on reconstructing match patterns, generally
• tests specifically around reconstructing only part of a nested match pattern
• record screen to demonstrate the fix to reconstructing nested match pats (maybe to be copied to changelog)
• based on the below feedback, potentially remove mentions of non-existant MPPartial, and just commit to the logic we agree upon. Edit: will leave the comments in, and follow up here soon when MPPartials are implemented.

---

In this re-implementation, I've reached for something like a MPPartial a few times - EPartials are used in reconstruction often when we've highlighted just part of a pattern, resulting in a way for the user to 'correct' the reconstructed code. For match pattern reconstruction, I've had to work around this. Would it make sense to add an MPPartial at some point?

In the meantime, here are the cases where I reached for MPPartial:

• when you've highlighted partway through a Null pattern
  my thought: reconstruct with a Null pattern
• when you've highlighted partway through a Bool pattern (with a value of true or false)
  my thought: reconstruct with the appropriate Bool pattern
• when you've highlighted partway through the name in a Constructor pattern
  my thought: reconstruct with a Blank pattern

What do you think about these cases?

---

When reconstructing a match, here's the logic to choose which cases should be reconstructed or skipped:

let newCases = List.filterMap(cases, ~f=((matchPattern, expr)) => {
  switch (reconstructMatchPattern(matchID, matchPattern), reconstructExpr(expr)) {
  | (Some(mp), Some(expr)) => Some(mp, expr)
  | (Some(mp), None) => Some(mp, None |> orDefaultExpr)
  | (None, Some(_expr)) => None // todo: reconsider
  | (None, None) => None
  }
})

This was recently (in a previous PR) written to match some previous logic.
I think this is generally appropriate, but wanted to get some input on the third branch:
| (None, Some(_expr)) => None

If we were able to reconstruct some Expr, but not reconstruct the Match Pattern (i.e. if you select the expr "downwards"), we don't create the new case. I don't think that makes sense - rather, I'd expect us to reconstruct a new case, but with a Blank pattern. What do you think about this branch?

---

I've lately become increasingly wary of searching for these magic strings "match-pattern-variable" within Fluid.res. I'm tempted to create a copy of the FluidTypes.Token.t which doesn't have the internals, and is a simple DU to use in these cases. That wouldn't help the existing CSS usages of the magic strings, but those are pretty rare anyway.

[StachuDotNet]

I'm currently working through the outlined TODOs, but think that this is at a good place for an initial review.

[StachuDotNet]

• adjust tokenization to allow for both Expr and MP inputs (previously, everything was Expr-focused)
  I used FluidExpression's fluidMatchPatOrExpr for this, which felt appropriate enough, though results in some Expr(*) wrapping noise in usages. e.g. FluidTokenizer.tokenize(expr) became FluidTokenizer.tokenize(Expr(expr)). I don't think it's a huge deal, but open to input.

I'm having to revert this, as fluidMatchPatOrExpr MatchPat case doesn't include the index of the case branch the pattern is on, which is needed for consistent tokenization. Trying to add that index into the MatchPat case caused a bunch of headaches, so:
Instead, for now, I'll just expose a separate function for tokenizing MPs.

[StachuDotNet]

outstanding question - why do we need this separate tokenizer?
when would we have a FF but no expr corresponding to the expr's ID?

[pbiggar]

In this re-implementation, I've reached for something like a MPPartial a few times - EPartials are used in reconstruction often when we've highlighted just part of a pattern, resulting in a way for the user to 'correct' the reconstructed code. For match pattern reconstruction, I've had to work around this. Would it make sense to add an MPPartial at some point?

In the meantime, here are the cases where I reached for MPPartial:

• when you've highlighted partway through a Null pattern
  my thought: reconstruct with a Null pattern
• when you've highlighted partway through a Bool pattern (with a value of true or false)
  my thought: reconstruct with the appropriate Bool pattern
• when you've highlighted partway through the name in a Constructor pattern
  my thought: reconstruct with a Blank pattern

What do you think about these cases?

Yes, I've been thinking the same thing for weeks, I'm surprised I didn't actually mention it. There are a few places where we could use partial expressions to get better results, and we would want those in patterns as well, see #4494, #4493, and #4499.

When reconstructing a match, here's the logic to choose which cases should be reconstructed or skipped:

If we were able to reconstruct some Expr, but not reconstruct the Match Pattern (i.e. if you select the expr "downwards"), we don't create the new case. I don't think that makes sense - rather, I'd expect us to reconstruct a new case, but with a Blank pattern. What do you think about this branch?

With this sort of thing, I can't really tell in the abstract. If you have some examples I'd be better able to give an opinion.

I've lately become increasingly wary of searching for these magic strings "match-pattern-variable" within Fluid.res. I'm tempted to create a copy of the FluidTypes.Token.t which doesn't have the internals, and is a simple DU to use in these cases. That wouldn't help the existing CSS usages of the magic strings, but those are pretty rare anyway.

Yes, I agree. I was trying to say something like this before, how maybe we could use caretTargets for this. Unsure if that's appropriate, but I agree the strong approach is untenable.

[pbiggar]

This is looking good.

[StachuDotNet]

MPPartials
Great, we're on the same page about moving forward with that. To not increase scope of this PR, I'll leave the relevant comments in, and plan to circle back to that soon unless you'd like to.

When reconstructing a match, here's the logic to choose which cases should be reconstructed or skipped:
If we were able to reconstruct some Expr, but not reconstruct the Match Pattern (i.e. if you select the expr "downwards"), we don't create the new case. I don't think that makes sense - rather, I'd expect us to reconstruct a new case, but with a Blank pattern. What do you think about this branch?

With this sort of thing, I can't really tell in the abstract. If you have some examples I'd be better able to give an opinion.

Here's what happens right now if you select only the -> and the expr of a match's case:

Here's what I would actually expect, and propose to update to:

This change would make the behavior consistent with what happens when you select only the MP and the -> of a match's case:

[StachuDotNet]

This PR has now accomplished its goals - original screenshot shows the main difference (able to reconstruct partially-selected match patterns), and everything is well-tested.

[pbiggar]

Will look shortly. I agree with the new behaviour in the screenshots.