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

v1 build-tag and import for side effects #7179

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from
10 changes: 9 additions & 1 deletion Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ install: generate
$(GO) install $(GO_TAGS) -ldflags $(LDFLAGS)

.PHONY: test
test: go-test wasm-test
test: go-test go-test-v0 go-test-v0-import wasm-test
Copy link
Contributor Author

Choose a reason for hiding this comment

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

go-tests are basically run twice, once for v0 and once for v1. This has impact on PR GHA execution time. Instead, we could narrow the scope of tests run for v0. The benefit of keeping it like this is we get great test-coverage, and will know if we break the v0 opt-in.


.PHONY: go-build
go-build: generate
Expand All @@ -122,6 +122,14 @@ go-build: generate
go-test: generate
$(GO) test $(GO_TAGS),slow ./...

.PHONY: go-test-v0
go-test-v0: generate
$(GO) test $(GO_TAGS),opa_rego_v0,slow ./...

.PHONY: go-test-v0-import
go-test-v0-import: generate
$(GO) test $(GO_TAGS),test_rego_default_v0_import ./test/rego_default/...

.PHONY: race-detector
race-detector: generate
$(GO) test $(GO_TAGS),slow -race -vet=off ./...
Expand Down
18 changes: 16 additions & 2 deletions ast/compile.go
Original file line number Diff line number Diff line change
Expand Up @@ -1717,7 +1717,7 @@ func (c *Compiler) checkDuplicateImports() {

for _, name := range c.sorted {
mod := c.Modules[name]
if c.strict || mod.regoV1Compatible() {
if c.strict || moduleIsRegoV1(mod) {
modules = append(modules, mod)
}
}
Expand All @@ -1731,7 +1731,7 @@ func (c *Compiler) checkDuplicateImports() {
func (c *Compiler) checkKeywordOverrides() {
for _, name := range c.sorted {
mod := c.Modules[name]
if c.strict || mod.regoV1Compatible() {
if c.strict || moduleIsRegoV1(mod) {
errs := checkRootDocumentOverrides(mod)
for _, err := range errs {
c.err(err)
Expand All @@ -1740,6 +1740,20 @@ func (c *Compiler) checkKeywordOverrides() {
}
}

func moduleIsRegoV1(mod *Module) bool {
switch mod.regoVersion {
case RegoUndefined:
switch DefaultRegoVersion() {
case RegoV1, RegoV0CompatV1:
return true
default:
return false
}
}

return mod.regoV1Compatible()
Comment on lines +1744 to +1754
Copy link
Contributor

Choose a reason for hiding this comment

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

[nit] the single-case switch could be an if:

Suggested change
switch mod.regoVersion {
case RegoUndefined:
switch DefaultRegoVersion() {
case RegoV1, RegoV0CompatV1:
return true
default:
return false
}
}
return mod.regoV1Compatible()
if mod.regoVersion == RegoUndefined {
switch DefaultRegoVersion() {
case RegoV1, RegoV0CompatV1:
return true
}
return false
}
return mod.regoV1Compatible()

...but this is code golf and doesn't matter 🙃

}

// resolveAllRefs resolves references in expressions to their fully qualified values.
//
// For instance, given the following module:
Expand Down
24 changes: 15 additions & 9 deletions ast/parser.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import (
"strings"
"unicode/utf8"

"github.com/open-policy-agent/opa/internal/rego/version"
Copy link
Contributor

Choose a reason for hiding this comment

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

[nit] let's cluster this one with the other opa imports below

"gopkg.in/yaml.v3"

"github.com/open-policy-agent/opa/ast/internal/scanner"
Expand All @@ -30,11 +31,10 @@ var RegoV1CompatibleRef = Ref{VarTerm("rego"), StringTerm("v1")}
// RegoVersion defines the Rego syntax requirements for a module.
type RegoVersion int

const DefaultRegoVersion = RegoVersion(0)

const (
RegoUndefined RegoVersion = iota
// RegoV0 is the default, original Rego syntax.
RegoV0 RegoVersion = iota
RegoV0
// RegoV0CompatV1 requires modules to comply with both the RegoV0 and RegoV1 syntax (as when 'rego.v1' is imported in a module).
// Shortly, RegoV1 compatibility is required, but 'rego.v1' or 'future.keywords' must also be imported.
RegoV0CompatV1
Expand All @@ -45,6 +45,10 @@ const (
RegoV1
)

func DefaultRegoVersion() RegoVersion {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Breaking change.
Very unlikely anyone is using this, though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Since DefaultRegoVersion was added so recently I think we can make this change confidently.

return RegoVersion(version.DefaultRegoVersion)
}

func (v RegoVersion) Int() int {
if v == RegoV1 {
return 1
Expand Down Expand Up @@ -156,8 +160,10 @@ type ParserOptions struct {
}

// EffectiveRegoVersion returns the effective RegoVersion to use for parsing.
// Deprecated: Use RegoVersion instead.
func (po *ParserOptions) EffectiveRegoVersion() RegoVersion {
func (po ParserOptions) EffectiveRegoVersion() RegoVersion {
if po.RegoVersion == RegoUndefined {
charlieegan3 marked this conversation as resolved.
Show resolved Hide resolved
return DefaultRegoVersion()
}
return po.RegoVersion
}

Expand Down Expand Up @@ -314,7 +320,7 @@ func (p *Parser) Parse() ([]Statement, []*Comment, Errors) {

allowedFutureKeywords := map[string]tokens.Token{}

if p.po.RegoVersion == RegoV1 {
if p.po.EffectiveRegoVersion() == RegoV1 {
// RegoV1 includes all future keywords in the default language definition
for k, v := range futureKeywords {
allowedFutureKeywords[k] = v
Expand Down Expand Up @@ -373,7 +379,7 @@ func (p *Parser) Parse() ([]Statement, []*Comment, Errors) {
}

selected := map[string]tokens.Token{}
if p.po.AllFutureKeywords || p.po.RegoVersion == RegoV1 {
if p.po.AllFutureKeywords || p.po.EffectiveRegoVersion() == RegoV1 {
for kw, tok := range allowedFutureKeywords {
selected[kw] = tok
}
Expand All @@ -394,7 +400,7 @@ func (p *Parser) Parse() ([]Statement, []*Comment, Errors) {
}
p.s.s = p.s.s.WithKeywords(selected)

if p.po.RegoVersion == RegoV1 {
if p.po.EffectiveRegoVersion() == RegoV1 {
for kw, tok := range allowedFutureKeywords {
p.s.s.AddKeyword(kw, tok)
}
Expand Down Expand Up @@ -2710,7 +2716,7 @@ func (p *Parser) regoV1Import(imp *Import) {
return
}

if p.po.RegoVersion == RegoV1 {
if p.po.EffectiveRegoVersion() == RegoV1 {
// We're parsing for Rego v1, where the 'rego.v1' import is a no-op.
return
}
Expand Down
7 changes: 6 additions & 1 deletion ast/parser_ext.go
Original file line number Diff line number Diff line change
Expand Up @@ -689,7 +689,12 @@ func parseModule(filename string, stmts []Statement, comments []*Comment, regoCo

// The comments slice only holds comments that were not their own statements.
mod.Comments = append(mod.Comments, comments...)
mod.regoVersion = regoCompatibilityMode

if regoCompatibilityMode == RegoUndefined {
mod.regoVersion = DefaultRegoVersion()
} else {
mod.regoVersion = regoCompatibilityMode
}

for i, stmt := range stmts[1:] {
switch stmt := stmt.(type) {
Expand Down
15 changes: 11 additions & 4 deletions ast/policy.go
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ var Wildcard = &Term{Value: Var("_")}
const WildcardPrefix = "$"

// Keywords contains strings that map to language keywords.
var Keywords = KeywordsForRegoVersion(DefaultRegoVersion)
var Keywords = KeywordsForRegoVersion(DefaultRegoVersion())

var KeywordsV0 = [...]string{
"not",
Expand Down Expand Up @@ -791,14 +791,21 @@ type toStringOpts struct {
regoVersion RegoVersion
}

func (o toStringOpts) RegoVersion() RegoVersion {
if o.regoVersion == RegoUndefined {
return DefaultRegoVersion()
}
return o.regoVersion
}

func (rule *Rule) stringWithOpts(opts toStringOpts) string {
buf := []string{}
if rule.Default {
buf = append(buf, "default")
}
buf = append(buf, rule.Head.stringWithOpts(opts))
if !rule.Default {
switch opts.regoVersion {
switch opts.RegoVersion() {
case RegoV1, RegoV0CompatV1:
buf = append(buf, "if")
}
Expand Down Expand Up @@ -861,7 +868,7 @@ func (rule *Rule) elseString(opts toStringOpts) string {
buf = append(buf, value.String())
}

switch opts.regoVersion {
switch opts.RegoVersion() {
case RegoV1, RegoV0CompatV1:
buf = append(buf, "if")
}
Expand Down Expand Up @@ -1043,7 +1050,7 @@ func (head *Head) stringWithOpts(opts toStringOpts) string {
case len(head.Args) != 0:
buf.WriteString(head.Args.String())
case len(head.Reference) == 1 && head.Key != nil:
switch opts.regoVersion {
switch opts.RegoVersion() {
case RegoV0:
buf.WriteRune('[')
buf.WriteString(head.Key.String())
Expand Down
4 changes: 2 additions & 2 deletions ast/policy_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -616,7 +616,7 @@ func TestRuleString(t *testing.T) {
assertRuleString(t, tc.rule, tc.expV0, toStringOpts{regoVersion: RegoV0})
assertRuleString(t, tc.rule, tc.expV1, toStringOpts{regoVersion: RegoV1})

switch DefaultRegoVersion {
switch DefaultRegoVersion() {
case RegoV0:
assertRuleString(t, tc.rule, tc.expV0, toStringOpts{})
case RegoV1:
Expand Down Expand Up @@ -863,7 +863,7 @@ import rego.v1
p := 7 if { true }`

if module.String() != exp {
t.Fatalf("expected %q but got %q", exp, module.String())
t.Fatalf("expected:\n\n%q\n\nbut got:\n\n%q", exp, module.String())
}
}

Expand Down
16 changes: 12 additions & 4 deletions bundle/bundle.go
Original file line number Diff line number Diff line change
Expand Up @@ -244,8 +244,8 @@ func (m Manifest) Copy() Manifest {
func (m Manifest) String() string {
m.Init()
if m.RegoVersion != nil {
return fmt.Sprintf("<revision: %q, rego_version: %d, roots: %v, wasm: %+v, metadata: %+v>",
m.Revision, *m.RegoVersion, *m.Roots, m.WasmResolvers, m.Metadata)
return fmt.Sprintf("<revision: %q, rego_version: %d, file_rego_versions: %v, roots: %v, wasm: %+v, metadata: %+v>",
m.Revision, *m.RegoVersion, m.FileRegoVersions, *m.Roots, m.WasmResolvers, m.Metadata)
}
return fmt.Sprintf("<revision: %q, roots: %v, wasm: %+v, metadata: %+v>",
m.Revision, *m.Roots, m.WasmResolvers, m.Metadata)
Expand Down Expand Up @@ -711,10 +711,10 @@ func (r *Reader) Read() (Bundle, error) {

// Parse modules
popts := r.ParserOptions()
popts.RegoVersion = bundle.RegoVersion(popts.RegoVersion)
popts.RegoVersion = bundle.RegoVersion(popts.EffectiveRegoVersion())
for _, mf := range modules {
modulePopts := popts
if modulePopts.RegoVersion, err = bundle.RegoVersionForFile(mf.RelativePath, popts.RegoVersion); err != nil {
if modulePopts.RegoVersion, err = bundle.RegoVersionForFile(mf.RelativePath, popts.EffectiveRegoVersion()); err != nil {
return bundle, err
}
r.metrics.Timer(metrics.RegoModuleParse).Start()
Expand Down Expand Up @@ -1203,6 +1203,10 @@ func (b *Bundle) SetRegoVersion(v ast.RegoVersion) {
// If there is no defined version for the given path, the default version def is returned.
// If the version does not correspond to ast.RegoV0 or ast.RegoV1, an error is returned.
func (b *Bundle) RegoVersionForFile(path string, def ast.RegoVersion) (ast.RegoVersion, error) {
if def == ast.RegoUndefined {
def = ast.DefaultRegoVersion()
}

version, err := b.Manifest.numericRegoVersionForFile(path)
if err != nil {
return def, err
Expand Down Expand Up @@ -1410,6 +1414,10 @@ func MergeWithRegoVersion(bundles []*Bundle, regoVersion ast.RegoVersion, usePat
return nil, errors.New("expected at least one bundle")
}

if regoVersion == ast.RegoUndefined {
regoVersion = ast.DefaultRegoVersion()
}

if len(bundles) == 1 {
result := bundles[0]
// We respect the bundle rego-version, defaulting to the provided rego version if not set.
Expand Down
2 changes: 1 addition & 1 deletion cmd/build.go
Original file line number Diff line number Diff line change
Expand Up @@ -309,7 +309,7 @@ func dobuild(params buildParams, args []string) error {
WithPartialNamespace(params.ns).
WithFollowSymlinks(params.followSymlinks)

regoVersion := ast.DefaultRegoVersion
regoVersion := ast.DefaultRegoVersion()
if params.v0Compatible {
// v0 takes precedence over v1
regoVersion = ast.RegoV0
Expand Down
4 changes: 2 additions & 2 deletions cmd/build_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -905,7 +905,7 @@ p2 := 2
tr := tar.NewReader(gr)

expManifest := strings.ReplaceAll(tc.manifest, "%REGO_VERSION%",
fmt.Sprintf("%d", ast.DefaultRegoVersion.Int()))
fmt.Sprintf("%d", ast.DefaultRegoVersion().Int()))

found := false
for {
Expand Down Expand Up @@ -1712,7 +1712,7 @@ q contains 1 if {
}
expVal = strings.ReplaceAll(expVal, "%ROOT%", root)
expVal = strings.ReplaceAll(expVal, "%DEFAULT_REGO_VERSION%",
fmt.Sprintf("%d", ast.DefaultRegoVersion.Int()))
fmt.Sprintf("%d", ast.DefaultRegoVersion().Int()))
if string(b) != expVal {
t.Fatalf("expected %v:\n\n%v\n\nbut got:\n\n%v", expName, expVal, string(b))
}
Expand Down
2 changes: 1 addition & 1 deletion cmd/check.go
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ func (p *checkParams) regoVersion() ast.RegoVersion {
if p.v1Compatible {
return ast.RegoV1
}
return ast.DefaultRegoVersion
return ast.DefaultRegoVersion()
}

const (
Expand Down
2 changes: 1 addition & 1 deletion cmd/deps.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ func (p *depsCommandParams) regoVersion() ast.RegoVersion {
if p.v1Compatible {
return ast.RegoV1
}
return ast.DefaultRegoVersion
return ast.DefaultRegoVersion()
}

const (
Expand Down
2 changes: 1 addition & 1 deletion cmd/eval.go
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ func (p *evalCommandParams) regoVersion() ast.RegoVersion {
} else if p.v1Compatible {
return ast.RegoV1
}
return ast.DefaultRegoVersion
return ast.DefaultRegoVersion()
}

func newEvalCommandParams() evalCommandParams {
Expand Down
2 changes: 1 addition & 1 deletion cmd/fmt.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ func (p *fmtCommandParams) regoVersion() ast.RegoVersion {
if p.v1Compatible {
return ast.RegoV1
}
return ast.DefaultRegoVersion
return ast.DefaultRegoVersion()
}

var formatCommand = &cobra.Command{
Expand Down
2 changes: 1 addition & 1 deletion cmd/oracle.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ func (p *findDefinitionParams) regoVersion() ast.RegoVersion {
if p.v1Compatible {
return ast.RegoV1
}
return ast.DefaultRegoVersion
return ast.DefaultRegoVersion()
}

func init() {
Expand Down
2 changes: 1 addition & 1 deletion cmd/parse.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ func (p *parseParams) regoVersion() ast.RegoVersion {
if p.v1Compatible {
return ast.RegoV1
}
return ast.DefaultRegoVersion
return ast.DefaultRegoVersion()
}

var configuredParseParams = parseParams{
Expand Down
4 changes: 2 additions & 2 deletions cmd/refactor.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ func (m *moveCommandParams) regoVersion() ast.RegoVersion {
if m.v1Compatible {
return ast.RegoV1
}
return ast.DefaultRegoVersion
return ast.DefaultRegoVersion()
}

func init() {
Expand Down Expand Up @@ -149,7 +149,7 @@ func doMove(params moveCommandParams, args []string, out io.Writer) error {
return err
}

formatted, err := format.Ast(mod)
formatted, err := format.AstWithOpts(mod, format.Opts{RegoVersion: params.regoVersion()})
if err != nil {
return newError("failed to parse Rego source file: %v", err)
}
Expand Down
14 changes: 12 additions & 2 deletions cmd/refactor_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,12 @@ func TestDoMoveRenamePackage(t *testing.T) {
t.Fatal(err)
}

formatted := format.MustAst(tc.expected)
var formatted []byte
if tc.v0Compatible {
formatted = format.MustAstWithOpts(tc.expected, format.Opts{RegoVersion: ast.RegoV0})
} else {
formatted = format.MustAstWithOpts(tc.expected, format.Opts{RegoVersion: ast.RegoV1})
}

if !reflect.DeepEqual(formatted, buf.Bytes()) {
t.Fatalf("Expected module:\n%v\n\nGot:\n%v\n", string(formatted), buf.String())
Expand Down Expand Up @@ -227,7 +232,12 @@ func TestDoMoveOverwriteFile(t *testing.T) {
t.Fatal(err)
}

actual := ast.MustParseModule(string(data))
var actual *ast.Module
if tc.v0Compatible {
actual = ast.MustParseModuleWithOpts(string(data), ast.ParserOptions{RegoVersion: ast.RegoV0})
} else {
actual = ast.MustParseModuleWithOpts(string(data), ast.ParserOptions{RegoVersion: ast.RegoV1})
}

if !tc.expected.Equal(actual) {
t.Fatalf("Expected module:\n%v\n\nGot:\n%v\n", tc.expected, actual)
Expand Down
Loading
Loading