All-or-nothing matches

[alex-dixon]

First, this library is awesome.

I noticed captures can return undefined: https://github.com/codemod-js/codemod/blob/ac879f8c0109735af7f6b0e150832c2897b352f5/packages/matchers/src/matchers/capture.ts#L9

I'm guessing this is to support partial matches, but sometimes I want matcher.match to fail if one of its captures fails.

export const matchRamdaExpr = (node:unknown, ramdaIdent = "R"): [t.MemberExpression, t.Identifier] | null => {
  let $methodName = m.capture(m.identifier())
  const matcher =
    m.memberExpression(
      m.identifier(ramdaIdent),
      $methodName,
    )

  return matcher.match(node)
   // need this guard if the capture can fail but the rest of the match can succeed
  // can remove it if capture.current returns `T` instead of `T` | undefined
   && $methodName.current
    ? [node, $methodName.current] : null
}

[eventualbuddha]

First, this library is awesome.

Thanks! I'm definitely open to suggestions so thanks for filing this issue. Is the thing you're using this for on Github? I'm curious to see how people are using it.

I'm guessing this is to support partial matches, but sometimes I want matcher.match to fail if one of its captures fails.

This should be the way it is now. The trouble is that the match has a side effect which I can't really encode into the type system easily, so the result of get current() needs to potentially be undefined since you can call it before a match happens. I worked around this issue with matchPath by only calling the callback with unwrapped optionals:

https://github.com/codemod-js/codemod/blob/ac879f8c0109735af7f6b0e150832c2897b352f5/packages/matchers/src/utils/matchPath.ts#L33-L35

But this won't work in your example since you're working with Node values and not NodePath. One possibility would be to change the behavior of get current() to throw if there isn't a captured value yet. This would be a breaking change, but I think it might be worth the ergonomic improvement. Maybe also add hasCaptured(): boolean to better support things like the or matcher where one or more of the branches might not have any captures.

What do you think?

[alex-dixon]

Is the thing you're using this for on Github? I'm curious to see how people are using it.

Just me playing around so far. Trying to define a query made up of one or more conditions for Babel's AST that I can name, reuse, combine, compose. If not all conditions are met, then the thing I'm looking for doesn't exist and I expect the query returns nothing. If all conditions are met I expect something truthy and all captures bound with the patterns they matched.

One possibility would be to change the behavior of get current() to throw if there isn't a captured value yet. This would be a breaking change, but I think it might be worth the ergonomic improvement.

throwing is an interesting proposal. Recently read this: https://overreacted.io/algebraic-effects-for-the-rest-of-us/. Could allow for some kind of continuation pattern or flexibility and communication not provided by normal functions.

I don't think this warrants a breaking change because the current behavior is really powerful and useful and working perfectly. I think if all-or-nothing matches are supported at all they should be added as a new thing. Based on what you're telling me, I'm worried that might require copying and pasting a large part of the codebase with minor alterations. I'll start looking into it.

[alex-dixon]

Something like this could be a userland solution.

type CaptureMap<C> = { [k: string]: m.CapturedMatcher<any, C> }
type BoundCaptures<C> = { [k: string]: C }

function query<T, C>(
  value: unknown,
  condition: m.Matcher<T>,
  bindings: CaptureMap<C>,
): BoundCaptures<C> | void {
  if (!condition.match(value)) return

  let boundCaptures = {} as BoundCaptures<C>
  for (let k in bindings) {
    let v = bindings[k].current
    if (!v) return
    boundCaptures[k] = v
  }

  return boundCaptures
}

[eventualbuddha]

throwing is an interesting proposal. Recently read this: https://overreacted.io/algebraic-effects-for-the-rest-of-us/. Could allow for some kind of continuation pattern or flexibility and communication not provided by normal functions.

That's funny. I just read that recently too.

I don't think this warrants a breaking change because the current behavior is really powerful and useful and working perfectly. I think if all-or-nothing matches are supported at all they should be added as a new thing. Based on what you're telling me, I'm worried that might require copying and pasting a large part of the codebase with minor alterations. I'll start looking into it.

I don't think it would actually be that big a deal. It would technically be a breaking change, but the difference would be fairly minimal. Instead of checking matcher.current you'd check matcher.hasCaptured() or something like that. Your example above would become this:

  return matcher.match(node) ? [node, $methodName.current] : null

You'd know it's safe to access $methodName.current because you'd know that match couldn't have returned true if the $methodName matcher didn't capture a value. This could be wrapped in something to make it a bit safer if you wanted, similar to matchPath:

  return m.matchNode(
    matcher, node,
    { $methodName },
    ({ $methodName }) => [node, $methodName]
  );

Might be overkill.

[eventualbuddha]

Actually, I totally forgot that this is what the match helper does. You can use it like so:

export const matchRamdaExpr = (node:unknown, ramdaIdent = "R"): [t.MemberExpression, t.Identifier] | null => {
  let $methodName = m.capture(m.identifier())
  const matcher =
    m.memberExpression(
      m.identifier(ramdaIdent),
      $methodName,
    )

  let result: ReturnType<typeof matchRamdaExpr> = null;

  m.match(
    matcher,
    { $methodName },
    node,
    ({ $methodName }) => result = [node as t.MemberExpression, $methodName]
  );

  return result;
}

If m.match returned the value of callback when matching, you could simplify to this:

export const matchRamdaExpr = (node:unknown, ramdaIdent = "R"): [t.MemberExpression, t.Identifier] | null => {
  let $methodName = m.capture(m.identifier())
  const matcher =
    m.memberExpression(
      m.identifier(ramdaIdent),
      $methodName,
    )

  return match(
    matcher,
    { $methodName },
    node,
    ({ $methodName }) => [node as t.MemberExpression, $methodName]
  ) || null;
}

What do you think?

[eventualbuddha]

I took a stab at it in #240. Let me know what you think.

[alex-dixon]

Thanks. I really admire your attentiveness. I think we'd all prefer no breaking changes unless absolutely necessary and plain returns instead of callbacks, but I haven't had time to look into the issue any deeper. I felt like the query sketch above was at the right level to implement something like this, above the API as it is now. In conjunction with it, your examples reminded me of Datomic's query syntax: https://docs.datomic.com/cloud/query/query-data-reference.html

;; query
[:find ?e
 :where [?e :artist/name "The Beatles"]]

;; args
db

;; Example query using the clojure API
(d/q '[:find ?e
      :where [?e :artist/name "The Beatles"]]
     db)

;; The previous example also works with datomic.client.api used for Datomic Cloud
;; Example query using the datomic.client.api arity-1

(d/q {:query '[:find ?e
               :where [?e :artist/name "The Beatles"]]
     :args [db]})

;; Result. Your exact value may differ.
[[26757714973567138]]

Seems like we're bordering on a similar signature though maybe not quite. (query find <bound-vars-I-want-back> <patterns-with-variable-bindings> <the-thing-to-match-against>).

Preserving partial matches would be important to me personally cause I think there's a fair amount of cool things and optimization that could be done with the information that some things matched but not others.