Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ast: Deprecating any() and all() built-in functions #4271

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
76 changes: 42 additions & 34 deletions ast/builtins.go
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,
),
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just moving these down to the "deprecated" section.

/**
* 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
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
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])
johanfylling marked this conversation as resolved.
Show resolved Hide resolved
`,
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
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