"opa fmt" makes policy with two "else" invalid

[srenatus]

# bug.rego
package bug
import future.keywords.if

p if false
else := 1 if { false }
else := 2

With this policy, opa eval -fpretty -d bug.rego data.bug.p gives 2:

$ opa eval -fpretty -d bug.rego data.bug.p
2

When running opa fmt -w on it, it is transformed to

# bug.rego
package bug

import future.keywords.if

p if false

else := 1 if false

else := 2

This file, however, OPA fails to evaluate:

$ opa eval -fpretty -d bug.rego data.bug.p
1 error occurred: bug.rego:10: rego_parse_error: unexpected else keyword
	else := 2
	^

Two solutions come to mind:

1. making the formatter not emit the short form for else bodies
2. make the parser accept the short form for else bodies

I think we should go with (2.). It's hard to explain why the short form isn't valid for else bodies in the first place.

[anderseknert]

Agreed!

[Ronnie-personal]

I'm interested in working on this bug, could you please point out which code files need to be modified?

Thanks!

[ashutosh-narkar]

Thanks for your interest @Ronnie-personal! The else processing starts here and then goes into the parseElse call. That would be a good place to start.

[Ronnie-personal]

I'm thinking of making the following code changes. Am I heading in the right direction?

'func (p *Parser) parseElse(head *Head) *Rule {

var rule Rule
rule.SetLoc(p.s.Loc())

rule.Head = head.Copy()
for i := range rule.Head.Args {
	if v, ok := rule.Head.Args[i].Value.(Var); ok && v.IsWildcard() {
		rule.Head.Args[i].Value = Var(p.genwildcard())
	}
}
rule.Head.SetLoc(p.s.Loc())

defer func() {
	rule.Location.Text = p.s.Text(rule.Location.Offset, p.s.lastEnd)
}()

p.scan()

switch p.s.tok {
case tokens.LBrace, tokens.If: // no value, but a body follows directly
	rule.Head.Value = BooleanTerm(true)
case tokens.Assign, tokens.Unify:
	rule.Head.Assign = tokens.Assign == p.s.tok
	p.scan()
	rule.Head.Value = p.parseTermInfixCall()
	if rule.Head.Value == nil {
		return nil
	}
	rule.Head.Location.Text = p.s.Text(rule.Head.Location.Offset, p.s.lastEnd)
default:
	p.illegal("expected else value term or rule body")
	return nil
}

hasIf := p.s.tok == tokens.If

if hasIf {
	p.scan()
}

if p.s.tok == tokens.LBrace {
	p.scan()

	if rule.Body = p.parseBody(tokens.RBrace); rule.Body == nil {
		return nil
	}
}

p.scan()

if p.s.tok == tokens.Else {
	if rule.Else = p.parseElse(head); rule.Else == nil {
		return nil
	}
}
return &rule

}`

[Ronnie-personal]

This code successfully evaluated the following two policies.

p if false
else := 1 if { false }
else := 2

p if false
else := 1 if false
else := 2

[anderseknert]

The best way to find out is submitting a PR with tests included :)

[ashutosh-narkar]

Fixed in #6204.