refactor: handles Cursor uniformly over all usage provenances#64319
Conversation
This change should not change any behaviour for now, but it prepares for syntactic/search-based pagination
varungandhi-src
left a comment
There was a problem hiding this comment.
I find the state transitions as done a bit confusing, but leaving the final decision up to you
| if cursor.IsPrecise() && !provsForSCIPData.Precise { | ||
| cursor = codenav.UsagesCursor{CursorType: codenav.CursorTypeSyntactic} |
There was a problem hiding this comment.
Shouldn't it be an error to end up in this state? More generally, depending on the original request, the cursor should be undergoing one of the following state transitions sequences:
- None -> Precise -> Syntactic -> Search-based -> Done
- None -> Precise -> Done
- None -> Syntactic -> Done
- None -> Search-based -> Done
So I think it should not be possible to have the cursor to be in this state unless someone deliberately sent a malformed request...
There was a problem hiding this comment.
With the way things are currently set up that's true, but I don't see why we shouldn't at some point be able to request "Syntactic + Search-based" or "Precise + Search-based"(to emulate IntelliJ's rename that also targets comments)
| cursor = nextPreciseCursor.UnwrapOr(codenav.UsagesCursor{CursorType: codenav.CursorTypeSyntactic}) | ||
| } | ||
|
|
||
| gitTreeTranslator := r.MakeGitTreeTranslator(&args.Repo) | ||
|
|
||
| var previousSyntacticSearch core.Option[codenav.PreviousSyntacticSearch] | ||
| if remainingCount > 0 && provsForSCIPData.Syntactic { | ||
| syntacticResult, prevSearch, err := r.svc.SyntacticUsages(ctx, gitTreeTranslator, usagesForSymbolArgs) | ||
| if err != nil { | ||
| switch err.Code { | ||
| case codenav.SU_Fatal: | ||
| return nil, err | ||
| case codenav.SU_NoSymbolAtRequestedRange: | ||
| case codenav.SU_NoSyntacticIndex: | ||
| case codenav.SU_FailedToSearch: | ||
| default: | ||
| // None of these errors should cause the whole request to fail | ||
| // TODO: We might want to log some of them in the future | ||
| } | ||
| } else { | ||
| for _, result := range syntacticResult.Matches { | ||
| usageResolvers = append(usageResolvers, NewSyntacticUsageResolver(result, args.Repo.Name, args.CommitID)) | ||
| } | ||
| numSyntacticResults = len(syntacticResult.Matches) | ||
| remainingCount = remainingCount - numSyntacticResults | ||
| previousSyntacticSearch = core.Some(prevSearch) | ||
| if cursor.IsSyntactic() && !provsForSCIPData.Syntactic { | ||
| cursor = codenav.UsagesCursor{CursorType: codenav.CursorTypeSearchBased} | ||
| } |
There was a problem hiding this comment.
I think it would be better to advance the cursor directly from Precise -> Done, or from Syntactic -> Done instead of setting it and then immediately skipping a branch. It seems weird for the cursor to get into a syntactic state when provsForSCIPData.Syntactic is false.
I understand that this branch here is basically handling the cursor being set to CursorTypeSyntactic inside the UnwrapOr, but I think it would be better to introduce a separate function and use that, something like:
func (ty CursorType) AdvanceCursor(nextCursor core.Option[UsagesCursor], provenances ForEachProvenance[bool]) (newCursor UsagesCursor) {
if next, ok := nextCursor.Get() {
return next
}
if ty.IsPrecise() && provenances.Syntactic {
return UsagesCursor{CursorType: codenav.CursorTypeSyntactic}
} else if (ty.IsPrecise() || ty.IsSyntactic()) && provenances.SearchBased {
return UsagesCursor{CursorType: codenav.CursorTypeSearchBased}
} else {
return UsagesCursor{CursorType: codenav.CursorTypeDone}
}
}
Or you could have func (ty CursorType) NextCursor(provenances ForEachProvenance[bool]) UsagesCursor and call that inside UnwrapOr.
|
I tried different variants of skipping directly to the next cursor state but always ended up duplicating logic somewhere. I'm pretty happy with the current solution because it basically lets you drop any Cursor anywhere in the control-flow and things will come out right. I'll merge like this for now, but am happy to revisit this, if it ends up causing problems the next time we need to change this logic. |
I don't see how the point about code duplication applies here? My suggestion was about consolidating the state transition logic into a single place. Why would that incur extra duplication?
I don't think this is quite true (if the syntactic logic somehow incorrectly returned a precise cursor, then we'd skip search-based because the check is for I'd rather have the code fail loudly (in this case, we don't want to I don't think the state transitions here are that complicated, so it's not a big deal, but I'm surprised by you pushing back on an alternate suggestion which avoids getting the cursor into an illegal state. I get your point about us changing the traversal in the future, but if that's the case, wouldn't it be better to have the state transitions in one place rather than in multiple places? |
I couldn't make it work with a single transition function that also allows accepting the initial cursor. But I've come around to having an
True, I was thinking about the initial cursor here, the "anywhere in the control flow" was nonsense. Still that's where I ended up having to debug while implementing this. I took your suggestion and ran with it here: https://github.com/sourcegraph/sourcegraph/pull/64321 |
This change should not change any behaviour, but it prepares for syntactic/search-based pagination.
Both syntactic and search based usages now return a "NextCursor", which is
Someif there are more results of their provenance to be had. For now they always returnNonewhich preserves the current behaviour. The logic inroot_resolverimplements a state machine that transitions the cursor through the provenances and makes sure to skip provenances if the cursor in the request starts a certain state.Reviewer hint: I wouldn't bother with the diff view for
root_resolver.goTest plan
Tests continue to pass, no behavioral changes