From f0154d8a15d588b627751f216d2085928ca3a826 Mon Sep 17 00:00:00 2001 From: Itay Shakury Date: Sat, 5 Mar 2022 19:27:43 +0200 Subject: [PATCH] parent cb01fba01562665fe197192ff204deed1ce2be57 author Itay Shakury 1646501263 +0200 committer Damien Burks 1646794189 -0600 docs: add missing title to annotations index (#4406) Signed-off-by: Itay Shakury Update philosophy.md (#4412) I am suggesting this change to fix grammatical errors which will increase readability for consumers of the OPA Philosophy document. --- 1. I noticed there is a missing word ("be") in the OPA Document Model section, just above the table that summarizes the different models for loading base documents into OPA. This change adds the omitted word. 2. I've also made a change to the first note at the bottom of the page ([1]). "However" is used as a conjunctive adverb, I've updated the sentence's punctuation to reflect this. 3. Lastly, I've made a punctuation change to note number three at the bottom of the page ([3]) to increase readability. Signed-off-by: Arthur Jones Co-authored-by: Stephan Renatus docs: mention who created OPA and who owns it (#4414) These are two common questions that come up and we do not mention this anywhere else in the docs. Signed-off-by: Torin Sandall committing latest changes Signed-off-by: Damien Burks committing latest changes to unsued imports function Signed-off-by: Damien Burks removing print statements and adding more test cases Signed-off-by: Damien Burks server: exposing disableInlining option via Compile API (#4378) This change enables consumers of Compile API to specify packages those should not be inlined in partial evaluation response. Fixes: #4357 Signed-off-by: skosunda docs: Update Envoy primer to use variables instead of objects Previously, the Envoy primer provided samples of policies for status-code, body, and headers that explicitly returned an object in individual rules instead of the more common and recommended approach of using separate variables for each concern. This change replaces those sample policies with ones that use variables for each of allow, status_code, headers, body, and it adds a section on the output expected by Envoy, where we include the snippet combining those variables into the JSON object expected by Envoy. Signed-off-by: Tim Hinrichs make: Disable WASM by default on darwin/arm64 (#4382) Signed-off-by: Anders Eknert docs: Update Istio tutorial to expose application to outside traffic (#4384) Signed-off-by: Ashutosh Narkar fixing linting issues Signed-off-by: Damien Burks committing latest changes for PR Signed-off-by: Damien Burks adding info to strict.md adding switch statement for 'in' operator adding additional case to if statement finalizing changes for broken test cases 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