diff --git a/RuleDocumentation/README.md b/RuleDocumentation/README.md index e9d3f66ce..b13e29c50 100644 --- a/RuleDocumentation/README.md +++ b/RuleDocumentation/README.md @@ -50,6 +50,7 @@ |[UseDeclaredVarsMoreThanAssignments](./UseDeclaredVarsMoreThanAssignments.md) | Warning | | |[UseLiteralInitializerForHashtable](./UseLiteralInitializerForHashtable.md) | Warning | | |[UseOutputTypeCorrectly](./UseOutputTypeCorrectly.md) | Information | | +|[UseProcessBlockForPipelineCommand](./UseProcessBlockForPipelineCommand.md) | Warning | | |[UsePSCredentialType](./UsePSCredentialType.md) | Warning | | |[UseShouldProcessForStateChangingFunctions](./UseShouldProcessForStateChangingFunctions.md) | Warning | | |[UseSingularNouns*](./UseSingularNouns.md) | Warning | | diff --git a/RuleDocumentation/UseProcessBlockForPipelineCommand.md b/RuleDocumentation/UseProcessBlockForPipelineCommand.md new file mode 100644 index 000000000..d66dc6f72 --- /dev/null +++ b/RuleDocumentation/UseProcessBlockForPipelineCommand.md @@ -0,0 +1,62 @@ +# UseProcessBlockForPipelineCommand + +**Severity Level: Warning** + +## Description + +Functions that support pipeline input should always handle parameter input in a process block. Unexpected behavior can result if input is handled directly in the body of a function where parameters declare pipeline support. + +## Example + +### Wrong + +``` PowerShell +Function Get-Number +{ + [CmdletBinding()] + Param( + [Parameter(ValueFromPipeline)] + [int] + $Number + ) + + $Number +} +``` + +#### Result + +``` +PS C:\> 1..5 | Get-Number +5 +``` + +### Correct + +``` PowerShell +Function Get-Number +{ + [CmdletBinding()] + Param( + [Parameter(ValueFromPipeline)] + [int] + $Number + ) + + process + { + $Number + } +} +``` + +#### Result + +``` +PS C:\> 1..5 | Get-Number +1 +2 +3 +4 +5 +``` diff --git a/Rules/Strings.Designer.cs b/Rules/Strings.Designer.cs index 42d5a846e..ad071a5a1 100644 --- a/Rules/Strings.Designer.cs +++ b/Rules/Strings.Designer.cs @@ -19,7 +19,7 @@ namespace Microsoft.Windows.PowerShell.ScriptAnalyzer { // class via a tool like ResGen or Visual Studio. // To add or remove a member, edit your .ResX file then rerun ResGen // with the /str option, or rebuild your VS project. - [global::System.CodeDom.Compiler.GeneratedCodeAttribute("System.Resources.Tools.StronglyTypedResourceBuilder", "15.0.0.0")] + [global::System.CodeDom.Compiler.GeneratedCodeAttribute("System.Resources.Tools.StronglyTypedResourceBuilder", "16.0.0.0")] [global::System.Diagnostics.DebuggerNonUserCodeAttribute()] [global::System.Runtime.CompilerServices.CompilerGeneratedAttribute()] internal class Strings { @@ -420,6 +420,42 @@ internal static string AvoidInvokingEmptyMembersName { } } + /// + /// Looks up a localized string similar to Avoid long lines. + /// + internal static string AvoidLongLinesCommonName { + get { + return ResourceManager.GetString("AvoidLongLinesCommonName", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to Line lengths should be less than the configured maximum. + /// + internal static string AvoidLongLinesDescription { + get { + return ResourceManager.GetString("AvoidLongLinesDescription", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to Line exceeds the configured maximum length of {0} characters. + /// + internal static string AvoidLongLinesError { + get { + return ResourceManager.GetString("AvoidLongLinesError", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to AvoidLongLines. + /// + internal static string AvoidLongLinesName { + get { + return ResourceManager.GetString("AvoidLongLinesName", resourceCulture); + } + } + /// /// Looks up a localized string similar to Avoid using null or empty HelpMessage parameter attribute.. /// @@ -536,61 +572,6 @@ internal static string AvoidTrailingWhitespaceName { return ResourceManager.GetString("AvoidTrailingWhitespaceName", resourceCulture); } } - - /// - /// Looks up a localized string similar to AvoidLongLines. - /// - internal static string AvoidLongLinesName - { - get - { - return ResourceManager.GetString("AvoidLongLinesName", resourceCulture); - } - } - - /// - /// Looks up a localized string similar to Avoid long lines. - /// - internal static string AvoidLongLinesCommonName - { - get - { - return ResourceManager.GetString("AvoidLongLinesCommonName", resourceCulture); - } - } - - /// - /// Looks up a localized string similar to Each line should be under 120 characters. - /// - internal static string AvoidLongLinesDescription - { - get - { - return ResourceManager.GetString("AvoidLongLinesDescription", resourceCulture); - } - } - - /// - /// Looks up a localized string similar to Line is longer than 120 characters. - /// - internal static string AvoidLongLinesError - { - get - { - return ResourceManager.GetString("AvoidLongLinesError", resourceCulture); - } - } - - /// - /// Looks up a localized string similar to AvoidLongLines. - /// - internal static string AvoidLongLinesWhitespaceName - { - get - { - return ResourceManager.GetString("AvoidLongLinesName", resourceCulture); - } - } /// /// Looks up a localized string similar to Module Must Be Loadable. @@ -2617,6 +2598,42 @@ internal static string UseOutputTypeCorrectlyName { } } + /// + /// Looks up a localized string similar to Use process block for command that accepts input from pipeline.. + /// + internal static string UseProcessBlockForPipelineCommandCommonName { + get { + return ResourceManager.GetString("UseProcessBlockForPipelineCommandCommonName", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to If a command parameter takes its value from the pipeline, the command must use a process block to bind the input objects from the pipeline to that parameter.. + /// + internal static string UseProcessBlockForPipelineCommandDescription { + get { + return ResourceManager.GetString("UseProcessBlockForPipelineCommandDescription", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to Command accepts pipeline input but has not defined a process block.. + /// + internal static string UseProcessBlockForPipelineCommandError { + get { + return ResourceManager.GetString("UseProcessBlockForPipelineCommandError", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to UseProcessBlockForPipelineCommand. + /// + internal static string UseProcessBlockForPipelineCommandName { + get { + return ResourceManager.GetString("UseProcessBlockForPipelineCommandName", resourceCulture); + } + } + /// /// Looks up a localized string similar to Use PSCredential type.. /// diff --git a/Rules/Strings.resx b/Rules/Strings.resx index dd5825e53..a11cf0922 100644 --- a/Rules/Strings.resx +++ b/Rules/Strings.resx @@ -1080,4 +1080,16 @@ UseCorrectCasing + + Use process block for command that accepts input from pipeline. + + + If a command parameter takes its value from the pipeline, the command must use a process block to bind the input objects from the pipeline to that parameter. + + + Command accepts pipeline input but has not defined a process block. + + + UseProcessBlockForPipelineCommand + diff --git a/Rules/UseProcessBlockForPipelineCommand.cs b/Rules/UseProcessBlockForPipelineCommand.cs new file mode 100644 index 000000000..4b2298855 --- /dev/null +++ b/Rules/UseProcessBlockForPipelineCommand.cs @@ -0,0 +1,98 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +using System; +using System.Collections.Generic; +using System.Management.Automation.Language; +using Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic; +#if !CORECLR +using System.ComponentModel.Composition; +#endif +using System.Globalization; + +namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules +{ +#if !CORECLR + [Export(typeof(IScriptRule))] +#endif + public class UseProcessBlockForPipelineCommand : IScriptRule + { + public IEnumerable AnalyzeScript(Ast ast, string fileName) + { + if (ast == null) + { + throw new ArgumentNullException(Strings.NullAstErrorMessage); + } + + IEnumerable scriptblockAsts = ast.FindAll(testAst => testAst is ScriptBlockAst, true); + + foreach (ScriptBlockAst scriptblockAst in scriptblockAsts) + { + if (scriptblockAst.ProcessBlock != null + || scriptblockAst.ParamBlock?.Attributes == null + || scriptblockAst.ParamBlock.Parameters == null) + { + continue; + } + + foreach (ParameterAst paramAst in scriptblockAst.ParamBlock.Parameters) + { + foreach (AttributeBaseAst paramAstAttribute in paramAst.Attributes) + { + if (!(paramAstAttribute is AttributeAst paramAttributeAst)) { continue; } + + if (paramAttributeAst.NamedArguments == null) { continue; } + + foreach (NamedAttributeArgumentAst namedArgument in paramAttributeAst.NamedArguments) + { + if (!namedArgument.ArgumentName.Equals("valuefrompipeline", StringComparison.OrdinalIgnoreCase) + && !namedArgument.ArgumentName.Equals("valuefrompipelinebypropertyname", StringComparison.OrdinalIgnoreCase)) + { + continue; + } + + yield return new DiagnosticRecord( + string.Format(CultureInfo.CurrentCulture, Strings.UseProcessBlockForPipelineCommandError, paramAst.Name.VariablePath.UserPath), + paramAst.Name.Extent, + GetName(), + DiagnosticSeverity.Warning, + fileName, + paramAst.Name.VariablePath.UserPath + ); + } + } + } + } + } + + public string GetName() + { + return string.Format(CultureInfo.CurrentCulture, Strings.NameSpaceFormat, GetSourceName(), Strings.UseProcessBlockForPipelineCommandName); + } + + public string GetCommonName() + { + return string.Format(CultureInfo.CurrentCulture, Strings.UseProcessBlockForPipelineCommandCommonName); + } + + public string GetDescription() + { + return string.Format(CultureInfo.CurrentCulture, Strings.UseProcessBlockForPipelineCommandDescription); + } + + public SourceType GetSourceType() + { + return SourceType.Builtin; + } + + public RuleSeverity GetSeverity() + { + return RuleSeverity.Warning; + } + + public string GetSourceName() + { + return string.Format(CultureInfo.CurrentCulture, Strings.SourceName); + } + } +} diff --git a/Tests/Engine/GetScriptAnalyzerRule.tests.ps1 b/Tests/Engine/GetScriptAnalyzerRule.tests.ps1 index 8b68c63f3..25d2bcc69 100644 --- a/Tests/Engine/GetScriptAnalyzerRule.tests.ps1 +++ b/Tests/Engine/GetScriptAnalyzerRule.tests.ps1 @@ -59,7 +59,7 @@ Describe "Test Name parameters" { It "get Rules with no parameters supplied" { $defaultRules = Get-ScriptAnalyzerRule - $expectedNumRules = 60 + $expectedNumRules = 61 if ((Test-PSEditionCoreClr) -or (Test-PSVersionV3) -or (Test-PSVersionV4)) { # for PSv3 PSAvoidGlobalAliases is not shipped because diff --git a/Tests/Rules/UseProcessBlockForPipelineCommand.tests.ps1 b/Tests/Rules/UseProcessBlockForPipelineCommand.tests.ps1 new file mode 100644 index 000000000..c4caf2c0c --- /dev/null +++ b/Tests/Rules/UseProcessBlockForPipelineCommand.tests.ps1 @@ -0,0 +1,57 @@ +Describe "UseProcessBlockForPipelineCommand" { + BeforeAll { + $RuleName = 'PSUseProcessBlockForPipelineCommand' + } + + Context "When there are violations" { + $Cases = @( + @{ + ScriptDefinition = 'function BadFunc1 { [CmdletBinding()] param ([Parameter(ValueFromPipeline)]$Param1) }' + Name = "function without process block" + } + @{ + ScriptDefinition = 'function $BadFunc2 { [CmdletBinding()] param ([Parameter(ValueFromPipelineByPropertyName)]$Param1) }' + Name = "function without process block by property name" + } + @{ + ScriptDefinition = '{ [CmdletBinding()] param ([Parameter(ValueFromPipeline)]$Param1) }' + Name = "scriptblock without process block" + } + ) + It "has 1 violation for " -TestCases $Cases { + param ($ScriptDefinition) + + Invoke-ScriptAnalyzer -ScriptDefinition $ScriptDefinition -IncludeRule $RuleName | Should -Not -BeNullOrEmpty + } + } + + Context "When there are no violations" { + $Cases = @( + @{ + ScriptDefinition = 'function GoodFunc1 { [CmdletBinding()] param ([Parameter(ValueFromPipeline)]$Param1) process { } }' + Name = "function with process block" + } + @{ + ScriptDefinition = 'function GoodFunc2 { [CmdletBinding()] param ([Parameter(ValueFromPipelineByPropertyName)]$Param1) process { } }' + Name = "function with process block by property name" + } + @{ + ScriptDefinition = 'function GoodFunc3 { [CmdletBinding()] param ([Parameter()]$Param1) }' + Name = "function without pipeline" + } + @{ + ScriptDefinition = 'function GoodFunc3 { [CmdletBinding()] param ([Parameter()][string]$Param1) }' + Name = "function with parameter type name" + } + @{ + ScriptDefinition = '{ [CmdletBinding()] param ([Parameter(ValueFromPipeline)]$Param1) process { } }' + Name = "scriptblock with process block" + } + ) + It "has no violations for function " -TestCases $Cases { + param ($ScriptDefinition) + + Invoke-ScriptAnalyzer -ScriptDefinition $ScriptDefinition -IncludeRule $RuleName | Should -BeNullOrEmpty + } + } +}