PSAvoidTrapStatement #188

Closed
KirkMunro opened this Issue May 21, 2015 · 4 comments

Projects

None yet

4 participants

@KirkMunro

The PSAvoidTrapStatement causes a warning to be raised whenever the trap statement is used. But trap has not been deprecated, and it actually results in cleaner code if you are trapping errors in the top level of a file. For example, consider a trap statement in the top of a psm1 file that looks like this:

trap {throw $_}

I use this in my psm1 files as a best practice because it ensures that if a terminating error is thrown from within my script module code, the error is actually treated as terminating and the module load fails properly with the module not loaded after Import-Module is called. Without that statement, if my psm1 file were to throw a terminating error the error would not be considered terminating, PowerShell would report the error to the console, but the module would "load", in some unknown/unexpected/undesirable state.

I can accomplish the same goal by surrounding the entire psm1 file body with this:

try {
# body goes here
} catch {
throw
}

That is what PSAvoidTrapStatement is trying to recommend, that I use try/catch instead of trap, however that means I need to indent my entire module just to tell PowerShell that I want terminating errors treated as terminating errors. It's more work (trap is much easier to type), and it ends up messing around with my entire script module layout rather than simply including a line at the top of the script that handles it all.

With this in mind, trap is a much better choice.

Further, since trap is not deprecated, unless it is somehow more limited than the equivalent try/catch in this scenario or others, why discourage its use?

At a minimum, PSAvoidTrapStatement should be downgraded from a Warning to Informational. But I question the use of this rule at all, so it would be worth considering removing it altogether.

@yutingc yutingc added the Discussion label May 21, 2015
@nightroman

I also find this rule strange. Is it even useful? What is its value then, what problems does it solve?

All that AvoidTrapStatement.md says about trap is

It is designed for administrators. For script developers, you should use try-catch-finally statement.

This is hardly a proper explanation. To be honest, I am not even sure what it really means.

Personally, I mostly use try-catch-finally. But there are cases when trap is better.

@nightroman

Besides, try-catch is not exactly a replacement for all trap use cases. For example, trap { ... ; continue } cannot be achieved with try-catch.

@yutingc
Collaborator
yutingc commented May 26, 2015

Hi all, as we discussed today in the meeting, we are going to remove this rule from PSScriptAnalyzer built-in rules. Thank you.

@raghushantha
Member

Thank you all for the input. This rule has been removed.

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