Skip to content

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

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.

Sign up for GitHub

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

Merged
adityapatwardhan merged 7 commits into from
Nov 30, 2017
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
602472d
Fix NotNullOrEmpty check logic
daxian-dbw Nov 13, 2017
c303e9f
Fix a test issue on Unix
daxian-dbw Nov 13, 2017
38364cd
Remove extra line ending
daxian-dbw Nov 13, 2017
5dfcb73
Address comments
daxian-dbw Nov 14, 2017
cdc06a9
Address Ilya's feedback
daxian-dbw Nov 20, 2017
f64c996
Fix minor error
daxian-dbw Nov 20, 2017
a73c7c1
[Feature] Address Ilya's comments
daxian-dbw Nov 28, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
127 changes: 68 additions & 59 deletions src/System.Management.Automation/engine/Attributes.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1865,11 +1865,40 @@ public ValidateUserDriveAttribute()
#endregion

#region NULL validation attributes

/// <summary>
/// Base type of Null Validation attributes.
/// </summary>
public abstract class NullValidationAttributeBase : ValidateArgumentsAttribute
{
/// <summary>
/// Check if the argument type is a collection.
/// </summary>
protected bool IsArgumentCollection(Type argumentType, out bool isElementValueType)
{
isElementValueType = false;
var information = new ParameterCollectionTypeInformation(argumentType);
switch (information.ParameterCollectionType)
{
// If 'arguments' is an array, or implement 'IList', or implement 'ICollection<>'
// then we continue to check each element of the collection.
case ParameterCollectionType.Array:
case ParameterCollectionType.IList:
case ParameterCollectionType.ICollectionGeneric:
Type elementType = information.ElementType;
isElementValueType = elementType != null && elementType.IsValueType;
return true;
default:
return false;
}
}
}

/// <summary>
/// Validates that the parameters's argument is not null
/// </summary>
[AttributeUsage(AttributeTargets.Field | AttributeTargets.Property)]
public sealed class ValidateNotNullAttribute : ValidateArgumentsAttribute
public sealed class ValidateNotNullAttribute : NullValidationAttributeBase
{
/// <summary>
/// Verifies the argument is not null and if it is a collection, that each
Expand All @@ -1891,20 +1920,23 @@ public sealed class ValidateNotNullAttribute : ValidateArgumentsAttribute
/// </exception>
protected override void Validate(object arguments, EngineIntrinsics engineIntrinsics)
{
IEnumerable ienum = null;
IEnumerator itor = null;

if (arguments == null || arguments == AutomationNull.Value)
{
throw new ValidationMetadataException(
"ArgumentIsNull",
null,
Metadata.ValidateNotNullFailure);
}
else if ((ienum = arguments as IEnumerable) != null)
else if (IsArgumentCollection(arguments.GetType(), out bool isElementValueType))
{
foreach (object element in ienum)
// 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) { return; }

IEnumerator ienum = LanguagePrimitives.GetEnumerator(arguments);
while (ienum.MoveNext())
{
object element = ienum.Current;
if (element == null || element == AutomationNull.Value)
{
throw new ValidationMetadataException(
Expand All @@ -1914,19 +1946,6 @@ protected override void Validate(object arguments, EngineIntrinsics engineIntrin
}
}
}
else if ((itor = arguments as IEnumerator) != null)
{
for (; itor.MoveNext() == true;)
{
if (itor.Current == null || itor.Current == AutomationNull.Value)
{
throw new ValidationMetadataException(
"ArgumentIsNull",
null,
Metadata.ValidateNotNullCollectionFailure);
}
}
}
}
}

Expand All @@ -1935,7 +1954,7 @@ protected override void Validate(object arguments, EngineIntrinsics engineIntrin
/// an empty string, and is not an empty collection.
/// </summary>
[AttributeUsage(AttributeTargets.Field | AttributeTargets.Property)]
public sealed class ValidateNotNullOrEmptyAttribute : ValidateArgumentsAttribute
public sealed class ValidateNotNullOrEmptyAttribute : NullValidationAttributeBase
{
/// <summary>
/// Validates that the parameters's argument is not null, is not
Expand All @@ -1954,18 +1973,14 @@ public sealed class ValidateNotNullOrEmptyAttribute : ValidateArgumentsAttribute
/// </exception>
protected override void Validate(object arguments, EngineIntrinsics engineIntrinsics)
{
IEnumerable ienum = null;
IEnumerator itor = null;
string str = null;

if (arguments == null || arguments == AutomationNull.Value)
{
throw new ValidationMetadataException(
"ArgumentIsNull",
null,
Metadata.ValidateNotNullOrEmptyFailure);
}
else if ((str = arguments as String) != null)
else if (arguments is string str)
{
if (String.IsNullOrEmpty(str))
{
Expand All @@ -1975,56 +1990,50 @@ protected override void Validate(object arguments, EngineIntrinsics engineIntrin
Metadata.ValidateNotNullOrEmptyFailure);
}
}
else if ((ienum = arguments as IEnumerable) != null)
else if (IsArgumentCollection(arguments.GetType(), out bool isElementValueType))
Copy link
Collaborator

@iSazonov iSazonov Nov 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is in the context isEmpty = !isElementValueType ?

Copy link
Member Author

@daxian-dbw daxian-dbw Nov 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
int validElements = 0;
foreach (object element in ienum)
{
validElements++;
if (element == null || element == AutomationNull.Value)
{
throw new ValidationMetadataException(
"ArgumentIsNull",
null,
Metadata.ValidateNotNullOrEmptyCollectionFailure);
}
bool isEmpty = true;
IEnumerator ienum = LanguagePrimitives.GetEnumerator(arguments);
if (ienum.MoveNext()) { isEmpty = false; }

string elementAsString = element as String;
if (elementAsString != null)
{
if (String.IsNullOrEmpty(elementAsString))
// 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 (!isEmpty && !isElementValueType)
{
do {
object element = ienum.Current;
if (element == null || element == AutomationNull.Value)
{
throw new ValidationMetadataException(
"ArgumentCollectionContainsEmpty",
"ArgumentIsNull",
null,
Metadata.ValidateNotNullOrEmptyFailure);
Metadata.ValidateNotNullOrEmptyCollectionFailure);
}
}

if (element is string elementAsString)
{
if (String.IsNullOrEmpty(elementAsString))
{
throw new ValidationMetadataException(
"ArgumentCollectionContainsEmpty",
null,
Metadata.ValidateNotNullOrEmptyCollectionFailure);
}
}
} while (ienum.MoveNext());
}

if (validElements == 0)
if (isEmpty)
{
throw new ValidationMetadataException(
"ArgumentIsEmpty",
null,
Metadata.ValidateNotNullOrEmptyCollectionFailure);
}
}
else if ((itor = arguments as IEnumerator) != null)
else if (arguments is IDictionary dict)
{
int validElements = 0;
for (; itor.MoveNext() == true;)
{
validElements++;
if (itor.Current == null || itor.Current == AutomationNull.Value)
{
throw new ValidationMetadataException(
"ArgumentIsNull",
null,
Metadata.ValidateNotNullOrEmptyCollectionFailure);
}
}
if (validElements == 0)
if (dict.Count == 0)
{
throw new ValidationMetadataException(
"ArgumentIsEmpty",
Expand Down
2 changes: 1 addition & 1 deletion src/System.Management.Automation/engine/CompiledCommandParameter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -698,7 +698,7 @@ internal ParameterCollectionTypeInformation(Type type)
/// <summary>
/// The type of the elements in the collection
/// </summary>
internal Type ElementType { get; set; }
internal Type ElementType { get; private set; }
}
}

Expand Down
26 changes: 17 additions & 9 deletions src/System.Management.Automation/engine/ParameterBinderBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -787,18 +787,26 @@ private void ValidateNullOrEmptyArgument(

// Ensure that each element abides by the metadata
bool isEmpty = true;
Type elementType = parameterMetadata.CollectionTypeInformation.ElementType;
bool isElementValueType = elementType != null && elementType.IsValueType;

// Note - we explicitly don't pass the context here because we don't want
// the overhead of the calls that check for stopping.
while (ParserOps.MoveNext(null, null, ienum))
if (ParserOps.MoveNext(null, null, ienum)) { 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 (!isEmpty && !isElementValueType)
{
object element = ParserOps.Current(null, ienum);
isEmpty = false;
ValidateNullOrEmptyArgument(
parameter,
parameterMetadata,
parameterMetadata.CollectionTypeInformation.ElementType,
element,
false);
do {
object element = ParserOps.Current(null, ienum);
ValidateNullOrEmptyArgument(
parameter,
parameterMetadata,
parameterMetadata.CollectionTypeInformation.ElementType,
element,
false);
} while (ParserOps.MoveNext(null, null, ienum));
}

if (isEmpty && !parameterMetadata.AllowsEmptyCollectionArgument)
Expand Down
1 change: 0 additions & 1 deletion src/System.Management.Automation/engine/runtime/Binding/Binders.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -594,7 +594,6 @@ public override DynamicMetaObject FallbackConvert(DynamicMetaObject target, Dyna
return (errorSuggestion ?? NullResult(target)).WriteToDebugLog(this);
}

// In CORECLR System.Data.DataTable does not have the DataRowCollection IEnumerable, so disabling code.
if (targetValue is DataTable)
{
// Generate:
Expand Down
120 changes: 120 additions & 0 deletions test/powershell/engine/Basic/ValidateAttributes.Tests.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -260,4 +260,124 @@ Describe 'Validate Attributes Tests' -Tags 'CI' {
$ScriptBlock | Should Not Throw
}
}

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

BeforeAll {
function MandatoryFunc {
param(
[Parameter(Mandatory, ParameterSetName = "ByteArray")]
[byte[]] $ByteArray,

[Parameter(Mandatory, ParameterSetName = "ByteList")]
[System.Collections.Generic.List[byte]] $ByteList,

[Parameter(Mandatory, ParameterSetName = "ByteCollection")]
[System.Collections.ObjectModel.Collection[byte]] $ByteCollection,

[Parameter(ParameterSetName = "Default")]
$Value
)
}

function NotNullFunc {
param(
[ValidateNotNull()]
$Value,
[string] $TestType
)

switch ($TestType) {
"COM-Enumerable" { $Value | ForEach-Object Name }
"Enumerator" {
$items = foreach ($i in $Value) { $i }
$items -join ","
}
}
}

function NotNullOrEmptyFunc {
param(
[ValidateNotNullOrEmpty()]
$Value,
[string] $TestType
Copy link
Member

@adityapatwardhan adityapatwardhan Nov 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@daxian-dbw daxian-dbw Nov 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

)

switch ($TestType) {
"COM-Enumerable" { $Value | ForEach-Object Name }
"Enumerator" {
$items = foreach ($i in $Value) { $i }
$items -join ","
}
}
}

$filePath = Join-Path -Path $PSHOME -ChildPath System.Management.Automation.dll
$byteArray = [System.IO.File]::ReadAllBytes($filePath)
$byteList = [System.Collections.Generic.List[byte]] $byteArray
$byteCollection = [System.Collections.ObjectModel.Collection[byte]] $byteArray
## Use the running time of 'MandatoryFunc -Value $byteArray' as the baseline time
$baseline = (Measure-Command { MandatoryFunc -Value $byteArray }).Milliseconds
## Running time should be less than 'expected'
$expected = $baseline + 20

if ($IsWindows) {
$null = New-Item -Path $TESTDRIVE/file1
}

$testCases = @(
@{ ScriptBlock = { MandatoryFunc -ByteArray $byteArray } }
@{ ScriptBlock = { MandatoryFunc -ByteList $byteList } }
@{ ScriptBlock = { MandatoryFunc -ByteCollection $byteCollection } }
@{ ScriptBlock = { NotNullFunc -Value $byteArray } }
@{ ScriptBlock = { NotNullFunc -Value $byteList } }
@{ ScriptBlock = { NotNullFunc -Value $byteCollection } }
@{ ScriptBlock = { NotNullOrEmptyFunc -Value $byteArray } }
@{ ScriptBlock = { NotNullOrEmptyFunc -Value $byteList } }
@{ ScriptBlock = { NotNullOrEmptyFunc -Value $byteCollection } }
)
}

It "Validate running time '<ScriptBlock>'" -TestCases $testCases {
param ($ScriptBlock)
(Measure-Command $ScriptBlock).Milliseconds | Should BeLessThan $expected
}

It "COM enumerable argument should work with 'ValidateNotNull' and 'ValidateNotNullOrEmpty'" -Skip:(!$IsWindows) {
$shell = New-Object -ComObject "Shell.Application"
$folder = $shell.Namespace("$TESTDRIVE")
$items = $folder.Items()

NotNullFunc -Value $items -TestType "COM-Enumerable" | Should Be "file1"
NotNullOrEmptyFunc -Value $items -TestType "COM-Enumerable" | Should Be "file1"
}

It "Enumerator argument should work with 'ValidateNotNull' and 'ValidateNotNullOrEmpty'" {
$data = @(1,2,3)
NotNullFunc -Value $data.GetEnumerator() -TestType "Enumerator" | Should Be "1,2,3"
NotNullOrEmptyFunc -Value $data.GetEnumerator() -TestType "Enumerator" | Should Be "1,2,3"
}

It "'ValidateNotNull' should throw on null element of a collection argument" {
## Should throw on null element
{ NotNullFunc -Value @("string", $null, 2) } | Should Throw
## Should not throw on empty string element
{ NotNullFunc -Value @("string", "", 2) } | Should Not Throw
## Should not throw on an empty collection
{ NotNullFunc -Value @() } | Should Not Throw
}

It "'ValidateNotNullOrEmpty' should throw on null element of a collection argument or empty collection/dictionary" {
{ NotNullOrEmptyFunc -Value @("string", $null, 2) } | Should Throw
{ NotNullOrEmptyFunc -Value @("string", "", 2) } | Should Throw
{ NotNullOrEmptyFunc -Value @() } | Should Throw
{ NotNullOrEmptyFunc -Value @{} } | Should Throw
}

It "Mandatory parameter should throw on empty collection" {
{ MandatoryFunc -ByteArray ([byte[]]@()) } | Should Throw
{ MandatoryFunc -ByteList ([System.Collections.Generic.List[byte]]@()) } | Should Throw
{ MandatoryFunc -ByteList ([System.Collections.ObjectModel.Collection[byte]]@()) } | Should Throw
}
}
}