Should the variable be renamed to exception?

I agree that the name should be replaced (if we'll keep the try-catch). I did search in all files - usually we use "catch (Exception e)" pattern.

exception would be a better, more descriptive name, even for a local variable with limited scope.

Would '{0}' is not supported on this platform. be better (to me it feels more consistent in terms of language compared to similar errors in .Net Core).

@bergmeister It is out the PR. Please push new PR or open new tracking issue.

I recommend not using Exception here; it's a bad practice. DeleteFileStream throws Win32Exception for File IO errors and those are what you want to catch. you can avoid the ArgumentNullException by annotation the cmdlet argument property to require non-null. that will provide a better user error than ArgumentNullException.

you can avoid the ArgumentNullException

Path and LiteralPath parameters is mandatory in the cmdlet that require non-null.

I recommend not using Exception here; it's a bad practice

I started with this but revert the change because seems we always wrap system exceptions (like Win32Exception) in PowerShell custom ones to get a customized message and keep system exception as InnerException.
Why should we do otherwise here?

The questions you should ask yourself...

• Will you provide a usable error message for every type of exception that can be throw?
  Should every cmdlet implement a policy that rightfully belongs in PowerShell?

This question comes up over and over and, too often, it's punted by using catch (Exception).
In my opinion, if there are specific exceptions where you can provide value (targeted user message, retry logic, fallback logic, etc.), then, of course, handle the exception.

If you cannot, then you should allow PowerShell to provide common behavior across cmdlets instead of replicating code and inconsistent logic.

Here's a good discussion about why it's often bad to catch Exception:
https://blogs.msdn.microsoft.com/dotnet/2009/02/19/why-catchexceptionempty-catch-is-bad/

@dantraMSFT Thanks! I didn't know that C# caught all system exceptions.

If we remove the try-catch the error message will be different only in CategoryInfo (PermissionDenied -> NotSpecified):

// before
writeErrorStream      : True
PSMessageDetails      :
Exception             : System.UnauthorizedAccessException: Access to the path 'C:\tmp\q.txt:Zone.Identifier' is
                        denied.
                           at System.IO.FileSystem.DeleteFile(String fullPath)
                           at System.IO.File.Delete(String path)
                           at
                        System.Management.Automation.Internal.AlternateDataStreamUtilities.DeleteFileStream(String
                        path, String streamName) in C:\Users\sie\Documents\GitHub\iSazonov\PowerShell\src\System.Manage
                        ment.Automation\namespaces\FileSystemProvider.cs:line 8544
                           at Microsoft.PowerShell.Commands.UnblockFileCommand.ProcessRecord() in C:\Users\sie\Document
                        s\GitHub\iSazonov\PowerShell\src\Microsoft.PowerShell.Commands.Utility\commands\utility\Unblock
                        File.cs:line 120
TargetObject          : C:\tmp\q.txt
CategoryInfo          : PermissionDenied: (C:\tmp\q.txt:String) [Unblock-File], UnauthorizedAccessException
FullyQualifiedErrorId : RemoveItemUnauthorizedAccessError,Microsoft.PowerShell.Commands.UnblockFileCommand
ErrorDetails          :
InvocationInfo        : System.Management.Automation.InvocationInfo
ScriptStackTrace      : at <ScriptBlock>, <No file>: line 1
PipelineIterationInfo : {0, 1}

-----------------------------
// after removing try-catch
PSMessageDetails      :
Exception             : System.UnauthorizedAccessException: Access to the path 'C:\tmp\q.txt:Zone.Identifier' is
                        denied.
                           at System.IO.FileSystem.DeleteFile(String fullPath)
                           at System.IO.File.Delete(String path)
                           at
                        System.Management.Automation.Internal.AlternateDataStreamUtilities.DeleteFileStream(String
                        path, String streamName) in C:\Users\sie\Documents\GitHub\iSazonov\PowerShell\src\System.Manage
                        ment.Automation\namespaces\FileSystemProvider.cs:line 8544
                           at Microsoft.PowerShell.Commands.UnblockFileCommand.ProcessRecord() in C:\Users\sie\Document
                        s\GitHub\iSazonov\PowerShell\src\Microsoft.PowerShell.Commands.Utility\commands\utility\Unblock
                        File.cs:line 118
                           at System.Management.Automation.Cmdlet.DoProcessRecord() in C:\Users\sie\Documents\GitHub\iS
                        azonov\PowerShell\src\System.Management.Automation\engine\cmdlet.cs:line 172
                           at System.Management.Automation.CommandProcessor.ProcessRecord() in C:\Users\sie\Documents\G
                        itHub\iSazonov\PowerShell\src\System.Management.Automation\engine\CommandProcessor.cs:line 358
TargetObject          :
CategoryInfo          : NotSpecified: (:) [Unblock-File], UnauthorizedAccessException
FullyQualifiedErrorId : System.UnauthorizedAccessException,Microsoft.PowerShell.Commands.UnblockFileCommand
ErrorDetails          :
InvocationInfo        : System.Management.Automation.InvocationInfo
ScriptStackTrace      : at <ScriptBlock>, <No file>: line 1
PipelineIterationInfo : {}

We could catch only IOException and UnauthorizedAccessException exception.
Although I agree to remove it.

This used to write out a non-terminating error, and now throw an exception which is a terminating error. I'm afraid that's not right.

I agree. For reference https://docs.microsoft.com/en-us/powershell/developer/cmdlet/cmdlet-error-reporting#terminating-and-nonterminating-errors

Is the error condition related to a specific input object or a subset of input objects? If so, this is a nonterminating error.

File.Delete ignores File_Not_Found error and throw exception for all other errors, see: http://source.dot.net/#System.IO.FileSystem/System/IO/FileSystem.Windows.cs,30dc9ae494d8f0a5

So, we should use catch (Exception e) here and write a non-terminating error. The error ID RemoveItemUnauthorizedAccessError and ErrorCategory.PermissionDenied may not be appropriate, we need a more general error.

Seems the more common message doesn't make sense - the only exception useful for user that we really can get is UnauthorizedAccessException. The rest may be silently ignored https://msdn.microsoft.com/en-us/library/system.io.ioexception(v=vs.110).aspx because they're a temporary. We could just don't catch its at all or insert Assert.

I don't understand your logic here.
The original code ignores "File_Not_Found" error and writes non-terminating errors for all other failures. With File.Delete, it ignores "File_Not_Found" and throws exceptions for all other failures, so we should just catch (Exception e) and write non-terminating error for the exception.

As @dantraMSFT wrote above catching all exception is not good practice and it is better catch only exceptions for which we can make behavior better.
I want to understand how we should do the best way. Now everywhere in our code we catch all exceptions and write customized errors or re-throw. It looks very costly.

Here only UnauthorizedAccessException is of interest. The remaining exceptions are edge cases and we could pass them.
Of course we can catch all exceptions, write a generic message and put the original exception in inner exception but it can mask a problem.

If you don't catch all exceptions, any uncaught exceptions will stop the cmdlet run and all following paths won't be processed. That would be a breaking change.

Whether catching all exception is right or wrong depends on the scenario. In this instance, we don't want the failure of processing one path to block processing other files. If you silently ignore all IOException, then when it fails with an IOException, user will see nothing and believe it was successful. That's also not right.

Originally, It's OK we only catch Win32Exception because only Win32Exception and DllNotFoundException could be thrown ...

Thanks for your clarification!

Fixed.

Thanks.
I think what @dantraMSFT means is that we should not blindly catch all exceptions. As long as we know why we are doing it, it's OK to use catch (Exception).

You use Exception e in other places. Better to be consistent.

Fixed.

It might be out the scope for this PR, but we don't check continueRemoval before removing the junction point ... that seems wrong.

Let's postpone to new PR.

I see a comment before the line says that we ask the confirmation only for removing root directory or recurse - but seems current behavior is that we remove the junction point as an object (not directory and no recurse) so we don't ask confirmation.

"DeleteJunctionFailed"

We should use a different name now. Symbolic link to a directory on Unix plats also have the attribute ReparsePoint, but Junction point is a Windows-only concept. Maybe just say DeleteSymbolicLinkFailed?

Fixed.