-
Notifications
You must be signed in to change notification settings - Fork 395
Add UseUsingScopeModifierInNewRunspaces rule #1419
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.
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
bergmeister
merged 59 commits into
PowerShell:master
from
Jawz84:1410_foreach_parallel_rule
Mar 24, 2020
Merged
Changes from 4 commits
Commits
Show all changes
59 commits
Select commit
Hold shift + click to select a range
8b9f115
Add tests for AvoidUnInitializedVarsInNewRunspaces
0ab5fbf
Add strings for AvoidUnInitializedVarsInNewRunspaces
7182edb
Add documentation
068b1fc
wrestling with Ast in C# - not working
adbeff0
Add sensible warning message
23183d2
remove unnecessary boilerplate from test
83394d6
clean up and finetune rule
99c2bea
Add RuleToTest parameter to Test-ScriptAnalyzer to aid with test driv…
91e6689
increment rule count to fix test
337ac20
add reference to rule documentation
406d7ec
exclude built-in variables
55b7316
change using directive => scoope modifier
ef4b79a
add tests for InlineScript, Invoke-Command and Start-(Thread)Job
36bb1fc
add rule implementation for InlineScript, Invoke-Command and Start-(T…
e7fd6cc
Refactor and cleanup
59ba18b
small cleanup
f64c81c
explain all applicable situations in documentation
db80c8f
simplify code for adding sessions to dict
d1f8df8
Revert "Add RuleToTest parameter to Test-ScriptAnalyzer to aid with t…
c53b65d
Rename rule to UseUsingScopeModifierInNewRunspaces
15bd608
refactor grouping of script blocks by session name
37a5cb1
Add suggested correction implementation
6024c50
Add test for suggested corrections, fix typos
b12d16f
Refactor to AstVisitor/AstVisitor2 WIP
9bc2195
Update Rules/UseUsingScopeModifierInNewRunspaces.cs
Jawz84 f8fa89a
Update Rules/UseUsingScopeModifierInNewRunspaces.cs
Jawz84 6ff3405
Process review comments
889b03a
Merge remote-tracking branch 'refs/remotes/origin/1410_foreach_parall…
6758748
Add tests for command name and icm -session
e5e6a58
Add icm -session logic to visitor and tidy up
921c1d2
fix build for windows powershell
f57c610
Revert "fix build for windows powershell"
7956eda
Change private class to internal class for Windows PowerShell
427461a
Add logic to detect DSCScriptResource
4f8bf36
Add tests for DSC Script resource
0cb8f9e
Enhance label test topic
Jawz84 af2ceee
repair test indentation and add newline at eof
52bece8
Merge remote-tracking branch 'refs/remotes/origin/1410_foreach_parall…
29176a8
Move testcases to BeforeAll blocks
b1bee39
Add documentation that DSC Script resource is supported
f6de07d
change string[] to IReadOnlyList<string>
Jawz84 5f6bae0
extract scriptBlockPosition as a variable for readability
Jawz84 bcaba83
put arguments on their own line for readability
1d6b55e
make visitor class a private, nested class in rule
92f19c9
change 'var' to type name for method calls
7943625
fix indentation for nested visitor class
a6acfd7
Remove explicit 'ToList()' for performance
Jawz84 cee3ab7
add check for strongly typed assignments
fe68244
Merge remote-tracking branch 'refs/remotes/origin/1410_foreach_parall…
c4c4fde
Add todo comments
490d8d9
refactor FindAll predicates to static methods
6767a58
Refactor GetSessionName for performance.
c824fd7
use full type for string expression variable
Jawz84 ab64ebf
use full type name for foreach variable initialization
Jawz84 d5b77a9
refactor diagnostic message out to local variable
Jawz84 6c0e90a
WIP: apply review suggestions
fa7c065
apply review suggestions
378fb16
Merge remote-tracking branch 'refs/remotes/origin/1410_foreach_parall…
c1247c0
Fix CR/LF -> LF
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
# AvoidUnInitializedVarsInNewRunspaces | ||
|
||
**Severity Level: Warning** | ||
|
||
## Description | ||
|
||
If a scriptblock is intended to be run as a new runspace, variables inside it should use $using: directive, or be initialized within the scriptblock. | ||
|
||
## How to Fix | ||
|
||
Within `Foreach-Object -Parallel {}`, instead of just using a variable from the parent scope, you have to use the `using:` directive: | ||
|
||
## Example | ||
|
||
### Wrong | ||
|
||
``````PowerShell | ||
$var = "foo" | ||
1..2 | ForEach-Object -Parallel { $var } | ||
`````` | ||
|
||
### Correct | ||
|
||
``````PowerShell | ||
$var = "foo" | ||
1..2 | ForEach-Object -Parallel { $using:var } | ||
`````` |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,173 @@ | ||
// Copyright (c) Microsoft Corporation. All rights reserved. | ||
Jawz84 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// Licensed under the MIT License. | ||
|
||
using System; | ||
using System.Collections; | ||
using System.Collections.Generic; | ||
#if !CORECLR | ||
using System.ComponentModel.Composition; | ||
#endif | ||
using System.Globalization; | ||
using System.Linq; | ||
using System.Management.Automation.Language; | ||
using Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic; | ||
|
||
namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules | ||
{ | ||
/// <summary> | ||
/// AvoidUnInitializedVarsInNewRunspaces: Analyzes the ast to check that variables in script blocks that run in new run spaces are properly initialized or passed in with '$using:(varName)'. | ||
/// </summary> | ||
#if !CORECLR | ||
[Export(typeof(IScriptRule))] | ||
#endif | ||
public class AvoidUnInitializedVarsInNewRunspaces : IScriptRule | ||
{ | ||
/// <summary> | ||
/// AnalyzeScript: Analyzes the ast to check variables in script blocks that will run in new runspaces are properly initialized or passed in with $using: | ||
/// </summary> | ||
/// <param name="ast">The script's ast</param> | ||
/// <param name="fileName">The script's file name</param> | ||
/// <returns>A List of results from this rule</returns> | ||
public IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string fileName) | ||
{ | ||
if (ast == null) | ||
{ | ||
throw new ArgumentNullException(Strings.NullAstErrorMessage); | ||
} | ||
|
||
var scriptBlockAsts = ast.FindAll(x => x is ScriptBlockAst, true); | ||
if (scriptBlockAsts == null) | ||
{ | ||
yield break; | ||
} | ||
|
||
foreach (var scriptBlockAst in scriptBlockAsts) | ||
{ | ||
var sbAst = scriptBlockAst as ScriptBlockAst; | ||
foreach (var diagnosticRecord in AnalyzeScriptBlockAst(sbAst, fileName)) | ||
{ | ||
yield return diagnosticRecord; | ||
} | ||
} | ||
} | ||
|
||
/// <summary> | ||
/// GetName: Retrieves the name of this rule. | ||
/// </summary> | ||
/// <returns>The name of this rule</returns> | ||
public string GetName() | ||
{ | ||
return string.Format(CultureInfo.CurrentCulture, Strings.NameSpaceFormat, GetSourceName(), Strings.AvoidUnInitializedVarsInNewRunspacesName); | ||
} | ||
|
||
/// <summary> | ||
/// GetCommonName: Retrieves the common name of this rule. | ||
/// </summary> | ||
/// <returns>The common name of this rule</returns> | ||
public string GetCommonName() | ||
{ | ||
return string.Format(CultureInfo.CurrentCulture, Strings.AvoidUnInitializedVarsInNewRunspacesCommonName); | ||
} | ||
|
||
/// <summary> | ||
/// GetDescription: Retrieves the description of this rule. | ||
/// </summary> | ||
/// <returns>The description of this rule</returns> | ||
public string GetDescription() | ||
{ | ||
return string.Format(CultureInfo.CurrentCulture, Strings.AvoidUnInitializedVarsInNewRunspacesDescription); | ||
} | ||
|
||
/// <summary> | ||
/// GetSourceType: Retrieves the type of the rule: builtin, managed or module. | ||
/// </summary> | ||
public SourceType GetSourceType() | ||
{ | ||
return SourceType.Builtin; | ||
} | ||
|
||
/// <summary> | ||
/// GetSeverity: Retrieves the severity of the rule: error, warning of information. | ||
/// </summary> | ||
/// <returns></returns> | ||
public RuleSeverity GetSeverity() | ||
{ | ||
return RuleSeverity.Warning; | ||
} | ||
|
||
/// <summary> | ||
/// GetSourceName: Retrieves the module/assembly name the rule is from. | ||
/// </summary> | ||
public string GetSourceName() | ||
{ | ||
return string.Format(CultureInfo.CurrentCulture, Strings.SourceName); | ||
} | ||
|
||
/// <summary> | ||
/// Checks if a variable is initialized and referenced in either its assignment or children scopes | ||
/// </summary> | ||
/// <param name="scriptBlockAst">Ast of type ScriptBlock</param> | ||
/// <param name="fileName">Name of file containing the ast</param> | ||
/// <returns>An enumerable containing diagnostic records</returns> | ||
private IEnumerable<DiagnosticRecord> AnalyzeScriptBlockAst(ScriptBlockAst scriptBlockAst, string fileName) | ||
{ | ||
var foreachObjectCmdletNamesAndAliases = Helper.Instance.CmdletNameAndAliases("Foreach-Object"); | ||
Jawz84 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
// Find all commandAst objects for `Foreach-Object -Parallel`. As for parametername matching, there are three | ||
// parameters starting with a 'p': Parallel, PipelineVariable and Process, so we use startsWith 'pa' as the shortest unambiguous form. | ||
// Because we are already going trough all ScriptBlockAst objects, we do not need to look for nested script blocks here. | ||
if (!(scriptBlockAst.FindAll( | ||
predicate: c => c is CommandAst commandAst && | ||
foreachObjectCmdletNamesAndAliases.Contains(commandAst.GetCommandName(), StringComparer.OrdinalIgnoreCase) && | ||
commandAst.CommandElements.Any( | ||
e => e is CommandParameterAst parameterAst && | ||
parameterAst.ParameterName.StartsWith("pa", StringComparison.OrdinalIgnoreCase)), | ||
searchNestedScriptBlocks: true) is IEnumerable<Ast> foreachObjectParallelAsts)) | ||
{ | ||
yield break; | ||
} | ||
|
||
foreach (var ast in foreachObjectParallelAsts) | ||
{ | ||
var commandAst = ast as CommandAst; | ||
|
||
if (commandAst == null) | ||
{ | ||
continue; | ||
} | ||
|
||
var varsInAssignments = commandAst.FindAll( | ||
predicate: a => a is AssignmentStatementAst assignment && | ||
assignment.Left.FindAll( | ||
predicate: aa => aa is VariableExpressionAst, | ||
searchNestedScriptBlocks: true) != null, | ||
searchNestedScriptBlocks: true); | ||
|
||
var commandElements = commandAst.CommandElements; | ||
var nonAssignedNonUsingVars = new List<Ast>() { }; | ||
foreach (var cmdEl in commandElements) | ||
{ | ||
nonAssignedNonUsingVars.AddRange( | ||
cmdEl.FindAll( | ||
predicate: aa => aa is VariableExpressionAst varAst && | ||
!(varAst.Parent is UsingExpressionAst) && | ||
!varsInAssignments.Contains(varAst), true)); | ||
} | ||
|
||
foreach (var variableExpression in nonAssignedNonUsingVars) | ||
{ | ||
var _temp = variableExpression as VariableExpressionAst; | ||
|
||
yield return new DiagnosticRecord( | ||
message: string.Format(CultureInfo.CurrentCulture, | ||
Strings.UseDeclaredVarsMoreThanAssignmentsError, _temp?.ToString()), | ||
extent: _temp?.Extent, | ||
ruleName: GetName(), | ||
severity: DiagnosticSeverity.Warning, | ||
scriptPath: fileName, | ||
ruleId: _temp?.ToString()); | ||
} | ||
} | ||
} | ||
} | ||
} |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.