Add Visual Basic support #26

sagi1623 · 2023-05-22T22:53:19Z

Previously, scip-dotnet only supported C#. This PR adds support for Visual Basic as well. We are able to reuse a lot of code from the C# indexer by extracting shared logic related to formatting symbols.

olafurpg

@sagi1623 Thank you so much for this contribution! It's fantastic that we can support Visual Basic with so much code reuse. Nice job porting all the tests from C# to Visual Basic 👏🏻

Only minor suggestions, I am happy to merge this PR once those are addressed.

olafurpg · 2023-05-23T04:29:40Z

ScipDotnet/ScipIndexer.cs

+                var walker = new ScipCSharpSyntaxWalker(symbolFormatter, semanticModel);
+                walker.Visit(root);
+            }
+            else


It might make sense to guard against this here

Suggested change

else

else if (language == "Visual Basic")

olafurpg · 2023-05-23T04:30:44Z

ScipDotnet/ScipVisualBasicSyntaxWalker.cs

+/// <summary>
+/// Walks a single VisualBasic syntax tree and produces a SCIP <code>Document</code>.
+/// </summary>
+public class ScipVisualBasicSyntaxWalker : VisualBasicSyntaxWalker


Impressive how much code reuse we can do between C# and Visual Basic!

olafurpg · 2023-05-23T04:34:29Z

ScipDotnet/ScipSymbolFormatter.cs

+        _doc = doc;
+        _options = options;
+        _globals = globals;
+        _languagePrefix = _doc.Language == "C#" ? "cs" : "vb";


nit: I had to check how this variable was used to understand what it does, this name might be clearer

Suggested change

_languagePrefix = _doc.Language == "C#" ? "cs" : "vb";

_markdownCodeFenceLanguage = _doc.Language == "C#" ? "cs" : "vb";

olafurpg · 2023-05-23T04:46:51Z

ScipDotnet/ScipIndexer.cs

@@ -13,10 +13,8 @@ namespace ScipDotnet;
 /// </summary>
 public class ScipIndexer


Suggested change

public class ScipIndexer

public class ScipProjectIndexer

olafurpg · 2023-05-23T04:47:31Z

ScipDotnet/ScipSymbolFormatter.cs

+/// <summary>
+/// Creates SCIP <code>Document</code> based on provided symbols.
+/// </summary>
+public class ScipSymbolFormatter


My initial interpretation of the name ScipSymbolFormatter was that it formats SCIP symbols like this interface we have in Go https://sourcegraph.com/github.com/sourcegraph/scip/-/blob/bindings/[go/scip/symbol_formatter.go?L10](https://www.golinks.io/scip/symbol_formatter.go?L10?trackSource=github)

I propose we rename this class into something else to make it clearer that this class does more than just format symbols

Suggested change

public class ScipSymbolFormatter

public class ScipDocumentIndexer

I also propose we rename ScipIndexer into ScipProjectIndexer

sagi1623 · 2023-05-24T21:16:09Z

@olafurpg,
I have applied the suggestions you had, and I have also added in the last commit, support for indexing individual visual basic projects.
Now I would have a few other questions for you if possible:

I have tried to follow the guide for adding support for a new language, however my windows is misbehaving and I am unable to install sg. Because of that I would only be able to open the PRs blindly without testing, which I am not super comfortable with. May I ask you or somebody from SourceGraph to do that part?
As VisualBasic declared member names are case-insensitive, it can happen that we index for example a parameter as definition scip-dotnet nuget . . VBMain/SomeClass#Method().(SomeParameter) and later reference as reference scip-dotnet nuget . . VBMain/SomeClass#Method().(SoMePaRaMetEr).
My question here is if SCIP symbol names are case sensitive or not? Will the engine know that it is the same symbol or not?
In case it is case sensitive, do you think it is at indexing that we need to handle this (for example convert the names to lower) or the engine should take care about it when loading the code intel? (I guess it depends if there are other languages with the same behavior)

olafurpg · 2023-05-25T05:22:37Z

May I ask you or somebody from SourceGraph to do that part?

Do you have a ready diff I can test locally? I'm happy to ensure we get Visual Basic support added.

My question here is if SCIP symbol names are case sensitive or not? Will the engine know that it is the same symbol or not?

Good question. SCIP symbols are case sensitive so I recommend we lowercase symbols during indexing time.

sagi1623 · 2023-05-27T14:39:10Z

Do you have a ready diff I can test locally? I'm happy to ensure we get Visual Basic support added.

Not yet, I will not have time next week to work on it. If you don't mind waiting, I can try to do it the week after and send you the PRs for testing and review?

Good question. SCIP symbols are case sensitive so I recommend we lowercase symbols during indexing time.

Do you think we could merge this PR to limit the scope of it and I can create a new one some time later to tackle the problem of case insensitivity?

olafurpg · 2023-05-28T06:49:30Z

Not yet, I will not have time next week to work on it. If you don't mind waiting, I can try to do it the week after and send you the PRs for testing and review?

Sounds good. FYI, I'm doing cross-Atlantic travel for work June 4-7th so I may be slow to respond that week.

Do you think we could merge this PR to limit the scope of it and I can create a new one some time later to tackle the problem of case insensitivity?

That's fine. I am OK with merging this PR once the CI is green. It might help to rebase on top of main now that the flaky tests have been fixed thanks to your PR #27 😄

olafurpg

LGTM 👍🏻 Thank you so much for this contribution! This is a great improvement 😄

olafurpg · 2023-06-05T19:30:01Z

ScipDotnet.Tests/SnapshotTests.cs

@@ -248,7 +253,7 @@ private string FormatDocument(Index index, Document document)
                if (isDefinition)
                {
                    var info = symtab.GetValueOrDefault(occurrence.Symbol, new SymbolInformation());
-                    var prefix = "//" + indent + new String(' ', length + 1);
+                    var prefix = commentChar + indent + new String(' ', length + 1);


Alternative solution, not a request to change, just thinking out loud

``suggestion
var prefix = commentChar + indent + new String(' ', length + commentChar.length - 1);

sagi1623 · 2023-06-18T11:16:19Z

Hello @olafurpg,

Coming back to our discussion regarding VbNet being case insensitive.
Actually it was a false alarm from me (sorry for that).
After revisiting the topic and testing it a bit it I realised that there is nothing to do, it works out of the box.
As we work with the semantic model and retrieve the symbols from there, it will always contain the fully defined name of the symbol as declared.
For example:
Public Sub New(ByVal wEiRdCaSiNg As Long) -> The definition will be VBMain/Classes#.ctor().(wEiRdCaSiNg)
And if it is used later with a different casing like Property = WeIrDcAsInG the retrieved symbol will still have the original wEiRdCaSiNg name so the reference will be -> VBMain/Classes#.ctor().(wEiRdCaSiNg)

olafurpg · 2023-06-18T14:47:19Z

@sagi1623 good to confirm that. Should we update the indexer to emit case sensitive names?

sagi1623 · 2023-06-18T16:24:22Z

@sagi1623 good to confirm that. Should we update the indexer to emit case sensitive names?

Could you please clarify what do you mean by that?

I think that the current behaviour is correct. I mean C# is case sensitive so with whichever casing you declare a symbol you will need to reference it with the same one. For VbNet that is not the case, if you declare the symbol with one casing, you can reference it with another one. However, this difference is not syntax level. On a semantic level it will be the same symbol.
As Roslyn will give the name from the symbol, which got it from the declaration, we will always use the same casing trough the whole indexing process and I think because of this everything will work as expected once the indexing results are uploaded.

Am I missing something?

And I will soon add a new case in the snapshot project, that will document the current behaviour and should act as a safety net in case the implementation changes in the future.

olafurpg · 2023-06-18T19:44:24Z

I thought we used all-lowercase for VB symbols but looking at the original discussion it seems like we left this change for a future PR. I agree the current behavior is correct and there's nothing we need to do 👍🏻

sagi1623 · 2023-06-19T20:22:42Z

I thought we used all-lowercase for VB symbols but looking at the original discussion it seems like we left this change for a future PR. I agree the current behavior is correct and there's nothing we need to do 👍🏻

Yes we said it will come in a future PR.
Once again sorry for creating the confusion.
I have created a PR to document the behaviour and to have a test case for the future.

sagi1623 mentioned this pull request May 22, 2023

Fix Unit test cases #25

Merged

olafurpg suggested changes May 23, 2023

View reviewed changes

olafurpg reviewed May 23, 2023

View reviewed changes

sagi1623 force-pushed the AraGimLeg/VisualBasic-support branch from ab76842 to c9f4c57 Compare May 24, 2023 20:56

sagi1623 added 14 commits June 4, 2023 13:27

Add VisualBasic support

ec1c4e4

Update Readme :)

0714118

Remvoe useless empty line from C# test case

6ff326d

Add VB test cases

9b8f558

Update expected snapshots for VB

318ef19

Address review comments

821ca74

Improve help message and cover case when indexing individual .vbproj

4a58bb1

Fix formatting

6804065

Fix formatting of VB code

9d51851

Update output snapshot

63324cc

Use expression bodied method wherever possible

6d91f3a

Fix formatting

fc1e3e6

Fix formatting for what I missed in the first round

f793ec8

Fix after rebase

5ed9b04

sagi1623 force-pushed the AraGimLeg/VisualBasic-support branch from 2f9532f to 5ed9b04 Compare June 4, 2023 12:47

olafurpg approved these changes Jun 5, 2023

View reviewed changes

olafurpg merged commit 0b7d9ca into sourcegraph:main Jun 5, 2023

sagi1623 deleted the AraGimLeg/VisualBasic-support branch June 6, 2023 09:26

This was referenced Jun 15, 2023

Ctags support for visual basic .net sourcegraph/go-ctags#11

Draft

Create language specification for VbNet sourcegraph/sourcegraph-public-snapshot#53578

Open

	_languagePrefix = _doc.Language == "C#" ? "cs" : "vb";
	_markdownCodeFenceLanguage = _doc.Language == "C#" ? "cs" : "vb";

		@@ -13,10 +13,8 @@ namespace ScipDotnet;
		/// </summary>
		public class ScipIndexer

	public class ScipSymbolFormatter
	public class ScipDocumentIndexer

Add Visual Basic support #26

Add Visual Basic support #26

Uh oh!

Conversation

sagi1623 commented May 22, 2023 • edited by olafurpg Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

olafurpg left a comment

Choose a reason for hiding this comment

Uh oh!

olafurpg May 23, 2023

Choose a reason for hiding this comment

Uh oh!

olafurpg May 23, 2023

Choose a reason for hiding this comment

Uh oh!

olafurpg May 23, 2023

Choose a reason for hiding this comment

Uh oh!

olafurpg May 23, 2023

Choose a reason for hiding this comment

Uh oh!

olafurpg May 23, 2023 • edited by golinks bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sagi1623 commented May 24, 2023

Uh oh!

olafurpg commented May 25, 2023

Uh oh!

sagi1623 commented May 27, 2023

Uh oh!

olafurpg commented May 28, 2023

Uh oh!

olafurpg left a comment

Choose a reason for hiding this comment

Uh oh!

olafurpg Jun 5, 2023

Choose a reason for hiding this comment

Uh oh!

sagi1623 commented Jun 18, 2023

Uh oh!

olafurpg commented Jun 18, 2023

Uh oh!

sagi1623 commented Jun 18, 2023

Uh oh!

olafurpg commented Jun 18, 2023

Uh oh!

sagi1623 commented Jun 19, 2023

Uh oh!

Uh oh!

sagi1623 commented May 22, 2023 •

edited by olafurpg

Loading

olafurpg May 23, 2023 •

edited by golinks bot

Loading