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

Suggestion: Add an $ErrorView option that displays concise, single-line error information and consider making that the default - default formatting of ErrorRecord #3647

Open
mklement0 opened this issue Apr 25, 2017 · 29 comments

Comments

@mklement0
Copy link
Contributor

commented Apr 25, 2017

The currently supported $ErrorView values are:

  • NormalView - the default, "noisy", multi-line display:
> Get-Item /NoSuch                                                                                                                                     
Get-Item : Cannot find path '/NoSuch' because it does not exist.
At line:1 char:1
+ Get-Item /NoSuch
+ ~~~~~~~~~~~~~~~~
    + CategoryInfo          : ObjectNotFound: (/NoSuch:String) [Get-Item], ItemNotFoundException
    + FullyQualifiedErrorId : PathNotFound,Microsoft.PowerShell.Commands.GetItemCommand
  • CategoryView - a terse, single-line display that doesn't always contain enough information:
> $ErrorView = 'CategoryView'; 1 / 0
NotSpecified: (:) [], RuntimeException

It would be nice to have another option - e.g., ConciseView - that limits output to a single line that contains the immediately relevant information (digging deeper - if needed - is always possible via $Error[0]):

> $ErrorView = 'ConciseView'; Get-Item /NoSuch   # wishful thinking
Get-Item : Cannot find path '/NoSuch' because it does not exist.

Arguably, this kind of display is the most useful to end users when viewed in the console, so perhaps it should be the default.

As an aside: requiring the $ErrorView values to end in *View seems redundant; related: #3644 and #3645.

Environment data

PowerShell Core v6.0.0-alpha (v6.0.0-alpha.18) on Darwin Kernel Version 16.5.0: Fri Mar  3 16:52:33 PST 2017; root:xnu-3789.51.2~3/RELEASE_X86_64
@lzybkr

This comment has been minimized.

Copy link
Member

commented Apr 26, 2017

I agree - there is a ton of room for improvement.

I do think the default view needs to be smart - e.g. the position information is sometimes useless, sometimes not. Maybe some heuristics are needed to decide when to show the error position. For example, if it's a PowerShell parser error, it's probably useful. If it's the entire line, maybe not. But maybe showing the line is useful.

It's easy enough to override the default error view with your own custom ps1xml - it would be good to prototype something, share it and get some feedback before we change the default.

@JamesWTruher

This comment has been minimized.

Copy link
Member

commented Apr 27, 2017

This becomes somewhat tricky because not all exception messages contain the target against which the action is taking place. $Error has all the data that we have at our disposal to construct the message, could you expand more on the different scenarios where errors can occur? Would it be acceptable if CategoryView has more useful data when the message was unhelpful rather than creating a new view?

@dragonwolf83

This comment has been minimized.

Copy link

commented Apr 27, 2017

Wouldn't changing CategoryView be considered a breaking change though? Personally, I find CategoryView useless so I can't imagine anyone relying on it.

I think what most of us are wanting is a view that just gives the Exception Message or maybe the same information but formatted better. This could be the CategoryView or a new View. If it is a new View, I would go with ErrorSummary as the name as it gives a better description of what we are wanting.

Option 1:

CommandName: Get-Item
Exception Message: Cannot find path '/NoSuch' because it does not exist.

Option 2:

Get-Item: Cannot find path '/NoSuch' because it does not exist.

Option 3:

Get-Item: Cannot find path '/NoSuch' because it does not exist. At line:1 char:1

Either one of those gives a simple view of the error that can be acted upon.

@mklement0

This comment has been minimized.

Copy link
Contributor Author

commented May 9, 2017

@dragonwolf83: I don't think there's a need to change CategoryView, I was thinking of a new view, so, yes, something like Summary[View] would work.

@JamesWTruher:

Yes, it's a challenge to come up with a concise message that still contains all relevant information for a human to get the gist of the problem at a glance.

It seems to me that CategoryView is more geared toward machine parsing, and it focuses on error properties of secondary interest to a human, so I think a new view makes more sense.
That said, if CategoryView is enhanced to give concise, meaningful error information, I won't complain. (I truly wonder whether anyone uses CategoryView in its present form at all.)

To elaborate on the motivation:

  • What's needed is a concise, single-line message that conveys all necessary information for a human to comprehend the gist of the error at a glance; if digging deeper is needed, $Error can be examined.

  • To a human, the current default view amounts to "scary, noisy, lengthy information overload" that not only makes it hard to quickly understand the error cause, but also crowds out the success output.

I agree with @lzybkr that heuristics and smarts are needed, because different properties are relevant to different errors; I'll see if I can come up with a prototype.

@JamesWTruher

This comment has been minimized.

Copy link
Member

commented May 23, 2017

@mklement0
IIRC, CategoryView was indeed targeted at a machine parsing scenario. We reckoned that we would need something along those lines. I don't have any problem in creating a new view, having you come up with a mock-up of what you would like to see would be great, and a prototype would be awesome, but capturing the output for a number of the scenarios would be best. If we start to see patterns we can work on the code to build it. If there's no pattern, that tells us something too.

@mklement0

This comment has been minimized.

Copy link
Contributor Author

commented Aug 26, 2017

At the bottom is a script you can run as-is, which redefines the default error view and outputs a few sample errors.
The redefinition stays in effect for the remainder of the session, so you can experiment interactively.

Here are the sample error representations, preceded by the commands that triggered them, as run on a Ubuntu system:

# Expression-statement-terminating error (from an all-PS expression)
      1 / 0
>>>1 / 0<<<: Attempted to divide by zero. (At /tmp/2294.ps1:5 char:7)
------------------------
# Expression-statement-terminating error (from a .NET method call)
      [int]::Parse('not-a-number')
>>>[int]::Parse('not-a-number')<<<: Exception calling "Parse" with "1" argument(s): "Input string was not in a correct format." (At /tmp/2294.ps1:12 char:7)
------------------------
# Non-terminating error
      Get-Item /NoSuchItem
Get-Item: Cannot find path '/NoSuchItem' because it does not exist. (At /tmp/2294.ps1:19 char:7)
------------------------
# Pipeline-statement-terminating error
      Get-Item -NoSuchParam
Get-Item: A parameter cannot be found that matches parameter name 'NoSuchParam'. (At /tmp/2294.ps1:26 char:16)
------------------------
# Indirect parse error, via Invoke-Expression. (If we tried ``1 * 2)`` directly in a script, the whole script would fail to parse.)
      Invoke-Expression '1 * 2)'
PARSE ERROR: Invoke-Expression: 1 * 2)<<< Unexpected token ')' in expression or statement. (At /tmp/2294.ps1:33 char:7)
------------------------
# Remoting (job) error
      Receive-Job -Wait -AutoRemoveJob (Start-Job { Get-Item /NoSuchItem })
[localhost] Get-Item: Cannot find path '/NoSuchItem' because it does not exist. (remoting/job)
------------------------
# Script-terminating error, via Throw
      Remove-Item $PSCommandPath # Since the Throw statement terminates the script
      Throw "Not at $HOME"
STOPPED: >>>Throw "Not at $HOME"<<<: Not at /home/jdoe (At /tmp/2294.ps1:49 char:7)

The overall format is:

[[<computer-name>] ]<immediate-error-source>: <message>[ (<context/position>)]

Things to note:

  • Each error is deliberately represented as a single line:

    • Depending on the terminal width and a given error, it may display across more than 1 line.
    • However, when redirecting to a file / transcribing, processing is easier if each error can be assumed to occupy just 1 line.
  • I've tried to anticipate many error situations, but I wouldn't be surprised if I forgot a few.

  • .PSMessageDetails is currently not being used; should it be, and under what circumstances?

  • In the context of remoting / jobs, other than determining the originating computer name, I wasn't able to find other useful information to report as context. Is there?

  • Parse errors and script-terminating errors are represented with all-caps prefixes PARSE ERROR: and STOPPED: , respectively.

  • Positional information (e.g, At line:1 char:1) is omitted if the error occurred in a command typed directly on the command line (if there is no script context).

  • If an alias was used to invoke a command, the alias is listed in parentheses after the full command name (e.g., Get-Item (gi)), but note that determining the actual invocation name doesn't seem to work in all case (see source code below).

  • Only the first parse error is reported; should an exception to the single-output-line rule be made to show them all?

  • Some text elements introduced that are currently hard-coded will have to be localized.

  • Source-code locations relating to the questions raised above are marked with comments containing ?? below.

Update-DefaultErrorView.ps1

# Define the PS code to embed in the XML below here.
$sb = {
  
  # Make sure we control the specific strict mode in effect.
  # (The strict-mode setting is inherited from the parent scope.)
  Set-StrictMode -Version 1

  # Save the error record at hand as well as its invocation info.
  $err = $_
  $myInv = $err.InvocationInfo

  function append($s, $new, $sep = ': ') { $s + $(if ($s) { $sep } else { '' }) + $new }

  switch ($ErrorView) {
    'CategoryView' { $err.CategoryInfo.GetMessage(); break }
    Default {  # 'NormalView'

      if ($err.FullyQualifiedErrorId -like 'NativeCommandError*') { # External-utility STDERR output - covers both 'NativeCommandError' and 'NativeCommandErrorMessage'
        # NOTE:
        #    By default, stderr lines output by external utilities are NOT processed here, because they now go straight to the host.
        #    Only if you redirect stder output to the success stream, using 2>&1, are stderr lines processed here - one by one.
        # The *first* stderr line is reported as 'NativeCommandError', and all subsequent lines as 'NativeCommandErrorMessage'
        # Generally, external utilities prefix their error message with their executable name, such as 'FINDSTR: ...', so no addtional
        # context is needed.
        # However, invoking `cmd.exe` builtins with `cmd /c` does NOT (e.g., `cmd /c 'dir /z'`), so, if the error message doesn't start with "<someIdentifier>:",
        # prepend the invoking command's name.
        $msg = $err.Exception.Message
        if ($err.FullyQualifiedErrorId -eq 'NativeCommandError' -and $msg -notmatch '^\w+:') {
          $msg = $myInv.MyCommand.Name + ': ' + $msg
        }
        # Output the message.
        $msg

      } else { # PS error

        # See if the error is a parsing error. 
        # !! Apparently, a parsing error that occurs *directly on the command line* results in an $err.Exception
        # !! of type [System.Management.Automation.ParentContainsErrorRecordException]
        # !! By contrast, if you inspect $Error[0] after provoking a parsing error directly on the command line
        # !! you get a [System.Management.Automation.ParseException] *directly*, which *itself* also implements the
        # !! [System.Management.Automation.IContainsErrorRecord] *interface*.
        $isParseError = $err.Exception -is [System.Management.Automation.ParseException] -or $err.Exception -is [System.Management.Automation.ParentContainsErrorRecordException]
        $isRemotingError = $err -is [System.Management.Automation.Runspaces.RemotingErrorRecord] # remoting error, including from jobs
        $isScriptTerminatingError = [bool] $err.Exception.WasThrownFromThrowStatement # $err.CategoryInfo.Category -eq 'OperationStopped'
        
        # Identify the *immediate* source of the error:
        $source = ''
        if ($isParseError) {
          # Clearly mark parsing errors as such.
          # NOTE: ?? This string will have to be localized.
          $source = 'PARSE ERROR'
          # If $myInv.MyCommand.Name has a value, the implication is that the exception 
          # is an *indirectly* triggered parse error, via Invoke-Expression, so we indicate that.
          # Otherwise, we rely on the exception message to provide sufficient information.
          if ($myInv.MyCommand.Name) {
            $source = append $source $myInv.MyCommand.Name
          }
        } else {
          if ($isScriptTerminatingError) {
            # Clearly mark a script-terminating error (triggered with Throw) as such.
            # NOTE: ?? This string will have to be localized.
            $source = 'STOPPED'
          }
          if ($isRemotingError) { # remoting error (including from jobs)
            # Prepend the name of the originating computer.
            $source = append $source ("[{0}]" -f $err.OriginInfo.PSComputerName)
            # Add the command name, if a command caused the error.
            if ($err.CategoryInfo.Activity) { # Contains the command name, if a *command* caused the error.
              $source = $source + ' ' + $err.CategoryInfo.Activity
            }
          } elseif ($myInv.MyCommand.Name) {
            # If a *command* is the error source: this generally evaluates to the resolved underlying cmdlet/function name, 
            # even if the invocation used an alias or & or .
            $source = append $source $myInv.MyCommand.Name
            # If the invocation name was different - i.e., if an alias was used - append the alias name (e.g., 'Get-Item (gi)')
            # ?? This doesn't always work, notably not with . and & (in which case $myInv.InvocationName contains '.' and '&',
            # !! and with statement-terminating errors such as `Get-Item -NoSuchParam`.
            # !! Examining .Line is not an option, because unrelated statements may preceded the offending one on the same line.
            if ($myInv.InvocationName -and $myInv.InvocationName -notmatch '[.&]' -and $myInv.InvocationName -ne $myInv.MyCommand.Name) {
              $source += ' ({0})' -f $myInv.InvocationName
            }
          } elseif ($myInv) { # expression such as `1/0` or `& { 1/0 }` or Throw statements
            # $source = append $source $myInv.Line.Trim() # the input expression / statement list
            $source = append $source ('>>>{0}<<<' -f $myInv.Line.Trim()) # the input expression / statement list
          }
        }
        
        # Identify the larger error context (script / function of origin) and source-code position,
        # if available.
        $context = ''
        if ($myInv.PositionMessage) {
          # Note: We only include a position message (e.g., 'At line:1 char:1') if we have a script filename,
          # as it isn't really useful for a command issued directly on the command line.
          if ($myInv.ScriptName) {
            # Use only the *first line* of  .PositionMessage, which usually provides
            # script path, and the line and column number.
            # The remaining lines are too verbose for our single-line representation.          
            $context = ($myInv.PositionMessage -split '\r?\n')[0]
          }
        } elseif ($isRemotingError) {
          # NOTE: ?? This string will have to be localized.
          $context = 'remoting/job' # ?? Is there more information available?
        } else { # ?? Does this ever happen?
          # NOTE: ?? This string will have to be localized.
          $context = 'unknown'
        }

        # Transform the message to a single-line string.
        if ($isParseError) {
          # Parse errors have multi-line messages in the following format:
          #    At ....:<line> char:<col>
          #     + <broken-part-of-statement>
          #     +         ~
          # where the "~" indicates the precise location of the error.
          # We reformat this message to a single line
          $contextPart, $offendingLinePart, $indicatorLine, $reason, $rest = $err.ToString() -split '\r?\n'
          # We omit the "~" line and directly append '<<<' to the offending part of the
          # line and also strip the '+ ' prefix, then append the specific parsing error message.
          # !! There could be MULTIPLE parsing errors, however, we only report the FIRST.
          # ?? Is this a reason to *always* present parsing errors as multi-line errors?
          $msg = '{0}<<< {1}' -f ($offendingLinePart -replace '^[+]\s+'), $reason
          # !! We IGNORE $contextPart, and instead use the $context value we've constructed above,
          # !! because it generally contains the same information and additionally contains the more
          # !! relevant script-context information when the parse error was triggered by a *string* passed to *Invoke-Expression*.
          # !! ($contextPart in that case only contains position information *inside the string* passed to Invoke-Expression)
        } else {
          # All other errors: replace newlines with spaces to output a single line.
          $msg = $err.ToString() -replace '\r?\n', ' '
        }
        # ?? When is $err.PSMessageDetails ever filled and does it matter?
        
        # Output the synthesized message as a *single-line* string.
        # Note that no effort is made to word-wrap overly long lines:
        #  * Hopefully, most terminals are wide enough to fit most errors on 1 line
        #  * When redirecting to a file / transcribing, processing is easier if every
        #    error can be assumed to occupy just 1 line.
        $combinedMsg = '{0}: {1}' -f $source, $msg
        if ($context) {
          $combinedMsg += ' ({0})' -f $context
        }
        $combinedMsg
      }

    }
  }

}

# Create a *.ps1xml file on the fly, using a fixed-per-session file path
# based on the process ID. This is helpful, because PS remembers previously
# loaded *.ps1xml files and tries to reload them, so if we used a different
# path every time (and deleted the file after), every subsequent
# Update-FormatData call would complain about missing files.
@"
<Configuration>
<ViewDefinitions>
      <View>
          <Name>ErrorInstance</Name>
          <OutOfBand />
          <ViewSelectedBy>
              <TypeName>System.Management.Automation.ErrorRecord</TypeName>
          </ViewSelectedBy>
          <CustomControl>
              <CustomEntries>
                  <CustomEntry>
                     <CustomItem>
                          <ExpressionBinding>
                              <ScriptBlock>
                                <![CDATA[
$sb
                                ]]>
                              </ScriptBlock>
                          </ExpressionBinding>
                      </CustomItem>
                  </CustomEntry>
              </CustomEntries>
          </CustomControl>
      </View>
</ViewDefinitions>
</Configuration>
"@ > ($tmpFile = [io.path]::GetTempPath() + "$PID.ps1xml")

# Load the format data via -PrependPath, which *preempts* preloaded definitions.
Update-FormatData -PrependPath $tmpFile

# Clean up.
Remove-Item $tmpFile


# === Create a test script that provokes various errors

$sbs = {
      # Expression-statement-terminating error (from an all-PS expression)
      1 / 0 
    },
    {  
      # Expression-statement-terminating error (from a .NET method call)
      [int]::Parse('not-a-number')
    },
    {
      # Non-terminating error
      Get-Item /NoSuchItem
    },
    {
      # Pipeline-statement-terminating error
      Get-Item -NoSuchParam
    },
    {
      # Indirect parse error, via Invoke-Expression. (If we tried ``1 * 2)`` directly in a script, the whole script would fail to parse.)
      Invoke-Expression '1 * 2)'
    },
    {
      # Remoting (job) error
      Receive-Job -Wait -AutoRemoveJob (Start-Job { Get-Item /NoSuchItem })
    },
    {
      # Script-terminating error, via Throw
      Remove-Item $PSCommandPath # Since the Throw statement below terminates this temp. script, we self-delete it first here.
      Throw "Not at $HOME"
    }
    # !! Even the current 'NormalView' view makes no attempt to identify
    # !! the *function* inside a script in which the error occurred.
    # {
    #   # Error inside a function
    #   Function Foo { Get-Item /NoSuchItem }; Foo
    # }

$tmpScript = [io.path]::GetTempPath() + "$PID.ps1"

$sbs | % { 
  @"
  '$(($_.ToString() -replace "'", "''").Trim())'
  $($_.Tostring())
  "------------------------" 
"@ 
} > $tmpScript

# Write-Verbose -Verbose $tmpScript

# Invoke the temp. script to show how the errors are formatted.
# !! Do not use try / finally here, because the first terminating error
# !! would trigger the finally block.
& $tmpScript

# Since the Throw statement inside the temp. script aborts this script too,
# we never get here.
# Remove-Item $tmpScript
@mklement0

This comment has been minimized.

Copy link
Contributor Author

commented Aug 26, 2017

To help with experimenting, find helper function out-ErrorDetails at the bottom, which visualizes a given error comprehensively, though not quite as detailed as the Resolve-ErrorRecord PSCX function that @rkeithhill mentions.
By default it targets the most recent error.

E.g.:

gi /NoSuch; out-ErrorDetails

yields the following:

Message: Cannot find path '/nosuch' because it does not exist.

CategoryInfo: 
	Category   : ObjectNotFound 
	Activity   : Get-Item 
	Reason     : ItemNotFoundException 
	TargetName : /nosuch 
	TargetType : String

Exception:
	Type: System.Management.Automation.ItemNotFoundException


InvocationInfo:
     
	MyCommand             : Get-Item 
	BoundParameters       : {} 
	UnboundArguments      : {} 
	ScriptLineNumber      : 1 
	OffsetInLine          : 1 
	HistoryId             : 86 
	ScriptName            :  
	Line                  : gi /nosuch 
	PositionMessage       : At line:1 char:1 
	                        + gi /nosuch 
	                        + ~~~~~~~~~~ 
	PSScriptRoot          :  
	PSCommandPath         :  
	InvocationName        : gi 
	PipelineLength        : 0 
	PipelinePosition      : 0 
	ExpectingInput        : False 
	CommandOrigin         : Internal 
	DisplayScriptPosition : 

Stack traces:
  * Script:
	at <ScriptBlock>, <No file>: line 1
  * Exception:
	at System.Management.Automation.LocationGlobber.ExpandMshGlobPath(String path, Boolean allowNonexistingPaths, PSDriveInfo drive, ContainerCmdletProvider provider, CmdletProviderContext context)
	at System.Management.Automation.LocationGlobber.ResolveDriveQualifiedPath(String path, CmdletProviderContext context, Boolean allowNonexistingPaths, CmdletProvider& providerInstance)
	at System.Management.Automation.LocationGlobber.GetGlobbedMonadPathsFromMonadPath(String path, Boolean allowNonexistingPaths, CmdletProviderContext context, CmdletProvider& providerInstance)
	at System.Management.Automation.LocationGlobber.GetGlobbedProviderPathsFromMonadPath(String path, Boolean allowNonexistingPaths, CmdletProviderContext context, ProviderInfo& provider, CmdletProvider& providerInstance)
	at System.Management.Automation.SessionStateInternal.GetItem(String[] paths, CmdletProviderContext context)
	at System.Management.Automation.ItemCmdletProviderIntrinsics.Get(String path, CmdletProviderContext context)
	at Microsoft.PowerShell.Commands.GetItemCommand.ProcessRecord()

Function out-ErrorDetails

# Print the specified error in detailed form - defaults to the most recent error.
# You may also specify an *index* into the $Errors array.
# Note: By default, if $Error[0] is [Management.Automation.ActionPreferenceStopException] rather than [Management.Automation.ErrorRecord],
#       $Error[1] is printed, because [Management.Automation.ActionPreferenceStopException] is a *wrapper* error that indicates that execution was
#       aborted due to -ErrorAction Stop or $ErrorActionPreference = 'Stop'.
function out-ErrorDetails([object] $eo = $Error[0]) {

  # Skip the escalation-to-script-terminating-error wrapper, if present.
  if ($eo -eq $Error[0] -and $eo -is [Management.Automation.ActionPreferenceStopException]) { 
    Write-Warning "Skipping escalation-to-script-terminating-error wrapper error, inspecting $Error[1] instead."
    $eo = $Error[1] 
  }
  if ($eo -is [int]) { $eo = $Error[$eo] } # if a mere index was specified, assume it refers to the $Errors array list
  if (-not $eo) { Write-Warning "No error object was passed, or no error has yet occurred in this session."; return }
  Write-Host -fore yellow @"
Message: $($eo.ToString())
$(if ($eo.OriginInfo) {
"  Origin: $($eo.OriginInfo.PSComputerName) (runspace ID: $($eo.OriginInfo.RunSpaceId))"
# Note: There's also an InstanceId property (PSv3+), but it is usually '00000000-0000-0000-0000-000000000000'. ?? what does it refer to?
# "global identifier of the origin of the error record" - https://msdn.microsoft.com/en-us/library/system.management.automation.remoting.origininfo(v=vs.85).aspx?cs-save-lang=1&cs-lang=csharp#code-snippet-1
})
CategoryInfo: $($eo.CategoryInfo | Format-List * | out-string -stream | % { if ($_) { ("`n`t" + $_) } })

$(if ($eo.Exception) {
"Exception:
`tType: $($eo.Exception.GetType().FullName)"
})
$(if ($eo.Exception -and $eo.Exception.InnerException) {
"`tInner exception:
`t  Type: $($eo.Exception.InnerException.GetType().FullName)
`t  Message: $($eo.Exception.InnerException.Message)"
})
$(if ($eo.PSMessageDetails) {
"  PSMessageDetails:
     $($eo.PSMessageDetails | Out-string -stream | % { if ($_) { ("`n`t" + $_) } })"
})
$(if ($eo.InvocationInfo) {
"InvocationInfo:
     $($eo.InvocationInfo | Out-string -stream | % { if ($_) { ("`n`t" + $_) } })"
})

Stack traces:
  * Script:
$("`t" + $eo.ScriptStackTrace -replace '\r?\n', "`n`t")
  * Exception:
$("`t" + ($eo.Exception.StackTrace -replace '^\s+' -replace '\r?\n\s+', "`n`t"))
"@

}

@mklement0 mklement0 changed the title Suggestion: Add an $ErrorView option that displays concise, single-line error information and consider making that the default Suggestion: Add an $ErrorView option that displays concise, single-line error information and consider making that the default - default formatting of ErrorRecord Aug 26, 2017

@SteveL-MSFT

This comment has been minimized.

Copy link
Member

commented Sep 7, 2017

I would also suggest that non-terminating errors have a different color (magenta?) than terminating errors

@mklement0

This comment has been minimized.

Copy link
Contributor Author

commented Sep 7, 2017

@SteveL-MSFT:

That's a great idea, but - given just a [System.Management.Automation.ErrorRecord], as when formatting for output - we currently have no way (that I know of) of determining whether a given error was terminating or not (see also: #3158).

If we clear that hurdle, we also need to decide whether we want to apply that coloring situationally: e.g., if a non-terminating error is promoted to (script)-terminating via -EA Stop, for instance, should it then print in red, not magenta?

Do we also need to color-distinguish between statement-terminating and script-terminating errors? (Though the latter are more easily spotted by coming last in the output.)

@SteveL-MSFT

This comment has been minimized.

Copy link
Member

commented Sep 7, 2017

We can add additional properties to the ErrorRecord type to differentiate terminating vs non-terminating. For script vs statement, it seems the simplest is to output:

Encountered statement-terminating error:
...
@mklement0

This comment has been minimized.

Copy link
Contributor Author

commented Sep 8, 2017

@SteveL-MSFT:

Re adding a new property: I've created a new issue: #4781.

Re script vs statement:

That's a little verbose for the single-line representation we're trying to create here.
Perhaps color coding alone will be enough?

As a wistful aside:

In a world without backward-compatibility constraints, ideally we wouldn't have to deal with the distinction between statement-terminating and script-terminating at all, because there should only be one type of terminating error: the script-terminating type. (If only the documentation were correct...):

The nature of the current statement-terminating errors is so severe that it makes no sense to let scripts continue by default.

@dragonwolf83

This comment has been minimized.

Copy link

commented Sep 8, 2017

For the one-line, I think the first word should be ERROR:. That is consistent with how Warning and Verbose outputs their text.

A key scenario that needs to be considered is logging the output to a file. The output should be easy to parse and won't have color coding to help. Using ERROR first aligns with that scenario.

Statement vs Non-Termination can be differentiated by one of the following:
ERROR: Type: Non-Terminating; Message:
ERROR: Severity: Non-Terminating; Message:
ERROR: Non-Terminating; Message:

Severity is what SQL Server calls the these types of errors and it makes the most sense. It would probably make the documentation clearer as well because a non-terminating error is not as severe as a terminating error which stops the script.

@mklement0

This comment has been minimized.

Copy link
Contributor Author

commented Sep 8, 2017

@dragonwolf83:

Good points re symmetry with VERBOSE: and WARNING: and parsing logs (selective coloring and wording aren't mutually exclusive, of course).

Again, though, I'm worried about verbosity for single-line representations:
There's a tension between:

  • favoring human eyeballs for at-a-glance comprehension - for which I'd leave out labels such as Type: or Message: as part of the output (except for the ERROR: prefix) and

  • suitability for machine parsing.

Even though CategoryView is probably insufficient in its current form, its purpose is to provide the latter, according to @JamesWTruher.

I'd say the default view should favor human eyeballs (with still some support for parsing, based on the ERROR: prefix and the overall message composition I've proposed above), with opt-in for strict machine parsing.

@alx9r

This comment has been minimized.

Copy link

commented Mar 1, 2018

FWIW, it seems that currently $errorRecord.Exception.WasThrownFromThrowStatement is insufficient to detect the script-terminating errors described in MicrosoftDocs/PowerShell-Docs#1583. For example,

try
{
    throw [System.Exception]::new('message')
}
catch
{
    [bool]$_.Exception.WasThrownFromThrowStatement
}

outputs $false.

@mklement0

This comment has been minimized.

Copy link
Contributor Author

commented Mar 1, 2018

@alx9r:

You've found an edge case: It works as intended if you throw anything other than a [System.Exception] (or derived) instance, which PowerShell conveniently lets you do.

The only exception type you can throw and still have it reflected in .Exception.WasThrownFromThrowStatement is [System.Management.Automation.RuntimeException], because that is what PowerShell uses internally to wrap whatever (non-exception) you pass to throw.

Thus, the following 2 commands are equivalent and both output $True:

try { throw 'message' } catch { $_.Exception.WasThrownFromThrowStatement }
try { throw [System.Management.Automation.RuntimeException]::new('message') } catch { $_.Exception.WasThrownFromThrowStatement }

I agree that throwing an actual exception-type instance should also be reflected in .Exception.WasThrownFromThrowStatement, however; not sure if there are technical challenges; I suggest you open a new issue.

@alx9r

This comment has been minimized.

Copy link

commented Mar 1, 2018

Thanks @mklement0. I opened issue #6288.

@SteveL-MSFT

This comment has been minimized.

Copy link
Member

commented Nov 2, 2018

Perhaps also consider showing the wrapped ErrorRecord or InnerException rather than having the user dig for it. Example of such a case is #8173

@rkeithhill

This comment has been minimized.

Copy link
Contributor

commented Nov 26, 2018

It sure would be nice if some form of simplified error output makes it into 6.2. :-)

@SteveL-MSFT

This comment has been minimized.

Copy link
Member

commented Nov 26, 2018

@rkeithhill I agree, but would need some proposals that we can discuss. I played with @mklement0's prototype and made some changes to leverage VT100 escape sequences: https://gist.github.com/SteveL-MSFT/edf969ee676206a47d520a9032dcf6e3

@vexx32

This comment has been minimized.

Copy link
Contributor

commented Feb 11, 2019

Something I would really love to see is some sort of indication as to what type of error is being shown. Since terminating errors behave pretty differently to non-terminating errors (ignoring -ErrorAction common parameter, etc) I think it would be valuable if we could distinguish the type of error at a glance.

@Jaykul

This comment has been minimized.

Copy link

commented Feb 12, 2019

Can we make this a lot easier to extend by just allowing anyone to write a Format-ErrorSimple and set $ErrorView = "Simple" -- basically by looking for matching commands in the formatter, and falling back to the current NormalView?

@Jaykul

This comment has been minimized.

Copy link

commented Feb 12, 2019

OK, I had a poke at this, just to provide another approach.

function ConvertTo-SimpleErrorView {
    [CmdletBinding()]
    param(
        [System.Management.Automation.ErrorRecord]
        $InputObject
    )

    if ($InputObject.FullyQualifiedErrorId -eq "NativeCommandErrorMessage") {
        $InputObject.Exception.Message
    } else {
        $myinv = $InputObject.InvocationInfo
        if ($myinv -and ($myinv.MyCommand -or ($InputObject.CategoryInfo.Category -ne 'ParserError'))) {
            # rip off lines that say "At line:1 char:1" (hopefully, in a language agnostic way)
            $posmsg  = $myinv.PositionMessage -replace "^At line:1 .*[\r\n]+"
            # rip off the underline and instead, put >>>markers<<< around the important bit
            # we could, instead, set the background to a highlight color?
            $pattern = $posmsg -split "[\r\n]+" -match "\+( +~+)\s*" -replace '(~+)', '($1)' -replace '( +)','($1)' -replace '~| ','.'
            $posmsg  = $posmsg -replace '[\r\n]+\+ +~+'
            if ($pattern) {
                $posmsg  = $posmsg -replace "\+$pattern", '+ $1>>>$2<<<'
            }
        } else {
            $posmsg = ""
        }

        if ($posmsg -ne "") {
            $posmsg = "`n" + $posmsg
        }

        if ( & { Set-StrictMode -Version 1; $InputObject.PSMessageDetails } ) {
            $posmsg = " : " + $InputObject.PSMessageDetails + $posmsg
        }

        $indent = 4
        $width = $host.UI.RawUI.BufferSize.Width - $indent - 2

        $originInfo = & { Set-StrictMode -Version 1; $InputObject.OriginInfo }
        if (($null -ne $originInfo) -and ($null -ne $originInfo.PSComputerName)) {
            $indentString = "+ PSComputerName        : " + $originInfo.PSComputerName
            $posmsg += "`n"
            foreach ($line in @($indentString -split "(.{$width})")) {
                if ($line) {
                    $posmsg += (" " * $indent + $line)
                }
            }
        }

        if (!$InputObject.ErrorDetails -or !$InputObject.ErrorDetails.Message) {
            $InputObject.Exception.Message + $posmsg + "`n "
        } else {
            $InputObject.ErrorDetails.Message + $posmsg
        }
    }
}

And then I made a quick demo module, setting the ErrorInstance view to call named functions ...

<ExpressionBinding>
  <ScriptBlock>
    <![CDATA[
      if ($formatter = Get-Command "ConvertTo-$($global:ErrorView -replace "View")ErrorView" -ListImported -ErrorAction Ignore -ParameterName InputObject -ParameterType [System.Management.Automation.ErrorRecord]) {
        &@($formatter)[0] $_
      } else {
        ConvertTo-NormalErrorView $_
      }
    ]]>
  </ScriptBlock>
</ExpressionBinding>

And of course, copying the existing Normal and Category views into ConvertTo-NormalErrorView and ConvertTo-CategoryErrorView ...

And then I wrote a hack of a Format-Error function so that I can switch between views without messing with the preference variable or having to do | Format-List * -Force ...

I'd definitely like to see something like this in core, so we can easily customize error formatting in our profiles without writing format files...

Here's my attempt (and it works down to PowerShell 5.x, at least) as a mini module called ErrorView

@GoateePFE

This comment has been minimized.

Copy link

commented May 31, 2019

While we're at it, how about making error text white characters on red background. That would be so much easier to read than dark red on black, especially on a projector.

@Kriegel

This comment has been minimized.

Copy link

commented Jun 4, 2019

Why not let the user pass an { Scriptblock } or Function to self define the error formatting and output?
Like in #6010

@KirkMunro

This comment has been minimized.

Copy link
Contributor

commented Jul 3, 2019

While a think a new error view could be helpful (to the point where I had implemented one in a module many years ago, but then I lost that module due to clumsy fingers on my keyboard), I prefer heading towards something smarter. @lzybkr talked about a smarter default view here. That's where my line of thinking tends to go as well.

For example, I was thinking about errors today, in particular about how they are presented. In some cases, the text we present is not as clear and straightforward as it could be. In some cases, we show a bunch of extra detail that is not necessary (position, etc.). In all cases, the error is red. We can do better.

How do we identify when errors should be presented differently?

For PowerShell use errors (an error that occurs from ad-hoc PowerShell use and is clearly an error that came from incorrect use of something in PowerShell, e.g. invoking a command ad-hoc and parameter binding/validation fails, trying to invoke a command that does not exist, and other parameter binder or argument validator errors that occur), or other user errors (permission denied when trying to do something, unable to find a resource because it does not exist, etc., still done through ad-hoc command invocation), position details don't matter, other than perhaps the marker indicating where the error came from in the command that was invoked. For errors that come from a script or module or remote invocation of a job, the script/file position matters.

Beyond determining whether or not position should be shown, based on whether or not the command came from ad-hoc use vs running a script, module, job, etc., color matters, to the point where some presenters, myself included, suggest people change their error color so that it's more presentable and easier to read. That general change aside, it would be beneficial for us to show different colors for use errors vs resource not found errors vs access denied errors, or other errors.

While this may be over-simplifying things, looking through the System.Management.Automation.ErrorCategory enumerations, I would group SyntaxError and ParserError together with errors that occur due to parameter binding/validation as one color, ObjectNotFound as another color by itself (this is so common in ad-hoc use, and being able to look through errors and identify when something simply was not found and resulted in an error could help solve many problems when working out errors in scripts), and the rest I would put in the "default" error color.

Personally I would not apply a different color for terminating errors vs non-terminating errors, because I don't think it adds enough value, and keeping the number of colors used to present errors down to a minimum is important. I would consider displaying something differently for a terminating error that comes from a command, script, or module (i.e. show whether or not the error was terminating in the CategoryInfo section of an ErrorRecord, for example).

This approach keeps the number of colors small enough to be manageable (3), and simplifies display of ad-hoc errors vs script errors so that users can work with error messages more easily and have a better chance at knowing at a glance where something came from and what happened.

@joeyaiello

This comment has been minimized.

Copy link
Member

commented Sep 12, 2019

Just stumbled across this one today, wanted to link it to PowerShell/PowerShell-RFC#215

@whereisaaron

This comment has been minimized.

Copy link

commented Sep 13, 2019

Excellent. Pleased to see some movement with the RFC @joeyaiello!

I think @dragonwolf83 has the right idea, ie. just display the error without all the junk that buries what you need to know.

The RFC starts off great, promising a single line error message. But depressingly the discussion is all about complex extensibility, formatting and 🌈 rainbow colors, rather than striving for being simple and actually useful 😄

@KirkMunro

This comment has been minimized.

Copy link
Contributor

commented Sep 13, 2019

@whereisaaron: There are diverse needs to consider. Making error messages simple for end users is one need. Making error messages more detailed for scripters is another. Doing so while at considering tool builders is important. It doesn't mean that is what will be built, but the entire discussion is worthwhile having.

@whereisaaron

This comment has been minimized.

Copy link

commented Sep 13, 2019

@KirkMunro sorry, I don't think anyone is - and I wasn't - suggesting taking away the full error object for debuggers, just displaying less of it to interactive shell users by default. The discussion here and on the RFC is very interesting, I read it through, but it has been two years of discussing so far, so I was hoping someone could get to a decision 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.