Skip to content
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

Need test to catch Pipeline-able input with missing process block #1368

Closed
EklipZgit opened this issue Nov 6, 2019 · 5 comments
Closed

Need test to catch Pipeline-able input with missing process block #1368

EklipZgit opened this issue Nov 6, 2019 · 5 comments

Comments

@EklipZgit
Copy link

EklipZgit commented Nov 6, 2019

Currently you can create functions / cmdlets with parameters with ValueFromPipeline or ValueFromPipelineByPropertyName, but if you forget the process { } block, only the last pipeline object will be processed by the cmdlet, instead of all of them, resulting in unexpected behavior that can slip through the cracks unnoticed until you pipe a batch of objects to the commandlet and realize that it only processed one of them. PSScriptAnalyzer should default to warning developers to use the process { } block when they use ValueFromPipeline or ValueFromPipelineByPropertyName attributes in a function / cmdlet.

Function Broken
{
	[CmdletBinding()]
	Param(
		[Parameter(ValueFromPipeline)]
		[int]
		$ToPrint
	)
	$ToPrint
}

Function Correct
{
	[CmdletBinding()]
	Param(
		[Parameter(ValueFromPipeline)]
		[int]
		$ToPrint
	)
	process
	{
		$ToPrint	
	}
}

1..10 | Broken
# only 10 was output. PSScriptAnalyzer ought to be catching this

1..10 | Correct
# 1 - 10 were each output, as you would expect
@bergmeister
Copy link
Collaborator

@rjmholt Is there a reason the Parser does not throw in this case?

@rjmholt
Copy link
Contributor

rjmholt commented Nov 8, 2019

Is there a reason the Parser does not throw in this case?

The syntax here is perfectly valid, so it's not a syntax error. More than that, it's not what PowerShell deems a semantic error either, since those are things that are valid syntax but can't possibly have a non-error behaviour at runtime (for example function Hi { param([void]$Str) $Str } presents a semantic error claiming to be a ParserError). Perhaps it should be in the latter cases but, because PowerShell currently supports parsing this syntax without error, it's likely the behaviour can't be changed.

Instead, we just need to write a simple rule that, when there's no process block on a ScriptBlockAst, looks for parameters marked with ValueFromPipeline.

@mattmcnabb
Copy link
Contributor

I'd like to throw my hat in the ring here - please consider PR #1373 to address this issue.

@mattmcnabb
Copy link
Contributor

mattmcnabb commented Nov 16, 2019

@EklipZgit I plagiarized your example code in the help doc for this rule - hope you don't mind.

bergmeister pushed a commit that referenced this issue Dec 9, 2019
…#1373)

* Add rule for missing process block #1368

* Fix build by regenerating strongly typed files by running '.\New-StronglyTypedCsFileForResx.ps1 Rules'

* Update strongly typed resources via Visual Studio

* minimise diff in resx file

* refactor UseProcessBlockForPipelineCommands rule

* add tests and docs for UseProcessBlockForPipelineCommands rule

* Fix logic non-pipeline function and add test case

* fixing some test failures

* incrememt number of expected rules

* fix style suggestions for PR #1373

* fix rule readme.md

* clean diff for Strings.resx to avoid future merge conflicts

* Fix last 2 rule documentation test failures by fixing markdown link

* Update Rules/Strings.resx

Co-Authored-By: Christoph Bergmeister [MVP] <c.bergmeister@gmail.com>

* Update Rules/Strings.resx

Co-Authored-By: Christoph Bergmeister [MVP] <c.bergmeister@gmail.com>

* Update Rules/Strings.resx

Co-Authored-By: Christoph Bergmeister [MVP] <c.bergmeister@gmail.com>

* empty-commit to re-trigger CI due to sporadic failure

* Update Rules/Strings.resx

Co-Authored-By: Robert Holt <rjmholt@gmail.com>

* Update Rules/Strings.resx

Co-Authored-By: Robert Holt <rjmholt@gmail.com>

* corrections to use process block rule and tests - candidate for merge

* More style fixes for #1373

* update localized strings

* replace implicitly typed vars
@EklipZgit
Copy link
Author

Awesome, thanks for taking a look at this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants