Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

opa build panics on second end-of-line comment in function call #3836

Closed
anbrsap opened this issue Sep 30, 2021 · 2 comments · Fixed by #3864
Closed

opa build panics on second end-of-line comment in function call #3836

anbrsap opened this issue Sep 30, 2021 · 2 comments · Fixed by #3864
Labels

Comments

@anbrsap
Copy link

anbrsap commented Sep 30, 2021

Expected Behavior

The following rego code should compile, run and opa build.

testcase.rego:

package testcase

rule1 = contains(
    "", # first comment
    "", # second comment lets `opa build` panic
)

Actual Behavior

opa build panics:

$ opa build testcase.rego 
panic: overwriting non-nil beforeEnd

goroutine 1 [running]:
github.com/open-policy-agent/opa/format.(*writer).beforeLineEnd(...)
	/src/format/format.go:1044
github.com/open-policy-agent/opa/format.(*writer).insertComments(0xc000114c00, {0xc00011e500, 0x1426700, 0xc00021f1b8}, 0xc0001473c0)
	/src/format/format.go:378 +0x117
github.com/open-policy-agent/opa/format.(*writer).writeTermParens(0xc000114c00, 0x3a, 0xc0004192f0, {0xc00011e500, 0x412afd, 0xc00011e4b0})
	/src/format/format.go:517 +0x57
github.com/open-policy-agent/opa/format.(*writer).writeTerm(...)
	/src/format/format.go:513
github.com/open-policy-agent/opa/format.(*writer).writeFunctionCallPlain(0xc000114c00, {0xc000419278, 0x3, 0x8}, {0xc000412e50, 0x2, 0x2})
	/src/format/format.go:498 +0x1e6
github.com/open-policy-agent/opa/format.(*writer).writeCall(0xc000114c00, 0x0, {0xc000419278, 0x3, 0x3}, {0xc000412e50, 0x2, 0x2})
	/src/format/format.go:602 +0x26d
github.com/open-policy-agent/opa/format.(*writer).writeTermParens(0xc000114c00, 0x54, 0xc000419260, {0xc000412e40, 0x13eb120, 0xc000412e40})
	/src/format/format.go:548 +0x405
github.com/open-policy-agent/opa/format.(*writer).writeTerm(...)
	/src/format/format.go:513
github.com/open-policy-agent/opa/format.(*writer).writeHead(0xc000114c00, 0xc00017e460, 0x0, 0x1, {0xc000412e40, 0x2, 0x2})
	/src/format/format.go:366 +0x312
github.com/open-policy-agent/opa/format.(*writer).writeRule(0xc000114c00, 0xc000147580, 0x0, {0xc000412e40, 0x2, 0x2})
	/src/format/format.go:248 +0x20a
github.com/open-policy-agent/opa/format.(*writer).writeRules(0xc000412de0, {0xc00011e4e8, 0x1, 0x0}, {0xc000412e30, 0x0, 0xc00021f5f8})
	/src/format/format.go:223 +0x72
github.com/open-policy-agent/opa/format.(*writer).writeModule(0xc000114c00, 0xc0004021c0)
	/src/format/format.go:190 +0x2d3
github.com/open-policy-agent/opa/format.Ast({0xc75320, 0xc000402150})
	/src/format/format.go:74 +0x193
github.com/open-policy-agent/opa/format.Source({0x7fff4a746288, 0xd}, {0xc000174200, 0xffffffffffffffff, 0x4a2c91})
	/src/format/format.go:25 +0x7d
github.com/open-policy-agent/opa/bundle.(*Bundle).FormatModules(0xc00014a580, 0x0)
	/src/bundle/bundle.go:778 +0xeb
github.com/open-policy-agent/opa/compile.(*Compiler).Build(0xc00021fb98, {0xe3a790, 0xc0000340c0})
	/src/compile/compile.go:200 +0x11f
github.com/open-policy-agent/opa/cmd.dobuild({0xc000130168, 0xc000118480, 0x0, 0x0, {{0x0, 0x0, 0x0}, 0x0}, {0xd0f294, 0xd}, ...}, ...)
	/src/cmd/build.go:293 +0x70d
github.com/open-policy-agent/opa/cmd.init.1.func2(0xc00013c280, {0xc00011c590, 0x1, 0x1})
	/src/cmd/build.go:210 +0x6c
github.com/spf13/cobra.(*Command).execute(0xc00013c280, {0xc00011c560, 0x1, 0x1})
	/src/vendor/github.com/spf13/cobra/command.go:860 +0x5f8
github.com/spf13/cobra.(*Command).ExecuteC(0x13ddb80)
	/src/vendor/github.com/spf13/cobra/command.go:974 +0x3bc
github.com/spf13/cobra.(*Command).Execute(...)
	/src/vendor/github.com/spf13/cobra/command.go:902
main.main()
	/src/main.go:15 +0x25

However, the code compiles and runs:

$ opa eval -d testcase.rego 'data.testcase.rule1'
{
  "result": [
    {
      "expressions": [
        {
          "value": true,
          "text": "data.testcase.rule1",
          "location": {
            "row": 1,
            "col": 1
          }
        }
      ]
    }
  ]
}

Steps to Reproduce the Problem

Save testcase.rego to disk and run the commands shown above.

  • OPA version: 0.32.1

Additional Info

@srenatus srenatus added the bug label Sep 30, 2021
@srenatus
Copy link
Contributor

FWIW this one and #3832 are related to formatting (pretty-printing) policies. This happens as one of the steps in using opa build. The same panics can be triggered by using opa fmt, which would only do the formatting: opa fmt testcase.rego

@srenatus
Copy link
Contributor

srenatus commented Oct 4, 2021

So what's happening here is that it attempts to make it a one-line statement:

rule1 = contains("", "") # first comment

and doesn't know what to do with the second comment.

srenatus added a commit to srenatus/opa that referenced this issue Oct 6, 2021
This snippet,

    r = contains(
        input.x,
        "y",
    )

would have been formatted as

    r = contains(input.x, "y")

before. Now, any new lines added between function arguments will be kept, and
the snippet will not be reformatted.

As a consequence, comments on the separate arguments we OK:

    r = contains(
        input.x, # haystack
        "y",     # needle
    )

and don't freak out the formatter.

Fixes open-policy-agent#3836.

Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
srenatus added a commit that referenced this issue Oct 6, 2021
This snippet,

    r = contains(
        input.x,
        "y",
    )

would have been formatted as

    r = contains(input.x, "y")

before. Now, any new lines added between function arguments will be kept, and
the snippet will not be reformatted.

As a consequence, comments on the separate arguments we OK:

    r = contains(
        input.x, # haystack
        "y",     # needle
    )

and don't freak out the formatter.

Fixes #3836.

Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
dolevf pushed a commit to dolevf/opa that referenced this issue Nov 4, 2021
…nt#3864)

This snippet,

    r = contains(
        input.x,
        "y",
    )

would have been formatted as

    r = contains(input.x, "y")

before. Now, any new lines added between function arguments will be kept, and
the snippet will not be reformatted.

As a consequence, comments on the separate arguments we OK:

    r = contains(
        input.x, # haystack
        "y",     # needle
    )

and don't freak out the formatter.

Fixes open-policy-agent#3836.

Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
Signed-off-by: Dolev Farhi <farhi.dolev@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants