Skip to content

Commit

Permalink
Avoid copying loop vars (Go 1.22+) (open-policy-agent#7191)
Browse files Browse the repository at this point in the history
This isn't needed anymore, so now we don't.
Also enabled the copyloopvar linter in case we
accidentally do this in the future.

Signed-off-by: Anders Eknert <anders@styra.com>
  • Loading branch information
anderseknert authored Nov 24, 2024
1 parent a60ef72 commit 270f31f
Show file tree
Hide file tree
Showing 23 changed files with 121 additions and 177 deletions.
1 change: 1 addition & 0 deletions .golangci.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -29,4 +29,5 @@ linters:
- gosimple
- prealloc
- unconvert
- copyloopvar
# - gosec # too many false positives
2 changes: 0 additions & 2 deletions cmd/bench_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -859,7 +859,6 @@ a[4] {

for _, mode := range modes {
for _, tc := range tests {
tc := tc // Reassignment prevents loop var scoping issue. (See: https://go.dev/blog/loopvar-preview)
t.Run(fmt.Sprintf("%s, %s", tc.note, mode.name), func(t *testing.T) {
t.Parallel()

Expand Down Expand Up @@ -1023,7 +1022,6 @@ a contains 4 if {
for _, bundleType := range bundleTypeCases {
for _, mode := range modes {
for _, tc := range tests {
tc := tc // Reassignment prevents loop var scoping issue. (See: https://go.dev/blog/loopvar-preview)
t.Run(fmt.Sprintf("%s, %s, %s", bundleType.note, tc.note, mode.name), func(t *testing.T) {
t.Parallel()

Expand Down
1 change: 0 additions & 1 deletion cmd/eval.go
Original file line number Diff line number Diff line change
Expand Up @@ -499,7 +499,6 @@ func evalOnce(ctx context.Context, ectx *evalContext) pr.Output {
result.Errors = pr.NewOutputErrors(resultErr)
if ectx.builtInErrorList != nil {
for _, err := range *(ectx.builtInErrorList) {
err := err
result.Errors = append(result.Errors, pr.NewOutputErrors(&err)...)
}
}
Expand Down
1 change: 0 additions & 1 deletion debug/debugger.go
Original file line number Diff line number Diff line change
Expand Up @@ -413,7 +413,6 @@ func (s *session) start() error {
defer s.mtx.Unlock()

for _, t := range s.threads {
t := t
go func() {
s.d.logger.Debug("Thread %d started", t.id)
s.d.sendEvent(Event{Type: ThreadEventType, Thread: t.id, Message: "started"})
Expand Down
1 change: 0 additions & 1 deletion storage/disk/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,6 @@ func TestBadgerConfigFromOptions(t *testing.T) {
}

for _, tc := range tests {
tc := tc // copy for capturing loop variable (not needed in Go 1.22+)
t.Run(tc.note, func(t *testing.T) {
t.Parallel()

Expand Down
5 changes: 0 additions & 5 deletions storage/disk/disk_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1144,7 +1144,6 @@ func TestDataPartitioningReadsAndWrites(t *testing.T) {
}

for _, tc := range tests {
tc := tc // copy for capturing loop variable (not needed in Go 1.22+)
t.Run(tc.note, func(t *testing.T) {
t.Parallel()

Expand Down Expand Up @@ -1301,7 +1300,6 @@ func TestDataPartitioningReadNotFoundErrors(t *testing.T) {
}

for _, tc := range tests {
tc := tc // copy for capturing loop variable (not needed in Go 1.22+)
t.Run(tc.note, func(t *testing.T) {
t.Parallel()

Expand Down Expand Up @@ -1458,7 +1456,6 @@ func TestDataPartitioningWriteNotFoundErrors(t *testing.T) {
}

for _, tc := range tests {
tc := tc // copy for capturing loop variable (not needed in Go 1.22+)
t.Run(tc.note, func(t *testing.T) {
t.Parallel()

Expand Down Expand Up @@ -1507,7 +1504,6 @@ func TestDataPartitioningWriteInvalidPatchError(t *testing.T) {
t.Parallel()

for _, pt := range []string{"/*", "/foo"} {
pt := pt // copy for capturing loop variable (not needed in Go 1.22+)
t.Run(pt, func(t *testing.T) {
t.Parallel()

Expand Down Expand Up @@ -1672,7 +1668,6 @@ func TestLookup(t *testing.T) {
}

for _, tc := range cases {
tc := tc // copy for capturing loop variable (not needed in Go 1.22+)
t.Run(tc.note, func(t *testing.T) {
t.Parallel()

Expand Down
1 change: 0 additions & 1 deletion storage/disk/partition_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,6 @@ func TestPartitionTrie(t *testing.T) {
}

for _, tc := range tests {
tc := tc // copy for capturing loop variable (not needed in Go 1.22+)
t.Run(strings.TrimPrefix(tc.path, "/"), func(t *testing.T) {
t.Parallel()

Expand Down
1 change: 0 additions & 1 deletion storage/disk/paths_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,6 @@ func TestIsDisjoint(t *testing.T) {
overlapped: true,
},
} {
tc := tc // copy for capturing loop variable (not needed in Go 1.22+)
t.Run(tc.note, func(t *testing.T) {
t.Parallel()

Expand Down
7 changes: 3 additions & 4 deletions topdown/crypto_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -667,7 +667,6 @@ xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
},
}
for name, testData := range tests {
testData := testData // copy for capturing loop variable (not needed in Go 1.22+)
t.Run(name, func(t *testing.T) {
t.Parallel()

Expand Down Expand Up @@ -829,9 +828,9 @@ func TestExtractX509VerifyOptions(t *testing.T) {
},
},
{ // KeyUsages as an array
jsonOption: ast.MustParseTerm(`{"DNSName": "test.com", "CurrentTime": 1708447636000000000,
"MaxConstraintComparisons": 5,
"KeyUsages" : ["KeyUsageAny", "KeyUsageAny", 1, 2,
jsonOption: ast.MustParseTerm(`{"DNSName": "test.com", "CurrentTime": 1708447636000000000,
"MaxConstraintComparisons": 5,
"KeyUsages" : ["KeyUsageAny", "KeyUsageAny", 1, 2,
"KeyUsageServerAuth","KeyUsageClientAuth"]}`),
expectVerifyOpt: x509.VerifyOptions{
DNSName: "test.com",
Expand Down
1 change: 0 additions & 1 deletion topdown/errors_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,6 @@ func TestErrorWrapping(t *testing.T) {
}

for _, tc := range tests {
tc := tc // copy for capturing loop variable (not needed in Go 1.22+)
t.Run(tc.note, func(t *testing.T) {
t.Parallel()

Expand Down
67 changes: 31 additions & 36 deletions topdown/eval_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,6 @@ func TestMergeWhenHittingNonObject(t *testing.T) {
}

for _, tc := range cases {
tc := tc // copy for capturing loop variable (not needed in Go 1.22+)
t.Run(tc.note, func(t *testing.T) {
t.Parallel()

Expand Down Expand Up @@ -172,7 +171,6 @@ func TestRefContainsNonScalar(t *testing.T) {
}

for _, tc := range cases {
tc := tc // copy for capturing loop variable (not needed in Go 1.22+)
t.Run(tc.note, func(t *testing.T) {
t.Parallel()

Expand Down Expand Up @@ -252,7 +250,6 @@ func TestContainsNestedRefOrCall(t *testing.T) {
}

for _, tc := range tests {
tc := tc // copy for capturing loop variable (not needed in Go 1.22+)
t.Run(tc.note, func(t *testing.T) {
t.Parallel()

Expand Down Expand Up @@ -586,13 +583,13 @@ func TestTopdownVirtualCache(t *testing.T) {
{
note: "partial object, ref-head, ref with unification scope",
module: `package test
a[x][y][z] := x + y + z if {
some x in [1, 2]
some y in [3, 4]
some z in [5, 6]
}
p if {
x := a[1][_][5] # miss, cache key: data.test.a[1][<_,5>]
some foo
Expand All @@ -606,14 +603,14 @@ func TestTopdownVirtualCache(t *testing.T) {
{
note: "partial object, ref-head, ref with unification scope, component order",
module: `package test
a[x][y][a][b] := i if {
some x in [1, 2]
some y in [3, 4]
some a in ["foo", "bar"]
some i, b in ["foo", "bar"]
}
p if {
x := a[1][_]["foo"]["bar"] # miss, cache key: data.test.a[1][<_,foo,bar>]
y := a[1][_]["bar"]["foo"] # miss, cache key: data.test.a[1][<_,bar,foo>]
Expand All @@ -626,13 +623,13 @@ func TestTopdownVirtualCache(t *testing.T) {
{
note: "partial object, ref-head, ref with unification scope, diverging key scope",
module: `package test
a[x][y][z] := x + y + z if {
some x in [1, 2]
some y in [3, 4]
some z in [5, 6]
}
p if {
x := a[1][_][5] # miss, cache key: data.test.a[1][<_,5>]
y := a[1][_][6] # miss, cache key: data.test.a[1][<_,6>]
Expand All @@ -647,13 +644,13 @@ func TestTopdownVirtualCache(t *testing.T) {
{
note: "partial object, ref-head, ref with unification scope, trailing vars don't contribute to key scope",
module: `package test
a[x][y][z][x] := x + y + z if {
some x in [1, 2]
some y in [3, 4]
some z in [5, 6]
}
p if {
x := a[1][_][5][_] # miss, cache key: data.test.a[1][<_,5>]
y := a[1][_][5] # hit, cache key: data.test.a[1][<_,5>]
Expand All @@ -667,11 +664,11 @@ func TestTopdownVirtualCache(t *testing.T) {
// Regression test for https://github.com/open-policy-agent/opa/issues/6926
note: "partial object, ref-head, leaf set, ref with unification scope",
module: `package p
obj.sub[x][x] contains x if some x in ["one", "two"]
obj[x][x] contains x if x := "whatever"
main contains x if {
[1 | obj.sub[_].one[_]] # miss, cache key: data.p.obj.sub[<_,one>]
x := obj.sub[_][_][_] # miss, cache key: data.p.obj.sub
Expand All @@ -683,7 +680,6 @@ func TestTopdownVirtualCache(t *testing.T) {
}

for _, tc := range tests {
tc := tc // copy for capturing loop variable (not needed in Go 1.22+)
t.Run(tc.note, func(t *testing.T) {
t.Parallel()

Expand Down Expand Up @@ -988,9 +984,9 @@ func TestPartialRule(t *testing.T) {
`,
query: `data = x`,
exp: `[{"x": {"test": {"p": {"foo": {
"0": {"bar": "a"},
"1": {"bar": "b"},
"2": {"bar": "c"},
"0": {"bar": "a"},
"1": {"bar": "b"},
"2": {"bar": "c"},
"bar": {"0": "a", "1": "b", "2": "c"}}}}}}]`,
},
// Intersections with object values
Expand Down Expand Up @@ -1131,8 +1127,8 @@ func TestPartialRule(t *testing.T) {
{
note: "deep query into partial object (ref head) and object value",
module: `package test
p.q[r] := x if {
r := "foo"
p.q[r] := x if {
r := "foo"
x := {"bar": {"baz": 1}}
}
`,
Expand Down Expand Up @@ -1258,7 +1254,7 @@ func TestPartialRule(t *testing.T) {
{ // enumeration
note: "deep query into partial object and object value, full depth, enumeration on object value",
module: `package test
p.q[r] := x if {
p.q[r] := x if {
r := ["foo", "bar"][_]
x := {"s": {"do": 0, "re": 1, "mi": 2}}
}
Expand All @@ -1269,7 +1265,7 @@ func TestPartialRule(t *testing.T) {
{ // enumeration
note: "deep query into partial object and object value, full depth, enumeration on rule path and object value",
module: `package test
p.q[r] := x if {
p.q[r] := x if {
r := ["foo", "bar"][_]
x := {"s": {"do": 0, "re": 1, "mi": 2}}
}
Expand All @@ -1292,8 +1288,8 @@ func TestPartialRule(t *testing.T) {
note: "deep query into partial object (general ref head) and set value",
module: `package test
import future.keywords
p.q[r] contains t if {
r := ["foo", "bar"][_]
p.q[r] contains t if {
r := ["foo", "bar"][_]
{"do", "re", "mi"}[t]
}
`,
Expand All @@ -1304,8 +1300,8 @@ func TestPartialRule(t *testing.T) {
note: "deep query into partial object (general ref head, static tail) and set value",
module: `package test
import future.keywords
p.q[r].s contains t if {
r := ["foo", "bar"][_]
p.q[r].s contains t if {
r := ["foo", "bar"][_]
{"do", "re", "mi"}[t]
}
`,
Expand All @@ -1316,8 +1312,8 @@ func TestPartialRule(t *testing.T) {
note: "deep query into general ref to set value",
module: `package test
import future.keywords
p.q[r].s contains t if {
r := ["foo", "bar"][_]
p.q[r].s contains t if {
r := ["foo", "bar"][_]
t := ["do", "re", "mi"][_]
}
`,
Expand All @@ -1327,8 +1323,8 @@ func TestPartialRule(t *testing.T) {
{
note: "deep query into general ref to object value",
module: `package test
p.q[r].s[t] := u if {
r := ["foo", "bar"][_]
p.q[r].s[t] := u if {
r := ["foo", "bar"][_]
t := ["do", "re", "mi"][u]
}
`,
Expand All @@ -1339,8 +1335,8 @@ func TestPartialRule(t *testing.T) {
note: "deep query into general ref enumerating set values",
module: `package test
import future.keywords
p.q[r].s contains t if {
r := ["foo", "bar"][_]
p.q[r].s contains t if {
r := ["foo", "bar"][_]
{"do", "re", "mi"}[t]
}
`,
Expand All @@ -1351,8 +1347,8 @@ func TestPartialRule(t *testing.T) {
{
note: "deep query into partial object and object value, non-tail var",
module: `package test
p.q[r].s := x if {
r := "foo"
p.q[r].s := x if {
r := "foo"
x := {"bar": {"baz": 1}}
}
`,
Expand Down Expand Up @@ -1470,7 +1466,7 @@ func TestPartialRule(t *testing.T) {
t := ["d", "e", "f"][u]
}`,
query: `data.test.p.q[x] = y`,
exp: `[{"x": "a", "y": {"s": {"d": 0, "e": 1, "f": 2}}},
exp: `[{"x": "a", "y": {"s": {"d": 0, "e": 1, "f": 2}}},
{"x": "b", "y": {"s": {"d": 0, "e": 1, "f": 2}}},
{"x": "c", "y": {"s": {"d": 0, "e": 1, "f": 2}}}]`,
},
Expand Down Expand Up @@ -1535,7 +1531,6 @@ func TestPartialRule(t *testing.T) {
}

for _, tc := range tests {
tc := tc // copy for capturing loop variable (not needed in Go 1.22+)
t.Run(tc.note, func(t *testing.T) {
t.Parallel()

Expand Down
Loading

0 comments on commit 270f31f

Please sign in to comment.