feat: Add SymbolRole for representing forward declarations #217
Conversation
varungandhi-src
changed the title
| @@ -0,0 +1,8 @@ | |||
| reference forward_definition abc# | |||
There was a problem hiding this comment.
nit: I would expect the repro syntax to work like this
| reference forward_definition abc# | |
| forward_definition abc# |
There was a problem hiding this comment.
I put it inside reference so that it's clearer that it is also a reference, not a tri-state of definition/reference/forward definition.
There was a problem hiding this comment.
Should we update the docstring to emphasize that you should set Definition and ForwardDefinition? It might be easy to misunderstand that setting ForwardDefinition alone implies Definition
There was a problem hiding this comment.
No, that would be incorrect. You should NOT set Definition if you set ForwardDefinition because a ForwardDefinition is actually a reference, it is not a Definition because it doesn't have a body.
I've added a lint for this in the scip lint command and mentioned the above requirement in the doc comment.
There was a problem hiding this comment.
It is a definition IMO because it's a valid destination for "Go to definition". When I trigger "Go to implementation" then I would expect it to go to the non-forward definition.
There was a problem hiding this comment.
If you still disagree, let's discuss it synchronously some time next week. It would be helpful for me to show some live examples of code nav behavior in C++.
There was a problem hiding this comment.
I trust your call on this. It might be worth renaming from ForwardDefinition if this is not technically a definition in your opinion.
There was a problem hiding this comment.
Or updating the docstring to emphasize that Definition should not be set
There was a problem hiding this comment.
Ugh, I thought I updated the docstring earlier, but I must've stashed that change instead of merging it. 🤦🏽
Yes, I'll do that.
There was a problem hiding this comment.
In my mental model, it is a definition with lower priority. If you have two ambiguous definitions and one has the ForwardDefinition role then we can discard it. I would rather use that application logic than requiring the client to treat a non-definition (which is confusingly named ForwardDefinition) as a definition
|
@olafurpg Can I merge? @varungandhi-src said he's going to be out rest of day and asked wehther I could merge this. |
|
No problem merging, the comment was only minor |
Adds a new
SymbolRolefor forward declarations so that certain occurrences can be markedwith this role for C-based languages, optionally allowing clients such as a web UI to filter out
or down-rank them.
Question:
.mlifiles. Should we change the nameForwardDeclarationto be something more generic else to account for that too? Should we add a separateSymbolRolefor that? Deal with it later instead of right now? -- Renamed toForwardDefinition.Fixes #131
Related PR: https://github.com/sourcegraph/sourcegraph/pull/58082
Test plan
I've added an optional
forward_declfield to reprolang along with a snapshot test for it.