Skip to content

Add support for a "Show Documentation" quick fix menu entry #789

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

Merged
merged 5 commits into from
Nov 15, 2018
Merged

Add support for a "Show Documentation" quick fix menu entry #789

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7c2cc1f
Add support for a "Show Documentation" quick fix menu entry
rkeithhill Nov 4, 2018
ede6866
Address PR comments by switching to StringBuilder
rkeithhill Nov 8, 2018
c0df728
Remove extra filesToAnalyze empty array check per PR feedback
rkeithhill Nov 12, 2018
dd73d26
Address PR feedback on StringBuilder
rkeithhill Nov 14, 2018
96e83e6
Fix indentation issue as per PR feedback
rkeithhill Nov 15, 2018
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
115 changes: 81 additions & 34 deletions src/PowerShellEditorServices.Protocol/Server/LanguageServer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
using System.Linq;
using System.Management.Automation;
using System.Management.Automation.Language;
using System.Text;
using System.Text.RegularExpressions;
using System.Threading;
using System.Threading.Tasks;
Expand All @@ -27,7 +28,7 @@ namespace Microsoft.PowerShell.EditorServices.Protocol.Server
{
public class LanguageServer
{
private static CancellationTokenSource existingRequestCancellation;
private static CancellationTokenSource s_existingRequestCancellation;

private static readonly Location[] s_emptyLocationResult = new Location[0];

Expand All @@ -48,6 +49,7 @@ public class LanguageServer
private LanguageServerEditorOperations editorOperations;
private LanguageServerSettings currentSettings = new LanguageServerSettings();

// The outer key is the file's uri, the inner key is a unique id for the diagnostic
private Dictionary<string, Dictionary<string, MarkerCorrection>> codeActionsPerFile =
new Dictionary<string, Dictionary<string, MarkerCorrection>>();

Expand Down Expand Up @@ -1182,6 +1184,7 @@ private bool IsQueryMatch(string query, string symbolName)
return symbolName.IndexOf(query, StringComparison.OrdinalIgnoreCase) >= 0;
}

// https://microsoft.github.io/language-server-protocol/specification#textDocument_codeAction
Copy link
Member

Choose a reason for hiding this comment

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

omg we should have comments like this all over... so great! (not in this PR 😄 )

protected async Task HandleCodeActionRequest(
CodeActionParams codeActionParams,
RequestContext<CodeActionCommand[]> requestContext)
Expand All @@ -1190,12 +1193,22 @@ protected async Task HandleCodeActionRequest(
Dictionary<string, MarkerCorrection> markerIndex = null;
List<CodeActionCommand> codeActionCommands = new List<CodeActionCommand>();

// If there are any code fixes, send these commands first so they appear at top of "Code Fix" menu in the client UI.
if (this.codeActionsPerFile.TryGetValue(codeActionParams.TextDocument.Uri, out markerIndex))
{
foreach (var diagnostic in codeActionParams.Context.Diagnostics)
{
if (!string.IsNullOrEmpty(diagnostic.Code) &&
markerIndex.TryGetValue(diagnostic.Code, out correction))
if (string.IsNullOrEmpty(diagnostic.Code))
{
this.Logger.Write(
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

LogLevel.Warning,
$"textDocument/codeAction skipping diagnostic with empty Code field: {diagnostic.Source} {diagnostic.Message}");

continue;
}

string diagnosticId = GetUniqueIdFromDiagnostic(diagnostic);
if (markerIndex.TryGetValue(diagnosticId, out correction))
{
codeActionCommands.Add(
new CodeActionCommand
Expand All @@ -1208,8 +1221,31 @@ protected async Task HandleCodeActionRequest(
}
}

// Add "show documentation" commands last so they appear at the bottom of the client UI.
// These commands do not require code fixes. Sometimes we get a batch of diagnostics
// to create commands for. No need to create multiple show doc commands for the same rule.
var ruleNamesProcessed = new HashSet<string>();
foreach (var diagnostic in codeActionParams.Context.Diagnostics)
{
if (string.IsNullOrEmpty(diagnostic.Code)) { continue; }

if (string.Equals(diagnostic.Source, "PSScriptAnalyzer", StringComparison.OrdinalIgnoreCase) &&
!ruleNamesProcessed.Contains(diagnostic.Code))
{
ruleNamesProcessed.Add(diagnostic.Code);

codeActionCommands.Add(
new CodeActionCommand
{
Title = $"Show documentation for \"{diagnostic.Code}\"",
Command = "PowerShell.ShowCodeActionDocumentation",
Arguments = JArray.FromObject(new[] { diagnostic.Code })
});
}
}

await requestContext.SendResult(
codeActionCommands.ToArray());
codeActionCommands.ToArray());
}

protected async Task HandleDocumentFormattingRequest(
Expand Down Expand Up @@ -1454,15 +1490,15 @@ private Task RunScriptDiagnostics(
// If there's an existing task, attempt to cancel it
try
{
if (existingRequestCancellation != null)
if (s_existingRequestCancellation != null)
{
// Try to cancel the request
existingRequestCancellation.Cancel();
s_existingRequestCancellation.Cancel();

// If cancellation didn't throw an exception,
// clean up the existing token
existingRequestCancellation.Dispose();
existingRequestCancellation = null;
s_existingRequestCancellation.Dispose();
s_existingRequestCancellation = null;
}
}
catch (Exception e)
Expand All @@ -1479,11 +1515,17 @@ private Task RunScriptDiagnostics(
return cancelTask.Task;
}

// If filesToAnalzye is empty, nothing to do so return early.
if (filesToAnalyze.Length == 0)
{
return Task.FromResult(true);
}

// Create a fresh cancellation token and then start the task.
// We create this on a different TaskScheduler so that we
// don't block the main message loop thread.
// TODO: Is there a better way to do this?
existingRequestCancellation = new CancellationTokenSource();
s_existingRequestCancellation = new CancellationTokenSource();
Task.Factory.StartNew(
() =>
DelayThenInvokeDiagnostics(
Expand All @@ -1494,36 +1536,14 @@ private Task RunScriptDiagnostics(
editorSession,
eventSender,
this.Logger,
existingRequestCancellation.Token),
s_existingRequestCancellation.Token),
CancellationToken.None,
TaskCreationOptions.None,
TaskScheduler.Default);

return Task.FromResult(true);
}

private static async Task DelayThenInvokeDiagnostics(
int delayMilliseconds,
ScriptFile[] filesToAnalyze,
bool isScriptAnalysisEnabled,
Dictionary<string, Dictionary<string, MarkerCorrection>> correctionIndex,
EditorSession editorSession,
EventContext eventContext,
ILogger Logger,
CancellationToken cancellationToken)
{
await DelayThenInvokeDiagnostics(
delayMilliseconds,
filesToAnalyze,
isScriptAnalysisEnabled,
correctionIndex,
editorSession,
eventContext.SendEvent,
Logger,
cancellationToken);
}


private static async Task DelayThenInvokeDiagnostics(
int delayMilliseconds,
ScriptFile[] filesToAnalyze,
Expand Down Expand Up @@ -1573,6 +1593,7 @@ private static async Task DelayThenInvokeDiagnostics(

await PublishScriptDiagnostics(
scriptFile,
// Concat script analysis errors to any existing parse errors
scriptFile.SyntaxMarkers.Concat(semanticMarkers).ToArray(),
correctionIndex,
eventSender);
Expand Down Expand Up @@ -1620,7 +1641,8 @@ private static async Task PublishScriptDiagnostics(
Diagnostic markerDiagnostic = GetDiagnosticFromMarker(marker);
if (marker.Correction != null)
{
fileCorrections.Add(markerDiagnostic.Code, marker.Correction);
string diagnosticId = GetUniqueIdFromDiagnostic(markerDiagnostic);
fileCorrections.Add(diagnosticId, marker.Correction);
}

diagnostics.Add(markerDiagnostic);
Expand All @@ -1639,13 +1661,38 @@ await eventSender(
});
}

// Generate a unique id that is used as a key to look up the associated code action (code fix) when
// we receive and process the textDocument/codeAction message.
private static string GetUniqueIdFromDiagnostic(Diagnostic diagnostic)
{
Position start = diagnostic.Range.Start;
Position end = diagnostic.Range.End;

var sb = new StringBuilder(256);
sb.Append(diagnostic.Source ?? "?");
sb.Append("_");
sb.Append(diagnostic.Code ?? "?");
sb.Append("_");
sb.Append((diagnostic.Severity != null) ? diagnostic.Severity.ToString() : "?");
sb.Append("_");
sb.Append(start.Line);
sb.Append(":");
sb.Append(start.Character);
sb.Append("-");
sb.Append(end.Line);
sb.Append(":");
sb.Append(end.Character);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Consider chaining these, e.g.

return new StringBuilder(256)
    .Append(diagnostic.Source ?? "?")
    .Append("_")
    .Append(diagnostic.Code ?? "?")
    .Append("_")
    .Append((diagnostic.Severity != null) ? diagnostic.Severity.ToString() : "?")
    .Append("_")
    .Append(start.Line)
    .Append(":")
    .Append(start.Character)
    .Append("-")
    .Append(end.Line)
    .Append(":")
    .Append(end.Character)
    .ToString();

Copy link
Member

@TylerLeonhardt TylerLeonhardt Nov 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

small nit... it might be useful to do:

StringBuilder sb = new StringBuilder(256)
    .Append(diagnostic.Source ?? "?")
    .Append("_")
    .Append(diagnostic.Code ?? "?")
    .Append("_")
    .Append((diagnostic.Severity != null) ? diagnostic.Severity.ToString() : "?")
    .Append("_")
    .Append(start.Line)
    .Append(":")
    .Append(start.Character)
    .Append("-")
    .Append(end.Line)
    .Append(":")
    .Append(end.Character)

return sb.ToString();

so that we can throw a breakpoint on the return line and see what the contents of sb are. But I won't block on doing that vs what Patrick suggested

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Heh, I was just going to suggest this very thing. :-) Will fix.

Copy link
Contributor

Choose a reason for hiding this comment

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

Honestly my biggest desire is for a function-return-variable in debug view.


return sb.ToString();
}

private static Diagnostic GetDiagnosticFromMarker(ScriptFileMarker scriptFileMarker)
{
return new Diagnostic
{
Severity = MapDiagnosticSeverity(scriptFileMarker.Level),
Message = scriptFileMarker.Message,
Code = scriptFileMarker.Source + Guid.NewGuid().ToString(),
Code = scriptFileMarker.RuleName,
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 so much nicer!

Source = scriptFileMarker.Source,
Range = new Range
{
Expand Down
9 changes: 7 additions & 2 deletions src/PowerShellEditorServices/Workspace/ScriptFileMarker.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,11 @@ public class ScriptFileMarker
/// Gets or sets the marker's message string.
/// </summary>
public string Message { get; set; }

/// <summary>
/// Gets or sets the ruleName associated with this marker.
/// </summary>
public string RuleName { get; set; }

/// <summary>
/// Gets or sets the marker's message level.
Expand Down Expand Up @@ -130,7 +135,6 @@ internal static ScriptFileMarker FromDiagnosticRecord(PSObject psObject)
// the diagnostic record's properties directly i.e. <instance>.<propertyName>
// without having to go through PSObject's Members property.
var diagnosticRecord = psObject as dynamic;
string ruleName = diagnosticRecord.RuleName as string;

if (diagnosticRecord.SuggestedCorrections != null)
{
Expand Down Expand Up @@ -160,7 +164,8 @@ internal static ScriptFileMarker FromDiagnosticRecord(PSObject psObject)

return new ScriptFileMarker
{
Message = $"{diagnosticRecord.Message as string} ({ruleName})",
Message = $"{diagnosticRecord.Message as string}",
RuleName = $"{diagnosticRecord.RuleName as string}",
Level = GetMarkerLevelFromDiagnosticSeverity((diagnosticRecord.Severity as Enum).ToString()),
ScriptRegion = ScriptRegion.Create(diagnosticRecord.Extent as IScriptExtent),
Correction = correction,
Expand Down