Skip to content

Commit

Permalink
ast: Deprecating any() and all() built-in functions (#4271)
Browse files Browse the repository at this point in the history 
Updating compiler strict mode to produce error when these deprecated methods are used.

This introduces a new stage to ast.Compiler and ast.QueryCompiler.

Fixes: #2437

Signed-off-by: Johan Fylling <johan.dev@fylling.se>
  • Loading branch information
@johanfylling
johanfylling committed Jan 31, 2022
1 parent 59810d0 commit 503a520
Show file tree
Hide file tree
Showing 4 changed files with 156 additions and 40 deletions.
76 changes: 42 additions & 34 deletions ast/builtins.go
View file
Expand Up @@ -663,36 +663,6 @@ var Min = &Builtin{
),
}

// All takes a list and returns true if all of the items
// are true. A collection of length 0 returns true.
var All = &Builtin{
Name: "all",
Decl: types.NewFunction(
types.Args(
types.NewAny(
types.NewSet(types.A),
types.NewArray(nil, types.A),
),
),
types.B,
),
}

// Any takes a collection and returns true if any of the items
// is true. A collection of length 0 returns false.
var Any = &Builtin{
Name: "any",
Decl: types.NewFunction(
types.Args(
types.NewAny(
types.NewSet(types.A),
types.NewArray(nil, types.A),
),
),
types.B,
),
}

/**
* Arrays
*/
Expand Down Expand Up @@ -2516,13 +2486,51 @@ var RegexMatchDeprecated = &Builtin{
),
}

// All takes a list and returns true if all of the items
// are true. A collection of length 0 returns true.
var All = &Builtin{
Name: "all",
Decl: types.NewFunction(
types.Args(
types.NewAny(
types.NewSet(types.A),
types.NewArray(nil, types.A),
),
),
types.B,
),
deprecated: true,
}

// Any takes a collection and returns true if any of the items
// is true. A collection of length 0 returns false.
var Any = &Builtin{
Name: "any",
Decl: types.NewFunction(
types.Args(
types.NewAny(
types.NewSet(types.A),
types.NewArray(nil, types.A),
),
),
types.B,
),
deprecated: true,
}

// Builtin represents a built-in function supported by OPA. Every built-in
// function is uniquely identified by a name.
type Builtin struct {
Name string `json:"name"` // Unique name of built-in function, e.g., <name>(arg1,arg2,...,argN)
Decl *types.Function `json:"decl"` // Built-in function type declaration.
Infix string `json:"infix,omitempty"` // Unique name of infix operator. Default should be unset.
Relation bool `json:"relation,omitempty"` // Indicates if the built-in acts as a relation.
Name string `json:"name"` // Unique name of built-in function, e.g., <name>(arg1,arg2,...,argN)
Decl *types.Function `json:"decl"` // Built-in function type declaration.
Infix string `json:"infix,omitempty"` // Unique name of infix operator. Default should be unset.
Relation bool `json:"relation,omitempty"` // Indicates if the built-in acts as a relation.
deprecated bool // Indicates if the built-in has been deprecated.
}

// IsDeprecated returns true if the Builtin function is deprecated and will be removed in a future release.
func (b *Builtin) IsDeprecated() bool {
return b.deprecated
}

// Expr creates a new expression for the built-in with the given operands.
Expand Down
53 changes: 48 additions & 5 deletions ast/compile.go
View file
Expand Up @@ -103,6 +103,7 @@ type Compiler struct {
builtins map[string]*Builtin // universe of built-in functions
customBuiltins map[string]*Builtin // user-supplied custom built-in functions (deprecated: use capabilities)
unsafeBuiltinsMap map[string]struct{} // user-supplied set of unsafe built-ins functions to block (deprecated: use capabilities)
deprecatedBuiltinsMap map[string]struct{} // set of deprecated, but not removed, built-in functions
enablePrintStatements bool // indicates if print statements should be elided (default)
comprehensionIndices map[*Term]*ComprehensionIndex // comprehension key index
initialized bool // indicates if init() has been called
Expand Down Expand Up @@ -240,11 +241,12 @@ func NewCompiler() *Compiler {
}, func(x util.T) int {
return x.(Ref).Hash()
}),
maxErrs: CompileErrorLimitDefault,
after: map[string][]CompilerStageDefinition{},
unsafeBuiltinsMap: map[string]struct{}{},
comprehensionIndices: map[*Term]*ComprehensionIndex{},
debug: debug.Discard(),
maxErrs: CompileErrorLimitDefault,
after: map[string][]CompilerStageDefinition{},
unsafeBuiltinsMap: map[string]struct{}{},
deprecatedBuiltinsMap: map[string]struct{}{},
comprehensionIndices: map[*Term]*ComprehensionIndex{},
debug: debug.Discard(),
}

c.ModuleTree = NewModuleTree(nil)
Expand Down Expand Up @@ -284,6 +286,7 @@ func NewCompiler() *Compiler {
{"CheckRecursion", "compile_stage_check_recursion", c.checkRecursion},
{"CheckTypes", "compile_stage_check_types", c.checkTypes},
{"CheckUnsafeBuiltins", "compile_state_check_unsafe_builtins", c.checkUnsafeBuiltins},
{"CheckDeprecatedBuiltins", "compile_state_check_deprecated_builtins", c.checkDeprecatedBuiltins},
{"BuildRuleIndices", "compile_stage_rebuild_indices", c.buildRuleIndices},
{"BuildComprehensionIndices", "compile_stage_rebuild_comprehension_indices", c.buildComprehensionIndices},
}
Expand Down Expand Up @@ -1173,6 +1176,15 @@ func (c *Compiler) checkUnsafeBuiltins() {
}
}

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

func (c *Compiler) runStage(metricName string, f func()) {
if c.metrics != nil {
c.metrics.Timer(metricName).Start()
Expand Down Expand Up @@ -1225,6 +1237,9 @@ func (c *Compiler) init() {

for _, bi := range c.capabilities.Builtins {
c.builtins[bi.Name] = bi
if c.strict && bi.IsDeprecated() {
c.deprecatedBuiltinsMap[bi.Name] = struct{}{}
}
}

for name, bi := range c.customBuiltins {
Expand Down Expand Up @@ -2015,6 +2030,7 @@ func (qc *queryCompiler) Compile(query Body) (Body, error) {
{"RewriteDynamicTerms", "query_compile_stage_rewrite_dynamic_terms", qc.rewriteDynamicTerms},
{"CheckTypes", "query_compile_stage_check_types", qc.checkTypes},
{"CheckUnsafeBuiltins", "query_compile_stage_check_unsafe_builtins", qc.checkUnsafeBuiltins},
{"CheckDeprecatedBuiltins", "query_compile_stage_check_deprecated_builtins", qc.checkDeprecatedBuiltins},
{"BuildComprehensionIndex", "query_compile_stage_build_comprehension_index", qc.buildComprehensionIndices},
}

Expand Down Expand Up @@ -2185,6 +2201,14 @@ func (qc *queryCompiler) checkUnsafeBuiltins(_ *QueryContext, body Body) (Body,
return body, nil
}

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

func (qc *queryCompiler) rewriteWithModifiers(_ *QueryContext, body Body) (Body, error) {
f := newEqualityFactory(newLocalVarGenerator("q", body))
body, err := rewriteWithModifiersInBody(qc.compiler, f, body)
Expand Down Expand Up @@ -4634,6 +4658,25 @@ func checkUnsafeBuiltins(unsafeBuiltinsMap map[string]struct{}, node interface{}
return errs
}

func checkDeprecatedBuiltins(deprecatedBuiltinsMap map[string]struct{}, node interface{}, strict bool) Errors {
// Early out; deprecatedBuiltinsMap is only populated in strict-mode.
if !strict {
return nil
}

errs := make(Errors, 0)
WalkExprs(node, func(x *Expr) bool {
if x.IsCall() {
operator := x.Operator().String()
if _, ok := deprecatedBuiltinsMap[operator]; ok {
errs = append(errs, NewError(TypeErr, x.Loc(), "deprecated built-in function calls in expression: %v", operator))
}
}
return false
})
return errs
}

func rewriteVarsInRef(vars ...map[Var]Var) varRewriter {
return func(node Ref) Ref {
i, _ := TransformVars(node, func(v Var) (Value, error) {
Expand Down
64 changes: 64 additions & 0 deletions ast/compile_test.go
View file
Expand Up @@ -1692,6 +1692,53 @@ func TestCompilerCheckKeywordOverrides(t *testing.T) {
runStrictnessTestCase(t, cases, true)
}

func TestCompilerCheckDeprecatedMethods(t *testing.T) {
cases := []strictnessTestCase{
{
note: "all() built-in",
module: `package test
p := all([true, false])
`,
expectedErrors: Errors{
&Error{
Location: NewLocation([]byte("all([true, false])"), "", 2, 10),
Message: "deprecated built-in function calls in expression: all",
},
},
},
{
note: "user-defined all()",
module: `package test
import future.keywords.in
all(arr) = {x | some x in arr} == {true}
p := all([true, false])
`,
},
{
note: "any() built-in",
module: `package test
p := any([true, false])
`,
expectedErrors: Errors{
&Error{
Location: NewLocation([]byte("any([true, false])"), "", 2, 10),
Message: "deprecated built-in function calls in expression: any",
},
},
},
{
note: "user-defined any()",
module: `package test
import future.keywords.in
any(arr) = true in arr
p := any([true, false])
`,
},
}

runStrictnessTestCase(t, cases, true)
}

type strictnessTestCase struct {
note string
module string
Expand Down Expand Up @@ -5647,6 +5694,23 @@ func TestQueryCompilerWithUnsafeBuiltins(t *testing.T) {
}
}

func TestQueryCompilerWithDeprecatedBuiltins(t *testing.T) {
cases := []strictnessQueryTestCase{
{
note: "all() built-in",
query: "all([true, false])",
expectedErrors: fmt.Errorf("1 error occurred: 1:1: rego_type_error: deprecated built-in function calls in expression: all"),
},
{
note: "any() built-in",
query: "any([true, false])",
expectedErrors: fmt.Errorf("1 error occurred: 1:1: rego_type_error: deprecated built-in function calls in expression: any"),
},
}

runStrictnessQueryTestCase(t, cases)
}

func TestQueryCompilerWithUnusedAssignedVar(t *testing.T) {
cases := []strictnessQueryTestCase{
{
Expand Down
3 changes: 2 additions & 1 deletion docs/content/strict.md
View file
Expand Up @@ -20,4 +20,5 @@ 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
`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
`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 503a520

Please sign in to comment.