fixed peek definition tokenizing #281

abist · 2017-03-15T23:51:29Z

The issue here was that the parser wasn't correctly getting the correct token position, thus failing Peek Definition if the cursor was right in the starting or the end of the object we want to look up. For example, Peek Definition would fail if, let's say, the cursor were anywhere outside this range - sys.d [atabase] s

According to the new way PeekDefinition works, instead of returning an error if an object isn't found, we first look at the children and parent tokens and return the definition of those. If nothing is defined, then we return the error.

mention-bot · 2017-03-15T23:51:31Z

@abist, thanks for your PR! By analyzing the history of the files in this pull request, we identified @kburtram, @sharonravindran and @llali to be potential reviewers.

msftclas · 2017-03-15T23:51:52Z

@abist,
Thanks for your contribution as a Microsoft full-time employee or intern. You do not need to sign a CLA.
Thanks,
Microsoft Pull Request Bot

coveralls · 2017-03-16T00:22:18Z

Coverage increased (+0.03%) to 80.849% when pulling 4be0639 on task/PeekDefinitionTokenizer into 7ba2011 on dev.

kevcunnane · 2017-03-16T00:54:19Z

@abist, could you clarify what this is actually fixing? Neither the issue nor this PR describe what the impact will be. For example: "fixes issue where Peek Definition fails to find the correct token position, resulting in failure to return results...." would be my guess? But this helps us prioritize reviewing and signoff since we know what we're looking for. Please also make sure this description is included on checkin so we can look back at commit history and see what was done. Thanks!

abist · 2017-03-16T00:59:01Z

@kevcunnane Agreed, I updated the description.

kevcunnane

Please add in tests to cover this scenario and capture what you're fixing - e.g. what failed before but would now pass? Also please fix the issue where PeekDefinition.GetScript now ends up adding +2 instead of +1 to the line (since you do it here, then PeekDefinition does too

kevcunnane · 2017-03-16T00:54:36Z

src/Microsoft.SqlTools.ServiceLayer/LanguageServices/ScriptDocumentInfo.cs

+        /// <summary>
+        /// Returns the token that is used for Peek Definition objects
+        /// </summary>
+        internal static Token GetPeakDefinitionToken(ScriptParseInfo scriptParseInfo, int startLine, int startColumn)


Typo: Peak -> Peek

kevcunnane · 2017-03-16T00:58:56Z

src/Microsoft.SqlTools.ServiceLayer/LanguageServices/LanguageService.cs

@@ -814,12 +814,17 @@ internal DefinitionResult GetDefinition(TextDocumentPosition textDocumentPositio
                        bindingTimeout: LanguageService.PeekDefinitionTimeout,
                        bindOperation: (bindingContext, cancelToken) =>
                        {
-                            string schemaName = this.GetSchemaName(scriptParseInfo, textDocumentPosition.Position, scriptFile);
+                            // Handle tokenizing for Peek Definition 
+                            Position tokenPosition = new Position();


2 suggestions:

Move the Position creation up to before `Token selectedToken = ScriptDocumentInfo.GetPeekDefinitionToken) and use its line and character values. This minimizes the times we need to convert the line/character counts by adding 1 to them. Every time we do this, we risk an error later since we're tracking in multiple places

Use the constructor pattern as follow since it's a clean way to create the object :

Position tokenPosition = new Position { Line = textDocumentPosition.Position.Line + 1, Character = textDocumentPosition.Position.Character + 1 };

kevcunnane · 2017-03-16T01:02:04Z

src/Microsoft.SqlTools.ServiceLayer/LanguageServices/LanguageService.cs

                            // Script object using SMO
                            PeekDefinition peekDefinition = new PeekDefinition(bindingContext.ServerConnection, connInfo);
                            return peekDefinition.GetScript(
                                scriptParseInfo.ParseResult, 
-                                textDocumentPosition.Position, 
+                                tokenPosition, 


Isn't this an error? PeekDefinition.GetScript already adjusts line & character by 1. You need to alter that method to no longer do so.

kevcunnane · 2017-03-16T01:04:33Z

src/Microsoft.SqlTools.ServiceLayer/LanguageServices/LanguageService.cs

@@ -808,18 +808,23 @@ internal DefinitionResult GetDefinition(TextDocumentPosition textDocumentPositio
            {
                try
                {
+                    // Handle tokenizing for Peek Definition 
+                    Position tokenPosition = new Position();


We have an internal class called BufferPosition, which tends to use the VSCode Position values but 1-indexed. Perhaps for clarity, we should change the code to use that? Otherwise it's kind of confusing since we're adjusting this but nobody would know why.

The arguments strictly require the Position class, so I'm not sure if we can use the BufferPosition class here.

I'm suggesting changing the PeekDefinition class to use BufferPosition, since the pattern we have is that this uses Position's values + 1. That way it's clear what the expected input is. Otherwise, if someone else uses this class they might happily send in an unmodified Position object.

benrr101 · 2017-03-16T21:36:47Z

src/Microsoft.SqlTools.ServiceLayer/LanguageServices/LanguageService.cs

@@ -795,7 +795,7 @@ internal DefinitionResult GetDefinition(TextDocumentPosition textDocumentPositio
            }

            // Get token from selected text
-            Token selectedToken = ScriptDocumentInfo.GetToken(scriptParseInfo, textDocumentPosition.Position.Line + 1, textDocumentPosition.Position.Character);
+            Token selectedToken = ScriptDocumentInfo.GetPeakDefinitionToken(scriptParseInfo, textDocumentPosition.Position.Line + 1, textDocumentPosition.Position.Character + 1);


Peek instead of Peak

coveralls · 2017-03-22T00:02:23Z

Coverage increased (+0.06%) to 83.278% when pulling 194b228 on task/PeekDefinitionTokenizer into d7ecfb1 on dev.

kevcunnane · 2017-03-22T18:58:20Z

src/Microsoft.SqlTools.ServiceLayer/LanguageServices/LanguageService.cs

@@ -796,76 +842,127 @@ internal DefinitionResult GetDefinition(TextDocumentPosition textDocumentPositio
            }


I'd put an isconnected check at the top and error out cleanly here.

kevcunnane · 2017-03-22T18:59:52Z

src/Microsoft.SqlTools.ServiceLayer/LanguageServices/LanguageService.cs

@@ -796,76 +842,127 @@ internal DefinitionResult GetDefinition(TextDocumentPosition textDocumentPositio
            }

            // Get token from selected text
-            Token selectedToken = ScriptDocumentInfo.GetToken(scriptParseInfo, textDocumentPosition.Position.Line + 1, textDocumentPosition.Position.Character);
+            var selectedToken = ScriptDocumentInfo.GetPeekDefinitionTokens(scriptParseInfo, textDocumentPosition.Position.Line + 1, textDocumentPosition.Position.Character + 1);


DOn't use var here - it's a complicated data type so be clear. Otherwise those reading the code won't know what's happening.

kevcunnane · 2017-03-22T19:00:37Z

src/Microsoft.SqlTools.ServiceLayer/LanguageServices/LanguageService.cs

            {
-                try
+                var token = selectedToken.Item1.Pop();


Please define what Item1 is. Assign to a clear, named variable.

kevcunnane · 2017-03-22T19:03:14Z

src/Microsoft.SqlTools.ServiceLayer/LanguageServices/LanguageService.cs

+                string tokenText = TextUtilities.RemoveSquareBracketSyntax(token.Text);
+                textDocumentPosition.Position.Line = token.StartLocation.LineNumber;
+                textDocumentPosition.Position.Character = token.StartLocation.ColumnNumber;
+                if (scriptParseInfo.IsConnected && Monitor.TryEnter(scriptParseInfo.BuildingMetadataLock))


Since you'll move the "not connected" error up, please add a separate error for when you fail to enter the monitor. Probably state "Timeout out waiting to query metadata from the server"

kevcunnane · 2017-03-22T19:04:21Z

src/Microsoft.SqlTools.ServiceLayer/LanguageServices/LanguageService.cs

+            // then check the parents 
+            foreach (var i in selectedToken.Item2.ToList())
+            {
+                var token = selectedToken.Item2.Dequeue();


What is Item2? What does it represent.

kevcunnane · 2017-03-22T19:07:20Z

src/Microsoft.SqlTools.ServiceLayer/LanguageServices/ScriptDocumentInfo.cs

+        {
+            Stack<Token> childrenTokens = new Stack<Token>();
+            Queue<Token> parentTokens = new Queue<Token>();
+            if (scriptParseInfo != null && scriptParseInfo.ParseResult != null && scriptParseInfo.ParseResult.Script != null && scriptParseInfo.ParseResult.Script.Tokens != null)


Minor: put each if check on its own line

kevcunnane · 2017-03-22T19:07:39Z

src/Microsoft.SqlTools.ServiceLayer/LanguageServices/ScriptDocumentInfo.cs

+                    int currentIndex = 0;
+                    foreach (var token in scriptParseInfo.ParseResult.Script.Tokens)
+                    {
+


Minor: remove unnecessary whitespace

kevcunnane · 2017-03-22T19:09:21Z

src/Microsoft.SqlTools.ServiceLayer/Workspace/Contracts/TextDocument.cs

+        /// <returns></returns>
+        public override bool Equals(object obj)
+        {
+            if (obj == null || GetType() != obj.GetType())


What if someone has a subclass of position? I think IsInstanceOf or similar (we have cases of this in the project) work better. You could also just do obj as Position and check if null

kevcunnane · 2017-03-22T19:10:36Z

test/Microsoft.SqlTools.ServiceLayer.IntegrationTests/LanguageServer/PeekDefinitionTests.cs


+            // place the cursor on every token
+            //cursor on objects
+            TextDocumentPosition objectDocument = new TextDocumentPosition


Use a helper method to make these all 1-line definitions

kevcunnane · 2017-03-22T19:13:05Z

test/Microsoft.SqlTools.ServiceLayer.IntegrationTests/LanguageServer/PeekDefinitionTests.cs

+        [Fact]
+        public async void GetDefinitionFromChildrenAndParents()
+        {
+            string queryString = "select * from master.sys.objects";


At a minimum please add procedure-specific tests, and tests with and without escaping of brackets on each one. Ideally make it easy to define multiple inputs so future bug regressions can be tested (e.g. add in a framework that takes JSON file input defining the query & positions to test from, and what's expected to be returned at each). This 2nd ask is a stretch goal though - can ship without it.

coveralls · 2017-03-23T18:53:18Z

Coverage increased (+0.1%) to 83.366% when pulling b12ab7d on task/PeekDefinitionTokenizer into 16b3874 on dev.

kevcunnane · 2017-03-23T19:34:22Z

src/Microsoft.SqlTools.ServiceLayer/LanguageServices/LanguageService.cs

+        private DefinitionResult GetDefinitionFromTokenList(TextDocumentPosition textDocumentPosition, List<Token> tokenList,
+                ScriptParseInfo scriptParseInfo, ScriptFile scriptFile, ConnectionInfo connInfo)
+        {
+


Minor: minimize whitespace lines

fixed peek definition tokenizing

4be0639

abist requested review from kburtram, kevcunnane and sharonravindran March 15, 2017 23:51

msftclas added the cla-not-required label Mar 15, 2017

fixed signature help test

b5a2fc5

kevcunnane suggested changes Mar 16, 2017

View reviewed changes

benrr101 reviewed Mar 16, 2017

View reviewed changes

Aditya Bist added 3 commits March 16, 2017 17:45

added new heuristic for PeekDefinition behaviour

9545e3b

added tests for new heuristic

0ed440d

merged from dev

194b228

kevcunnane suggested changes Mar 22, 2017

View reviewed changes

Aditya Bist added 4 commits March 22, 2017 15:57

Merge branch 'dev' into task/PeekDefinitionTokenizer

bdea4c3

fixed code according to Kevin's CR

e660c8c

fixed failing test due to shared connection

5418dc5

changed uri for procedure test

b12ab7d

kevcunnane reviewed Mar 23, 2017

View reviewed changes

kevcunnane approved these changes Mar 23, 2017

View reviewed changes

abist merged commit 9e8e1df into dev Mar 23, 2017

abist deleted the task/PeekDefinitionTokenizer branch March 23, 2017 20:20

kburtram mentioned this pull request Aug 29, 2017

Task/script refactor #446

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fixed peek definition tokenizing #281

fixed peek definition tokenizing #281

abist commented Mar 15, 2017 •

edited

Loading

mention-bot commented Mar 15, 2017

msftclas commented Mar 15, 2017

coveralls commented Mar 16, 2017

kevcunnane commented Mar 16, 2017

abist commented Mar 16, 2017

kevcunnane left a comment

kevcunnane Mar 16, 2017

kevcunnane Mar 16, 2017

kevcunnane Mar 16, 2017

kevcunnane Mar 16, 2017

abist Mar 16, 2017

kevcunnane Mar 16, 2017

benrr101 Mar 16, 2017 •

edited

Loading

coveralls commented Mar 22, 2017

kevcunnane Mar 22, 2017

kevcunnane Mar 22, 2017

kevcunnane Mar 22, 2017

kevcunnane Mar 22, 2017

kevcunnane Mar 22, 2017

kevcunnane Mar 22, 2017

kevcunnane Mar 22, 2017

kevcunnane Mar 22, 2017

kevcunnane Mar 22, 2017

kevcunnane Mar 22, 2017

coveralls commented Mar 23, 2017

kevcunnane Mar 23, 2017

		@@ -796,76 +842,127 @@ internal DefinitionResult GetDefinition(TextDocumentPosition textDocumentPositio
		}

fixed peek definition tokenizing #281

fixed peek definition tokenizing #281

Conversation

abist commented Mar 15, 2017 • edited Loading

mention-bot commented Mar 15, 2017

msftclas commented Mar 15, 2017

coveralls commented Mar 16, 2017

kevcunnane commented Mar 16, 2017

abist commented Mar 16, 2017

kevcunnane left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benrr101 Mar 16, 2017 • edited Loading

Choose a reason for hiding this comment

coveralls commented Mar 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Mar 23, 2017

Choose a reason for hiding this comment

abist commented Mar 15, 2017 •

edited

Loading

benrr101 Mar 16, 2017 •

edited

Loading