[#23030] Disable literal Ctrl+Click navigation #24910

isc30 · 2018-02-17T04:30:23Z

📝 By @sharwell: These tests now pass per #24910 (comment)

TestCSharpLiteralGoToDefinition
TestCSharpStringLiteralGoToDefinition
TestVisualBasicLiteralGoToDefinition
TestVisualBasicStringLiteralGoToDefinition

Bugs this fixes

#23030

Root cause analysis

Added tests for both C# and VB covering SymbolNavigation and GoToDefinition for literals

dnfclas · 2018-02-17T04:30:34Z

All CLA requirements met.

CyrusNajmabadi · 2018-02-17T04:39:32Z

src/EditorFeatures/Core/GoToDefinition/AbstractGoToDefinitionSymbolService.cs

+
+            return includeLiterals
+                ? (symbol ?? semanticInfo.Type)
+                : symbol;


i don't see any check that is seeing we're on a literal or not.

Literals don't have an AliasSymbol, ReferencedSymbols, or DeclaredSymbol. Their type for navigation is determined through Type.

My assumption was: in case there is no [alias, referenced or declared] symbol it is a literal

This means no navigation on anything that doesn't have a symbol, but does have a type. I would prefer we actually just exclude literals (instread of trying back-infer that we were on a literal).

I would recommend instead doing:

var syntaxFacts = document.GetLanguageService<ISyntaxFactsService>(); var isLiteral = syntaxFacts.IsLiteral(whateverTokenYoureOn);

That would be an accurate check. Thanks!

You can do this in the code that creates the TokenSemanticInfo. And the TokenSemanticInfo can include that as a boolean in it. Or, alternatively, the TokenSemanticInfo can just contain hte token, and you can check it at the call site. Either way is fine with me.

Thanks! :)

Perfect, thanks for the comments 👍

After some investigation, I have some doubts. Token "literality": is it a semantic or syntactic information? In case you agree that is syntactic info, what do you think about adding struct TokenSyntacticInfo and providing methods to work with it? Then I can use it in this PR and we can easily extend it with more properties if future tasks require them

Token "literality" is it a semantic or syntactic information?

Currently, it's somethign that can be determined entirely syntactically.

In case you agree that is syntactic info, what do you think about adding struct TokenSyntacticInfo

I think that's overkill. TokenSemanticInfo is really just a struct that means "collect a bunch of useful information about this token for me." It doesn't mean "it can't contain anything syntactic in it".

Then I can use it in this PR and we can easily extend it with more properties if future tasks require them

Too speculative :) Right now we really don't need that. All we need to know is what token we're ok. Then ISyntaxFactsService can be trivially used to ask "Is this a literal".

CyrusNajmabadi · 2018-02-17T04:40:47Z

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)

Tagging @rchande

You're putting tests in the normal f12-goto-def test path. @rchande Do you have specific tests anywhere that validate the behavior of ctrl-click-goto-def?

sharwell · 2018-02-17T16:18:42Z

@isc30 It's a holiday weekend for the US and much (most) of the team is not working until Tuesday. I'll leave a couple notes for people to make sure we aren't any later than that in replying.

rchande · 2018-02-20T16:24:50Z

@isc30 @CyrusNajmabadi NavigableSymbolsTest is the right place to add tests that specifically test ctrl-click gtd. Looks like some were added there.

isc30 · 2018-02-20T16:32:44Z

Hi @rchande, after some investigation it looks like Test method in GoToDefinitionTests.vb doesn't work properly for built-in types (int, string in my tests)

rchande · 2018-02-20T16:34:44Z

@isc30 What doesn't work?

isc30 · 2018-02-20T16:36:53Z

@rchande in GoToDefinitionTests.vb, symbol navigation (using goto-definition) is being checked inside the Test method. As far as I have seen, it works well for user-defined types but not for built-in ones (int, string, etc). This makes my tests for literals fail. Please correct me if I'm wrong

rchande · 2018-02-20T16:44:17Z

@isc30 I haven't looked at this code in a long time, can you share your specific test failure message? thanks!

JieCarolHu · 2018-02-20T23:54:37Z

The test cases failed because it resolves IDocumentNavigationService to DefaultSymbolNavigationService instead of MockDocumentNavigationService at runtime. MockDocumentNavigationService.TryNavigateToSymbol() always return true while DefaultSymbolNavigationService.TryNavigateToSymbol() always return false.

I need to check with @sharwell to see if we need special symbol like [|symbol definition|] in the workspace document in these test cases for the tests to resolve to right DocumentNavigationService . I will get back to this thread tomorrow.

isc30 · 2018-02-21T00:23:34Z

That's a nice observation, I just updated the tests to explicitly specify the target symbol. They seem to still fail, but they look better now

JieCarolHu · 2018-02-21T04:55:47Z

I took a quick look, the workspace still resolves IDocumentNavigationService to DefaultSymbolNavigationService even after the target symbol is explicitly specified.
@sharwell Could you confirm if this following way is correct to explicitly specify the target symbol?
[|int|] x = 1$$23;

Neme12 · 2018-02-21T20:51:49Z

I have a question. When I first came across this issue I thought the underlying problem was that the ctrl+click navigation actually considers the type of the expression as a symbol to navigate to. That doesn't seem like an intuitive behavior to me given that most of the time when I use that to navigate to a symbol, it's usually a declaration or a variable, not its type.

So when I approached this issue and tried to fix it, instead of having bool includeLiterals, I had bool includeType in GetSymbolAndBoundSpanAsync. What things other than literals might that affect? Would that be an acceptable solution? It seems a lot simpler to me but more importantly, it seems to me like that's the underlying problem here - when I click on an expression I don't expect it to take me to its type. The fact that this affects literals seemed to me like just a symptom of the real issue.

isc30 · 2018-02-21T21:24:02Z

instead of having bool includeLiterals, I had bool includeType in GetSymbolAndBoundSpanAsync

@Neme12 yep, I did the same thing in the first version of this PR but we reduced the scope of the changes to only literals by removing the heuristic that tried to guess the "literality" without knowing the token itself.

Would that be an acceptable solution?

This fixes the problem described in the ticket, but it can have side-effects. The truth is that we wanted to maintain the navigation to tokens that don't have a symbol but have a type.

What things other than literals would that affect?

Not sure, maybe @CyrusNajmabadi can throw a light on this

Neme12 · 2018-02-21T21:30:48Z

The truth is that we wanted to maintain the navigation to tokens that don't have a symbol but have a type

Fair enough, if that's what the team wants (to special case this for literals). I considered the fact that this navigates to the type of an expression at all to be undesirable. It's not what I would expect personally.

CyrusNajmabadi · 2018-02-21T22:29:45Z

Not sure, maybe @CyrusNajmabadi can throw a light on this

I can't think of anything. Note: i would be ok with this being written as an 'includeType' flag. My major critique before was that hte flag was called "includeLiterals" but wasn't actually checking for literal-ness.

CyrusNajmabadi · 2018-02-21T22:30:40Z

That doesn't seem like an intuitive behavior to me given that most of the time when I use that to navigate to a symbol, it's usually a declaration or a variable, not its type.

We've commonly had people use go-to-def on things like strings so that they can easily see the entire System.String api. That's why go-to-def works on literals.

Neme12 · 2018-02-21T22:43:40Z

@CyrusNajmabadi

We've commonly had people use go-to-def on things like strings so that they can easily see the entire System.String api. That's why go-to-def works on literals.

Yes I understand (that's why the flag). I think that go to definition is a lot more "explicit" action so it's OK for it to also navigate to the type, but when holding ctrl and hovering over literals, it probably shouldn't be offered - in this case it's a lot more distracting. Seeing something underlined makes me think there's an actual definition for the value. I wouldn't expect that to be offered just for the type.

I can't think of anything either. A single token that is an expression with a type, doesn't refer to a symbol and also not a literal?

isc30 · 2018-02-21T22:44:36Z

Good to know, I focused on literals because that's what the exact ticket suggests but, to be honest, I prefer not changing the SemanticInfo etc but having the flag for including the type or not (like in the first version of the PR). I cannot think of any situation that will end up using the Symbol.Type apart from literals.

Maybe I can change it to the simple flag and leave a comment in the heuristic: "if a token has no aliased, referenced or declared symbols (mostly literals) we don't want to enable Ctrl+Click on them". What do you think?

👍 yep
👎 nope

sharwell · 2018-02-22T00:02:30Z

@isc30 Sorry I'm slow getting to this review. I've been tied up with some performance issues but I should get this reviewed tonight or tomorrow morning. 😄

isc30 · 2018-02-22T00:14:13Z

Thanks @sharwell, please don't spend too much time reviewing the implementation as I will refactor it in a short time. It would be very helpful if you could investigate the failing tests as I have no idea about the reasons

sharwell

The implementation approach seems to make sense to me. It's getting late here so I'll review the unit tests tomorrow. 😄

sharwell · 2018-02-22T00:16:54Z

src/Workspaces/Core/Portable/FindSymbols/SymbolFinder.cs

+        {
+            var syntaxTree = semanticModel.SyntaxTree;
+            var syntaxFacts = workspace.Services.GetLanguageServices(semanticModel.Language).GetService<ISyntaxFactsService>();
+            var token = await syntaxTree.GetTouchingTokenAsync(position, syntaxFacts.IsBindableToken, cancellationToken, findInsideTrivia: true).ConfigureAwait(false);


💡 This can be changed to simply return the result of GetTouchingTokenAsync directly, and remove the async modifier from the GetTokenAtPositionAsync method. This change will avoid the creation of a new async state machine and Task<T> instance on this code path.

this seems like something that should be optimized by the compiler

@sharwell what about having an analyzer for this?

sharwell · 2018-02-22T00:19:22Z

src/EditorFeatures/Core/GoToDefinition/AbstractGoToSymbolService.cs

@@ -15,7 +15,7 @@ internal abstract class AbstractGoToSymbolService : ForegroundThreadAffinitizedO
            var cancellationToken = context.CancellationToken;

            var service = document.GetLanguageService<IGoToDefinitionSymbolService>();
-            var (symbol, span) = await service.GetSymbolAndBoundSpanAsync(document, position, cancellationToken).ConfigureAwait(false);
+            var (symbol, span) = await service.GetSymbolAndBoundSpanAsync(document, position, includeLiterals: false, cancellationToken).ConfigureAwait(false);


💡 It's not obvious to me why AbstractGoToSymbolService and AbstractGoToDefinitionService are different. A comment on the types would be helpful for future readers.

sharwell · 2018-02-22T00:22:30Z

as I will refactor it in a short time.

❓ Why refactor? One thing I liked about this is how the changes didn't impact very many lines of non-test code. If the refactoring makes the review even simpler, then go for it. However, if this is a larger effort to consolidate services where the resulting code is simple but the diff is large, I would recommend waiting and sending that as a separate pull request.

isc30 · 2018-02-22T00:33:33Z

The idea is to ignore all the tokens that have no symbols instead of only literals. This avoids the need for the flag 'IsLiteral' in 'SemanticInfo' and we can rely just on the "has related symbol or not" for Ctrl+Click It will be very similar to my first solution and reduce the scope of the changes.

JieCarolHu · 2018-04-11T20:38:43Z

@isc30 I merged the latest master and created MockSymbolNavigationService for your literal go to definition tests, I created a PR here and it is working. Feel free to merge it back to your branch. Thanks :)
#26096

isc30 · 2018-04-11T23:05:36Z

Looks like all tests are passing after @JieCarolHu's changes, thanks a lot to everyone! 😄

isc30 · 2018-04-26T14:55:33Z

No idea why windows_release_vs-integration_prtest is failing, am I missing something in the PR?

sharwell

Need to update the test MEF export but otherwise looks good

sharwell · 2018-05-01T21:35:17Z

src/EditorFeatures/TestUtilities2/Utilities/GoToHelpers/MockSymbolNavigationServiceFactory.vb

+Imports Microsoft.CodeAnalysis.Navigation
+
+Namespace Microsoft.CodeAnalysis.Editor.UnitTests.Utilities.GoToHelpers
+    <ExportWorkspaceServiceFactory(GetType(ISymbolNavigationService), ServiceLayer.Editor), [Shared]>


❗️ Needs the following:

Use ServiceLayer.Test

Add <PartNotDiscoverable>

➡️ ServiceLayer.Test isn't a thing, so ServiceLayer.Host was used instead.

sharwell · 2018-05-01T23:16:35Z

@jinujoseph Requesting approval to merge

jinujoseph · 2018-05-01T23:42:07Z

Approved to merge for 15.8.Preview2

jinujoseph · 2018-05-02T01:42:35Z

test windows_debug_spanish_unit32_prtest

sharwell · 2018-05-02T15:57:28Z

@isc30 Thanks for your contribution here and patience while we figured out the unexpected issues that arose. 😄

isc30 · 2018-05-08T15:24:31Z

Thanks a lot to everyone, I really enjoyed doing the contribution!

[dotnet#23030] Disable literal Ctrl+Click navigation

fc50bd7

isc30 requested a review from a team as a code owner February 17, 2018 04:30

CyrusNajmabadi reviewed Feb 17, 2018

View reviewed changes

sharwell assigned JieCarolHu Feb 17, 2018

sharwell added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Feb 17, 2018

Move literal information to TokenSemanticInfo using SyntaxFactsService

bd4aa1e

tests: specify target symbol

0ea1d95

sharwell reviewed Feb 22, 2018

View reviewed changes

jcouv added the Area-IDE label Mar 31, 2018

JieCarolHu added 2 commits April 10, 2018 16:09

merge 23030-DisableLiteralCtrlClickNavigation

d0ee34c

use MockSymbolNavigationService for literal go to definition tests

85fc080

JieCarolHu mentioned this pull request Apr 11, 2018

Fix tests in community PR 24910 #26096

Closed

isc30 added 2 commits April 12, 2018 00:05

merge master into

8d57129

merge 23030_carol into

4471049

JieCarolHu approved these changes Apr 12, 2018

View reviewed changes

Update NavigableSymbolsTest.vb

99da5c3

sharwell requested changes May 1, 2018

View reviewed changes

Use ServiceLayer.Host and Add <PartNotDiscoverable>

8529918

sharwell approved these changes May 1, 2018

View reviewed changes

jinujoseph added the Approved to merge label May 1, 2018

sharwell merged commit 3adcd5e into dotnet:master May 2, 2018

sharwell added this to the 15.8 milestone May 2, 2018

This was referenced Oct 13, 2020

Ctrl+click a URL in a string literal takes you to the String class definition instead of following the link #38770

Closed

Ctrl+Click does not follow link in spite of tooltip (regression?) VsVim/VsVim#2857

Open

[#23030] Disable literal Ctrl+Click navigation #24910

[#23030] Disable literal Ctrl+Click navigation #24910

Conversation

isc30 commented Feb 17, 2018 • edited Loading

Bugs this fixes

Root cause analysis

dnfclas commented Feb 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isc30 Feb 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi Feb 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isc30 Feb 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi commented Feb 17, 2018

sharwell commented Feb 17, 2018

rchande commented Feb 20, 2018

isc30 commented Feb 20, 2018

rchande commented Feb 20, 2018

isc30 commented Feb 20, 2018 • edited Loading

rchande commented Feb 20, 2018

JieCarolHu commented Feb 20, 2018

isc30 commented Feb 21, 2018

JieCarolHu commented Feb 21, 2018

Neme12 commented Feb 21, 2018 • edited Loading

isc30 commented Feb 21, 2018

Neme12 commented Feb 21, 2018 • edited Loading

CyrusNajmabadi commented Feb 21, 2018

CyrusNajmabadi commented Feb 21, 2018

Neme12 commented Feb 21, 2018 • edited Loading

isc30 commented Feb 21, 2018 • edited Loading

sharwell commented Feb 22, 2018

isc30 commented Feb 22, 2018

sharwell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Neme12 Feb 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharwell commented Feb 22, 2018

isc30 commented Feb 22, 2018 via email • edited Loading

JieCarolHu commented Apr 11, 2018

isc30 commented Apr 11, 2018 • edited Loading

isc30 commented Apr 26, 2018

sharwell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharwell commented May 1, 2018

jinujoseph commented May 1, 2018

jinujoseph commented May 2, 2018

sharwell commented May 2, 2018

isc30 commented May 8, 2018

isc30 commented Feb 17, 2018 •

edited

Loading

dnfclas commented Feb 17, 2018 •

edited

Loading

isc30 Feb 17, 2018 •

edited

Loading

CyrusNajmabadi Feb 17, 2018 •

edited

Loading

isc30 Feb 17, 2018 •

edited

Loading

isc30 commented Feb 20, 2018 •

edited

Loading

Neme12 commented Feb 21, 2018 •

edited

Loading

Neme12 commented Feb 21, 2018 •

edited

Loading

Neme12 commented Feb 21, 2018 •

edited

Loading

isc30 commented Feb 21, 2018 •

edited

Loading

Neme12 Feb 22, 2018 •

edited

Loading

isc30 commented Feb 22, 2018 via email •

edited

Loading

isc30 commented Apr 11, 2018 •

edited

Loading