Roslyn: Ctrl+click GTD on a simple number argument creates link to type

[jinujoseph]

Reported by Mark Wilson-Thomas
Ctrl+Click Go To Definition on a simple number argument creates link to type! Not what I wanted...

[jinujoseph]

also look at https://github.com/dotnet/roslyn/issues/22786

[CyrusNajmabadi]

I'm curious what they did want to happen. That said, ctrl-click navigation on literals seems unlikely for people to do. I'm guessing it was just fast-fingering that let to a suboptimal result. At least with symbols, people can see hte value. But the value for literals is so low that it would likely be better to just suppress things entirely than show a result that won't be appreciated.

[sharwell]

I don't see why we would treat literals for this feature any differently than we treat literals for other features:

• Literals are the same as identifiers for Go To Definition
• Literals are the same as identifiers for Peek Definition
• Literals are the same as identifiers for Find References
• Literals are the same as identifiers for Quick Info

I think we can treat this issue as a "learning curve" issue given we already accepted the muscle memory breaking change (Ctrl+Click for selecting a token is no longer usable, which may have led to the discovery of this issue but would not be restored by simply fixing this issue).

[jinujoseph]

per our design discussion today:

Remove literal support from Ctrl+Click entirely (there will be no hyperlink when holding Ctrl and hovering over any literal).
The above should allow ctrl+click on a url to navigate to the webpage.

[JieCarolHu]

I'm reserving this for a new first time contributor.
Interested in trying your hand at this? Here's what you need to know:
If you are serious about working on this, add a comment here. While multiple people are allowed to participate, I'm willing to go out of my way to make sure the first contributor working on this gets their code merged into Roslyn.
I know more about this issue than I've provided here. If you get stuck, ask any questions you want and I'll fill out details. I don't want this to be a boring task, but I also don't want you to get stuck.
1) Remove literal support from Ctrl+Click entirely (there will be no hyperlink when holding Ctrl and hovering over any literal). For example, the hyperlink under the 100 should not appear when holding Ctrl in the following screenshot:

2) When the literal is an uri string like "https://www.microsoft.com/", Ctrl + Click should navigate to the webpage using default browser.
3) F12 and Right Click → Go To Definition on literal should still go to the definition of data type of the literal (no change from current behavior)

Additional information:
1) Roslyn getting started instructions:
https://github.com/dotnet/roslyn/wiki
2) Source file where the change needs to be made:
http://source.roslyn.io/#Microsoft.CodeAnalysis.Workspaces/FindSymbols/SymbolFinder.cs
3) Source file where the test(s) for this change will go:
http://source.roslyn.io/#Roslyn.Services.Editor2.UnitTests/NavigableSymbols/NavigableSymbolsTest.vb
http://source.roslyn.io/#Roslyn.Services.Editor2.UnitTests/GoToDefinition/GoToDefinitionTests.vb

[dwalleck]

I'd be interesting in grabbing this one, though I might not be able to start until this evening (juggling a newborn!). I'll get everything setup and jump back with the inevitable questions.

[JieCarolHu]

@dwalleck Thanks Daryl! Please do not hesitate to reply to this thread if you run into any issues or need more information etc.

[Neme12]

Is that really the only file that has to be changed?

[sharwell]

@Neme12 It's not necessarily the only file that needs to change, but contains the central functionality relevant to the feature. Since the code base is very large, we like to provide a starting point for the investigation as a new contributor won't otherwise know where to look.

[Neme12]

I made this work by adding a parameter to AbstractGoToDefinitionSymbolService.GetSymbolAndBoundSpanAsync and using a different option in AbstractGoToDefinitionService vs AbstractGoToSymbolService but I'm not sure if this is the right approach and what things other than literals it might affect. I didn't touch SymbolFinder at all.

[sharwell]

@Neme12 That sounds almost exactly like the approach I saw someone internally propose. A good hint for @dwalleck.

I see you are also a potential first-time contributor. If you'd like, I could make a recommendation for an issue you could take on. 😄

[Neme12]

@sharwell I'd love to give it a try but I'm a little discouraged by the fact that it takes me about 10 minutes to load the whole solution including all the package restores until VS becomes at least a little responsive, and even then being VS extremely slow, frequently freezing and sometimes just giving up entirely and hanging until I kill it manually, or the time it takes to build & run (I can't even dream of debugging) or the 90 minutes it took the tests to run from the command line.

[sharwell]

@Neme12

I'd love to give it a try but I'm a little discouraged by the fact that it takes me about 10 minutes to load the whole solution including all the package restores until VS becomes at least a little responsive, and even then being VS extremely slow, frequently freezing and sometimes just giving up entirely and hanging until I kill it manually

This is pretty much my focus, and I'm not the only one working to fix this. 😄

Here are some tips to improve the experience in the meantime:

1. Use 15.6 Preview 3 (the latest public preview)
2. Make sure to start working with one of the recent branches (e.g. master) - we recently disabled the NuGet restore in the IDE while we improve its performance
3. Disable source-based test discovery - this should be fixed for 15.6 Preview 4

If you have performance problems after the above steps, please let us know by following the steps here:
https://github.com/dotnet/roslyn/wiki/Reporting-Visual-Studio-crashes-and-performance-issues#performance-issues

or the 90 minutes it took the tests to run from the command line.

@jaredpar Does this sound normal for the current state of things?

[jaredpar]

90 minutes from the command line is way too high. Should be around 13-15 minutes on a decent dev machine 20 on a slower one. Unit Tests should never drift anywhere near the 90 minute mark.

[sharwell]

@Neme12 How were you running the tests that took 90 minutes?

[Neme12]

@sharwell By running Test.cmd. I guess my laptop is just too slow, I wouldn't call it a decent dev machine. (I never thought I'd need so much processing power for writing code)

Is there a way to install the 15.6 preview as an update rather than side by side?
EDIT: Never mind. The installation was a lot faster and smaller than I expected.

[dwalleck]

Sorry I'm dragging on this. Who thought coding with a newborn around would be difficult? :-) I have a few questions that I'm trying to find the time to put together in sensible form. For the sake of expediency, if I can't get this knocked out in the next day or two would it make sense to let @Neme12 proceed with their solution?

[Neme12]

The first half of the tests takes 10 minutes and the rest takes at least another hour - they seem to get progressively slower. Is that normal? What's weird is that the test summary shows a run time of only about 12 minutes and a finish time an hour earlier (as if it only took 12 minutes). When running them, they take up so much CPU usage that the whole OS struggles to stay responsive, so the problem must be with me and not Visual Studio.

I also get 2 failures related to localization, which is unexpected since I have everything on my machine set to en-US except for timezone & date formats, but the failure contains a localized error message for some reason, nothing related to date formatting... (why would it care about that setting?). I've never seen these localized messages and didn't know they existed. I even lie to my OS about my location being in the US.

@dwalleck
I just wanted to try it out myself but I'm not gonna steal this one from you, I'm sorry if it seemed like I'm putting pressure on you. I think if it was urgent they would have done it themselves in the first place?

[sharwell]

@dwalleck My suggestion is to take your time, but if you get to a point where you don't think you'll get to it then let us know. Currently @Neme12 is helping us with another issue (difficulty working in the project), plus we can always find new issues to help people get started whenever. The big advantage for these issues reserved for new contributors is removing pressure knowing there's no way a new user will
work as quickly as an established contributor. 😄

[Neme12]

@sharwell
I tried working on another issue (#20983) in the 15.6 preview and after implementing it, when I run I get some errors and exceptions. First I thought I probably caused this by my changes but the errors remain when I go back to the previous commit. Is this because of the preview?

This one when loading a project but only when debugging and can be easily skipped:

Managed Debugging Assistant 'LoaderLock' 
  Message=Managed Debugging Assistant 'LoaderLock' : 'Attempting managed execution inside OS Loader lock. Do not attempt to run managed code inside a DllMain or image initialization function since doing so can cause the application to hang.'

Microsoft.VisualStudio.LanguageServices.dll!Microsoft.VisualStudio.LanguageServices.Implementation.LanguageService.AbstractPackage<Microsoft.VisualStudio.LanguageServices.CSharp.LanguageService.CSharpPackage, Microsoft.VisualStudio.LanguageServices.CSharp.LanguageService.CSharpLanguageService>.Initialize.AnonymousMethod__9_0() Line 51
    at C:\Users\neme1\Source\Repos\roslyn\src\VisualStudio\Core\Def\Implementation\LanguageService\AbstractPackage`2.cs(51)

but this one happens all over the place when I open a file and prevents me from doing anything:

Microsoft.VisualStudio.Composition.CompositionFailedException
  HResult=0x80131500
  Message=Expected 1 export(s) with contract name "Microsoft.VisualStudio.Text.Editor.Commanding.IEditorCommandHandlerServiceFactory" but found 0 after applying applicable constraints.
  Source=Microsoft.VisualStudio.Composition
  StackTrace:
   at Microsoft.VisualStudio.Composition.ExportProvider.GetExports(ImportDefinition importDefinition)
   at Microsoft.VisualStudio.Composition.ExportProvider.GetExports[T,TMetadataView](String contractName, ImportCardinality cardinality)
   at Microsoft.VisualStudio.Composition.ExportProvider.GetExport[T,TMetadataView](String contractName)
   at Microsoft.VisualStudio.Composition.ExportProvider.GetExport[T](String contractName)
   at Microsoft.VisualStudio.Composition.ExportProvider.GetExport[T]()
   at Microsoft.VisualStudio.Composition.ExportProvider.GetExportedValue[T]()
   at Microsoft.VisualStudio.ComponentModelHost.ComponentModel.GetService[T]()
   at Microsoft.VisualStudio.Editor.Implementation.CommandHandlerServiceAdapter..ctor(ITextView textView, ITextBuffer subjectBuffer, IOleCommandTarget nextCommandTarget)
   at Microsoft.VisualStudio.Editor.Implementation.CommandHandlerServiceFilter.EnsureCommandHandlerServiceAdapter()
   at Microsoft.VisualStudio.Editor.Implementation.CommandHandlerServiceFilter.QueryStatus(Guid& pguidCmdGroup, UInt32 cCmds, OLECMD[] prgCmds, IntPtr pCmdText)
   at Microsoft.VisualStudio.Editor.Implementation.CommandChainNode.InnerQueryStatus(Guid& pguidCmdGroup, UInt32 cCmds, OLECMD[] prgCmds, IntPtr pCmdText)
   at Microsoft.VisualStudio.Editor.Implementation.CommandChainNode.QueryStatus(Guid& pguidCmdGroup, UInt32 cCmds, OLECMD[] prgCmds, IntPtr pCmdText)
   at Microsoft.VisualStudio.LanguageServices.Implementation.AbstractOleCommandTarget.QueryVisualStudio2000Status(Guid& pguidCmdGroup, UInt32 commandCount, OLECMD[] prgCmds, IntPtr commandText) in C:\Users\neme1\Source\Repos\roslyn\src\VisualStudio\Core\Def\Implementation\AbstractOleCommandTarget.Query.cs:line 208
   at Microsoft.VisualStudio.LanguageServices.Implementation.AbstractOleCommandTarget.QueryStatus(Guid& pguidCmdGroup, UInt32 commandCount, OLECMD[] prgCmds, IntPtr commandText) in C:\Users\neme1\Source\Repos\roslyn\src\VisualStudio\Core\Def\Implementation\AbstractOleCommandTarget.Query.cs:line 30
   at Microsoft.VisualStudio.Editor.Implementation.CommandChainNode.InnerQueryStatus(Guid& pguidCmdGroup, UInt32 cCmds, OLECMD[] prgCmds, IntPtr pCmdText)
   at Microsoft.VisualStudio.Editor.Implementation.SimpleTextViewWindow.QueryStatus(Guid& pguidCmdGroup, UInt32 commandCount, OLECMD[] prgCmds, IntPtr commandText)
   at Microsoft.VisualStudio.Editor.Implementation.CompoundTextViewWindow.QueryStatus(Guid& pguidCmdGroup, UInt32 cCmds, OLECMD[] prgCmds, IntPtr pCmdText)
   at Microsoft.VisualStudio.Platform.WindowManagement.DocumentObjectSite.QueryStatus(Guid& pguidCmdGroup, UInt32 cCmds, OLECMD[] prgCmds, IntPtr pCmdText)
   at Microsoft.VisualStudio.Platform.WindowManagement.WindowFrame.QueryStatus(Guid& pguidCmdGroup, UInt32 cCmds, OLECMD[] prgCmds, IntPtr pCmdText)

Microsoft.VisualStudio.LanguageServices.dll!Microsoft.VisualStudio.LanguageServices.Implementation.AbstractOleCommandTarget.QueryVisualStudio2000Status(ref System.Guid pguidCmdGroup, uint commandCount, Microsoft.VisualStudio.OLE.Interop.OLECMD[] prgCmds, System.IntPtr commandText) Line 208
    at C:\Users\neme1\Source\Repos\roslyn\src\VisualStudio\Core\Def\Implementation\AbstractOleCommandTarget.Query.cs(208)

and it brings me to some code that looks more like C-half#.

This does not happen with my regular 15.5.5 installation (not even with my changes, they seem to be working correctly). Can I somehow debug the regular installation from the preview to use for development? I was able to do that by hardcoding executablePath in launchSettings.json.

[isc30]

Hi, I want to start contributing and getting familiar with the project. Is this a good starting task?

[sharwell]

@dwalleck Are you still working on this?

@isc30 I would be glad to find you another starting item. Or if @dwalleck is not working on this then it would be good. While we wait for an answer, you can go through the instructions to make sure you have the code checked out and building on your machine: https://github.com/dotnet/roslyn/blob/dev15.7.x/docs/contributing/Building,%20Debugging,%20and%20Testing%20on%20Windows.md#developing-with-visual-studio-2017 😄

[isc30]

@sharwell Thanks a lot for the information! After following the steps I always get this Test result:

0 running, 0 queued, 52 completed, 20 failures

Is it normal?

[JieCarolHu]

@isc30 Could you start working on this issue as we haven't heard back from @dwalleck
Thanks!

[sharwell]

@isc30 If your default operating system language is something other than en-US, then than is currently expected, but we are working to fix it. It should still be possible to run individual tests from Test Explorer in Visual Studio 2017 version 15.6 Preview 4 and newer.

[dwalleck]

@sharwell @JieCarolHu Sorry about the communication blackout. Picking up a new task with a newborn was bad planning on my part :-) If there's any other first timers issues in the future, I'd definitely be interested in picking one up

[isc30]

Thanks @dwalleck , then I'm officially working on it!

[isc30]

I managed to fix it. Some of the tests I introduced aren't working as expected:

• TestCSharpLiteralGoToDefinition
• TestCSharpStringLiteralGoToDefinition
• TestVisualBasicLiteralGoToDefinition
• TestVisualBasicStringLiteralGoToDefinition

I need some assistance to know if I'm going in the proper direction and some clue about why those tests are failing (with manual testing the cases are working well)

[JieCarolHu]

change it to 15.8 as we are in escrow for 15.7 now