From 7fe4599a802a25a22ecf3a4dd915af2ac8df90c0 Mon Sep 17 00:00:00 2001 From: Johan Fylling Date: Fri, 28 Jan 2022 19:43:44 +0100 Subject: [PATCH] ast: Making input and data reserved keywords in strict-mode Fixes: #2600 Signed-off-by: Johan Fylling --- ast/compile.go | 47 ++++++++ ast/compile_test.go | 242 +++++++++++++++++++++++++++++++++++------ docs/content/strict.md | 3 +- 3 files changed, 259 insertions(+), 33 deletions(-) diff --git a/ast/compile.go b/ast/compile.go index 9d89ea5628..4b52ce8c23 100644 --- a/ast/compile.go +++ b/ast/compile.go @@ -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. @@ -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: @@ -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}, @@ -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 diff --git a/ast/compile_test.go b/ast/compile_test.go index 1262e16e24..cbd1828fcb 100644 --- a/ast/compile_test.go +++ b/ast/compile_test.go @@ -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 @@ -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 @@ -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)) } } @@ -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]", @@ -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)) diff --git a/docs/content/strict.md b/docs/content/strict.md index 24194baa81..99b5bd3643 100644 --- a/docs/content/strict.md +++ b/docs/content/strict.md @@ -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 \ No newline at end of file +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 \ No newline at end of file