Skip to content

Commit

Permalink
ast/compile: error on unused imports (strict mode)(#4377)
Browse files Browse the repository at this point in the history
Fixes #4354.

Signed-off-by: Damien Burks <damien@damienjburks.com>
  • Loading branch information
damienjburks committed Mar 11, 2022
1 parent c8a6e6f commit 785f742
Show file tree
Hide file tree
Showing 4 changed files with 201 additions and 0 deletions.
58 changes: 58 additions & 0 deletions ast/compile.go
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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 {
Expand Down
141 changes: 141 additions & 0 deletions ast/compile_test.go
Expand Up @@ -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{
{
Expand Down
1 change: 1 addition & 0 deletions ast/fuzz.go
@@ -1,3 +1,4 @@
//go:build gofuzz
// +build gofuzz

package ast
Expand Down
1 change: 1 addition & 0 deletions docs/content/strict.md
Expand Up @@ -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

0 comments on commit 785f742

Please sign in to comment.