build: add new mustcallcheck analyzer and fix instances#2886
build: add new mustcallcheck analyzer and fix instances#2886tstirrat15 merged 5 commits intomainfrom
Conversation
github-actions
bot
added
area/cli
tstirrat15
force-pushed
the
new-mustcallcheck-analyzer
branch
from
193cb16 to
c63662d
Compare
| mapped := NewSubjectByTypeSet() | ||
| for key, subjectset := range s.byType { | ||
| ns, rel := tuple.MustSplitRelRef(key) | ||
| ns, rel, err := tuple.SplitRelRef(key) |
There was a problem hiding this comment.
I added this new function and then used it where it was appropriate.
| // RegisterCacheFlags registers flags used to configure SpiceDB's various caches. | ||
| func RegisterCacheFlags(flags *pflag.FlagSet, flagPrefix, flagDescription string, config, defaults *CacheConfig) error { |
There was a problem hiding this comment.
This probably would have been fine as an exception because the error case is hard to hit, but it also wasn't too bad to make it return an error.
| dispatcher, err := graph.NewLocalOnlyDispatcher(params) | ||
| if err != nil { | ||
| return nil, nil, err | ||
| } |
There was a problem hiding this comment.
Similar here - hard to hit, but also probably not a problem.
| nameAllowList = []string{ | ||
| // MustBugf returns an error in production, only panics in tests | ||
| "MustBugf", | ||
| // MustPanicf is explicitly for violating the linter when necessary | ||
| "MustPanicf", | ||
| // MustFromContext is used to grab a datastore handle out of context; | ||
| // if the datastore reference isn't available we should probably panic. | ||
| "MustFromContext", | ||
| // MustValue is used to pull ctxkey values out of context. If the value isn't | ||
| // present, it means there's probably a bug and a panic is appropriate. | ||
| "MustValue", | ||
| // MustNewInterceptorPooler comes from pgx and doesn't have an alternative | ||
| "MustNewInterceptorPooler", | ||
| // MustRequirePresharedKey is intended to be a guardrail if the preshared key isn't | ||
| // present at the callsite. | ||
| "MustRequirePresharedKey", | ||
| } | ||
| packageAllowList = []string{ | ||
| // cobrautil has many Must* functions but doesn't have non-Must variants | ||
| "cobrautil", | ||
| } |
There was a problem hiding this comment.
This is the list of exceptions - take a look and lemme know what you think.
Codecov Report❌ Patch coverage is ❌ Your project status has failed because the head coverage (74.87%) is below the target coverage (75.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #2886 +/- ##
==========================================
- Coverage 74.89% 74.87% -0.01%
==========================================
Files 484 484
Lines 57997 58036 +39
==========================================
+ Hits 43430 43449 +19
- Misses 11530 11545 +15
- Partials 3037 3042 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| "MustRequirePresharedKey", | ||
| } | ||
| packageAllowList = []string{ | ||
| // cobrautil has many Must* functions but doesn't have non-Must variants |
There was a problem hiding this comment.
Yeah, but I'm also not too worried about it because they typically happen during process startup and if they panic it indicates a bug in the code rather than invalid input.
miparnisari
changed the title
tstirrat15
force-pushed
the
new-mustcallcheck-analyzer
branch
from
c63662d to
e3272ee
Compare
2 participants
Description
Follow-on to #2878. We realized we could write an analyzer that checks for that condition explicitly, so we did that and fixed the findings.
Changes
mustcallcheckanalyzerTesting
Review. See that analyzers and tests pass.