Skip to content

chore: enable gocritic #2682

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

chore: enable gocritic #2682

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .golangci.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ linters:
- "errcheck"
- "errname"
- "errorlint"
# - "gocritic" TODO: fix issues
- "gocritic"
- "goprintffuncname"
- "gosec"
# - "govet" TODO: fix issues
Expand Down
2 changes: 1 addition & 1 deletion cmd/spicedb/servetesting_integration_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ func TestTestServer(t *testing.T) {
"--http-addr", ":8443",
"--readonly-http-addr", ":8444",
"--http-enabled",
//"--readonly-http-enabled",
// "--readonly-http-enabled",
},
ExposedPorts: []string{"50051/tcp", "50052/tcp", "8443/tcp", "8444/tcp"},
},
Expand Down
28 changes: 7 additions & 21 deletions internal/cursorediterator/blocks_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -639,9 +639,7 @@ func TestCursoredProducerMapperIterator(t *testing.T) {
}

// Cursor converter functions for int
intFromString := func(s string) (int, error) {
return strconv.Atoi(s)
}
Comment on lines -642 to -644
Copy link
Contributor Author

@tstirrat15 tstirrat15 Oct 31, 2025

Choose a reason for hiding this comment

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

This is the unlambda rule - basically don't wrap a function in a lambda if the lambda is exactly the same as the function.

Copy link
Contributor

@miparnisari miparnisari Oct 31, 2025

Choose a reason for hiding this comment

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

I don't like lambdas in general because they make reading stacktraces a pain in the behind

intFromString := strconv.Atoi

intToString := func(i int) (string, error) {
return strconv.Itoa(i), nil
Expand Down Expand Up @@ -1084,9 +1082,7 @@ func TestCursoredProducerMapperIterator(t *testing.T) {
}

// Cursor converter functions for int
intFromString := func(s string) (int, error) {
return strconv.Atoi(s)
}
intFromString := strconv.Atoi

intToString := func(i int) (string, error) {
return strconv.Itoa(i), nil
Expand Down Expand Up @@ -1383,9 +1379,7 @@ func TestCursoredProducerMapperIteratorConcurrency1SpecialCase(t *testing.T) {
ctx := t.Context()

// Cursor converter functions for int
intFromString := func(s string) (int, error) {
return strconv.Atoi(s)
}
intFromString := strconv.Atoi

intToString := func(i int) (string, error) {
return strconv.Itoa(i), nil
Expand Down Expand Up @@ -1718,9 +1712,7 @@ func TestConcurrencyConsistency(t *testing.T) {

t.Run("CursoredProducerMapperIterator consistency", func(t *testing.T) {
// Cursor converter functions
intFromString := func(s string) (int, error) {
return strconv.Atoi(s)
}
intFromString := strconv.Atoi
intToString := func(i int) (string, error) {
return strconv.Itoa(i), nil
}
Expand Down Expand Up @@ -1972,9 +1964,7 @@ func TestDisableCursorsInContext(t *testing.T) {
t.Run("CursoredProducerMapperIterator with cursors disabled", func(t *testing.T) {
ctxWithoutCursors := DisableCursorsInContext(ctx)

intFromString := func(s string) (int, error) {
return strconv.Atoi(s)
}
intFromString := strconv.Atoi

intToString := func(i int) (string, error) {
return strconv.Itoa(i), nil
Expand Down Expand Up @@ -2024,9 +2014,7 @@ func TestDisableCursorsInContext(t *testing.T) {
})

t.Run("CursoredProducerMapperIterator with cursors enabled", func(t *testing.T) {
intFromString := func(s string) (int, error) {
return strconv.Atoi(s)
}
intFromString := strconv.Atoi

intToString := func(i int) (string, error) {
return strconv.Itoa(i), nil
Expand Down Expand Up @@ -2071,9 +2059,7 @@ func TestDisableCursorsInContext(t *testing.T) {
t.Run("CursoredProducerMapperIterator with cursors disabled concurrency=1", func(t *testing.T) {
ctxWithoutCursors := DisableCursorsInContext(ctx)

intFromString := func(s string) (int, error) {
return strconv.Atoi(s)
}
intFromString := strconv.Atoi

intToString := func(i int) (string, error) {
return strconv.Itoa(i), nil
Expand Down
4 changes: 1 addition & 3 deletions internal/cursorediterator/context_cancellation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,9 +281,7 @@ func TestContextCancellationComprehensive(t *testing.T) {
concurrencyLevels := []uint16{1, 2, 4}

// Cursor converter functions
intFromString := func(s string) (int, error) {
return strconv.Atoi(s)
}
intFromString := strconv.Atoi
intToString := func(i int) (string, error) {
return strconv.Itoa(i), nil
}
Expand Down
4 changes: 1 addition & 3 deletions internal/cursorediterator/cursor_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,9 +151,7 @@ func TestCursorCustomHeadValue(t *testing.T) {
})

// Test with bool converter
boolConverter := func(s string) (bool, error) {
return strconv.ParseBool(s)
}
boolConverter := strconv.ParseBool

t.Run("bool converter success", func(t *testing.T) {
c := Cursor{"true", "false"}
Expand Down
21 changes: 12 additions & 9 deletions internal/datastore/crdb/schema/indexutil.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,11 +188,12 @@ func checkFilterColumnMatchesFilter(colName string, filter datastore.Relationshi
foundCount++
}
}
if foundCount == 0 {
switch {
case foundCount == 0:
Comment on lines +191 to +192
Copy link
Contributor Author

@tstirrat15 tstirrat15 Oct 31, 2025

Choose a reason for hiding this comment

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

Some if/elseif/else rewriting to cases

return columnFilterNoMatch, nil
} else if foundCount < len(filter.OptionalSubjectsSelectors) {
case foundCount < len(filter.OptionalSubjectsSelectors):
return columnFilterForceNoMatch, nil
} else {
default:
return columnFilterMatch, nil
}

Expand All @@ -207,11 +208,12 @@ func checkFilterColumnMatchesFilter(colName string, filter datastore.Relationshi
foundCount++
}
}
if foundCount == 0 {
switch {
case foundCount == 0:
return columnFilterNoMatch, nil
} else if foundCount < len(filter.OptionalSubjectsSelectors) {
case foundCount < len(filter.OptionalSubjectsSelectors):
return columnFilterForceNoMatch, nil
} else {
default:
return columnFilterMatch, nil
}

Expand All @@ -226,11 +228,12 @@ func checkFilterColumnMatchesFilter(colName string, filter datastore.Relationshi
foundCount++
}
}
if foundCount == 0 {
switch {
case foundCount == 0:
return columnFilterNoMatch, nil
} else if foundCount < len(filter.OptionalSubjectsSelectors) {
case foundCount < len(filter.OptionalSubjectsSelectors):
return columnFilterForceNoMatch, nil
} else {
default:
return columnFilterMatch, nil
}

Expand Down
6 changes: 2 additions & 4 deletions internal/datastore/postgres/postgres_shared_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1981,11 +1981,9 @@ func RevisionTimestampAndTransactionIDTest(t *testing.T, ds datastore.Datastore)
require.True(transactionIDPresent, "expected transactionID to be present in revision")

checkedUpdate = true
} else {
} else if checkedUpdate {
// we wait for a checkpoint right after the update. Checkpoints could happen at any time off band.
if checkedUpdate {
checkedCheckpoint = true
}
checkedCheckpoint = true
}

time.Sleep(1 * time.Millisecond)
Expand Down
2 changes: 1 addition & 1 deletion internal/graph/lookupresources3.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1399,7 +1399,7 @@
return dsIndexPrefix + index.chunkID, nil
}

return dsIndexPrefix + index.chunkID + ":" + (*index.dbCursor).String(), nil
return dsIndexPrefix + index.chunkID + ":" + index.dbCursor.RelationshipReference.String(), nil

Check warning on line 1402 in internal/graph/lookupresources3.go

View check run for this annotation

Codecov / codecov/patch

internal/graph/lookupresources3.go#L1402 

Added line #L1402 was not covered by tests
Copy link
Contributor Author

@tstirrat15 tstirrat15 Oct 31, 2025

Choose a reason for hiding this comment

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

I'm not entirely sure how this deref worked previously - this both makes things a little clearer and also satisfies gocritic.

Copy link
Contributor Author

@tstirrat15 tstirrat15 Oct 31, 2025

Choose a reason for hiding this comment

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

Except that it doesn't work 🤔

Copy link
Contributor

@miparnisari miparnisari Oct 31, 2025

Choose a reason for hiding this comment

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

does this explain the broken tests or are you talking about something else?

Copy link
Contributor Author

@tstirrat15 tstirrat15 Oct 31, 2025

Choose a reason for hiding this comment

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

I thought this was the test failure but it's not 🤔

}

type datastoreIndex struct {
Expand Down
20 changes: 20 additions & 0 deletions internal/graph/lookupresources3_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
"github.com/stretchr/testify/require"

cter "github.com/authzed/spicedb/internal/cursorediterator"
"github.com/authzed/spicedb/pkg/datastore/options"
core "github.com/authzed/spicedb/pkg/proto/core/v1"
v1 "github.com/authzed/spicedb/pkg/proto/dispatch/v1"
"github.com/authzed/spicedb/pkg/tuple"
Expand Down Expand Up @@ -489,3 +490,22 @@
require.Equal(t, "resource1", yieldedResults[0].Item.resourceID)
require.Equal(t, "resource2", yieldedResults[1].Item.resourceID)
}

func TestMustDatastoreIndexToString(t *testing.T) {
t.Run("nil dbCursor", func(t *testing.T) {
index := datastoreIndex{
chunkID: "someChunkID",
}
string, _ := mustDatastoreIndexToString(index)

Check failure on line 499 in internal/graph/lookupresources3_test.go

View workflow job for this annotation

GitHub Actions / Lint Go 

variable string has same name as predeclared identifier (predeclared)
require.Equal(t, "$dsi:someChunkID", string)
})
t.Run("defined dbCursor", func(t *testing.T) {
relation := tuple.MustParse("group:second#member@user:tom")
index := datastoreIndex{
chunkID: "someChunkID",
dbCursor: options.Cursor(&relation),
}
string, _ := mustDatastoreIndexToString(index)

Check failure on line 508 in internal/graph/lookupresources3_test.go

View workflow job for this annotation

GitHub Actions / Lint Go 

variable string has same name as predeclared identifier (predeclared)
require.Equal(t, "$dsi:someChunkID:group:second#member@user:tom#...", string)
})
}
4 changes: 2 additions & 2 deletions internal/services/shared/errors_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,7 @@ func TestRewriteError(t *testing.T) {
expectedCode: codes.FailedPrecondition,
expectedContains: "failed precondition: relation/permission `view` under definition `document` is missing type information",
},
//{ TODO this doesn't pass
// { TODO this doesn't pass
// name: "compiler error",
// inputError: &compiler.WithContextError{
// BaseCompilerError: compiler.BaseCompilerError{
Expand All @@ -191,7 +191,7 @@ func TestRewriteError(t *testing.T) {
// config: nil,
// expectedCode: codes.InvalidArgument,
// expectedContains: "syntax error in schema",
//},
// },
{
name: "source error",
inputError: spiceerrors.NewWithSourceError(
Expand Down
5 changes: 4 additions & 1 deletion internal/services/shared/schema.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -489,7 +489,10 @@
fullMetadata = make(map[string]string)
}
fullMetadata["relationship"] = strValue
newArgs := append(args, strValue)
// newArgs := append(args, strValue)
newArgs := make([]any, 0, len(args)+len(strValue))
newArgs = append(newArgs, args)
newArgs = append(newArgs, strValue)

Check warning on line 495 in internal/services/shared/schema.go

View check run for this annotation

Codecov / codecov/patch

internal/services/shared/schema.go#L493-L495 

Added lines #L493 - L495 were not covered by tests
return NewSchemaWriteDataValidationError(message+": %s", newArgs, fullMetadata)
}

Expand Down
3 changes: 1 addition & 2 deletions pkg/cmd/server/server_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import (
"context"
"errors"
"fmt"
"log"
"testing"
"time"

Expand Down Expand Up @@ -276,7 +275,7 @@ func TestRetryPolicy(t *testing.T) {
datastore.WithRequestHedgingEnabled(false),
)
if err != nil {
log.Fatalf("unable to start memdb datastore: %s", err)
Copy link
Contributor Author

@tstirrat15 tstirrat15 Oct 31, 2025

Choose a reason for hiding this comment

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

It was complaining here that log.Fatalf prevents defer blocks from running. t.Fatalf should be equivalent and also work around that.

t.Fatalf("unable to start memdb datastore: %s", err)
}

var interceptor countingInterceptor
Expand Down
4 changes: 2 additions & 2 deletions pkg/composableschemadsl/generator/generator_impl.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,12 +29,12 @@ func (sg *sourceGenerator) ensureBlankLineOrNewScope() {

// indent increases the current indentation.
func (sg *sourceGenerator) indent() {
sg.indentationLevel = sg.indentationLevel + 1
sg.indentationLevel++
}

// dedent decreases the current indentation.
func (sg *sourceGenerator) dedent() {
sg.indentationLevel = sg.indentationLevel - 1
sg.indentationLevel--
}

// appendIssue adds an issue found in generation.
Expand Down
8 changes: 2 additions & 6 deletions pkg/datastore/test/pagination.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -406,9 +406,7 @@ func foreachTxType(
}

func sortedStandardData(resourceType string, order options.SortOrder) []tuple.Relationship {
asTuples := slicez.Map(testfixtures.StandardRelationships, func(item string) tuple.Relationship {
return tuple.MustParse(item)
})
Comment on lines -409 to -411
Copy link
Contributor Author

@tstirrat15 tstirrat15 Oct 31, 2025

Choose a reason for hiding this comment

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

Same unlambda thing here.

asTuples := slicez.Map(testfixtures.StandardRelationships, tuple.MustParse)

filteredToType := slicez.Filter(asTuples, func(item tuple.Relationship) bool {
return item.Resource.ObjectType == resourceType
Expand All @@ -433,9 +431,7 @@ func sortedStandardData(resourceType string, order options.SortOrder) []tuple.Re
}

func sortedStandardDataBySubject(subjectType string, order options.SortOrder) []tuple.Relationship {
asTuples := slicez.Map(testfixtures.StandardRelationships, func(item string) tuple.Relationship {
return tuple.MustParse(item)
})
asTuples := slicez.Map(testfixtures.StandardRelationships, tuple.MustParse)

filteredToType := slicez.Filter(asTuples, func(item tuple.Relationship) bool {
if subjectType == "" {
Expand Down
4 changes: 2 additions & 2 deletions pkg/genutil/slicez/slicez_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,7 @@ func TestMapDifferentTypes(t *testing.T) {
{
name: "convert int to string",
input: []int{1, 2, 3},
fn: func(x int) string { return strconv.Itoa(x) },
fn: strconv.Itoa,
expected: []string{"1", "2", "3"},
},
{
Expand All @@ -174,7 +174,7 @@ func TestMapDifferentTypes(t *testing.T) {
{
name: "empty slice different types",
input: []int{},
fn: func(x int) string { return strconv.Itoa(x) },
fn: strconv.Itoa,
expected: []string{},
},
}
Expand Down
7 changes: 4 additions & 3 deletions pkg/query/arrow.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,13 +77,14 @@
// Combine caveats from both sides using Path-based approach
// For arrow operations (left->right), both conditions must be satisfied (AND logic)
var combinedCaveat *core.CaveatExpression
if path.Caveat != nil && checkPath.Caveat != nil {
switch {
case path.Caveat != nil && checkPath.Caveat != nil:

Check warning on line 81 in pkg/query/arrow.go

View check run for this annotation

Codecov / codecov/patch

pkg/query/arrow.go#L80-L81 

Added lines #L80 - L81 were not covered by tests
// Both sides have caveats - create combined caveat expression
combinedCaveat = caveats.And(path.Caveat, checkPath.Caveat)
} else if path.Caveat != nil {
case path.Caveat != nil:

Check warning on line 84 in pkg/query/arrow.go

View check run for this annotation

Codecov / codecov/patch

pkg/query/arrow.go#L84 

Added line #L84 was not covered by tests
// Only left side has caveat
combinedCaveat = path.Caveat
} else if checkPath.Caveat != nil {
case checkPath.Caveat != nil:

Check warning on line 87 in pkg/query/arrow.go

View check run for this annotation

Codecov / codecov/patch

pkg/query/arrow.go#L87 

Added line #L87 was not covered by tests
// Only right side has caveat
combinedCaveat = checkPath.Caveat
}
Expand Down
14 changes: 8 additions & 6 deletions pkg/query/simplify_caveat.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,13 +178,14 @@
return nil, false, err
}

if simplified == nil && passes {
switch {
case simplified == nil && passes:

Check warning on line 182 in pkg/query/simplify_caveat.go

View check run for this annotation

Codecov / codecov/patch

pkg/query/simplify_caveat.go#L181-L182 

Added lines #L181 - L182 were not covered by tests
// Child evaluated to true unconditionally - remove it from AND
continue
} else if !passes {
case !passes:

Check warning on line 185 in pkg/query/simplify_caveat.go

View check run for this annotation

Codecov / codecov/patch

pkg/query/simplify_caveat.go#L185 

Added line #L185 was not covered by tests
// Child failed - entire AND fails
return simplified, false, nil
} else {
default:

Check warning on line 188 in pkg/query/simplify_caveat.go

View check run for this annotation

Codecov / codecov/patch

pkg/query/simplify_caveat.go#L188 

Added line #L188 was not covered by tests
// Child is conditional - keep it
simplifiedChildren = append(simplifiedChildren, simplified)
}
Expand Down Expand Up @@ -227,13 +228,14 @@
return nil, false, err
}

if simplified == nil && passes {
switch {
case simplified == nil && passes:

Check warning on line 232 in pkg/query/simplify_caveat.go

View check run for this annotation

Codecov / codecov/patch

pkg/query/simplify_caveat.go#L231-L232 

Added lines #L231 - L232 were not covered by tests
// Child evaluated to true unconditionally - entire OR is true
return nil, true, nil
} else if !passes {
case !passes:

Check warning on line 235 in pkg/query/simplify_caveat.go

View check run for this annotation

Codecov / codecov/patch

pkg/query/simplify_caveat.go#L235 

Added line #L235 was not covered by tests
// Child failed - remove it from OR
continue
} else {
default:

Check warning on line 238 in pkg/query/simplify_caveat.go

View check run for this annotation

Codecov / codecov/patch

pkg/query/simplify_caveat.go#L238 

Added line #L238 was not covered by tests
// Child is conditional - keep it
simplifiedChildren = append(simplifiedChildren, simplified)
}
Expand Down
2 changes: 2 additions & 0 deletions pkg/validationfile/fileformat.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,12 @@ type ValidationFile struct {
ExpectedRelations blocks.ParsedExpectedRelations `yaml:"validation"`

// NamespaceConfigs are the namespace configuration protos, in text format.
//
// Deprecated: only for internal use. Use `schema`.
NamespaceConfigs []string `yaml:"namespace_configs"`

// ValidationTuples are the validation tuples, in tuple string syntax.
//
// Deprecated: only for internal use. Use `relationships`.
ValidationTuples []string `yaml:"validation_tuples"`

Expand Down
Loading