Add and populate new SCIP SymbolInformation fields#677
Closed
nicolas-guichard wants to merge 9 commits intosourcegraph:mainfrom
Closed
Add and populate new SCIP SymbolInformation fields#677nicolas-guichard wants to merge 9 commits intosourcegraph:mainfrom
nicolas-guichard wants to merge 9 commits intosourcegraph:mainfrom
Conversation
This copies the latest additions to the Document message: * a new language field and Language enum * a new text field, to embed the document content itself. This is meant for the new SymbolInformation::signature_documentation field. This also updates some documentation comments.
This copies the latest additions to the SymbolInformation message: * the documentation field is explicitly not meant for signature documentation anymore, instead a new signature_documentation field is added * a new display_name field is added * a new enclosing_symbol field is added for local symbols * a new kind field is added along with a Kind enum to have a finer-grained classification than the one provided by descriptor suffixes (and is especially useful for local symbols which don't have suffixes)
The SemanticDB schema already provides a display_name field, forward it to the SCIP output in scip-semanticdb. This also adds support to the ScipPrinters testing utility and updates the tests accordingly.
SemanticDB provides a structured version of the signature in the signature field. Instead of turning it into a markdown-encoded string for the documentation field, this builds a Document for the signature_documentation field. This also updates the ScipPrinters testing utility and the tests accordingly.
SemanticDB used to have a SymbolInformation::owner field with id 15. This re-introduces the field with the same semantics under the name enclosing_symbol. To be able to re-use the field 15, this moves the out-of-spec definition_relationships field to id 21.
This also adds support to the ScipPrinters testing utility.
This only populates the enclosing_symbol for local symbols, and updates the tests accordingly.
This also updates the ScipPrinters testing utility and the tests accordingly.
Contributor
Author
nicolas-guichard
left a comment
nicolas-guichard
left a comment
There was a problem hiding this comment.
Pointing to some things that I believe should be fixed upstream.
Comment on lines
44
to
+47
| // ^^^ definition semanticdb maven maven/com.lihaoyi/ujson_2.13 1.4.0 ujson/BaseByteRenderer#out. | ||
| // documentation ```scala\nprivate[this] val out: T\n``` | ||
| // display_name out | ||
| // signature_documentation scala private[this] val out: T | ||
| // kind Method |
Contributor
Author
There was a problem hiding this comment.
This for instance should be a Field rather than a Method, but I believe this is due to a bug in semanticdb-scalac rather than scip-semanticdb.
Contributor
There was a problem hiding this comment.
I'm just going to confer with the council of Semanticdb experts to see if this is intentional due to Scalac class encoding, or accidental.
| internal class ActivityRecyclerPool { | ||
| // ^^^^^^^^^^^^^^^^^^^^ definition semanticdb maven . . com/airbnb/epoxy/ActivityRecyclerPool# | ||
| // display_name ActivityRecyclerPool | ||
| // documentation ```kt\ninternal final class ActivityRecyclerPool\n``` |
Contributor
Author
There was a problem hiding this comment.
In kt files the signature documentation remains in documentation for now because semanticdb-kotlinc does not provide the structured signature field and adds the signature to the SematicDB documentation field already.
Contributor
|
@nicolas-guichard thanks for the PR! I looked at some of the snapshots and the changes look great. Before I review the implementation, could you please sync this PR with main? I don't see anything controversial in the PR so far so hopefully we can get this merged and release soon. Alternatively, I can take over the PR and sync it myself. Let me know. In the background I will investigate the semanticdb scalac issue you highlighted - but it won't block this PR as I can reproduce it outside of scip-java. |
Contributor
|
Thanks for the PR! I moved the commits on top of master in separate PR #707 along with a couple of fixes. We will likely need to wait before releasing this, until Sourcegraph product itself can support signature_documentation (given it's removed from |
keynmol
added a commit
that referenced
this pull request
Jun 19, 2024
…play_name (#677 #707) * Update Document from upstream scip.proto This copies the latest additions to the Document message: * a new language field and Language enum * a new text field, to embed the document content itself. This is meant for the new SymbolInformation::signature_documentation field. This also updates some documentation comments. * Update SymbolInformation from upstream scip.proto This copies the latest additions to the SymbolInformation message: * the documentation field is explicitly not meant for signature documentation anymore, instead a new signature_documentation field is added * a new display_name field is added * a new enclosing_symbol field is added for local symbols * a new kind field is added along with a Kind enum to have a finer-grained classification than the one provided by descriptor suffixes (and is especially useful for local symbols which don't have suffixes) * Forward display_name from SemanticDB to SCIP The SemanticDB schema already provides a display_name field, forward it to the SCIP output in scip-semanticdb. This also adds support to the ScipPrinters testing utility and updates the tests accordingly. * Move signature documentation to its new dedicated field SemanticDB provides a structured version of the signature in the signature field. Instead of turning it into a markdown-encoded string for the documentation field, this builds a Document for the signature_documentation field. This also updates the ScipPrinters testing utility and the tests accordingly. * Add back SemanticDB SymbolInformation::owner field SemanticDB used to have a SymbolInformation::owner field with id 15. This re-introduces the field with the same semantics under the name enclosing_symbol. To be able to re-use the field 15, this moves the out-of-spec definition_relationships field to id 21. * Forward enclosing_symbol from SemanticDB to SCIP This also adds support to the ScipPrinters testing utility. * Populate SymbolInformation::enclosing_symbol in semanticdb-javac This only populates the enclosing_symbol for local symbols, and updates the tests accordingly. * Build SCIP kind from SematicDB kind and properties This also updates the ScipPrinters testing utility and the tests accordingly. * semanticdb-javac: set kind to Variable for local variables --------- Co-authored-by: Nicolas Guichard <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #666
Note that the result can only be as good as what the SemanticDB data provides, notably:
kindandpropertiesfields inSymbolInformationscip-kotlin#66Test plan
This adds the new fields to the snapshot tests and makes sure they pass.
I have obviously not manually reviewed all 13054 insertions and 3893 deletions to tests/snapshots, but what I think is a reasonable subset.