Updating formatter to not drop rego.v1 and future.keywords imports (

#7224)

to maximize compatibility surface across OPA versions.

Adding `--drop-v0-imports` flag to `opa fmt` for opting in to dropping redundant v0 imports.

Signed-off-by: Johan Fylling <[email protected]>

cmd/build_test.go

@@ -2393,12 +2393,14 @@ allow if {
 					1 < 2
 				}`,
 			},
-			// the rego.v1 import is obsolete in rego-v1, and is removed
+			// the rego.v1 import is kept to maximize compatibility surface
 			expectedFiles: map[string]string{
 				".manifest": `{"revision":"","roots":[""],"rego_version":1}
 `,
 				"test.rego": `package test
 
+import rego.v1
+
 allow if {
 	1 < 2
 }
@@ -2415,12 +2417,39 @@ allow if {
 					1 < 2
 				}`,
 			},
-			// future.keywords imports are obsolete in rego-v1, and are removed
+			// future.keywords imports are kept to maximize compatibility surface
 			expectedFiles: map[string]string{
 				".manifest": `{"revision":"","roots":[""],"rego_version":1}
 `,
 				"test.rego": `package test
 
+import future.keywords.if
+
+allow if {
+	1 < 2
+}
+`,
+			},
+		},
+		{
+			note:         "v1 compatibility: policy with rego.v1 and future.keywords imports",
+			v1Compatible: true,
+			files: map[string]string{
+				"test.rego": `package test
+				import rego.v1
+				import future.keywords.if
+				allow if {
+					1 < 2
+				}`,
+			},
+			// future.keywords are dropped as these are covered by rego.v1
+			expectedFiles: map[string]string{
+				".manifest": `{"revision":"","roots":[""],"rego_version":1}
+`,
+				"test.rego": `package test
+
+import rego.v1
+
 allow if {
 	1 < 2
 }

cmd/fmt.go

@@ -21,14 +21,15 @@ import (
 )
 
 type fmtCommandParams struct {
-	overwrite    bool
-	list         bool
-	diff         bool
-	fail         bool
-	regoV1       bool
-	v0Compatible bool
-	v1Compatible bool
-	checkResult  bool
+	overwrite     bool
+	list          bool
+	diff          bool
+	fail          bool
+	regoV1        bool
+	v0Compatible  bool
+	v1Compatible  bool
+	checkResult   bool
+	dropV0Imports bool
 }
 
 var fmtParams = fmtCommandParams{}
@@ -133,7 +134,8 @@ func formatFile(params *fmtCommandParams, out io.Writer, filename string, info o
 	}
 
 	opts := format.Opts{
-		RegoVersion: params.regoVersion(),
+		RegoVersion:   params.regoVersion(),
+		DropV0Imports: params.dropV0Imports,
 	}
 
 	if params.v0Compatible {
@@ -254,6 +256,7 @@ func init() {
 	addV0CompatibleFlag(formatCommand.Flags(), &fmtParams.v0Compatible, false)
 	addV1CompatibleFlag(formatCommand.Flags(), &fmtParams.v1Compatible, false)
 	formatCommand.Flags().BoolVar(&fmtParams.checkResult, "check-result", true, "assert that the formatted code is valid and can be successfully parsed (default true)")
+	formatCommand.Flags().BoolVar(&fmtParams.dropV0Imports, "drop-v0-imports", false, "drop v0 imports from the formatted code, such as 'rego.v1' and 'future.keywords'")
 
 	RootCommand.AddCommand(formatCommand)
 }

cmd/fmt_test.go

@@ -887,10 +887,11 @@ q := all([true, false])
 
 func TestFmt_DefaultRegoVersion(t *testing.T) {
 	tests := []struct {
-		note         string
-		input        string
-		expected     string
-		expectedErrs []string
+		note          string
+		dropV0Imports bool
+		input         string
+		expected      string
+		expectedErrs  []string
 	}{
 		{
 			note: "no keywords used",
@@ -939,6 +940,86 @@ p if {
 	input.x == 1
 }
 
+q contains "foo" if {
+	input.x == 2
+}
+`,
+			// NOTE: We keep the future imports to create the broadest possible compatibility surface
+			expected: `package test
+
+import future.keywords
+
+p if {
+	input.x == 1
+}
+
+q contains "foo" if {
+	input.x == 2
+}
+`,
+		},
+		{
+			note: "future imports, drop v0 imports",
+			input: `package test
+import future.keywords.if
+import future.keywords.contains
+p if {
+	input.x == 1
+}
+
+q contains "foo" if {
+	input.x == 2
+}
+`,
+			expected: `package test
+
+import future.keywords.contains
+import future.keywords.if
+
+p if {
+	input.x == 1
+}
+
+q contains "foo" if {
+	input.x == 2
+}
+`,
+		},
+		{
+			note: "rego.v1 import",
+			input: `package test
+import rego.v1
+p if {
+	input.x == 1
+}
+
+q contains "foo" if {
+	input.x == 2
+}
+`,
+			// NOTE: We keep the rego.v1 import to create the broadest possible compatibility surface
+			expected: `package test
+
+import rego.v1
+
+p if {
+	input.x == 1
+}
+
+q contains "foo" if {
+	input.x == 2
+}
+`,
+		},
+		{
+			note:          "rego.v1 import, drop v0 imports",
+			dropV0Imports: true,
+			input: `package test
+import rego.v1
+p if {
+	input.x == 1
+}
+
 q contains "foo" if {
 	input.x == 2
 }
@@ -998,6 +1079,7 @@ q := all([true, false])
 	for _, tc := range tests {
 		t.Run(tc.note, func(t *testing.T) {
 			params := fmtCommandParams{}
+			params.dropV0Imports = tc.dropV0Imports
 
 			files := map[string]string{
 				"policy.rego": tc.input,

v1/format/format.go

@@ -31,6 +31,10 @@ type Opts struct {
 
 	// ParserOptions is the parser options used when parsing the module to be formatted.
 	ParserOptions *ast.ParserOptions
+
+	// DropV0Imports instructs the formatter to drop all v0 imports from the module; i.e. 'rego.v1' and 'future.keywords' imports.
+	// Imports are only removed if [Opts.RegoVersion] makes them redundant.
+	DropV0Imports bool
 }
 
 func (o Opts) effectiveRegoVersion() ast.RegoVersion {
@@ -140,6 +144,7 @@ type fmtOpts struct {
 	refHeads bool
 
 	regoV1         bool
+	regoV1Imported bool
 	futureKeywords []string
 }
 
@@ -200,6 +205,7 @@ func AstWithOpts(x interface{}, opts Opts) ([]byte, error) {
 
 			switch {
 			case isRegoV1Compatible(n):
+				o.regoV1Imported = true
 				o.contains = true
 				o.ifs = true
 			case future.IsAllFutureKeywords(n):
@@ -234,14 +240,21 @@ func AstWithOpts(x interface{}, opts Opts) ([]byte, error) {
 
 	switch x := x.(type) {
 	case *ast.Module:
-		if regoVersion == ast.RegoV1 {
+		if regoVersion == ast.RegoV1 && opts.DropV0Imports {
 			x.Imports = filterRegoV1Import(x.Imports)
 		} else if regoVersion == ast.RegoV0CompatV1 {
 			x.Imports = ensureRegoV1Import(x.Imports)
 		}
 
-		if regoVersion == ast.RegoV0CompatV1 || regoVersion == ast.RegoV1 || moduleIsRegoV1Compatible(x) {
-			x.Imports = future.FilterFutureImports(x.Imports)
+		regoV1Imported := moduleIsRegoV1Compatible(x)
+		if regoVersion == ast.RegoV0CompatV1 || regoVersion == ast.RegoV1 || regoV1Imported {
+			if !opts.DropV0Imports && !regoV1Imported {
+				for _, kw := range o.futureKeywords {
+					x.Imports = ensureFutureKeywordImport(x.Imports, kw)
+				}
+			} else {
+				x.Imports = future.FilterFutureImports(x.Imports)
+			}
 		} else {
 			for kw := range extraFutureKeywordImports {
 				x.Imports = ensureFutureKeywordImport(x.Imports, kw)

v1/format/testfiles/v1/test_future_kw_import.rego

@@ -0,0 +1,21 @@
+package p
+
+# existing future.keywords imports kept for broadest compatibility surface
+import future.keywords.every
+import future.keywords.if
+
+r if {
+	every x in [1,3,5] {
+        is_odd(x)
+    }
+
+	every x in [1,3,5] { is_odd(x); true }
+
+	every x in [1,3,5] {
+        is_odd(x)
+        true
+        x < 10
+    }
+}
+
+is_odd(x) = x % 2 == 0

v1/format/testfiles/v1/test_future_kw_import.rego.formatted

@@ -0,0 +1,21 @@
+package p
+
+# existing future.keywords imports kept for broadest compatibility surface
+import future.keywords.every
+import future.keywords.if
+
+r if {
+	every x in [1, 3, 5] {
+		is_odd(x)
+	}
+
+	every x in [1, 3, 5] { is_odd(x); true}
+
+	every x in [1, 3, 5] {
+		is_odd(x)
+		true
+		x < 10
+	}
+}
+
+is_odd(x) := (x % 2) == 0

v1/format/testfiles/v1/test_rego_v1.rego

@@ -1,6 +1,7 @@
 package example
 
-import rego.v1 # import will be dropped
+import rego.v1 # rego.v1 import kept for broadest compatibility surface
+import future.keywords.if # future.keywords imports are dropped, as they're covered by rego.v1
 
 # R1: constant
 a := 1

v1/format/testfiles/v1/test_rego_v1.rego.formatted

@@ -1,6 +1,8 @@
 package example
 
-# import will be dropped
+import rego.v1 # rego.v1 import kept for broadest compatibility surface
+
+# future.keywords imports are dropped, as they're covered by rego.v1
 
 # R1: constant
 a := 1