Open uhthomas opened 3 months ago
mvdan
commented
3 months ago We indeed want to move towards making staticcheck happy. We already enforce vet, but we don't enforce staticcheck yet, due to all the deprecation warnings (https://github.com/cue-lang/cue/issues/2480).
We are moving in that direction, it just won't happen overnight due to the amount of work required. I think we can continue using #2480 as a tracking issue for that work. I've given it a slightly better title too.
uhthomas
commented
3 months ago Thanks for pointing out that other issue 😄 I did look, and think I even read it but didn't feel they were entirely the same. Are there plans to enforce static check in CI? Would #2480 also cover the other static check suggestions like unused functions and code simplifications? It would be great if gofumpt could be involved in this somewhere too.
mvdan
commented
3 months ago Yes we do intend to enforce staticcheck in CI - there isn't a tracking issue for it as https://github.com/cue-lang/cue/issues/2480 is a fairly big blocker for that. Although we could start by enforcing staticcheck in CI minus the deprecation warnings - that's something I've thought about before, but haven't looked into yet.
Looking at staticcheck -checks inherit,-SA1019 ./..., there are two vendored sets of packages which cause a significant amount of the warnings:
internal/third_party/yaml - an old fork of go-yaml, which we will delete for v0.11 givne that https://github.com/cue-lang/cue/issues/3027 happened some time ago alreadyinternal/golangorgx/tools - various packages from gopls vendored in for CUE's LSP, some of which will be deleted as they aren't really needed, and some others will be heavily refactoredIn both of the cases above, trying to fix all the warnings now is not particularly interesting, as much of the code will go away. So I think it makes sense to start the staticcheck push in earnest once they're gone. But by all means, if you see warnings that are easy to fix, send patches and I'll review and merge :)
As for gofumpt - I would agree, but it's lower priority, at least right now. Enforcing staticcheck feels more urgent. Plus, we don't enforce gofmt nor cue fmt yet, so that's step one!
uhthomas
commented
3 months ago I agree, linting stuff in third_party/ is probably not worthwhile. It should be fairly easy to exclude that. Even filtering out third_party, golangorgx and deprecated, there are around 80 suggestions (52 of which are unused).
❯ go run honnef.co/go/tools/cmd/staticcheck@latest ./...
cmd/cue/cmd/custom.go:116:3: the surrounding loop is unconditionally terminated (SA4004)
cue/build/import.go:107:11: empty branch (SA9003)
cue/format/import.go:28:6: func sortImports is unused (U1000)
cue/format/import.go:51:6: func setRelativePos is unused (U1000)
cue/format/import.go:63:6: func hasDoc is unused (U1000)
cue/format/import.go:72:6: func importPath is unused (U1000)
cue/format/import.go:80:6: func importName is unused (U1000)
cue/format/import.go:88:6: func importComment is unused (U1000)
cue/format/import.go:98:6: func collapse is unused (U1000)
cue/format/import.go:110:6: type posSpan is unused (U1000)
cue/format/import.go:115:6: func sortSpecs is unused (U1000)
cue/format/import.go:151:6: type byImportSpec is unused (U1000)
cue/format/import.go:153:23: func byImportSpec.Len is unused (U1000)
cue/format/import.go:154:23: func byImportSpec.Swap is unused (U1000)
cue/format/import.go:155:23: func byImportSpec.Less is unused (U1000)
cue/instance.go:168:23: func (*Instance).setError is unused (U1000)
cue/instance.go:173:23: func (*Instance).eval is unused (U1000)
cue/interpreter/wasm/call.go:78:2: should use 'return u == 1' instead of 'if u == 1 { return true }; return false' (S1008)
cue/interpreter/wasm/wasm_test.go:96:6: func copyWasmFiles is unused (U1000)
cue/interpreter/wasm/wasm_test.go:108:6: func copyFile is unused (U1000)
cue/interpreter/wasm/wasm_test.go:116:6: func check is unused (U1000)
cue/interpreter/wasm/wasm_test.go:129:6: func loadDir is unused (U1000)
cue/interpreter/wasm/wasm_test.go:135:6: func dirInstance is unused (U1000)
cue/interpreter/wasm/wasm_test.go:155:6: func loadFile is unused (U1000)
cue/interpreter/wasm/wasm_test.go:159:6: func must is unused (U1000)
cue/load/loader_common.go:175:3: redundant return statement (S1023)
cue/types.go:806:6: func hasDisjunction is unused (U1000)
cue/types.go:828:6: func stripNonDefaults is unused (U1000)
cue/types_test.go:3392:9: identical expressions on the left and right side of the '&&' operator (SA4000)
encoding/openapi/build.go:239:2: empty branch (SA9003)
internal/ci/checks/commit.go:54:10: error strings should not be capitalized (ST1005)
internal/core/adt/composite.go:565:2: empty branch (SA9003)
internal/core/adt/constraints.go:47:23: func (*nodeContext).insertListEllipsis is unused (U1000)
internal/core/adt/dev.go:77:24: func combinedFlags.ignore is unused (U1000)
internal/core/adt/disjunct2.go:584:6: func isPartialNode is unused (U1000)
internal/core/adt/disjunct2.go:585:2: should use 'return d.node.status == finalized' instead of 'if d.node.status == finalized { return true }; return false' (S1008)
internal/core/adt/disjunct2.go:741:3: empty branch (SA9003)
internal/core/adt/errors.go:132:6: func isIncomplete is unused (U1000)
internal/core/adt/eval.go:57:5: var structSentinel is unused (U1000)
internal/core/adt/eval.go:1546:2: field index is unused (U1000)
internal/core/adt/eval.go:2217:2: should use 'return len(n.exprs) < len(exprs)' instead of 'if len(n.exprs) < len(exprs) { return true }; return false' (S1008)
internal/core/adt/eval.go:2471:17: type assertion to the same type: m.Src already has type ast.Expr (S1040)
internal/core/adt/eval_test.go:124:4: empty branch (SA9003)
internal/core/adt/eval_test.go:235:2: should merge variable declaration with assignment on next line (S1021)
internal/core/adt/export_test.go:94:3: this value of ci is never used (SA4006)
internal/core/adt/fields.go:534:2: empty branch (SA9003)
internal/core/adt/fields.go:600:4: empty branch (SA9003)
internal/core/adt/overlay.go:226:3: should replace loop with d.disjunctions = append(d.disjunctions, n.disjunctions...) (S1011)
internal/core/adt/overlay.go:343:2: empty branch (SA9003)
internal/core/adt/overlay.go:362:4: empty branch (SA9003)
internal/core/adt/sched.go:112:23: func (*taskContext).newTask is unused (U1000)
internal/core/adt/sched.go:139:21: func schedState.done is unused (U1000)
internal/core/adt/sched.go:390:3: empty branch (SA9003)
internal/core/adt/sched.go:599:2: field blockStack is unused (U1000)
internal/core/adt/sched_test.go:186:3: field err is unused (U1000)
internal/core/adt/states.go:173:2: const leftOfMaxCoreCondition is unused (U1000)
internal/core/adt/states.go:175:2: const finalStateKnown is unused (U1000)
internal/core/adt/states.go:177:2: const preValidation is unused (U1000)
internal/core/adt/states.go:209:2: const scalarConjunct is unused (U1000)
internal/core/adt/tasks.go:193:6: func processFinalizeDisjunctions is unused (U1000)
internal/core/adt/unify.go:429:2: redundant return statement (S1023)
internal/core/adt/unify.go:459:2: empty branch (SA9003)
internal/core/compile/builtin.go:27:2: var stringParam is unused (U1000)
internal/core/compile/compile.go:126:20: func (*compiler).reset is unused (U1000)
internal/core/subsume/structural.go:21:20: func (*subsumer).subsumes is unused (U1000)
internal/core/subsume/structural.go:40:20: func (*subsumer).conjunct is unused (U1000)
internal/core/subsume/structural.go:44:20: func (*subsumer).c is unused (U1000)
internal/core/subsume/structural.go:48:6: func isBottomConjunct is unused (U1000)
internal/core/subsume/structural.go:53:20: func (*subsumer).node is unused (U1000)
internal/core/subsume/structural.go:60:20: func (*subsumer).structural is unused (U1000)
internal/core/subsume/structural.go:178:20: func (*subsumer).structLit is unused (U1000)
internal/core/subsume/structural.go:225:6: type collatedDecls is unused (U1000)
internal/core/subsume/structural.go:236:6: func newCollatedDecls is unused (U1000)
internal/core/subsume/structural.go:240:6: type field is unused (U1000)
internal/core/subsume/structural.go:245:25: func (*collatedDecls).collate is unused (U1000)
internal/core/subsume/vertex.go:435:6: identical expressions on the left and right side of the '||' operator (SA4000)
internal/envflag/flag.go:89:5: error var InvalidError should have name of the form ErrFoo (ST1012)
internal/mod/modload/tidy.go:712:2: should use 'return ctx.Err() == nil' instead of 'if ctx.Err() != nil { return false }; return true' (S1008)
internal/pkg/types.go:72:2: should use 'return ot&^adt.HasDynamic != 0' instead of 'if ot&^adt.HasDynamic != 0 { return true }; return false' (S1008)
internal/vcs/vcs_test.go:140:2: this value of err is never used (SA4006)
exit status 1Would it make sense for this to be the tracking issue for:
mvdan
commented
3 months ago Sure. Let's use it just to enforce a subset of staticcheck (minus SA1019 for the deprecation warnings, as that's https://github.com/cue-lang/cue/issues/2480, and minus third_party and golangorgx). I don't think it's worthwhile to use golangci-lint for this purpose - staticcheck on its own is basically what we want on top of vet and gofmt or gofumpt.
I don't think we should aim to enforce gofumpt just yet. We need to enforce gofmt and cue fmt first. Enforcing gofumpt is also a sweeping change that will likely cause plenty of merge conflicts, so I'd want to coordinate with everyone if we wanted to attempt it.
mvdan
commented
2 months ago internal/third_party is now gone since https://review.gerrithub.io/c/cue-lang/cue/+/1199541, so that's one chunk we no longer need to worry about.
uhthomas
commented
2 months ago Fantastic! I'm happy to apply some static check fixes if you like.
mvdan
commented
2 months ago Sounds good. Just give me five minutes as I'm about to merge some I did this morning.
mvdan
commented
1 month ago Even filtering out
third_party,golangorgxanddeprecated, there are around 80 suggestions (52 of which areunused).
With third_party now gone, and after chipping away at some more of these, we are down to 53 non-deprecation warnings:
$ staticcheck -checks inherit,-SA1019 ./... |& grep -v /golangorgx/ | wc -l
53Fixing a few more in this stack: https://review.gerrithub.io/c/cue-lang/cue/+/1202602
Whilst working on CUE, I've noticed a lot of static check messages in my editor. Would it be worth adding something like golangci-lint?