Skip null-element check for collections with a value-type element type #5432

daxian-dbw · 2017-11-13T17:39:35Z

For Mandatory parameter, ValidateNotNull and ValidateNotNullOrEmpty attributes, skip null-element check if the collection's element type is value type.
For ValidateNotNull and ValidateNotNullOrEmpty, they are changed to do null/empty check only on collections (as defined in ParameterCollectionTypeInformation) and hashtable. They currently enumerate elements of any object that implements IEnumerable and also enumerate elements if the argument is an enumerator. I think this behavior is not right for the following reasons:
- The argument may be an object that talks to Azure to retrieve elements back. It doesn't seem right to have the ValidateNotNull/ValidateNotNullOrEmpty attributes wait on that.
- On Windows, COM object may be enumerable, but the GetEnumerator() method is not supported in .NET Core on COM object. (See the COM example below)
- When the argument is an enumerator, the enumerator may not be useful after the validate attribute walks through all its elements. (See the enumerator example below)

COM Example

function bar {
   param ([ValidateNotNull()] $object)
}
> $shell = New-Object -ComObject Shell.Application
> $folder = $shell.NameSpace("F:\tmp\dd\")
> bar -object ($folder.Items())
bar : Cannot validate argument on parameter 'object'. Could not load type 'System.Runtime.InteropServices.ComTypes.IEnumerable' from assembly 'System.Private.CoreLib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e'.
At line:1 char:13
+ bar -object ($folder.Items())
+             ~~~~~~~~~~~~~~~~~
+ CategoryInfo          : InvalidData: (:) [bar], ParameterBindingValidationException
+ FullyQualifiedErrorId : ParameterArgumentValidationError,bar

Enumerator Example

function bar {
   param ([ValidateNotNull()] $object)
   "MoveNext:  $($object.MoveNext())"
}

> bar -object @(1,2,3).GetEnumerator()
MoveNext:  False

adityapatwardhan · 2017-11-13T18:34:53Z

build.psm1

            $ReleaseVersion = $ReleaseTagToUse
        } else {
-            $ReleaseVersion = (Get-PSCommitId) -replace '^v'
+            $ReleaseVersion = (Get-PSCommitId -WarningAction SilentlyContinue) -replace '^v'


This change does not seem related to this PR.

Yes, I will revert it.

adityapatwardhan · 2017-11-13T18:49:23Z

test/powershell/engine/Basic/ValidateAttributes.Tests.ps1

+            $expected = $baseline + 20
+
+            if ($IsWindows) {
+                $null = New-Item -Path $TESTDRIVE/file1 -ItemType File


-ItemType File is not necessary anymore. The default itemtype is file

OK. I will remove it.

adityapatwardhan · 2017-11-13T18:53:39Z

test/powershell/engine/Basic/ValidateAttributes.Tests.ps1

+                param(
+                    [ValidateNotNullOrEmpty()]
+                    $Value,
+                    [string] $TestType


Can you add a parameter like: [System.Nullable``1[[System.Int32]]] $nullableInt

A [System.Nullable[int]] parameter is not useful in this case because the type is not a collection. Hence ValidateNotNullOrEmpty attribute won't validate it as a collection.

iSazonov

Leave a comment

iSazonov · 2017-11-14T12:49:13Z

src/System.Management.Automation/engine/Attributes.cs

+                    isEmpty = false;
+                    // If the element of the collection is of value type, then no need to check for null
+                    // because a value-type value cannot be null.
+                    if (isElementValueType) { break; }


It seems we can move this before while line 1997 - we check first element in ParameterCollectionTypeInformation in IsArgumentCollection.

We don't check the elements of arguments in IsArgumentCollection. The argument passed in is the type of arguments, not arguments itself.

Yes, I see that no element check is in ParameterCollectionTypeInformation.

But the code is not clear. Maybe:

# Check if the collection is empty. if (ienum.MoveNext()) isEmpty = false; # Check every element in the collection only if the element type of the collection is NOT a value type. if (!isElementValueType && !isEmpty) { do { ... object element = ienum.Current; ... } while(ienum.MoveNext())) }

I think the logic is clear -- if the element type of the collection is a value type, then skip checking each element. The isEmpty check is needed in either case.

The logic is right but the code is slightly tricky for me. I added your comments in my sample above. Also we do two actions that can be carried before the cycle - if we are thinking about performance it is important - I think Measure-Object show a delay starting with 10000 elements.

I get your point. I will update as you suggested. Thanks!

iSazonov · 2017-11-14T12:53:51Z

src/System.Management.Automation/engine/Attributes.cs

                }
            }
-            else if ((ienum = arguments as IEnumerable) != null)
+            else if (IsArgumentCollection(arguments.GetType(), out bool isElementValueType))


Is in the context isEmpty = !isElementValueType ?

No, the check of element type doesn't involve inspecting the elements of the collection. You can see that the type of arguments is passed in instead of the arguments itself.
Please see the constructor of ParameterCollectionTypeInformation for details.

iSazonov · 2017-11-14T13:21:27Z

src/System.Management.Automation/engine/ParameterBinderBase.cs

                isEmpty = false;
+                // If the element of the collection is of value type, then no need to check for null
+                // because a value-type value cannot be null.
+                if (isElementValueType) { break; }


Why if we calculate this before the cycle?

Same reason as above. The element type is resolved using the collection type.

daxian-dbw · 2017-11-20T22:11:14Z

@iSazonov You comments have been addressed. Please take another look when you have time. Thanks!

iSazonov

LGTM with two minor comments.

iSazonov · 2017-11-21T06:14:59Z

test/powershell/engine/Basic/ValidateAttributes.Tests.ps1

+    Context "ValidateNotNull, ValidateNotNullOrEmpty and Not-Null-Or-Empty check for Mandatory parameter" {
+
+        BeforeAll {
+            function MandType {


Could you please use more nice name? MandatoryType?

iSazonov · 2017-11-21T06:15:36Z

src/System.Management.Automation/engine/Attributes.cs

    #region NULL validation attributes
+
+    /// <summary>
+    /// Base type of Null Validation attributes


Could you please add final dot in public comments?

SteveL-MSFT · 2017-11-29T23:42:16Z

@PowerShell/powershell-committee reviewed this and approves with this change

adityapatwardhan · 2017-11-29T23:52:26Z

@daxian-dbw restarted AppVeyor build. Failed due to #5567

PowerShell#5432) * Fix NotNullOrEmpty check logic * Fix a test issue on Unix

#5432) * Fix NotNullOrEmpty check logic * Fix a test issue on Unix

daxian-dbw added 2 commits November 12, 2017 22:14

Fix NotNullOrEmpty check logic

602472d

Fix a test issue on Unix

c303e9f

daxian-dbw assigned adityapatwardhan Nov 13, 2017

daxian-dbw requested review from adityapatwardhan, iSazonov, lzybkr and markekraus November 13, 2017 17:39

daxian-dbw requested review from BrucePay and TravisEz13 as code owners November 13, 2017 17:39

Remove extra line ending

38364cd

daxian-dbw removed request for BrucePay and TravisEz13 November 13, 2017 17:43

daxian-dbw added the Breaking-Change breaking change that may affect users label Nov 13, 2017

adityapatwardhan requested changes Nov 13, 2017

View reviewed changes

iSazonov reviewed Nov 14, 2017

View reviewed changes

Address comments

5dfcb73

adityapatwardhan approved these changes Nov 20, 2017

View reviewed changes

Address Ilya's feedback

cdc06a9

daxian-dbw added the Review - Committee The PR/Issue needs a review from the PowerShell Committee label Nov 20, 2017

Fix minor error

f64c996

iSazonov approved these changes Nov 21, 2017

View reviewed changes

[Feature] Address Ilya's comments

a73c7c1

SteveL-MSFT added Committee-Reviewed PS-Committee has reviewed this and made a decision and removed Review - Committee The PR/Issue needs a review from the PowerShell Committee labels Nov 29, 2017

SteveL-MSFT added this to the 6.0.0-GA milestone Nov 29, 2017

TravisEz13 approved these changes Nov 30, 2017

View reviewed changes

adityapatwardhan merged commit b5f84c2 into PowerShell:master Nov 30, 2017

daxian-dbw deleted the perf branch November 30, 2017 23:18

TravisEz13 modified the milestones: 6.0.0-GA, 6.0.0-RC.2 Dec 2, 2017

TravisEz13 pushed a commit to TravisEz13/PowerShell that referenced this pull request Dec 2, 2017

Skip null-element check for collections with a value-type element type (

e3f0ffd

PowerShell#5432) * Fix NotNullOrEmpty check logic * Fix a test issue on Unix

TravisEz13 mentioned this pull request Dec 2, 2017

Merge 6.0.0 rc.2 changes #5606

Merged

TravisEz13 pushed a commit that referenced this pull request Dec 2, 2017

Skip null-element check for collections with a value-type element type (

130ec8b

#5432) * Fix NotNullOrEmpty check logic * Fix a test issue on Unix

Skip null-element check for collections with a value-type element type #5432

Skip null-element check for collections with a value-type element type #5432

Uh oh!

Conversation

daxian-dbw commented Nov 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iSazonov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iSazonov Nov 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daxian-dbw commented Nov 20, 2017

Uh oh!

iSazonov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SteveL-MSFT commented Nov 29, 2017

Uh oh!

adityapatwardhan commented Nov 29, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

daxian-dbw commented Nov 13, 2017 •

edited

Loading

iSazonov Nov 14, 2017 •

edited

Loading