Parsing upcoming let-expression

[springcomp]

This PR implements the parsing of let-expression rules as they are anticipated to land in an upcoming proposal.

This PR does not implement the lexer, nor does it include any expression evaluation logic.
This is a work in progress.

Fixes #53

[codecov]

Codecov Report

Patch coverage: 89.74% and project coverage change: +0.46 🎉

Comparison is base (7d8d162) 87.88% compared to head (ce48442) 88.34%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #66      +/-   ##
==========================================
+ Coverage   87.88%   88.34%   +0.46%     
==========================================
  Files          11       12       +1     
  Lines        2435     2557     +122     
==========================================
+ Hits         2140     2259     +119     
  Misses        204      204              
- Partials       91       94       +3     

Impacted Files	Coverage Δ
pkg/parsing/astnodetype_string.go	40.00% <ø> (ø)
pkg/parsing/toktype_string.go	40.00% <ø> (ø)
pkg/interpreter/interpreter.go	76.70% <78.94%> (+0.72%)	⬆️
pkg/parsing/parser.go	91.07% <91.20%> (+1.64%)	⬆️
pkg/api/api.go	100.00% <100.00%> (ø)
pkg/binding/binding.go	100.00% <100.00%> (ø)
pkg/parsing/lexer.go	97.32% <100.00%> (+0.05%)	⬆️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[springcomp]

My setup seems broken, I do not understand how to lint my code ☹

[eddycharly]

@springcomp does it work when you try to parse let $root = @ in $root.a ?

I have Unexpected token at the end of the expression: TOKRoot.

[eddycharly]

In let $root it looks like $ is tokenized as TOKRoot.

[springcomp]

I must have forgotten to push this change 🙄.

The reason is that we use $ for root-node as well. I have actually made the same fix in the Python port.

Basically, $ is no longer an ambiguous single char token but must be handled with a dedicated case.

If you can follow the Python diff you will see what I mean. If you need, howevee, I can have a look at it tomorrow.

[eddycharly]

Basically, $ is no longer an ambiguous single char token but must be handled with a dedicated case.

Yeah i was tracking it down, it see it in:

var basicTokens = map[rune]TokType{
	'.':      TOKDot,
	'*':      TOKStar,
	',':      TOKComma,
	':':      TOKColon,
	'{':      TOKLbrace,
	'}':      TOKRbrace,
	']':      TOKRbracket, // tLbracket not included because it could be "[]"
	'(':      TOKLparen,
	')':      TOKRparen,
	'@':      TOKCurrent,
	'$':      TOKRoot,
	'+':      TOKPlus,
	'%':      TOKModulo,
	'\u2212': TOKMinus,
	'\u00d7': TOKMultiply,
	'\u00f7': TOKDivide,
}

I will give a look at the python implementation, i think i can read some basic python 🤞

[eddycharly]

Ok, parsing is passing now, I'll start implementing the bindings tomorrow.

[springcomp]

Ok, parsing is passing now, I'll start implementing the bindings tomorrow.

Perfect. Please note that I did not include the lexing either 😏

[eddycharly]

Please note that I did not include the lexing either

I feel like parsing is not robust enough, see #68 for example (| is not necessarily a ASTPipe, depending on the context it can be an ASTField too).

[eddycharly]

I did not include the lexing either

I did this:

func (lexer *Lexer) consumeVarRef() token {
	// Consume runes until we reach the end of an unquoted
	// identifier.
	start := lexer.currentPos - lexer.lastWidth
	for {
		r := lexer.next()
		if r < 0 || r > 128 || identifierTrailingBits[uint64(r)/64]&(1<<(uint64(r)%64)) == 0 {
			lexer.back()
			break
		}
	}
	value := lexer.expression[start:lexer.currentPos]
	return token{
		tokenType: TOKVarref,
		value:     value,
		position:  start,
		length:    lexer.currentPos - start,
	}
}

[springcomp]

I did this:

This seems perfect. 👍 As the check for a character that starts an unquoted-identifier is handled when testing root-node vs varref.

[eddycharly]

@springcomp I implemented the interpreter part, WDYT ?

[springcomp]

That’s nice. I think we are almost there.

Currently, there are three compliance tests failing.

• let $a = 'top-a' in let $a = 'in-a', $b = $a in $b (bindings only visible within expression clause)

I have the feeling that scopes are not correctly pushed / popped appropriately.

• $noexist (currently returns null whereas it should fail with undefined-variable )
• [let $scope = 'foo' in [$scope], $scope] (referencing out of scope variables)

[eddycharly]

I didn't implement undefined-variable error (there's a TODO in this PR).

[springcomp]

I didn't implement undefined-variable error (there's a TODO in this PR).

Sorry I missed that.

[eddycharly]

let $a = 'top-a' in let $a = 'in-a', $b = $a in $b i might have done this one wrong... I hoped to be able to let each binding register itself but this won't work with this :(

[eddycharly]

variable

I'll do that asap, should not be long.

[eddycharly]

@springcomp i pushed the fixes

[springcomp]

That works great !

Implementation looks good to me.
I like how go enables to discard a scope using the defer keyword.

👍

[eddycharly]

I like how go enables to discard a scope using the defer keyword.

It's similar to finally in .NET

[eddycharly]

I'm curious why we don't allow referencing previous bindings though jmespath/jmespath.py#307 (comment)

[springcomp]

Well we do allow referencing parent bindings.
But as we are creating the current level, the binding does not exist yet.

I'm curious why we don't allow referencing previous bindings though

My understanding is that:

let $a = 'top-a' in let $a = 'in-a', $b = $a in $b

As you are creating the second nested level of bindings, you are defining a shadow value for a but this value is not yet available - visible as the proposal posits - to the other expressions used to create the binding (i.e the expression $a that creates the binding reference for $b)

This seems logical to me. As you would not want to have side effects based upon the order in which each implementation would want to create the binding.

Let's say an implementation would create the binding in alphabetical order of variable names. Or in reverse order. Or in an unspecified order. They would result in different output from one another.

[eddycharly]

Well, i see advantages in allowing it too:
let $curent = ..., $priority = $current.priority in ...

To me it looks very similar to what we do with local variables:

func f(a int) {
  a := 10 // `a` shadows the `a` function argument
  b := a  // `b` references `a` local variable
}

[springcomp]

Well, i see advantages in allowing it too [...]
To me it looks very similar to what we do with local variables [...]

Sure it makes sense either way.
In the end this is a convention based upon the spec.

For this to work though, the spec would need to absolutely specify the order in which binding variables are created.

[eddycharly]

For this to work though, the spec would need to absolutely specify the order in which binding variables are created.

I was assuming variables were initialised in the declaration order.

[springcomp]

I was assuming variables were initialised in the declaration order.

That's worth clarifying indeed.

[eddycharly]

@springcomp shall we get this in ?

[springcomp]

@springcomp shall we get this in ?

Good question. I actually have no idea how much time James will allow for the proposal to reach consensus.
In any case, this is a better proposition than the let() function, so I would include this anyway.

I will proceed with other implementations as well and start the deprecation process in the spec.

I think we need to care about code coverage which seems to have decreased.

[eddycharly]

@springcomp i added unit tests for the bindings package.

[springcomp]

@eddycharly can we include in this pull request provisions for settings a global scope from the jp command-line.
I’m referring to this part of the spec:

The JEP also requires that unbound values error at evaluation time, not at compile time. This enables implementations to bind an initial (global) scope when a query is evaluated. This is something that other query languages provide, and is useful to define fixed queries that only vary by the value of specific variables.

[eddycharly]

@eddycharly can we include in this pull request provisions for settings a global scope from the jp command-line.

I'll do that tomorrow 👍

[springcomp]

@eddycharly FYI let-expression is now part of the language having recently been approved by James.

[eddycharly]

@springcomp yeah i saw that, awesome !

[springcomp]

Hello @eddycharly @JimBugwadia should this pull request be merged ?

[eddycharly]

@springcomp sorry, we were quite busy during the last weeks.

I think this is good to merge, we can improve in follow-up PRs if it's ok.

[springcomp]

I think this is good to merge, we can improve in follow-up PRs if it's ok.

That’s perfect for me.
Please, merge at your own pace.

Thanks a lot.