ast: parsed `Call` location `Text` attribute off by one character

[anderseknert]

package p

r if foo(bar(1))

opa parse p.rego --format json --json-include locations

{
    "location": {
        "file": "p.rego",
        "row": 3,
        "col": 10,
        "text": "YmFyKDEpKQ=="
    },
    "type": "call",
    "value": [

    ]
}

echo "YmFyKDEpKQ==" | base64 -d
bar(1))

The text should of course be bar(1) with only a single closing paren.

[schmitd]

looks like parseCall scans to check for a set term before returning.

if p.s.tok == tokens.RParen { // no args, i.e. set() or any.func()
		end = p.s.tokEnd
		p.scanWS()
		if operator.Equal(setConstructor) {
			return SetTerm()
		}
		return CallTerm(operator)

then when back in parseRef we use tokEnd

case tokens.LParen:
			term = p.parseCall(p.setLoc(RefTerm(ref...), loc, offset, p.s.loc.Offset), offset)
			if term != nil {
				switch p.s.tok {
				case tokens.Whitespace:
					p.scan()
					end = p.s.lastEnd
					return term
				case tokens.Dot, tokens.LBrack:
					term = p.parseRef(term, offset)
				}
			}
			end = p.s.tokEnd // Change to p.s.lastEnd?
			return term

If line 1981 in parser.go is changed to end = p.s.lastEnd
then we get:

    "location": {
        "file": "p.rego",
        "row": 3,
        "col": 10,
        "text": "YmFyKDEp"
    }

echo "YmFyKDEp" | base64 -d
bar(1)

as expected.

[anderseknert]

Hey @schmitd, and sorry for not getting back to you on this until now! If the fix is that simple, that's amazing! And just as before, your contributions are much appreciated ❤️

Considering how little the text attribute is used by OPA itself, I expect our test coverage to be rather minuscule here, which would also help explain why this wasn't caught before. The only reason I found this was via Regal development. Regal makes extensive use of the text attribute in order to attempt determine the end location of AST nodes, as OPA Locations only provide their start position. So while we should certainly add a test or two in OPA to cover this, the best test suite for this change is undoubtedly the one we have in Regal. Adding it as a TODO to try this change there, and will get back to you with my findings. If you'd like to try it out yourself before that, you can always clone the Regal repo and use a replace directive on the OPA dependency to see what happens when you run those tests. Could be some tests fail not because the fix is wrong, but because we previously made some wrong assumptions. Only one way to find out :)

[schmitd]

I amended the commit from earlier to actually contain the change (blaming that one on the flu).

With the fix checked out and the replace in go.mod:

--- FAIL: TestSerializedModuleSize (0.01s)
    module_test.go:227: expected 85979 but got 85981
FAIL

Seems to be "just" this under go test.

"This test will fail whenever the size of the serialized module changes, which not often and when it happens it's good to know about it, update and move on."

otherwise:

$ ./regal test bundle
PASS: 878/878

[anderseknert]

Amazing! Looking forward to the PR 🙂