diff --git a/Rules/Strings.resx b/Rules/Strings.resx index c7645c9cf..a60f478cb 100644 --- a/Rules/Strings.resx +++ b/Rules/Strings.resx @@ -1236,4 +1236,16 @@ The reserved word '{0}' was used as a function name. This should be avoided. + + Use a single ValueFromPipeline parameter per parameter set + + + Use at most a single ValueFromPipeline parameter per parameter set to avoid undefined or unexpected behaviour. + + + Multiple parameters ({0}) in parameter set '{1}' are marked as ValueFromPipeline. Only one parameter per parameter set should accept pipeline input. + + + UseSingleValueFromPipelineParameter + \ No newline at end of file diff --git a/Rules/UseSingleValueFromPipelineParameter.cs b/Rules/UseSingleValueFromPipelineParameter.cs new file mode 100644 index 000000000..d3f661ce4 --- /dev/null +++ b/Rules/UseSingleValueFromPipelineParameter.cs @@ -0,0 +1,188 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +using Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic; +using System; +using System.Collections.Generic; +using System.Globalization; +using System.Linq; +using System.Management.Automation.Language; +#if !CORECLR +using System.ComponentModel.Composition; +#endif + +namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules +{ +#if !CORECLR + [Export(typeof(IScriptRule))] +#endif + + /// + /// Rule that identifies parameter blocks with multiple parameters in + /// the same parameter set that are marked as ValueFromPipeline=true, which + /// can cause undefined behavior. + /// + public class UseSingleValueFromPipelineParameter : ConfigurableRule + { + private const string AllParameterSetsName = "__AllParameterSets"; + + /// + /// Analyzes the PowerShell AST for parameter sets with multiple ValueFromPipeline parameters. + /// + /// The PowerShell Abstract Syntax Tree to analyze. + /// The name of the file being analyzed (for diagnostic reporting). + /// A collection of diagnostic records for each violating parameter. + public override IEnumerable AnalyzeScript(Ast ast, string fileName) + { + if (ast == null) + { + throw new ArgumentNullException(Strings.NullAstErrorMessage); + } + // Find all param blocks that have a Parameter attribute with + // ValueFromPipeline set to true. + var paramBlocks = ast.FindAll(testAst => testAst is ParamBlockAst, true) + .Where(paramBlock => paramBlock.FindAll( + attributeAst => attributeAst is AttributeAst attr && + ParameterAttributeAstHasValueFromPipeline(attr), + true + ).Any()); + + foreach (var paramBlock in paramBlocks) + { + // Find all parameter declarations in the current param block + // Convert the generic ast objects into ParameterAst Objects + // For each ParameterAst, find all it's attributes that have + // ValueFromPipeline set to true (either explicitly or + // implicitly). Flatten the results into a single collection of + // Annonymous objects relating the parameter with it's attribute + // and then group them by parameter set name. + // + // + // https://learn.microsoft.com/en-us/powershell/module/microsoft.powershell.core/about/about_parameter_sets?#reserved-parameter-set-name + // + // The default parameter set name is '__AllParameterSets'. + // Not specifying a parameter set name and using the parameter + // set name '__AllParameterSets' are equivalent, so we shouldn't + // treat them like they're different just because one is an + // empty string and the other is not. + // + // Filter the list to only keep parameter sets that have more + // than one ValueFromPipeline parameter. + var parameterSetGroups = paramBlock.FindAll(n => n is ParameterAst, true) + .Cast() + .SelectMany(parameter => parameter.FindAll( + a => a is AttributeAst attr && ParameterAttributeAstHasValueFromPipeline(attr), + true + ).Cast().Select(attr => new { Parameter = parameter, Attribute = attr })) + .GroupBy(item => GetParameterSetForAttribute(item.Attribute) ?? AllParameterSetsName) + .Where(group => group.Count() > 1); + + + foreach (var group in parameterSetGroups) + { + // __AllParameterSets being the default name is...obscure. + // Instead we'll show the user "default". It's more than + // likely the user has not specified a parameter set name, + // so default will make sense. If they have used 'default' + // as their parameter set name, then we're still correct. + var parameterSetName = group.Key == AllParameterSetsName ? "default" : group.Key; + + // Create a concatenated string of parameter names that + // conflict in this parameter set + var parameterNames = string.Join(", ", group.Select(item => item.Parameter.Name.VariablePath.UserPath)); + + // We emit a diagnostic record for each offending parameter + // attribute in the parameter set so it's obvious where all the + // occurrences are. + foreach (var item in group) + { + var message = string.Format(CultureInfo.CurrentCulture, + Strings.UseSingleValueFromPipelineParameterError, + parameterNames, + parameterSetName); + + yield return new DiagnosticRecord( + message, + item.Attribute.Extent, + GetName(), + DiagnosticSeverity.Warning, + fileName, + parameterSetName); + } + } + } + } + + /// + /// Returns whether the specified AttributeAst represents a Parameter attribute + /// that has the ValueFromPipeline named argument set to true (either explicitly or + /// implicitly). + /// + /// The Parameter attribute to examine. + /// Whether the attribute has the ValueFromPipeline named argument set to true. + private static bool ParameterAttributeAstHasValueFromPipeline(AttributeAst attributeAst) + { + // Exit quickly if the attribute is null, has no named arguments, or + // is not a parameter attribute. + if (attributeAst?.NamedArguments == null || + !string.Equals(attributeAst.TypeName?.Name, "Parameter", StringComparison.OrdinalIgnoreCase)) + { + return false; + } + + return attributeAst.NamedArguments + .OfType() + .Any(namedArg => string.Equals( + namedArg?.ArgumentName, + "ValueFromPipeline", + StringComparison.OrdinalIgnoreCase + // Helper.Instance.GetNamedArgumentAttributeValue handles both explicit ($true) + // and implicit (no value specified) ValueFromPipeline declarations + ) && Helper.Instance.GetNamedArgumentAttributeValue(namedArg)); + } + + /// + /// Gets the ParameterSetName value from a Parameter attribute. + /// + /// The Parameter attribute to examine. + /// The parameter set name, or null if not found or empty. + private static string GetParameterSetForAttribute(AttributeAst attributeAst) + { + // Exit quickly if the attribute is null, has no named arguments, or + // is not a parameter attribute. + if (attributeAst?.NamedArguments == null || + !string.Equals(attributeAst.TypeName.Name, "Parameter", StringComparison.OrdinalIgnoreCase)) + { + return null; + } + + return attributeAst.NamedArguments + .OfType() + .Where(namedArg => string.Equals( + namedArg?.ArgumentName, + "ParameterSetName", + StringComparison.OrdinalIgnoreCase + )) + .Select(namedArg => namedArg?.Argument) + .OfType() + .Select(stringConstAst => stringConstAst?.Value) + .FirstOrDefault(value => !string.IsNullOrWhiteSpace(value)); + } + + public override string GetCommonName() => Strings.UseSingleValueFromPipelineParameterCommonName; + + public override string GetDescription() => Strings.UseSingleValueFromPipelineParameterDescription; + + public override string GetName() => string.Format( + CultureInfo.CurrentCulture, + Strings.NameSpaceFormat, + GetSourceName(), + Strings.UseSingleValueFromPipelineParameterName); + + public override RuleSeverity GetSeverity() => RuleSeverity.Warning; + + public override string GetSourceName() => Strings.SourceName; + + public override SourceType GetSourceType() => SourceType.Builtin; + } +} \ No newline at end of file diff --git a/Tests/Engine/GetScriptAnalyzerRule.tests.ps1 b/Tests/Engine/GetScriptAnalyzerRule.tests.ps1 index c3b744803..fbd076af5 100644 --- a/Tests/Engine/GetScriptAnalyzerRule.tests.ps1 +++ b/Tests/Engine/GetScriptAnalyzerRule.tests.ps1 @@ -63,7 +63,7 @@ Describe "Test Name parameters" { It "get Rules with no parameters supplied" { $defaultRules = Get-ScriptAnalyzerRule - $expectedNumRules = 71 + $expectedNumRules = 72 if ($PSVersionTable.PSVersion.Major -le 4) { # for PSv3 PSAvoidGlobalAliases is not shipped because diff --git a/Tests/Rules/UseSingleValueFromPipelineParameter.Tests.ps1 b/Tests/Rules/UseSingleValueFromPipelineParameter.Tests.ps1 new file mode 100644 index 000000000..945130b9e --- /dev/null +++ b/Tests/Rules/UseSingleValueFromPipelineParameter.Tests.ps1 @@ -0,0 +1,335 @@ +# Copyright (c) Microsoft Corporation. All rights reserved. +# Licensed under the MIT License. + +BeforeAll { + $ruleName = 'PSUseSingleValueFromPipelineParameter' + + $settings = @{ + IncludeRules = @($ruleName) + Rules = @{ + $ruleName = @{ + Enable = $true + } + } + } +} + +Describe 'UseSingleValueFromPipelineParameter' { + + Context 'When multiple parameters have ValueFromPipeline in same parameter set' { + + It "Should flag explicit ValueFromPipeline=`$true in default parameter set" { + $scriptDefinition = @' +function Test-Function { + param( + [Parameter(ValueFromPipeline=$true)] + $InputObject, + + [Parameter(ValueFromPipeline=$true)] + $AnotherParam + ) +} +'@ + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $settings + $violations.Count | Should -Be 2 + $violations[0].Message | Should -Match "Multiple parameters \(InputObject, AnotherParam\) in parameter set 'default'" + } + + It 'Should flag implicit ValueFromPipeline in default parameter set' { + $scriptDefinition = @' +function Test-Function { + param( + [Parameter(ValueFromPipeline)] + $InputObject, + + [Parameter(ValueFromPipeline)] + $SecondParam + ) +} +'@ + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $settings + $violations.Count | Should -Be 2 + } + + It 'Should flag mixed explicit and implicit ValueFromPipeline' { + $scriptDefinition = @' +function Test-Function { + param( + [Parameter(ValueFromPipeline=$true)] + $InputObject, + + [Parameter(ValueFromPipeline)] + $SecondParam + ) +} +'@ + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $settings + $violations.Count | Should -Be 2 + } + + It 'Should flag multiple parameters in named parameter set' { + $scriptDefinition = @' +function Test-Function { + param( + [Parameter(ValueFromPipeline=$true, ParameterSetName='MySet')] + $InputObject, + + [Parameter(ValueFromPipeline=$true, ParameterSetName='MySet')] + $SecondParam + ) +} +'@ + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $settings + $violations.Count | Should -Be 2 + $violations[0].Message | Should -Match "parameter set 'MySet'" + } + + It 'Should flag three parameters in same parameter set' { + $scriptDefinition = @' +function Test-Function { + param( + [Parameter(ValueFromPipeline=$true)] + $First, + + [Parameter(ValueFromPipeline=$true)] + $Second, + + [Parameter(ValueFromPipeline=$true)] + $Third + ) +} +'@ + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $settings + $violations.Count | Should -Be 3 + $violations[0].Message | Should -Match 'Multiple parameters \(First, Second, Third\)' + } + } + + Context 'When parameters are in different parameter sets' { + + It 'Should not flag parameters in different parameter sets' { + $scriptDefinition = @' +function Test-Function { + param( + [Parameter(ValueFromPipeline=$true, ParameterSetName='Set1')] + $InputObject1, + + [Parameter(ValueFromPipeline=$true, ParameterSetName='Set2')] + $InputObject2 + ) +} +'@ + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $settings + $violations.Count | Should -Be 0 + } + + It 'Should handle mix of named and default parameter sets correctly' { + $scriptDefinition = @' +function Test-Function { + param( + [Parameter(ValueFromPipeline=$true)] + $DefaultSetParam, + + [Parameter(ValueFromPipeline=$true, ParameterSetName='NamedSet')] + $NamedSetParam + ) +} +'@ + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $settings + $violations.Count | Should -Be 0 + } + } + + Context 'When only one parameter has ValueFromPipeline' { + + It 'Should not flag single ValueFromPipeline parameter' { + $scriptDefinition = @' +function Test-Function { + param( + [Parameter(ValueFromPipeline=$true)] + $InputObject, + + [Parameter()] + $OtherParam + ) +} +'@ + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $settings + $violations.Count | Should -Be 0 + } + } + + Context 'When ValueFromPipeline is explicitly set to false' { + + It "Should not flag parameters with ValueFromPipeline=`$false" { + $scriptDefinition = @' +function Test-Function { + param( + [Parameter(ValueFromPipeline=$false)] + $InputObject, + + [Parameter(ValueFromPipeline=$false)] + $AnotherParam + ) +} +'@ + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $settings + $violations.Count | Should -Be 0 + } + + It 'Should only flag the true ValueFromPipeline parameter' { + $scriptDefinition = @' +function Test-Function { + param( + [Parameter(ValueFromPipeline=$true)] + $TrueParam, + + [Parameter(ValueFromPipeline=$false)] + $FalseParam, + + [Parameter()] + $NoValueParam + ) +} +'@ + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $settings + $violations.Count | Should -Be 0 + } + } + + Context 'When non-Parameter attributes have ValueFromPipeline property' { + + It 'Should not flag custom attributes with ValueFromPipeline property' { + $scriptDefinition = @' +function Test-Function { + param( + [Parameter(ValueFromPipeline=$true)] + [CustomAttribute(ValueFromPipeline=$true)] + $InputObject, + + [CustomAttribute(ValueFromPipeline=$true)] + $NonPipelineParam + ) +} +'@ + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $settings + $violations.Count | Should -Be 0 + } + + It 'Should not flag ValidateSet with ValueFromPipeline property' { + $scriptDefinition = @' +function Test-Function { + param( + [Parameter(ValueFromPipeline=$true)] + [ValidateSet('Value1', 'Value2', ValueFromPipeline=$true)] + $InputObject, + + [ValidateSet('Value1', 'Value2', ValueFromPipeline=$true)] + $NonPipelineParam + ) +} +'@ + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $settings + $violations.Count | Should -Be 0 + } + } + + Context 'When there are no Parameter attributes' { + + It 'Should not flag functions without Parameter attributes' { + $scriptDefinition = @' +function Test-Function { + param( + $InputObject, + $AnotherParam + ) +} +'@ + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $settings + $violations.Count | Should -Be 0 + } + + It 'Should not flag functions with only non-ValueFromPipeline Parameter attributes' { + $scriptDefinition = @' +function Test-Function { + param( + [Parameter(Mandatory=$true)] + $InputObject, + + [Parameter(Position=0)] + $AnotherParam + ) +} +'@ + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $settings + $violations.Count | Should -Be 0 + } + } + + Context 'Complex parameter set scenarios' { + + It 'Should flag violations in multiple parameter sets independently' { + $scriptDefinition = @' +function Test-Function { + param( + [Parameter(ValueFromPipeline=$true, ParameterSetName='Set1')] + $Set1Param1, + + [Parameter(ValueFromPipeline=$true, ParameterSetName='Set1')] + $Set1Param2, + + [Parameter(ValueFromPipeline=$true, ParameterSetName='Set2')] + $Set2Param1, + + [Parameter(ValueFromPipeline=$true, ParameterSetName='Set2')] + $Set2Param2 + ) +} +'@ + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $settings + $violations.Count | Should -Be 4 # 2 violations per parameter set, each parameter gets flagged + + # Check that both parameter sets are mentioned in violations + $violationMessages = $violations.Message -join ' ' + $violationMessages | Should -Match "parameter set 'Set1'" + $violationMessages | Should -Match "parameter set 'Set2'" + } + + It 'Should handle __AllParameterSets parameter set name correctly' { + $scriptDefinition = @' +function Test-Function { + param( + [Parameter(ValueFromPipeline=$true, ParameterSetName='__AllParameterSets')] + $ExplicitAllSets, + + [Parameter(ValueFromPipeline=$true)] + $ImplicitAllSets + ) +} +'@ + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $settings + $violations.Count | Should -Be 2 + $violations[0].Message | Should -Match "parameter set 'default'" + } + } + + Context 'Suppression scenarios' { + + It 'Should be suppressible by parameter set name' { + $scriptDefinition = @' +function Test-Function { + [Diagnostics.CodeAnalysis.SuppressMessage('PSUseSingleValueFromPipelineParameter', 'MySet')] + param( + [Parameter(ValueFromPipeline=$true, ParameterSetName='MySet')] + $InputObject, + + [Parameter(ValueFromPipeline=$true, ParameterSetName='MySet')] + $AnotherParam + ) +} +'@ + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $settings + $violations.Count | Should -Be 0 + } + } +} \ No newline at end of file diff --git a/docs/Rules/README.md b/docs/Rules/README.md index da1058bc2..90fb43f6c 100644 --- a/docs/Rules/README.md +++ b/docs/Rules/README.md @@ -76,6 +76,7 @@ The PSScriptAnalyzer contains the following rule definitions. | [UseProcessBlockForPipelineCommand](./UseProcessBlockForPipelineCommand.md) | Warning | Yes | | | [UsePSCredentialType](./UsePSCredentialType.md) | Warning | Yes | | | [UseShouldProcessForStateChangingFunctions](./UseShouldProcessForStateChangingFunctions.md) | Warning | Yes | | +| [UseSingleValueFromPipelineParameter](./UseSingleValueFromPipelineParameter.md) | Warning | No | | | [UseSingularNouns](./UseSingularNouns.md) | Warning | Yes | Yes | | [UseSupportsShouldProcess](./UseSupportsShouldProcess.md) | Warning | Yes | | | [UseToExportFieldsInManifest](./UseToExportFieldsInManifest.md) | Warning | Yes | | diff --git a/docs/Rules/UseSingleValueFromPipelineParameter.md b/docs/Rules/UseSingleValueFromPipelineParameter.md new file mode 100644 index 000000000..9cac391f5 --- /dev/null +++ b/docs/Rules/UseSingleValueFromPipelineParameter.md @@ -0,0 +1,101 @@ +--- +description: Use at most a single ValueFromPipeline parameter per parameter set. +ms.date: 08/08/2025 +ms.topic: reference +title: UseSingleValueFromPipelineParameter +--- +# UseSingleValueFromPipelineParameter + +**Severity Level: Warning** + +## Description + +Parameter sets should have at most one parameter marked as +`ValueFromPipeline=true`. + +This rule identifies functions where multiple parameters within the same +parameter set have `ValueFromPipeline` set to `true` (either explicitly or +implicitly). + +## How + +Ensure that only one parameter per parameter set accepts pipeline input by +value. If you need multiple parameters to accept different types of pipeline +input, use separate parameter sets. + +## Example + +### Wrong + +```powershell +function Process-Data { + [CmdletBinding()] + param( + [Parameter(ValueFromPipeline)] + [string]$InputData, + + [Parameter(ValueFromPipeline)] + [string]$ProcessingMode + ) + + process { + Write-Output "$ProcessingMode`: $InputData" + } +} +``` + + +### Correct + +```powershell +function Process-Data { + [CmdletBinding()] + param( + [Parameter(ValueFromPipeline)] + [string]$InputData, + + [Parameter(Mandatory)] + [string]$ProcessingMode + ) + process { + Write-Output "$ProcessingMode`: $InputData" + } +} +``` +## Suppression + +To suppress this rule for a specific parameter set, use the `SuppressMessage` +attribute with the parameter set name: + +```powershell +function Process-Data { + [Diagnostics.CodeAnalysis.SuppressMessage('PSUseSingleValueFromPipelineParameter', 'MyParameterSet')] + [CmdletBinding()] + param( + [Parameter(ValueFromPipeline, ParameterSetName='MyParameterSet')] + [string]$InputData, + + [Parameter(ValueFromPipeline, ParameterSetName='MyParameterSet')] + [string]$ProcessingMode + ) + process { + Write-Output "$ProcessingMode`: $InputData" + } +} +``` + +For the default parameter set, use `'default'` as the suppression target: + +```powershell +[Diagnostics.CodeAnalysis.SuppressMessage('PSUseSingleValueFromPipelineParameter', 'default')] +``` + +## Notes + +- This rule applies to both explicit `ValueFromPipeline=$true` and implicit + `ValueFromPipeline` (which is the same as using `=$true`) +- Parameters with `ValueFromPipeline=$false` are not flagged by this rule +- The rule correctly handles the default parameter set (`__AllParameterSets`) + and named parameter sets +- Different parameter sets can each have their own single `ValueFromPipeline` + parameter without triggering this rule