Skip to content

Conversation

@powercode
Copy link
Collaborator

Tracking issue: #12631.

@ghost ghost assigned iSazonov Nov 19, 2020
@iSazonov iSazonov requested a review from vexx32 November 20, 2020 04:34
@ghost ghost added the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Nov 20, 2020
@iSazonov iSazonov added CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log CL-CodeCleanup Indicates that a PR should be marked as a Code Cleanup change in the Change Log and removed CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log labels Nov 20, 2020
@powercode
Copy link
Collaborator Author

The checks I see are of the type

 var icer = exception as IContainsErrorRecord;
if (icer == null)...

But that is just checking for the interface.

        /// <summary>
        /// Encode exception.
        /// </summary>
        private static PSObject EncodeException(Exception exception)
        {
            // We are encoding exceptions as ErrorRecord objects because exceptions written
            // to the wire are lost during serialization. By sending across ErrorRecord objects
            // we are able to preserve the exception as well as the stack trace.

            ErrorRecord errorRecord = null;
            IContainsErrorRecord containsErrorRecord = exception as IContainsErrorRecord;
            if (containsErrorRecord == null)
            {
                // If this is a .NET exception then wrap in an ErrorRecord.
                errorRecord = new ErrorRecord(exception, "RemoteHostExecutionException", ErrorCategory.NotSpecified, null);
            }
            else
            {
                // Exception inside the error record is ParentContainsErrorRecordException which
                // doesn't have stack trace. Replace it with top level exception.
                errorRecord = containsErrorRecord.ErrorRecord;
                errorRecord = new ErrorRecord(errorRecord, exception);
            }

            PSObject errorRecordPSObject = RemotingEncoder.CreateEmptyPSObject();
            errorRecord.ToPSObjectForRemoting(errorRecordPSObject);
            return errorRecordPSObject;
        }

And

        public ErrorRecord(ErrorRecord errorRecord,
                             Exception replaceParentContainsErrorRecordException)
        {
            if (errorRecord == null)
            {
                throw new PSArgumentNullException(nameof(errorRecord));
            }
}

Clearly, the expectation is that it is non-null.

I don't think the checks you see for null are what you think they are.

@ghost ghost removed the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Nov 20, 2020
@powercode
Copy link
Collaborator Author

I think those checks are just extra precautions since we haven't had a language mechanism to help us with this.
So devs add safeguards.

@ghost ghost added the Review - Needed The PR is being reviewed label Nov 29, 2020
@ghost
Copy link

ghost commented Nov 29, 2020

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

@iSazonov iSazonov merged commit 33ce474 into PowerShell:master Jan 9, 2021
@ghost ghost removed the Review - Needed The PR is being reviewed label Jan 9, 2021
@iSazonov iSazonov added this to the 7.2.0-preview.3 milestone Jan 9, 2021
@powercode powercode deleted the nullable/IContainsErrorRecord branch January 13, 2021 15:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CL-CodeCleanup Indicates that a PR should be marked as a Code Cleanup change in the Change Log

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants