Correctly re-map source ranges in new SCIP-based APIs#63630
Conversation
|
I'll take this PR over and start pushing commits onto the branch. |
ba32a70 to
4f062f4
Compare
a8c24b3 to
78f6262
Compare
I'm not worried about the "performance" hit here
We want usages at the commit the user requested, rather than at the commit we indexed
78f6262 to
c8e271e
Compare
|
I ran some manual tests and see occurrences being shifted accordingly. Because this PR doesn't really change any of the GitTreeTranslator code, I'm assuming that to be correct for now. |
6774bab to
c457861
Compare
| // This Range is the range of the symbol in the requested commit, | ||
| // which can be different from the range of the symbol in the syntactic | ||
| // upload, iff it was adjusted by the git tree translator. | ||
| Range scip.Range |
There was a problem hiding this comment.
Shouldn't Range be equal to scip.NewRangeUnchecked(occ.Range)? I'm not sure why we need to record it separately here? 🤔
We should be updating the range on the Occurrence value, right? Similar to the other API?
There was a problem hiding this comment.
It won't be, because this call is fetching from lsifStore.SCIPDocument so it returns the unmapped ranges. I'm a bit worried about modifying the occurrence in place, because if there is any caching anywhere we might end up remapping the location twice.
I'll change the SyntacticMatch type to not return *scip.Occurrence and instead return symbol and range separately.
This assumption keeps biting us again and again. 🙈 I think aiming for "not more incorrect than before" is good enough for now. 😆😭
That's OK. I would also support introducing wrapper types such as |
This way we don't have two conflicting ranges around
This PR fixes the following:
(Fixes https://linear.app/sourcegraph/issue/GRAPH-705)
search-based and syntactic usagesForSymbol implementations
Throwing this PR up in its current state as I think adding the bulk conversion
API will be a somewhat complex task, so we should split them into separate
PRs anyways, and I don't have time to continue working on this right now.
TODO:
Some design notes:
which is why I chose to create a smaller UploadSummary type and decided to pass
around GitTreeTranslator as that is the minimal thing we need to handle range re-mapping.
Test plan
TODO
Changelog