Opa format panic caused by comments

[sirpi]

Short description

Wrongly placed comments can cause a panic error message in cli.

OPA version: any (tested even with 0.57.0) Type: CLI

Steps To Reproduce

opa fmt mypolicy.rego

Shortest rego content:

package a

value := {"a":  #
"b"} #

For that, the following stack trace appears:

panic: overwriting non-nil beforeEnd

goroutine 1 [running]:
[github.com/open-policy-agent/opa/format.(*writer).beforeLineEnd(..](http://github.com/open-policy-agent/opa/format.(*writer).beforeLineEnd(..).)
        /src/format/format.go:1326
[github.com/open-policy-agent/opa/format.(*writer).insertComments(0xc0003d6af0](http://github.com/open-policy-agent/opa/format.(*writer).insertComments(0xc0003d6af0), {0xc0000a65d0, 0x1, 0x2ba5c80?}, 0xc0004656c0)
        /src/format/format.go:513 +0x2b9
[github.com/open-policy-agent/opa/format.(*writer).writeTermParens(0xc0003d6af0](http://github.com/open-policy-agent/opa/format.(*writer).writeTermParens(0xc0003d6af0), 0xa0?, 0xc00049cfc0, {0xc0000a65d0?, 0x1d27373?, 0x1b25340?})
        /src/format/format.go:699 +0x4b
[github.com/open-policy-agent/opa/format.(*writer).writeTerm(0x45540f](http://github.com/open-policy-agent/opa/format.(*writer).writeTerm(0x45540f)?, 0x1d27373?, {0xc0000a65d0?, 0x0?, 0x0?})
        /src/format/format.go:695 +0x26
[github.com/open-policy-agent/opa/format.(*writer).writeObject.(*writer).objectWriter.func2({0x1b58f60](http://github.com/open-policy-agent/opa/format.(*writer).writeObject.(*writer).objectWriter.func2(%7B0x1b58f60)?, 0xc00009edd0?}, {0xc0000a65b8?, 0xc00009ede0?, 0xc0004392e4?})
        /src/format/format.go:1045 +0xa8
[github.com/open-policy-agent/opa/format.(*writer).writeIterableLine(0xc0003d6af0](http://github.com/open-policy-agent/opa/format.(*writer).writeIterableLine(0xc0003d6af0), {0xc00009ee30, 0x1, 0xc000465740?}, {0xc0000a65b8?, 0x2ba5c80?, 0xc000465bc0?}, 0xc0005bf188)
        /src/format/format.go:1037 +0x110
[github.com/open-policy-agent/opa/format.(*writer).writeIterable(0xc0003d6af0](http://github.com/open-policy-agent/opa/format.(*writer).writeIterable(0xc0003d6af0), {0xc00009ede0?, 0x9e0905?, 0x0?}, 0x0?, 0x0?, {0xc0000a65b8, 0x1, 0x1}, 0xc0005bf188)
        /src/format/format.go:1013 +0x1a9
[github.com/open-policy-agent/opa/format.(*writer).writeObject(0xc0003d6af0](http://github.com/open-policy-agent/opa/format.(*writer).writeObject(0xc0003d6af0), {0x21e9d20, 0xc000465800}, 0xc00049d680?, {0xc0000a65b8, 0x1, 0x1})
        /src/format/format.go:864 +0x19e
[github.com/open-policy-agent/opa/format.(*writer).writeTermParens(0xc0003d6af0](http://github.com/open-policy-agent/opa/format.(*writer).writeTermParens(0xc0003d6af0), 0xae?, 0xc00049cf30, {0xc0000a65b0?, 0x2?, 0x45540f?})
        /src/format/format.go:708 +0x4fb
[github.com/open-policy-agent/opa/format.(*writer).writeTerm(..](http://github.com/open-policy-agent/opa/format.(*writer).writeTerm(..).)
        /src/format/format.go:695
[github.com/open-policy-agent/opa/format.(*writer).writeHead(0xc0003d6af0](http://github.com/open-policy-agent/opa/format.(*writer).writeHead(0xc0003d6af0), 0xc0003d6a80, 0x0, 0x1, {0x3d?, 0x0?, 0xc0?}, {0xc0000a65b0, 0x1, 0x1})
        /src/format/format.go:501 +0x59c
[github.com/open-policy-agent/opa/format.(*writer).writeRule(0xc0003d6af0](http://github.com/open-policy-agent/opa/format.(*writer).writeRule(0xc0003d6af0), 0xc00007eb90, 0x2?, {0x0?, 0x0?, 0x0?}, {0xc0000a65b0, 0x1, 0x1})
        /src/format/format.go:346 +0x1ec
[github.com/open-policy-agent/opa/format.(*writer).writeRules(0x0](http://github.com/open-policy-agent/opa/format.(*writer).writeRules(0x0)?, {0xc0000a4720, 0x4, 0xc0000b0f80?}, {0x28?, 0xf5?, 0x5b?}, {0xc000465ac0, 0x5, 0x8})
        /src/format/format.go:321 +0x99
[github.com/open-policy-agent/opa/format.(*writer).writeModule(0xc0003d6af0](http://github.com/open-policy-agent/opa/format.(*writer).writeModule(0xc0003d6af0), 0xc0000b1000?, {0x20?, 0xd3?, 0x49?})
        /src/format/format.go:280 +0x4ea
[github.com/open-policy-agent/opa/format.AstWithOpts({0x1c3fbe0](http://github.com/open-policy-agent/opa/format.AstWithOpts(%7B0x1c3fbe0)?, 0xc0000b0f80?}, {0x0?})
        /src/format/format.go:157 +0x5f1
[github.com/open-policy-agent/opa/format.Ast(..](http://github.com/open-policy-agent/opa/format.Ast(..).)
        /src/format/format.go:65
[github.com/open-policy-agent/opa/format.Source({0xc00000a0e8](http://github.com/open-policy-agent/opa/format.Source(%7B0xc00000a0e8), 0x8}, {0xc000022200?, 0x64?, 0x4cf107b8?})
        /src/format/format.go:44 +0x9b
[github.com/open-policy-agent/opa/cmd.formatFile(0x2c069a4](http://github.com/open-policy-agent/opa/cmd.formatFile(0x2c069a4), {0x21d3080, 0xc000074030}, {0xc00000a0e8, 0x8}, {0x21e0b88, 0xc0003d61c0}, {0x0?, 0x0?})
        /src/cmd/fmt.go:111 +0x265
[github.com/open-policy-agent/opa/cmd.opaFmt.func1({0xc00000a0e8](http://github.com/open-policy-agent/opa/cmd.opaFmt.func1(%7B0xc00000a0e8)?, 0xc0003d61c0?}, {0x21e0b88?, 0xc0003d61c0?}, {0x0?, 0x0?})
        /src/cmd/fmt.go:76 +0x59
path/filepath.walk({0xc00000a0e8, 0x8}, {0x21e0b88, 0xc0003d61c0}, 0x2084818)
        /usr/local/go/src/path/filepath/path.go:484 +0xff
path/filepath.Walk({0xc00000a0e8, 0x8}, 0x2084818)
        /usr/local/go/src/path/filepath/path.go:579 +0x66
[github.com/open-policy-agent/opa/cmd.opaFmt({0xc00009e740](http://github.com/open-policy-agent/opa/cmd.opaFmt(%7B0xc00009e740)?, 0x1, 0x0?})
        /src/cmd/fmt.go:75 +0xe5
[github.com/open-policy-agent/opa/cmd.glob..func2(0x1a86760](http://github.com/open-policy-agent/opa/cmd.glob..func2(0x1a86760)?, {0xc00009e740?, 0x4?, 0x1d27ee2?})
        /src/cmd/fmt.go:53 +0x25
[github.com/spf13/cobra.(*Command).execute(0x1a86760](http://github.com/spf13/cobra.(*Command).execute(0x1a86760), {0xc00009e700, 0x1, 0x1})
        /src/vendor/[github.com/spf13/cobra/command.go:944](http://github.com/spf13/cobra/command.go:944) +0x863
[github.com/spf13/cobra.(*Command).ExecuteC(0x1a86480)](http://github.com/spf13/cobra.(*Command).ExecuteC(0x1a86480))
        /src/vendor/[github.com/spf13/cobra/command.go:1068](http://github.com/spf13/cobra/command.go:1068) +0x3a5
[github.com/spf13/cobra.(*Command).Execute(0xc00006a000](http://github.com/spf13/cobra.(*Command).Execute(0xc00006a000)?)
        /src/vendor/[github.com/spf13/cobra/command.go:992](http://github.com/spf13/cobra/command.go:992) +0x13
main.main()
        /src/main.go:14 +0x1a

Expected behavior

A comprehensive error message without a stack trace would be more suitable.

The root cause of this is the clever mechanism which moves comments to the end of a formatted line if that would appear in the middle. Though if there is already a comment there, we get this error.

Additional context

[anderseknert]

Thanks for reporting this @sirpi 👍 Sounds like you've done some research already. Would you want to try and fix it? 🙂

Is an error message the expected behavior though? Perhaps I'm missing something obvious, but I'd expect the formatter to format anything as long as it's valid Rego.

[philipaconrad]

@anderseknert How should the two comments be merged together? Is that a case the formatter can resolve on its own?

[sirpi]

Thanks for reporting this @sirpi 👍 Sounds like you've done some research already. Would you want to try and fix it? 🙂

Is an error message the expected behavior though? Perhaps I'm missing something obvious, but I'd expect the formatter to format anything as long as it's valid Rego.

Formatting the rego file without any complaint would be even better :) But I don't see how the comments should be merged automatically. Because a general solution should solve this for any number of comments as well:

value := {"a": # this is
{"b": # my ridiculous
{"c": # way of
"d"}}} # commenting code

just to give an idea.

Thanks for asking to fix this, but I am not familiar enough with Go to confidently touch this code. My observation came from simply running format commands with different inputs.

And I also see there is a fix with proper error handling and I appreciate it!

[anderseknert]

Yes, sorry if I wasn't clear on that — if the formatter can't prettify some lines, I'd expect it to just skip those and proceed to format the rest of the file.

The formatter should never bail out on valid Rego.

[stale[bot]]

This issue has been automatically marked as inactive because it has not had any activity in the last 30 days. Although currently inactive, the issue could still be considered and actively worked on in the future. More details about the use-case this issue attempts to address, the value provided by completing it or possible solutions to resolve it would help to prioritize the issue.