Add UseUsingScopeModifierInNewRunspaces rule (#1419)

* Add tests for AvoidUnInitializedVarsInNewRunspaces * Add strings for AvoidUnInitializedVarsInNewRunspaces * Add documentation * wrestling with Ast in C# - not working * Add sensible warning message * remove unnecessary boilerplate from test * clean up and finetune rule * Add RuleToTest parameter to Test-ScriptAnalyzer to aid with test driven rule development * increment rule count to fix test * add reference to rule documentation * exclude built-in variables * change using directive => scoope modifier * add tests for InlineScript, Invoke-Command and Start-(Thread)Job * add rule implementation for InlineScript, Invoke-Command and Start-(Thread)Job * Refactor and cleanup * small cleanup * explain all applicable situations in documentation * simplify code for adding sessions to dict * Revert "Add RuleToTest parameter to Test-ScriptAnalyzer to aid with test driven rule development" This reverts commit 99c2bea. Undo -RuleToTest param * Rename rule to UseUsingScopeModifierInNewRunspaces Because this has a posive ring to it and points the user to the solution * refactor grouping of script blocks by session name * Add suggested correction implementation * Add test for suggested corrections, fix typos * Refactor to AstVisitor/AstVisitor2 WIP TODO: finish refactor for `Invoke-Command -Session` logic * Update Rules/UseUsingScopeModifierInNewRunspaces.cs Co-Authored-By: Robert Holt <rjmholt@gmail.com> * Update Rules/UseUsingScopeModifierInNewRunspaces.cs Co-Authored-By: Robert Holt <rjmholt@gmail.com> * Process review comments - Always use braces with if statements - Use ast.VariablePath.UserPath instead of ast.Extent.Text - simplify return list of corrections - add TODO's fo invoke-command-session code - Add TODO for commandAst.GetCommandName() can be null - Invert if for readability * Add tests for command name and icm -session * Add icm -session logic to visitor and tidy up * fix build for windows powershell * Revert "fix build for windows powershell" This reverts commit 921c1d2. revert fix, because multiple unintended changes were pushed along with it. * Change private class to internal class for Windows PowerShell * Add logic to detect DSCScriptResource * Add tests for DSC Script resource * Enhance label test topic Co-Authored-By: Robert Holt <rjmholt@gmail.com> * repair test indentation and add newline at eof * Move testcases to BeforeAll blocks * Add documentation that DSC Script resource is supported * change string[] to IReadOnlyList<string> Co-Authored-By: Robert Holt <rjmholt@gmail.com> * extract scriptBlockPosition as a variable for readability Co-Authored-By: Robert Holt <rjmholt@gmail.com> * put arguments on their own line for readability * make visitor class a private, nested class in rule * change 'var' to type name for method calls * fix indentation for nested visitor class * Remove explicit 'ToList()' for performance Co-Authored-By: Robert Holt <rjmholt@gmail.com> * add check for strongly typed assignments * Add todo comments * refactor FindAll predicates to static methods to avoid closure allocation * Refactor GetSessionName for performance. * use full type for string expression variable Co-Authored-By: Robert Holt <rjmholt@gmail.com> * use full type name for foreach variable initialization Co-Authored-By: Robert Holt <rjmholt@gmail.com> * refactor diagnostic message out to local variable Co-Authored-By: Robert Holt <rjmholt@gmail.com> * WIP: apply review suggestions refactoring FindVarsInAssignmentAsts to return a dictionary in progress * apply review suggestions refactoredFindVarsInAssignmentAsts to return a dictionary * Fix CR/LF -> LF Co-authored-by: Jos Koelewijn <Jos.Koelewijn@ogd.nl> Co-authored-by: Robert Holt <rjmholt@gmail.com>
PowerShell · Mar 24, 2020 · 791ee64 · 791ee64
1 parent c49fb06
commit 791ee64
Show file tree

Hide file tree

Showing 7 changed files with 940 additions and 10 deletions.
diff --git a/RuleDocumentation/README.md b/RuleDocumentation/README.md
@@ -16,6 +16,7 @@
 |[AvoidOverwritingBuiltInCmdlets](./AvoidOverwritingBuiltInCmdlets.md) | Warning | |
 |[AvoidNullOrEmptyHelpMessageAttribute](./AvoidNullOrEmptyHelpMessageAttribute.md) | Warning | |
 |[AvoidShouldContinueWithoutForce](./AvoidShouldContinueWithoutForce.md) | Warning | |
+|[UseUsingScopeModifierInNewRunspaces](./UseUsingScopeModifierInNewRunspaces.md) | Warning | |
 |[AvoidUsingCmdletAliases](./AvoidUsingCmdletAliases.md) | Warning | Yes |
 |[AvoidUsingComputerNameHardcoded](./AvoidUsingComputerNameHardcoded.md) | Error | |
 |[AvoidUsingConvertToSecureStringWithPlainText](./AvoidUsingConvertToSecureStringWithPlainText.md) | Error | |

diff --git a/RuleDocumentation/UseUsingScopeModifierInNewRunspaces.md b/RuleDocumentation/UseUsingScopeModifierInNewRunspaces.md
@@ -0,0 +1,65 @@
+# UseUsingScopeModifierInNewRunspaces
+
+**Severity Level: Warning**
+
+## Description
+
+If a ScriptBlock is intended to be run in a new RunSpace, variables inside it should use $using: scope modifier, or be initialized within the ScriptBlock.
+This applies to:
+
+- Invoke-Command *
+- Workflow { InlineScript {}}
+- Foreach-Object **
+- Start-Job
+- Start-ThreadJob
+- The `Script` resource in DSC configurations, specifically for the `GetScript`, `TestScript` and `SetScript` properties
+
+\* Only with the -ComputerName or -Session parameter.  
+\*\* Only with the -Parallel parameter
+
+## How to Fix
+
+Within the ScriptBlock, instead of just using a variable from the parent scope, you have to add the `using:` scope modifier to it.
+
+## Example
+
+### Wrong
+
+```PowerShell
+$var = "foo"
+1..2 | ForEach-Object -Parallel { $var }
+```
+
+### Correct
+
+```PowerShell
+$var = "foo"
+1..2 | ForEach-Object -Parallel { $using:var }
+```
+
+## More correct examples
+
+```powershell
+$bar = "bar"
+Invoke-Command -ComputerName "foo" -ScriptBlock { $using:bar }
+```
+
+```powershell
+$bar = "bar"
+$s = New-PSSession -ComputerName "foo"
+Invoke-Command -Session $s -ScriptBlock { $using:bar }
+```
+
+```powershell
+# Remark: Workflow is supported on Windows PowerShell only
+Workflow { 
+    $foo = "foo"
+    InlineScript { $using:foo }
+}
+```
+
+```powershell
+$foo = "foo"
+Start-ThreadJob -ScriptBlock { $using:foo }
+Start-Job -ScriptBlock {$using:foo }
+```
diff --git a/Rules/Strings.Designer.cs b/Rules/Strings.Designer.cs
diff --git a/Rules/Strings.resx b/Rules/Strings.resx
@@ -1114,12 +1114,27 @@
     <value>ReviewUnusedParameter</value>
   </data>
   <data name="ReviewUnusedParameterDescription" xml:space="preserve">
-    <value>Ensure all parameters are used within the same script, scriptblock, or function where they are declared.</value>	
-  </data>	
-  <data name="ReviewUnusedParameterError" xml:space="preserve">	
-    <value>The parameter '{0}' has been declared but not used. </value>	
-  </data>	
-  <data name="ReviewUnusedParameterName" xml:space="preserve">	
-    <value>ReviewUnusedParameter</value>	
-  </data>
-</root>
+    <value>Ensure all parameters are used within the same script, scriptblock, or function where they are declared.</value>
+  </data>
+  <data name="ReviewUnusedParameterError" xml:space="preserve">
+    <value>The parameter '{0}' has been declared but not used. </value>
+  </data>
+  <data name="ReviewUnusedParameterName" xml:space="preserve">
+    <value>ReviewUnusedParameter</value>
+  </data>
+  <data name="UseUsingScopeModifierInNewRunspacesCommonName" xml:space="preserve">
+    <value>Use 'Using:' scope modifier in RunSpace ScriptBlocks</value>
+  </data>
+  <data name="UseUsingScopeModifierInNewRunspacesDescription" xml:space="preserve">
+    <value>If a ScriptBlock is intended to be run as a new RunSpace, variables inside it should use 'Using:' scope modifier, or be initialized within the ScriptBlock.</value>
+  </data>
+  <data name="UseUsingScopeModifierInNewRunspacesName" xml:space="preserve">
+    <value>UseUsingScopeModifierInNewRunspaces</value>
+  </data>
+  <data name="UseUsingScopeModifierInNewRunspacesError" xml:space="preserve">
+    <value>The variable '{0}' is not declared within this ScriptBlock, and is missing the 'Using:' scope modifier.</value>
+  </data>
+  <data name="UseUsingScopeModifierInNewRunspacesCorrectionDescription" xml:space="preserve">
+    <value>Replace {0} with {1}</value>
+  </data>
+</root>