Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fixed peek definition tokenizing #281

Merged
merged 9 commits into from
Mar 23, 2017
Merged

fixed peek definition tokenizing #281

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
4be0639
fixed peek definition tokenizing
Mar 15, 2017
b5a2fc5
fixed signature help test
Mar 16, 2017
9545e3b
added new heuristic for PeekDefinition behaviour
Mar 17, 2017
0ed440d
added tests for new heuristic
Mar 20, 2017
194b228
merged from dev
Mar 21, 2017
bdea4c3
Merge branch 'dev' into task/PeekDefinitionTokenizer
Mar 22, 2017
e660c8c
fixed code according to Kevin's CR
Mar 23, 2017
5418dc5
fixed failing test due to shared connection
Mar 23, 2017
b12ab7d
changed uri for procedure test
Mar 23, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
199 changes: 148 additions & 51 deletions src/Microsoft.SqlTools.ServiceLayer/LanguageServices/LanguageService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -774,6 +774,52 @@ internal CompletionItem ResolveCompletionItem(CompletionItem completionItem)
return completionItem;
}


/// <summary>
/// Queue a task to the binding queue
/// </summary>
/// <param name="textDocumentPosition"></param>
/// <param name="scriptParseInfo"></param>
/// <param name="connectionInfo"></param>
/// <param name="scriptFile"></param>
/// <param name="tokenText"></param>
/// <returns> Returns the result of the task as a DefinitionResult </returns>
private DefinitionResult QueueTask(TextDocumentPosition textDocumentPosition, ScriptParseInfo scriptParseInfo,
ConnectionInfo connInfo, ScriptFile scriptFile, string tokenText)
{
// Queue the task with the binding queue
QueueItem queueItem = this.BindingQueue.QueueBindingOperation(
key: scriptParseInfo.ConnectionKey,
bindingTimeout: LanguageService.PeekDefinitionTimeout,
bindOperation: (bindingContext, cancelToken) =>
{
string schemaName = this.GetSchemaName(scriptParseInfo, textDocumentPosition.Position, scriptFile);
// Script object using SMO
Scripter scripter = new Scripter(bindingContext.ServerConnection, connInfo);
return scripter.GetScript(
scriptParseInfo.ParseResult,
textDocumentPosition.Position,
bindingContext.MetadataDisplayInfoProvider,
tokenText,
schemaName);
},
timeoutOperation: (bindingContext) =>
{
// return error result
return new DefinitionResult
{
IsErrorResult = true,
Message = SR.PeekDefinitionTimedoutError,
Locations = null
};
});

// wait for the queue item
queueItem.ItemProcessed.WaitOne();
var result = queueItem.GetResultAsT<DefinitionResult>();
return result;
}

/// <summary>
/// Get definition for a selected sql object using SMO Scripting
/// </summary>
Expand All @@ -796,76 +842,127 @@ internal DefinitionResult GetDefinition(TextDocumentPosition textDocumentPositio
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


// 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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (selectedToken == null)
{
return null;
}
// Strip "[" and "]"(if present) from the token text to enable matching with the suggestions.
// The suggestion title does not contain any sql punctuation
string tokenText = TextUtilities.RemoveSquareBracketSyntax(selectedToken.Text);

if (scriptParseInfo.IsConnected && Monitor.TryEnter(scriptParseInfo.BuildingMetadataLock))
DefinitionResult lastResult = null;

//try children tokens first
foreach (var i in selectedToken.Item1.ToList())
{
try
var token = selectedToken.Item1.Pop();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// Strip "[" and "]"(if present) from the token text to enable matching with the suggestions.
// The suggestion title does not contain any sql punctuation
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))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"

{
// Queue the task with the binding queue
QueueItem queueItem = this.BindingQueue.QueueBindingOperation(
key: scriptParseInfo.ConnectionKey,
bindingTimeout: LanguageService.PeekDefinitionTimeout,
bindOperation: (bindingContext, cancelToken) =>
try
{
var result = QueueTask(textDocumentPosition, scriptParseInfo, connInfo, scriptFile, tokenText);
if (!result.IsErrorResult)
{
string schemaName = this.GetSchemaName(scriptParseInfo, textDocumentPosition.Position, scriptFile);
// Script object using SMO
Scripter scripter = new Scripter(bindingContext.ServerConnection, connInfo);
return scripter.GetScript(
scriptParseInfo.ParseResult,
textDocumentPosition.Position,
bindingContext.MetadataDisplayInfoProvider,
tokenText,
schemaName);
},
timeoutOperation: (bindingContext) =>
return result;
}
}
catch (Exception ex)
{
// if any exceptions are raised return error result with message
Logger.Write(LogLevel.Error, "Exception in GetDefinition " + ex.ToString());
return new DefinitionResult
{
// return error result
return new DefinitionResult
{
IsErrorResult = true,
Message = SR.PeekDefinitionTimedoutError,
Locations = null
};
});

// wait for the queue item
queueItem.ItemProcessed.WaitOne();
return queueItem.GetResultAsT<DefinitionResult>();
IsErrorResult = true,
Message = SR.PeekDefinitionError(ex.Message),
Locations = null
};
}
finally
{
Monitor.Exit(scriptParseInfo.BuildingMetadataLock);
}
}
catch (Exception ex)
else
{
// if any exceptions are raised return error result with message
Logger.Write(LogLevel.Error, "Exception in GetDefinition " + ex.ToString());
// User is not connected.
return new DefinitionResult
{
IsErrorResult = true,
Message = SR.PeekDefinitionError(ex.Message),
Message = SR.PeekDefinitionNotConnectedError,
Locations = null
};
}
finally
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is Item2? What does it represent.

// Strip "[" and "]"(if present) from the token text to enable matching with the suggestions.
// The suggestion title does not contain any sql punctuation
string tokenText = TextUtilities.RemoveSquareBracketSyntax(token.Text);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is common logic - use a helper method

textDocumentPosition.Position.Line = token.StartLocation.LineNumber;
textDocumentPosition.Position.Character = token.StartLocation.ColumnNumber;
if (scriptParseInfo.IsConnected && Monitor.TryEnter(scriptParseInfo.BuildingMetadataLock))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is the exact same logic, refactor to have 1 method, and just call with the relevant tokens

{
try
{
var result = QueueTask(textDocumentPosition, scriptParseInfo, connInfo, scriptFile, tokenText);
lastResult = result;
if (!result.IsErrorResult)
{
return result;
}
}
catch (Exception ex)
{
// if any exceptions are raised return error result with message
Logger.Write(LogLevel.Error, "Exception in GetDefinition " + ex.ToString());
return new DefinitionResult
{
IsErrorResult = true,
Message = SR.PeekDefinitionError(ex.Message),
Locations = null
};
}
finally
{
Monitor.Exit(scriptParseInfo.BuildingMetadataLock);
}
}
else
{
Monitor.Exit(scriptParseInfo.BuildingMetadataLock);
// User is not connected.
return new DefinitionResult
{
IsErrorResult = true,
Message = SR.PeekDefinitionNotConnectedError,
Locations = null
};
}
}
else
return (lastResult != null) ? lastResult : null;
}

/// <summary>
/// Wrapper around find token method
/// </summary>
/// <param name="scriptParseInfo"></param>
/// <param name="startLine"></param>
/// <param name="startColumn"></param>
/// <returns> token index</returns>
private int FindTokenWrapper(ScriptParseInfo scriptParseInfo, int startLine, int startColumn)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: should this be a global utility? Does any other scenario need this? Also I'd rename to "FindTokenWithCorrectOffset" or similar. "Wrapper" doesn't make clear why this is.

{
var tokenIndex = scriptParseInfo.ParseResult.Script.TokenManager.FindToken(startLine, startColumn);
var end = scriptParseInfo.ParseResult.Script.TokenManager.GetToken(tokenIndex).EndLocation;
if (end.LineNumber == startLine && end.ColumnNumber == startColumn)
{
// User is not connected.
return new DefinitionResult
{
IsErrorResult = true,
Message = SR.PeekDefinitionNotConnectedError,
Locations = null
};
return tokenIndex + 1;
}
return tokenIndex;
}

/// <summary>
Expand All @@ -878,13 +975,13 @@ internal DefinitionResult GetDefinition(TextDocumentPosition textDocumentPositio
private string GetSchemaName(ScriptParseInfo scriptParseInfo, Position position, ScriptFile scriptFile)
{
// Offset index by 1 for sql parser
int startLine = position.Line + 1;
int startColumn = position.Character + 1;
int startLine = position.Line;
int startColumn = position.Character;

// Get schema name
if (scriptParseInfo != null && scriptParseInfo.ParseResult != null && scriptParseInfo.ParseResult.Script != null && scriptParseInfo.ParseResult.Script.Tokens != null)
{
var tokenIndex = scriptParseInfo.ParseResult.Script.TokenManager.FindToken(startLine, startColumn);
var tokenIndex = FindTokenWrapper(scriptParseInfo, startLine, startColumn);
var prevTokenIndex = scriptParseInfo.ParseResult.Script.TokenManager.GetPreviousSignificantTokenIndex(tokenIndex);
var prevTokenText = scriptParseInfo.ParseResult.Script.TokenManager.GetText(prevTokenIndex);
if (prevTokenText != null && prevTokenText.Equals("."))
Expand Down
57 changes: 57 additions & 0 deletions src/Microsoft.SqlTools.ServiceLayer/LanguageServices/ScriptDocumentInfo.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,11 @@
// Licensed under the MIT license. See LICENSE file in the project root for full license information.
//

using System;
using Microsoft.SqlServer.Management.SqlParser.Parser;
using Microsoft.SqlTools.Utility;
using Microsoft.SqlTools.ServiceLayer.Workspace.Contracts;
using System.Collections.Generic;

namespace Microsoft.SqlTools.ServiceLayer.LanguageServices.Completion
{
Expand Down Expand Up @@ -129,5 +131,60 @@ internal static Token GetToken(ScriptParseInfo scriptParseInfo, int startLine, i
}
return null;
}

/// <summary>
/// Returns the token that is used for Peek Definition objects
/// </summary>
internal static Tuple<Stack<Token>, Queue<Token>> GetPeekDefinitionTokens(ScriptParseInfo scriptParseInfo, int startLine, int startColumn)
{
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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: put each if check on its own line

{
var tokenIndex = scriptParseInfo.ParseResult.Script.TokenManager.FindToken(startLine, startColumn);
if (tokenIndex >= 0)
{
// return the current token and the ones to its right
// until we hit a whitespace token
int currentIndex = 0;
foreach (var token in scriptParseInfo.ParseResult.Script.Tokens)
{

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: remove unnecessary whitespace

if (currentIndex == tokenIndex)
{
// push all parent tokens until we hit whitespace
int parentIndex = currentIndex;
while (true)
{
if (scriptParseInfo.ParseResult.Script.TokenManager.GetToken(parentIndex).Type != "LEX_WHITE")
{
parentTokens.Enqueue(scriptParseInfo.ParseResult.Script.TokenManager.GetToken(parentIndex));
parentIndex--;
}
else
{
break;
}
}
}
else if (currentIndex > tokenIndex)
{
// push all children tokens until we hit whitespace
if (scriptParseInfo.ParseResult.Script.TokenManager.GetToken(currentIndex).Type != "LEX_WHITE")
{
childrenTokens.Push(token);
}
else
{
break;
}
}
++currentIndex;
}
return Tuple.Create(childrenTokens, parentTokens);
}
}
return null;
}
}
}
4 changes: 2 additions & 2 deletions src/Microsoft.SqlTools.ServiceLayer/Scripting/ScripterCore.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,8 +133,8 @@ private void AddSupportedType(DeclarationType type, ScriptGetter scriptGetter, s
/// <returns>Location object of the script file</returns>
internal DefinitionResult GetScript(ParseResult parseResult, Position position, IMetadataDisplayInfoProvider metadataDisplayInfoProvider, string tokenText, string schemaName)
{
int parserLine = position.Line + 1;
int parserColumn = position.Character + 1;
int parserLine = position.Line;
int parserColumn = position.Character;
// Get DeclarationItems from The Intellisense Resolver for the selected token. The type of the selected token is extracted from the declarationItem.
IEnumerable<Declaration> declarationItems = GetCompletionsForToken(parseResult, parserLine, parserColumn, metadataDisplayInfoProvider);
if (declarationItems != null && declarationItems.Count() > 0)
Expand Down
Loading