Skip to content

Commit

Permalink
ast: Making input and data reserved keywords in strict-mode
Browse files Browse the repository at this point in the history 
Fixes: #2600
Signed-off-by: Johan Fylling <johan.dev@fylling.se>
  • Loading branch information
@johanfylling
johanfylling committed Jan 28, 2022
1 parent 1f98d49 commit 7fe4599
Show file tree
Hide file tree
Showing 3 changed files with 259 additions and 33 deletions.
47 changes: 47 additions & 0 deletions ast/compile.go
View file
Expand Up @@ -256,6 +256,7 @@ func NewCompiler() *Compiler {
f func()
}{
{"CheckDuplicateImports", "compile_stage_check_duplicate_imports", c.checkDuplicateImports},
{"CheckKeywordOverrides", "compile_stage_check_keyword_overrides", c.checkKeywordOverrides},
// Reference resolution should run first as it may be used to lazily
// load additional modules. If any stages run before resolution, they
// need to be re-run after resolution.
Expand Down Expand Up @@ -1305,6 +1306,44 @@ func (c *Compiler) checkDuplicateImports() {
}
}

func (c *Compiler) checkKeywordOverrides() {
for _, name := range c.sorted {
mod := c.Modules[name]
errs := checkKeywordOverrides(mod, c.strict)
for _, err := range errs {
c.err(err)
}
}
}

func checkKeywordOverrides(node interface{}, strict bool) Errors {
if !strict {
return nil
}

errors := Errors{}

WalkRules(node, func(rule *Rule) bool {
name := rule.Head.Name.String()
if RootDocumentRefs.Contains(RefTerm(VarTerm(name))) {
errors = append(errors, NewError(CompileErr, rule.Location, "rules must not shadow %v (use a different rule name)", name))
}
return true
})

WalkExprs(node, func(expr *Expr) bool {
if expr.IsAssignment() {
name := expr.Operand(0).String()
if RootDocumentRefs.Contains(RefTerm(VarTerm(name))) {
errors = append(errors, NewError(CompileErr, expr.Location, "variables must not shadow %v (use a different variable name)", name))
}
}
return false
})

return errors
}

// resolveAllRefs resolves references in expressions to their fully qualified values.
//
// For instance, given the following module:
Expand Down Expand Up @@ -1958,6 +1997,7 @@ func (qc *queryCompiler) Compile(query Body) (Body, error) {
metricName string
f func(*QueryContext, Body) (Body, error)
}{
{"CheckKeywordOverrides", "query_compile_stage_check_keyword_overrides", qc.checkKeywordOverrides},
{"ResolveRefs", "query_compile_stage_resolve_refs", qc.resolveRefs},
{"RewriteLocalVars", "query_compile_stage_rewrite_local_vars", qc.rewriteLocalVars},
{"CheckVoidCalls", "query_compile_stage_check_void_calls", qc.checkVoidCalls},
Expand Down Expand Up @@ -2005,6 +2045,13 @@ func (qc *queryCompiler) applyErrorLimit(err error) error {
return err
}

func (qc *queryCompiler) checkKeywordOverrides(_ *QueryContext, body Body) (Body, error) {
if errs := checkKeywordOverrides(body, qc.compiler.strict); len(errs) > 0 {
return nil, errs
}
return body, nil
}

func (qc *queryCompiler) resolveRefs(qctx *QueryContext, body Body) (Body, error) {

var globals map[Var]Ref
Expand Down
242 changes: 210 additions & 32 deletions ast/compile_test.go
View file
Expand Up @@ -1464,12 +1464,7 @@ func TestCompilerRewriteExprTerms(t *testing.T) {
}

func TestCompilerCheckDuplicateImports(t *testing.T) {
cases := []struct {
note string
module string
expectedErrors Errors
strict bool
}{
cases := []strictnessTestCase{
{
note: "shadow",
module: `package test
Expand All @@ -1488,7 +1483,6 @@ func TestCompilerCheckDuplicateImports(t *testing.T) {
Message: "import must not shadow import input.foo",
},
},
strict: true,
}, {
note: "alias shadow",
module: `package test
Expand All @@ -1502,35 +1496,192 @@ func TestCompilerCheckDuplicateImports(t *testing.T) {
Message: "import must not shadow import input.foo",
},
},
strict: true,
}, {
note: "no strict",
},
}

runStrictnessTestCase(t, cases, true)
}

func TestCompilerCheckKeywordOverrides(t *testing.T) {
cases := []strictnessTestCase{
{
note: "rule names",
module: `package test
import input.noconflict
import input.foo
import data.foo
import data.bar.foo
import input.bar as foo
input { true }
p { true }
data { true }
`,
expectedErrors: Errors{
&Error{
Location: NewLocation([]byte("input { true }"), "", 2, 5),
Message: "rules must not shadow input (use a different rule name)",
},
&Error{
Location: NewLocation([]byte("data { true }"), "", 4, 5),
Message: "rules must not shadow data (use a different rule name)",
},
},
},
{
note: "global assignments",
module: `package test
input = 1
p := 2
data := 3
`,
strict: false,
expectedErrors: Errors{
&Error{
Location: NewLocation([]byte("input = 1"), "", 2, 5),
Message: "rules must not shadow input (use a different rule name)",
},
&Error{
Location: NewLocation([]byte("data := 3"), "", 4, 5),
Message: "rules must not shadow data (use a different rule name)",
},
},
},
{
note: "rule-local assignments",
module: `package test
p {
input := 1
x := 2
} else {
data := 3
}
q {
input := 4
}
`,
expectedErrors: Errors{
&Error{
Location: NewLocation([]byte("input := 1"), "", 3, 6),
Message: "variables must not shadow input (use a different variable name)",
},
&Error{
Location: NewLocation([]byte("data := 3"), "", 6, 6),
Message: "variables must not shadow data (use a different variable name)",
},
&Error{
Location: NewLocation([]byte("input := 4"), "", 9, 6),
Message: "variables must not shadow input (use a different variable name)",
},
},
},
{
note: "array comprehension-local assignments",
module: `package test
p = [ x |
input := 1
x := 2
data := 3
]
`,
expectedErrors: Errors{
&Error{
Location: NewLocation([]byte("input := 1"), "", 3, 6),
Message: "variables must not shadow input (use a different variable name)",
},
&Error{
Location: NewLocation([]byte("data := 3"), "", 5, 6),
Message: "variables must not shadow data (use a different variable name)",
},
},
},
{
note: "set comprehension-local assignments",
module: `package test
p = { x |
input := 1
x := 2
data := 3
}
`,
expectedErrors: Errors{
&Error{
Location: NewLocation([]byte("input := 1"), "", 3, 6),
Message: "variables must not shadow input (use a different variable name)",
},
&Error{
Location: NewLocation([]byte("data := 3"), "", 5, 6),
Message: "variables must not shadow data (use a different variable name)",
},
},
},
{
note: "object comprehension-local assignments",
module: `package test
p = { x: 1 |
input := 1
x := 2
data := 3
}
`,
expectedErrors: Errors{
&Error{
Location: NewLocation([]byte("input := 1"), "", 3, 6),
Message: "variables must not shadow input (use a different variable name)",
},
&Error{
Location: NewLocation([]byte("data := 3"), "", 5, 6),
Message: "variables must not shadow data (use a different variable name)",
},
},
},
{
note: "nested override",
module: `package test
p {
[ x |
input := 1
x := 2
data := 3
]
}
`,
expectedErrors: Errors{
&Error{
Location: NewLocation([]byte("input := 1"), "", 4, 7),
Message: "variables must not shadow input (use a different variable name)",
},
&Error{
Location: NewLocation([]byte("data := 3"), "", 6, 7),
Message: "variables must not shadow data (use a different variable name)",
},
},
},
}

for _, tc := range cases {
t.Run(tc.note, func(t *testing.T) {
compiler := NewCompiler().WithStrict(tc.strict)
runStrictnessTestCase(t, cases, true)
}

type strictnessTestCase struct {
note string
module string
expectedErrors Errors
}

func runStrictnessTestCase(t *testing.T, cases []strictnessTestCase, assertLocation bool) {
t.Helper()
makeTestRunner := func(tc strictnessTestCase, strict bool) func(t *testing.T) {
return func(t *testing.T) {
compiler := NewCompiler().WithStrict(strict)
compiler.Modules = map[string]*Module{
"test": MustParseModule(tc.module),
}
compileStages(compiler, nil)

compileStages(compiler, compiler.checkDuplicateImports)

if len(tc.expectedErrors) > 0 {
assertErrors(t, compiler.Errors, tc.expectedErrors, true)
if strict {
assertErrors(t, compiler.Errors, tc.expectedErrors, assertLocation)
} else {
assertNotFailed(t, compiler)
}
})
}
}

for _, tc := range cases {
t.Run(tc.note+"_strict", makeTestRunner(tc, true))
t.Run(tc.note+"_non-strict", makeTestRunner(tc, false))
}
}

Expand Down Expand Up @@ -5218,13 +5369,7 @@ func TestQueryCompilerWithUnsafeBuiltins(t *testing.T) {
}

func TestQueryCompilerWithUnusedAssignedVar(t *testing.T) {
type testCase struct {
note string
query string
expectedErrors error
}

cases := []testCase{
cases := []strictnessQueryTestCase{
{
note: "array comprehension",
query: "[1 | x := 2]",
Expand All @@ -5242,7 +5387,40 @@ func TestQueryCompilerWithUnusedAssignedVar(t *testing.T) {
},
}

makeTestRunner := func(tc testCase, strict bool) func(t *testing.T) {
runStrictnessQueryTestCase(t, cases)
}

func TestQueryCompilerCheckKeywordOverrides(t *testing.T) {
cases := []strictnessQueryTestCase{
{
note: "input assigned",
query: "input := 1",
expectedErrors: fmt.Errorf("1 error occurred: 1:1: rego_compile_error: variables must not shadow input (use a different variable name)"),
},
{
note: "data assigned",
query: "data := 1",
expectedErrors: fmt.Errorf("1 error occurred: 1:1: rego_compile_error: variables must not shadow data (use a different variable name)"),
},
{
note: "nested input assigned",
query: "d := [input | input := 1]",
expectedErrors: fmt.Errorf("1 error occurred: 1:15: rego_compile_error: variables must not shadow input (use a different variable name)"),
},
}

runStrictnessQueryTestCase(t, cases)
}

type strictnessQueryTestCase struct {
note string
query string
expectedErrors error
}

func runStrictnessQueryTestCase(t *testing.T, cases []strictnessQueryTestCase) {
t.Helper()
makeTestRunner := func(tc strictnessQueryTestCase, strict bool) func(t *testing.T) {
return func(t *testing.T) {
c := NewCompiler().WithStrict(strict)
result, err := c.QueryCompiler().Compile(MustParseBody(tc.query))
Expand Down
3 changes: 2 additions & 1 deletion docs/content/strict.md
View file
Expand Up @@ -19,4 +19,5 @@ Compiler Strict mode is supported by the `check` command, and can be enabled thr
Name | Description | Enforced by default in OPA version
--- | --- | ---
Duplicate imports | Duplicate [imports](../policy-language/#imports), where one import shadows another, are prohibited. | 1.0
Unused local assignments | Unused [assignments](../policy-reference/#assignment-and-equality) local to a rule, function or comprehension are prohibited | 1.0
Unused local assignments | Unused [assignments](../policy-reference/#assignment-and-equality) local to a rule, function or comprehension are prohibited | 1.0
`input` and `data` reserved keywords | `input` and `data` are reserved keywords, and may not be used as names for rules and variable assignment. | 1.0

0 comments on commit 7fe4599

Please sign in to comment.