Skip to content

Add new ParseError level to ScriptFileMarkerLevel and only have it send parse errors #888

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
Mar 25, 2019
+52 −62
Merged

Add new ParseError level to ScriptFileMarkerLevel and only have it send parse errors #888

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b8f56fa
Add new PSSA 1.18 ParseError Severity
TylerLeonhardt Mar 21, 2019
4937f56
ignore parseerrors from PSSA
TylerLeonhardt Mar 22, 2019
a81d4ae
address feedback
TylerLeonhardt Mar 22, 2019
0e05c92
back to empty list since we supply parse errors in PSES again
TylerLeonhardt Mar 22, 2019
a57fbd0
address feedback
TylerLeonhardt Mar 22, 2019
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
16 changes: 9 additions & 7 deletions src/PowerShellEditorServices.Protocol/Server/LanguageServer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1192,7 +1192,7 @@ protected async Task HandleCommentHelpRequestAsync(
funcText = string.Join("\n", lines);
}

ScriptFileMarker[] analysisResults = await this.editorSession.AnalysisService.GetSemanticMarkersAsync(
List<ScriptFileMarker> analysisResults = await this.editorSession.AnalysisService.GetSemanticMarkersAsync(
funcText,
AnalysisService.GetCommentHelpRuleSettings(
enable: true,
Expand Down Expand Up @@ -1743,7 +1743,7 @@ await PublishScriptDiagnosticsAsync(
// Get the requested files
foreach (ScriptFile scriptFile in filesToAnalyze)
{
ScriptFileMarker[] semanticMarkers = null;
List<ScriptFileMarker> semanticMarkers = null;
if (isScriptAnalysisEnabled && editorSession.AnalysisService != null)
{
using (Logger.LogExecutionTime($"Script analysis of {scriptFile.FilePath} completed."))
Expand All @@ -1755,13 +1755,15 @@ await PublishScriptDiagnosticsAsync(
{
// Semantic markers aren't available if the AnalysisService
// isn't available
semanticMarkers = new ScriptFileMarker[0];
semanticMarkers = new List<ScriptFileMarker>();
}

scriptFile.SyntaxMarkers.AddRange(semanticMarkers);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really want to modify the SyntaxMarkers collection in ScriptFile by adding semantic markers? Maybe we should have two collections in ScriptFile (SyntaxMarkers and SemanticMarkers) or perhaps rename the property to DiagnosticMarkers (or something like that).

Copy link
Member Author

Choose a reason for hiding this comment

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

I went with the rename since the syntax markers and semantic markers are both treated the same when they get sent to vscode.


await PublishScriptDiagnosticsAsync(
scriptFile,
// Concat script analysis errors to any existing parse errors
scriptFile.SyntaxMarkers.Concat(semanticMarkers).ToArray(),
scriptFile.SyntaxMarkers,
correctionIndex,
eventSender);
}
Expand All @@ -1772,14 +1774,14 @@ private async Task ClearMarkersAsync(ScriptFile scriptFile, EventContext eventCo
// send empty diagnostic markers to clear any markers associated with the given file
await PublishScriptDiagnosticsAsync(
scriptFile,
new ScriptFileMarker[0],
new List<ScriptFileMarker>(),
this.codeActionsPerFile,
eventContext);
}

private static async Task PublishScriptDiagnosticsAsync(
ScriptFile scriptFile,
ScriptFileMarker[] markers,
List<ScriptFileMarker> markers,
Dictionary<string, Dictionary<string, MarkerCorrection>> correctionIndex,
EventContext eventContext)
{
Expand All @@ -1792,7 +1794,7 @@ await PublishScriptDiagnosticsAsync(

private static async Task PublishScriptDiagnosticsAsync(
ScriptFile scriptFile,
ScriptFileMarker[] markers,
List<ScriptFileMarker> markers,
Dictionary<string, Dictionary<string, MarkerCorrection>> correctionIndex,
Func<NotificationType<PublishDiagnosticsNotification, object>, PublishDiagnosticsNotification, Task> eventSender)
{
Expand Down
18 changes: 10 additions & 8 deletions src/PowerShellEditorServices/Analysis/AnalysisService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ public class AnalysisService : IDisposable
/// <summary>
/// An empty script marker result to return when no script markers can be returned.
/// </summary>
private static readonly ScriptFileMarker[] s_emptyScriptMarkerResult = new ScriptFileMarker[0];
private static readonly List<ScriptFileMarker> s_emptyScriptMarkerResult = new List<ScriptFileMarker>();
Copy link
Contributor

Choose a reason for hiding this comment

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

If this is supposed to be a readonly empty value, it can't be a List<T>.

Otherwise, if we need to return a mutable collection, we should either:

  • Return a readonly collection here and create a new mutable collection around it in the caller if it needs extending, or;
  • Return a List and not have an s_emptyScriptMarkerResult, but instead return a new List<T>() every time.

If we return s_emptyScriptMarkerResult and the caller then mutates it, those additions will become permanently added to s_emptyScriptMarkerResult.

Copy link
Member Author

Choose a reason for hiding this comment

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

good point. I've switched to just some new lists.


private static readonly string[] s_emptyGetRuleResult = new string[0];

Expand Down Expand Up @@ -266,7 +266,7 @@ public static Hashtable GetPSSASettingsHashtable(IDictionary<string, Hashtable>
/// </summary>
/// <param name="file">The ScriptFile which will be analyzed for semantic markers.</param>
/// <returns>An array of ScriptFileMarkers containing semantic analysis results.</returns>
public async Task<ScriptFileMarker[]> GetSemanticMarkersAsync(ScriptFile file)
public async Task<List<ScriptFileMarker>> GetSemanticMarkersAsync(ScriptFile file)
{
return await GetSemanticMarkersAsync<string>(file, ActiveRules, SettingsPath);
}
Expand All @@ -277,7 +277,7 @@ public async Task<ScriptFileMarker[]> GetSemanticMarkersAsync(ScriptFile file)
/// <param name="file">The ScriptFile to be analyzed.</param>
/// <param name="settings">ScriptAnalyzer settings</param>
/// <returns></returns>
public async Task<ScriptFileMarker[]> GetSemanticMarkersAsync(ScriptFile file, Hashtable settings)
public async Task<List<ScriptFileMarker>> GetSemanticMarkersAsync(ScriptFile file, Hashtable settings)
{
return await GetSemanticMarkersAsync<Hashtable>(file, null, settings);
}
Expand All @@ -288,7 +288,7 @@ public async Task<ScriptFileMarker[]> GetSemanticMarkersAsync(ScriptFile file, H
/// <param name="scriptContent">The script content to be analyzed.</param>
/// <param name="settings">ScriptAnalyzer settings</param>
/// <returns></returns>
public async Task<ScriptFileMarker[]> GetSemanticMarkersAsync(
public async Task<List<ScriptFileMarker>> GetSemanticMarkersAsync(
string scriptContent,
Hashtable settings)
{
Expand Down Expand Up @@ -379,7 +379,7 @@ public async Task<string> FormatAsync(

#region Private Methods

private async Task<ScriptFileMarker[]> GetSemanticMarkersAsync<TSettings>(
private async Task<List<ScriptFileMarker>> GetSemanticMarkersAsync<TSettings>(
ScriptFile file,
string[] rules,
TSettings settings) where TSettings : class
Expand All @@ -398,7 +398,7 @@ private async Task<ScriptFileMarker[]> GetSemanticMarkersAsync<TSettings>(
}
}

private async Task<ScriptFileMarker[]> GetSemanticMarkersAsync<TSettings>(
private async Task<List<ScriptFileMarker>> GetSemanticMarkersAsync<TSettings>(
string scriptContent,
string[] rules,
TSettings settings) where TSettings : class
Expand All @@ -407,7 +407,7 @@ private async Task<ScriptFileMarker[]> GetSemanticMarkersAsync<TSettings>(
&& (rules != null || settings != null))
{
var scriptFileMarkers = await GetDiagnosticRecordsAsync(scriptContent, rules, settings);
return scriptFileMarkers.Select(ScriptFileMarker.FromDiagnosticRecord).ToArray();
return scriptFileMarkers.Select(ScriptFileMarker.FromDiagnosticRecord).ToList();
}
else
{
Expand Down Expand Up @@ -514,7 +514,9 @@ private async Task<PSObject[]> GetDiagnosticRecordsAsync<TSettings>(
new Dictionary<string, object>
{
{ "ScriptDefinition", scriptContent },
{ settingParameter, settingArgument }
{ settingParameter, settingArgument },
// We ignore ParseErrors from PSSA because we already send them when we parse the file.
{ "Severity", new [] { ScriptFileMarkerLevel.Error, ScriptFileMarkerLevel.Information, ScriptFileMarkerLevel.Warning }}
});

diagnosticRecords = result?.Output;
Expand Down
4 changes: 2 additions & 2 deletions src/PowerShellEditorServices/Workspace/ScriptFile.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ public string Contents
/// Gets the list of syntax markers found by parsing this
/// file's contents.
/// </summary>
public ScriptFileMarker[] SyntaxMarkers
public List<ScriptFileMarker> SyntaxMarkers
{
get;
private set;
Expand Down Expand Up @@ -657,7 +657,7 @@ private void ParseFileContents()
this.SyntaxMarkers =
parseErrors
.Select(ScriptFileMarker.FromParseError)
.ToArray();
.ToList();

// Untitled files have no directory
// Discussed in https://github.com/PowerShell/PowerShellEditorServices/pull/815.
Expand Down
63 changes: 28 additions & 35 deletions src/PowerShellEditorServices/Workspace/ScriptFileMarker.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,20 +33,22 @@ public class MarkerCorrection
/// </summary>
public enum ScriptFileMarkerLevel
{
/// <summary>
/// The marker represents an informational message.
/// </summary>
Information = 0,

/// <summary>
/// The marker represents a warning message.
/// </summary>
Warning,

/// <summary>
/// The marker represents an error message.
/// </summary>
Error
/// <summary>
        /// Information: This warning is trivial, but may be useful. They are recommended by PowerShell best practice.
        /// </summary>
        Information = 0,
        /// <summary>
        /// WARNING: This warning may cause a problem or does not follow PowerShell's recommended guidelines.
        /// </summary>
        Warning = 1,
        /// <summary>
        /// ERROR: This warning is likely to cause a problem or does not follow PowerShell's required guidelines.
        /// </summary>
        Error = 2,
        /// <summary>
        /// ERROR: This diagnostic is caused by an actual parsing error, and is generated only by the engine.
        /// </summary>
        ParseError = 3
};

/// <summary>
Expand All @@ -62,7 +64,7 @@ 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>
Expand Down Expand Up @@ -162,36 +164,27 @@ internal static ScriptFileMarker FromDiagnosticRecord(PSObject psObject)
};
}

string severity = diagnosticRecord.Severity.ToString();
if(!Enum.TryParse(severity, out ScriptFileMarkerLevel level))
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor nit, space between if and opening (.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm glad I wasn't the only one that felt like this

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch - fixed.

{
throw new ArgumentException(
string.Format(
"The provided DiagnosticSeverity value '{0}' is unknown.",
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor nit: Using string interpolation ($"") we could merge 3 lines into 1 whilst having better readability.

severity),
"diagnosticSeverity");
}

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

private static ScriptFileMarkerLevel GetMarkerLevelFromDiagnosticSeverity(
string diagnosticSeverity)
{
switch (diagnosticSeverity)
{
case "Information":
return ScriptFileMarkerLevel.Information;
case "Warning":
return ScriptFileMarkerLevel.Warning;
case "Error":
return ScriptFileMarkerLevel.Error;
default:
throw new ArgumentException(
string.Format(
"The provided DiagnosticSeverity value '{0}' is unknown.",
diagnosticSeverity),
"diagnosticSeverity");
}
}
#endregion
}
}
Expand Down
4 changes: 2 additions & 2 deletions test/PowerShellEditorServices.Test/Session/ScriptFileTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -550,7 +550,7 @@ public void PropertiesInitializedCorrectlyForFile()
Assert.True(scriptFile.IsAnalysisEnabled);
Assert.False(scriptFile.IsInMemory);
Assert.Empty(scriptFile.ReferencedFiles);
Assert.Empty(scriptFile.SyntaxMarkers);
Assert.Null(scriptFile.SyntaxMarkers);
Assert.Single(scriptFile.ScriptTokens);
Assert.Single(scriptFile.FileLines);
}
Expand All @@ -576,7 +576,7 @@ public void PropertiesInitializedCorrectlyForUntitled()
Assert.True(scriptFile.IsAnalysisEnabled);
Assert.True(scriptFile.IsInMemory);
Assert.Empty(scriptFile.ReferencedFiles);
Assert.Empty(scriptFile.SyntaxMarkers);
Assert.Null(scriptFile.SyntaxMarkers);
Assert.Equal(10, scriptFile.ScriptTokens.Length);
Assert.Equal(3, scriptFile.FileLines.Count);
}
Expand Down