From 785f7427d16e6eb3626f4994eb3ae96f27175676 Mon Sep 17 00:00:00 2001 From: Damien Burks Date: Fri, 11 Mar 2022 02:56:05 -0600 Subject: [PATCH] ast/compile: error on unused imports (strict mode)(#4377) Fixes #4354. Signed-off-by: Damien Burks --- ast/compile.go | 58 +++++++++++++++++ ast/compile_test.go | 141 +++++++++++++++++++++++++++++++++++++++++ ast/fuzz.go | 1 + docs/content/strict.md | 1 + 4 files changed, 201 insertions(+) diff --git a/ast/compile.go b/ast/compile.go index 1c4722f915..d7238d4b8f 100644 --- a/ast/compile.go +++ b/ast/compile.go @@ -259,6 +259,7 @@ func NewCompiler() *Compiler { f func() }{ {"CheckDuplicateImports", "compile_stage_check_duplicate_imports", c.checkDuplicateImports}, + {"CheckUnusedImports", "compile_stage_check_unused_imports", c.checkUnusedImports}, {"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 @@ -1321,6 +1322,62 @@ func (c *Compiler) GetAnnotationSet() *AnnotationSet { return c.annotationSet } +func (c *Compiler) checkUnusedImports() { + if !c.strict { + return + } + + for _, name := range c.sorted { + mod := c.Modules[name] + + for _, imp := range mod.Imports { + var impCounter int + importName := imp.Name().String() + + WalkExprs(mod, func(expr *Expr) bool { + switch t := expr.Terms.(type) { + case *Every: + _ = t // term is unused + if strings.Contains(expr.String(), importName) { + impCounter++ + } + default: + WalkTerms(mod, func(term *Term) bool { + termStr := term.Value.String() + if importName == "in" { + switch { + case termStr == "internal.member_2": + if importName == "in" { + impCounter++ + } + case termStr == "internal.member_3": + if importName == "in" { + impCounter++ + } + default: + if strings.Contains(expr.String(), termStr) { + impCounter++ + } + } + } else { + if importName == termStr { + impCounter++ + } + } + return false + }) + } + + return true + }) + + if impCounter == 0 { + c.err(NewError(CompileErr, imp.Location, "%v unused", imp.String())) + } + } + } +} + func (c *Compiler) checkDuplicateImports() { if !c.strict { return @@ -1332,6 +1389,7 @@ func (c *Compiler) checkDuplicateImports() { for _, imp := range mod.Imports { name := imp.Name() + if processed, conflict := processedImports[name]; conflict { c.err(NewError(CompileErr, imp.Location, "import must not shadow %v", processed)) } else { diff --git a/ast/compile_test.go b/ast/compile_test.go index d6c785dc4b..d65be7718d 100644 --- a/ast/compile_test.go +++ b/ast/compile_test.go @@ -1500,6 +1500,147 @@ func TestCompilerRewriteExprTerms(t *testing.T) { } } +func TestCompilerCheckUnusedImports(t *testing.T) { + cases := []strictnessTestCase{ + { + note: "fail_case_1", + module: `package p + import data.foo.bar as bar + r { + input.bar == 11 + } + `, + expectedErrors: Errors{ + &Error{ + Location: NewLocation([]byte("import"), "", 2, 4), + Message: "import data.foo.bar as bar unused", + }, + }, + }, + { + note: "fail_case_2", + module: `package p + import data.foo # unused + r { data.foo == 10 } + `, + expectedErrors: Errors{ + &Error{ + Location: NewLocation([]byte("import"), "", 2, 4), + Message: "import data.foo unused", + }, + }, + }, + { + note: "fail_case_3", + module: `package p + import data.foo + import data.x.power #unused + r { foo == 10 } + `, + expectedErrors: Errors{ + &Error{ + Location: NewLocation([]byte("import"), "", 3, 4), + Message: "import data.x.power unused", + }, + }, + }, + { + note: "fail_case_4", + module: `package p + import data.foo + import data.x.power #unused + r { input.unused == 10 } + `, + expectedErrors: Errors{ + &Error{ + Location: NewLocation([]byte("import"), "", 2, 4), + Message: "import data.foo unused", + }, + &Error{ + Location: NewLocation([]byte("import"), "", 3, 4), + Message: "import data.x.power unused", + }, + }, + }, + { + note: "pass_case_1", + module: `package p + import data.foo.x + r { x == 10 } + `, + }, + { + note: "pass_case_2", + module: `package p + import data.foo.x + r { input.data == x } + `, + }, + { + note: "pass_case_3", + module: `package p + import data.foo.x + import data.power.ranger + r { ranger == x } + `, + }, + { + note: "pass_case_4", + module: `package p + import data.foo.x + import data.power.ranger + r { ranger == 23 } + t { x == 1 } + `, + }, + { + note: "pass_case_5", + module: `package p + import data.foo + r = count(foo) > 1 # only one operand + `, + }, + { + note: "pass_case_6", + module: `package p + import data.foo + r = sprintf("%v %d", [foo, 0]) # more complicated operands + `, + }, + { + note: "pass_case_7", + module: `package p + import data.foo + r { + foo # not a call, plain term + } + `, + }, + { + note: "pass_case_8", + module: `package p + import future.keywords.every + import data.foo + r { + every x in foo { x > 1 } # import in 'every' domain + } + `, + }, + { + note: "pass_case_9", + module: `package p + import future.keywords.every + import data.foo + r { + every x in [1,2,3] { x > foo } # import in 'every' body + } + `, + }, + } + + runStrictnessTestCase(t, cases, true) +} + func TestCompilerCheckDuplicateImports(t *testing.T) { cases := []strictnessTestCase{ { diff --git a/ast/fuzz.go b/ast/fuzz.go index 6ff7e35a8e..10651d3179 100644 --- a/ast/fuzz.go +++ b/ast/fuzz.go @@ -1,3 +1,4 @@ +//go:build gofuzz // +build gofuzz package ast diff --git a/docs/content/strict.md b/docs/content/strict.md index 013a0cbcbe..1e6cf1ae81 100644 --- a/docs/content/strict.md +++ b/docs/content/strict.md @@ -20,5 +20,6 @@ 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 imports | Unused [imports](../policy-language/#imports) 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 `any()` and `all()` removed | The `any()` and `all()` built-in functions have been deprecated, and will be removed in OPA 1.0. Use of these functions is prohibited. | 1.0 \ No newline at end of file