jmespath-community / jmespath.spec

JMESPath Specification
6 stars 3 forks source link

Clarification needed for `let()` function behaviour and sub-expressions #152

Closed springcomp closed 1 year ago

springcomp commented 1 year ago

In light of this comment, is appears clarification is needed.

In fact, @jamesls was aware of potential confusions. and @mtdowling talked him out of moving the JEP forward.

[@jamesls] In this scenario we're evaluating foo.qux. foo evaluates to {"bar": "baz"}. So far so good. Next we evaluate the RHS of the sub expression, qux. We see the qux is not defined in the current object {"bar": "baz"} so we look in the scope object and see it's defined as "qux", so we use that value. I can see how some people might find that confusing. Maybe not.

[@mtdowling] We've spoken about this in person, but I wanted to leave feedback here to hopefully help drive this proposal forward. I stated this earlier, but I think that the current behavior of this JEP would be confusing.

I still think this feature brings more benefits as is as it brings confusions but I agree clarification is needed and changes may be made if necessary.

Sub Expressions

It appears the most common source for confusion is trying to use the scope when evaluating the RHS of a sub-expression. The current implementations of the specification behave is unconditionnally simple:

The confusion arises when trying such expressions:

Whereas intuition would dictate that expression to evaluate to null instead, as bar is not a property of the "foo" string.

Pipe Expressions

It’s worth mentioning that JEP-19 standardized a small difference between evaluating a sub-expression vs a pipe-expression.

Thoughts must be applied when clarifying this JEP-11 in this context. For the record, assuming we closed the confusion with sub-expression, I would still be comfortable with the following evaluations:

springcomp commented 1 year ago

Here are some pointers for discussion.

I think the main – if not only – source of confusion comes from the arguably counter-intuitive behaviour when evaluating the RHS of a sub-expression. In that case, I would be happy to close that source of confusion. This would be quite easy (I think) to implement, as each new sub-expression would create a new fresh scope chain.

Here is how it could be implemented:

    case parsing.ASTSubexpression:
      left, err := intr.Execute(node.Children[0], value)
      if err != nil {
        return nil, err
      }
      if left == nil {
        return nil, nil
      }
-    return intr.Execute(node.Children[1], left)
+    return intr.WithScope(nil).Execute(node.Children[1], left)