diff --git a/ast/compile.go b/ast/compile.go index c4c89ca6f4..b577d9e47d 100644 --- a/ast/compile.go +++ b/ast/compile.go @@ -7,7 +7,6 @@ package ast import ( "fmt" "io" - "os" "sort" "strconv" "strings" @@ -146,7 +145,6 @@ type Compiler struct { keepModules bool // whether to keep the unprocessed, parse modules (below) parsedModules map[string]*Module // parsed, but otherwise unprocessed modules, kept track of when keepModules is true useTypeCheckAnnotations bool // whether to provide annotated information (schemas) to the type checker - generalRuleRefsEnabled bool } // CompilerStage defines the interface for stages in the compiler. @@ -335,8 +333,6 @@ func NewCompiler() *Compiler { {"BuildComprehensionIndices", "compile_stage_rebuild_comprehension_indices", c.buildComprehensionIndices}, } - _, c.generalRuleRefsEnabled = os.LookupEnv("EXPERIMENTAL_GENERAL_RULE_REFS") - return c } @@ -1005,50 +1001,8 @@ func (c *Compiler) checkRuleConflicts() { // data.p.q[r][s] { r := input.r; s := input.s } // data.p[q].r.s { q := input.q } - if c.generalRuleRefsEnabled { - if r.Ref().IsGround() && len(node.Children) > 0 { - conflicts = node.flattenChildren() - } - } else { // TODO: Remove when general rule refs are enabled by default. - if r.Head.RuleKind() == SingleValue && len(node.Children) > 0 { - if len(ref) > 1 && !ref[len(ref)-1].IsGround() { // p.q[x] and p.q.s.t => check grandchildren - for _, c := range node.Children { - grandchildrenFound := false - - if len(c.Values) > 0 { - childRules := extractRules(c.Values) - for _, childRule := range childRules { - childRef := childRule.Ref() - if childRule.Head.RuleKind() == SingleValue && !childRef[len(childRef)-1].IsGround() { - // The child is a partial object rule, so it's effectively "generating" grandchildren. - grandchildrenFound = true - break - } - } - } - - if len(c.Children) > 0 { - grandchildrenFound = true - } - - if grandchildrenFound { - conflicts = node.flattenChildren() - break - } - } - } else { // p.q.s and p.q.s.t => any children are in conflict - conflicts = node.flattenChildren() - } - } - - // Multi-value rules may not have any other rules in their extent; e.g.: - // - // data.p[v] { v := ... } - // data.p.q := 42 # In direct conflict with data.p[v], which is constructing a set and cannot have values assigned to a sub-path. - - if r.Head.RuleKind() == MultiValue && len(node.Children) > 0 { - conflicts = node.flattenChildren() - } + if r.Ref().IsGround() && len(node.Children) > 0 { + conflicts = node.flattenChildren() } if r.Head.RuleKind() == SingleValue && r.Head.Ref().IsGround() { @@ -1811,18 +1765,6 @@ func (c *Compiler) rewriteRuleHeadRefs() { } for i := 1; i < len(ref); i++ { - // NOTE: Unless enabled via the EXPERIMENTAL_GENERAL_RULE_REFS env var, non-string values in the refs are forbidden - // except for the last position, e.g. - // OK: p.q.r[s] - // NOT OK: p[q].r.s - // TODO: Remove when general rule refs are enabled by default. - if !c.generalRuleRefsEnabled && i != len(ref)-1 { // last - if _, ok := ref[i].Value.(String); !ok { - c.err(NewError(TypeErr, rule.Loc(), "rule head must only contain string terms (except for last): %v", ref[i])) - continue - } - } - // Rewrite so that any non-scalar elements that in the last position of // the rule are vars: // p.q.r[y.z] { ... } => p.q.r[__local0__] { __local0__ = y.z } diff --git a/ast/compile_test.go b/ast/compile_test.go index a771f5439e..2fb3532494 100644 --- a/ast/compile_test.go +++ b/ast/compile_test.go @@ -467,8 +467,6 @@ func toRef(s string) Ref { } func TestCompilerCheckRuleHeadRefs(t *testing.T) { - t.Setenv("EXPERIMENTAL_GENERAL_RULE_REFS", "true") - tests := []struct { note string modules []*Module @@ -607,64 +605,6 @@ func TestCompilerCheckRuleHeadRefs(t *testing.T) { } } -// TODO: Remove when general rule refs are enabled by default. -func TestCompilerCheckRuleHeadRefsWithGeneralRuleRefsDisabled(t *testing.T) { - - tests := []struct { - note string - modules []*Module - expected *Rule - err string - }{ - { - note: "ref contains var", - modules: modules( - `package x - p.q[i].r = 1 { i := 10 }`, - ), - err: "rego_type_error: rule head must only contain string terms (except for last): i", - }, - { - note: "invalid: ref in ref", - modules: modules( - `package x - p.q[arr[0]].r { i := 10 }`, - ), - err: "rego_type_error: rule head must only contain string terms (except for last): arr[0]", - }, - { - note: "invalid: non-string in ref (not last position)", - modules: modules( - `package x - p.q[10].r { true }`, - ), - err: "rego_type_error: rule head must only contain string terms (except for last): 10", - }, - } - - for _, tc := range tests { - t.Run(tc.note, func(t *testing.T) { - mods := make(map[string]*Module, len(tc.modules)) - for i, m := range tc.modules { - mods[fmt.Sprint(i)] = m - } - c := NewCompiler() - c.Modules = mods - compileStages(c, c.rewriteRuleHeadRefs) - if tc.err != "" { - assertCompilerErrorStrings(t, c, []string{tc.err}) - } else { - if len(c.Errors) > 0 { - t.Fatalf("expected no errors, got %v", c.Errors) - } - if tc.expected != nil { - assertRulesEqual(t, tc.expected, mods["0"].Rules[0]) - } - } - }) - } -} - func TestRuleTreeWithDotsInHeads(t *testing.T) { // TODO(sr): multi-val with var key in ref @@ -1940,8 +1880,6 @@ func TestCompilerCheckRuleConflictsDefaultFunction(t *testing.T) { } func TestCompilerCheckRuleConflictsDotsInRuleHeads(t *testing.T) { - t.Setenv("EXPERIMENTAL_GENERAL_RULE_REFS", "true") - tests := []struct { note string modules []*Module @@ -2187,70 +2125,6 @@ func TestCompilerCheckRuleConflictsDotsInRuleHeads(t *testing.T) { } } -// TODO: Remove when general rule refs are enabled by default. -func TestGeneralRuleRefsDisabled(t *testing.T) { - // EXPERIMENTAL_GENERAL_RULE_REFS env var not set - - tests := []struct { - note string - modules []*Module - err string - }{ - { - note: "single-value with other rule overlap, unknown key", - modules: modules( - `package pkg - p.q[r] = x { r = input.key; x = input.foo } - p.q.r.s = x { true } - `), - err: "rego_type_error: rule data.pkg.p.q[r] conflicts with [data.pkg.p.q.r.s]", - }, - { - note: "single-value with other rule overlap, unknown ref var and key", - modules: modules( - `package pkg - p.q[r][s] = x { r = input.key1; s = input.key2; x = input.foo } - p.q.r.s.t = x { true } - `), - err: "rego_type_error: rule head must only contain string terms (except for last): r", - }, - { - note: "single-value partial object with other partial object rule overlap, unknown keys (regression test for #5855; invalidated by multi-var refs)", - modules: modules( - `package pkg - p[r] := x { r = input.key; x = input.bar } - p.q[r] := x { r = input.key; x = input.bar } - `), - err: "rego_type_error: rule data.pkg.p[r] conflicts with [data.pkg.p.q[r]]", - }, - { - note: "single-value partial object with other partial object (implicit 'true' value) rule overlap, unknown keys", - modules: modules( - `package pkg - p[r] := x { r = input.key; x = input.bar } - p.q[r] { r = input.key } - `), - err: "rego_type_error: rule data.pkg.p[r] conflicts with [data.pkg.p.q[r]]", - }, - } - for _, tc := range tests { - t.Run(tc.note, func(t *testing.T) { - mods := make(map[string]*Module, len(tc.modules)) - for i, m := range tc.modules { - mods[fmt.Sprint(i)] = m - } - c := NewCompiler() - c.Modules = mods - compileStages(c, c.checkRuleConflicts) - if tc.err != "" { - assertCompilerErrorStrings(t, c, []string{tc.err}) - } else { - assertCompilerErrorStrings(t, c, []string{}) - } - }) - } -} - func TestCompilerCheckUndefinedFuncs(t *testing.T) { module := ` @@ -6861,8 +6735,6 @@ func TestCompilerMockVirtualDocumentPartially(t *testing.T) { } func TestCompilerCheckUnusedAssignedVar(t *testing.T) { - t.Setenv("EXPERIMENTAL_GENERAL_RULE_REFS", "true") - type testCase struct { note string module string diff --git a/ast/parser.go b/ast/parser.go index d32ae4ec5c..7cbc0b2892 100644 --- a/ast/parser.go +++ b/ast/parser.go @@ -11,7 +11,6 @@ import ( "io" "math/big" "net/url" - "os" "regexp" "sort" "strconv" @@ -98,14 +97,13 @@ func (e *parsedTermCacheItem) String() string { // ParserOptions defines the options for parsing Rego statements. type ParserOptions struct { - Capabilities *Capabilities - ProcessAnnotation bool - AllFutureKeywords bool - FutureKeywords []string - SkipRules bool - JSONOptions *astJSON.Options - unreleasedKeywords bool // TODO(sr): cleanup - generalRuleRefsEnabled bool + Capabilities *Capabilities + ProcessAnnotation bool + AllFutureKeywords bool + FutureKeywords []string + SkipRules bool + JSONOptions *astJSON.Options + unreleasedKeywords bool // TODO(sr): cleanup } // NewParser creates and initializes a Parser. @@ -114,7 +112,6 @@ func NewParser() *Parser { s: &state{}, po: ParserOptions{}, } - _, p.po.generalRuleRefsEnabled = os.LookupEnv("EXPERIMENTAL_GENERAL_RULE_REFS") return p } @@ -599,11 +596,6 @@ func (p *Parser) parseRules() []*Rule { return []*Rule{&rule} } - if !p.po.generalRuleRefsEnabled && usesContains && !rule.Head.Reference.IsGround() { - p.error(p.s.Loc(), "multi-value rules need ground refs") - return nil - } - // back-compat with `p[x] { ... }`` hasIf := p.s.tok == tokens.If diff --git a/format/format_test.go b/format/format_test.go index 0c545fb557..8d363210ff 100644 --- a/format/format_test.go +++ b/format/format_test.go @@ -78,8 +78,6 @@ func TestFormatSourceError(t *testing.T) { } func TestFormatSource(t *testing.T) { - t.Setenv("EXPERIMENTAL_GENERAL_RULE_REFS", "true") - regoFiles, err := filepath.Glob("testfiles/*.rego") if err != nil { panic(err) diff --git a/internal/planner/planner_test.go b/internal/planner/planner_test.go index eb6252ee3d..a6ff37bba3 100644 --- a/internal/planner/planner_test.go +++ b/internal/planner/planner_test.go @@ -16,8 +16,6 @@ import ( ) func TestPlannerHelloWorld(t *testing.T) { - t.Setenv("EXPERIMENTAL_GENERAL_RULE_REFS", "true") - // NOTE(tsandall): These tests are not meant to give comprehensive coverage // of the planner. Currently we have a suite of end-to-end tests in the // test/wasm/ directory that are specified in YAML, compiled into Wasm, and diff --git a/test/cases/testdata/refheads/test-generic-refs.yaml b/test/cases/testdata/refheads/test-generic-refs.yaml index bdbdf9392d..7fe16bbe0e 100644 --- a/test/cases/testdata/refheads/test-generic-refs.yaml +++ b/test/cases/testdata/refheads/test-generic-refs.yaml @@ -1,7 +1,5 @@ cases: - note: 'refheads/general, single var' - env: - EXPERIMENTAL_GENERAL_RULE_REFS: "true" modules: - | package test @@ -17,8 +15,6 @@ cases: c: r: 2 - note: 'refheads/general, multiple vars' - env: - EXPERIMENTAL_GENERAL_RULE_REFS: "true" modules: - | package test @@ -34,8 +30,6 @@ cases: c: 2: true - note: 'refheads/general, deep query' - env: - EXPERIMENTAL_GENERAL_RULE_REFS: "true" modules: - | package test @@ -46,8 +40,6 @@ cases: - x: 1: true - note: 'refheads/general, overlapping rule, no conflict' - env: - EXPERIMENTAL_GENERAL_RULE_REFS: "true" modules: - | package test @@ -64,8 +56,6 @@ cases: c: r: 2 - note: 'refheads/general, overlapping rule, different dynamic depths, no conflict' - env: - EXPERIMENTAL_GENERAL_RULE_REFS: "true" modules: - | package test @@ -89,8 +79,6 @@ cases: r: 3 e: 4 - note: 'refheads/general, self-conflict' - env: - EXPERIMENTAL_GENERAL_RULE_REFS: "true" modules: - | package test @@ -99,8 +87,6 @@ cases: query: data.test.p = x want_error_code: eval_conflict_error - note: 'refheads/general, overlapping rule, conflict' - env: - EXPERIMENTAL_GENERAL_RULE_REFS: "true" modules: - | package test @@ -110,8 +96,6 @@ cases: query: data.test.p = x want_error_code: eval_conflict_error - note: 'refheads/general, overlapping rule, deep override inside other rule object value, conflict' - env: - EXPERIMENTAL_GENERAL_RULE_REFS: "true" modules: - | package test @@ -129,8 +113,6 @@ cases: query: data.test.p = x want_error_code: eval_conflict_error - note: 'refheads/general, overlapping rule, deep injection into other rule object value, conflict' - env: - EXPERIMENTAL_GENERAL_RULE_REFS: "true" modules: - | package test @@ -148,8 +130,6 @@ cases: query: data.test.p = x want_error_code: eval_conflict_error - note: 'refheads/general, set leaf (shallow ref)' - env: - EXPERIMENTAL_GENERAL_RULE_REFS: "true" modules: - | package test @@ -170,8 +150,6 @@ cases: b: [ "a", "c", "foo" ] c: [ "a", "b" ] - note: 'refheads/general, set leaf (other rule defines dynamic ref portion)' - env: - EXPERIMENTAL_GENERAL_RULE_REFS: "true" modules: - | package test @@ -188,8 +166,6 @@ cases: q: [ "a", "b", "c" ] foo: bar - note: 'refheads/general, set leaf' - env: - EXPERIMENTAL_GENERAL_RULE_REFS: "true" modules: - | package test @@ -213,8 +189,6 @@ cases: c: r: [ "a", "b" ] - note: 'refheads/general, set leaf, deep query' - env: - EXPERIMENTAL_GENERAL_RULE_REFS: "true" modules: - | package test @@ -232,8 +206,6 @@ cases: want_result: - x: "c" - note: 'refheads/general, input var' - env: - EXPERIMENTAL_GENERAL_RULE_REFS: "true" modules: - | package test @@ -247,8 +219,6 @@ cases: bar: r: "foo" - note: 'refheads/general, external non-ground var' - env: - EXPERIMENTAL_GENERAL_RULE_REFS: "true" modules: - | package test @@ -271,8 +241,6 @@ cases: bar: 1 baz: 2 - note: 'refheads/general, multiple result-set entries' - env: - EXPERIMENTAL_GENERAL_RULE_REFS: "true" modules: - | package test diff --git a/test/cases/testdata/refheads/test-refs-as-rule-heads.yaml b/test/cases/testdata/refheads/test-refs-as-rule-heads.yaml index 8daabc43a0..ebe69d0027 100644 --- a/test/cases/testdata/refheads/test-refs-as-rule-heads.yaml +++ b/test/cases/testdata/refheads/test-refs-as-rule-heads.yaml @@ -26,8 +26,6 @@ cases: r: 1 s: 2 - note: 'refheads/single-value, with var, conflict' - env: - EXPERIMENTAL_GENERAL_RULE_REFS: "true" modules: - | package test diff --git a/topdown/eval_test.go b/topdown/eval_test.go index bd24f987fe..9b4e565ff9 100644 --- a/topdown/eval_test.go +++ b/topdown/eval_test.go @@ -247,9 +247,6 @@ func TestContainsNestedRefOrCall(t *testing.T) { } func TestTopdownVirtualCache(t *testing.T) { - // TODO: break out into separate tests - t.Setenv("EXPERIMENTAL_GENERAL_RULE_REFS", "true") - ctx := context.Background() store := inmem.New() @@ -604,8 +601,6 @@ func TestTopdownVirtualCache(t *testing.T) { } func TestPartialRule(t *testing.T) { - t.Setenv("EXPERIMENTAL_GENERAL_RULE_REFS", "true") - ctx := context.Background() store := inmem.New() @@ -1451,27 +1446,3 @@ func TestPartialRule(t *testing.T) { }) } } - -// TODO: Remove when general rule refs are enabled by default. -func TestGeneralRuleRefsFeatureFlag(t *testing.T) { - module := ast.MustParseModule(`package test - p[q].r { q := "q" }`) - mods := map[string]*ast.Module{ - "": module, - } - c := ast.NewCompiler() - c.Compile(mods) - - if !strings.Contains(c.Errors.Error(), "rego_type_error: rule head must only contain string terms (except for last)") { - t.Fatal("Expected error but got:", c.Errors) - } - - t.Setenv("EXPERIMENTAL_GENERAL_RULE_REFS", "true") - - c = ast.NewCompiler() - c.Compile(mods) - - if c.Errors != nil { - t.Fatal("Unexpected error:", c.Errors) - } -} diff --git a/topdown/topdown_partial_test.go b/topdown/topdown_partial_test.go index 126c7273a4..617800f58b 100644 --- a/topdown/topdown_partial_test.go +++ b/topdown/topdown_partial_test.go @@ -18,9 +18,6 @@ import ( ) func TestTopDownPartialEval(t *testing.T) { - // TODO: break out into separate tests - t.Setenv("EXPERIMENTAL_GENERAL_RULE_REFS", "true") - tests := []struct { note string unknowns []string