Skip to content
This repository was archived by the owner on Jul 15, 2023. It is now read-only.

Add missing contracts in System.Linq.Expressions.Expression and derived types #171

Merged
SergeyTeplyakov merged 6 commits into from
Jul 28, 2015
Merged

Add missing contracts in System.Linq.Expressions.Expression and derived types #171

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
7ff6fee
Added missing Expression methods.
fedotovalex Jul 23, 2015
ecdaabe
Contracts and missing methods on specific expression classes.
fedotovalex Jul 24, 2015
e8f2e7e
Added auto-generated comments for consistency.
fedotovalex Jul 24, 2015
24192e6
Updates to address review feedback.
fedotovalex Jul 25, 2015
693f570
Additional postconditions on collection members that are guaranteed t…
fedotovalex Jul 26, 2015
eb1ce00
Fix for the incorrect comment.
fedotovalex Jul 27, 2015
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
1 change: 1 addition & 0 deletions Microsoft.Research/Contracts/System.Core.Contracts/System.Core.Contracts10.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,6 +243,7 @@
<Compile Include="System.Linq.ParallelQuery.cs" />
<Compile Include="System.Linq.ParallelQuery_1.cs" />
<Compile Include="System.Linq.Queryable.cs" />
<Compile Include="System.Runtime.CompilerServices.DebugInfoGenerator.cs" />
<Compile Include="System.Runtime.CompilerServices.CallSite.cs" />
<Compile Include="System.Runtime.CompilerServices.CallSiteBinder.cs" />
<Compile Include="System.Runtime.CompilerServices.CallSite_1.cs" />
Expand Down
30 changes: 30 additions & 0 deletions ...soft.Research/Contracts/System.Core.Contracts/System.Linq.Expressions.BinaryExpression.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,5 +99,35 @@ public Expression Right
return default(Expression);
}
}

#if NETFRAMEWORK_4_0 || SILVERLIGHT_4_0 || SILVERLIGHT_5_0
//
// Summary:
// Creates a new expression that is like this one, but using the supplied children.
// If all of the children are the same, it will return this expression.
//
// Parameters:
// left:
// The System.Linq.Expressions.BinaryExpression.Left property of the result.
//
// conversion:
// The System.Linq.Expressions.BinaryExpression.Conversion property of the result.
//
// right:
// The System.Linq.Expressions.BinaryExpression.Right property of the result.
//
// Returns:
// This expression if no children are changed or an expression with the updated
// children.
[Pure]
public BinaryExpression Update(Expression left, LambdaExpression conversion, Expression right)
{
Contract.Requires(left != null);
Contract.Requires(right != null);
Contract.Ensures(Contract.Result<BinaryExpression>() != null);
return default(BinaryExpression);
}
#endif

}
}
3 changes: 2 additions & 1 deletion ...osoft.Research/Contracts/System.Core.Contracts/System.Linq.Expressions.BlockExpression.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ public ReadOnlyCollection<Expression> Expressions
get
{
Contract.Ensures(Contract.Result<ReadOnlyCollection<Expression>>() != null);
Contract.Ensures(Contract.Result<ReadOnlyCollection<Expression>>().Count >= 1);
return default(ReadOnlyCollection<Expression>);
}
}
Expand Down Expand Up @@ -135,9 +136,9 @@ protected internal override Expression Accept(ExpressionVisitor visitor)
// Returns:
// This expression if no children changed, or an expression with the updated
// children.
[Pure]
public BlockExpression Update(IEnumerable<ParameterExpression> variables, IEnumerable<Expression> expressions)
{
Contract.Requires(variables != null);
Contract.Requires(expressions != null);
Contract.Ensures(Contract.Result<BlockExpression>() != null);
return default(BlockExpression);
Expand Down
5 changes: 5 additions & 0 deletions Microsoft.Research/Contracts/System.Core.Contracts/System.Linq.Expressions.CatchBlock.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,11 @@ sealed public partial class CatchBlock {
#region Methods and constructors
private CatchBlock() {}

[Pure]
public CatchBlock Update(ParameterExpression variable, Expression filter, Expression body)
{
Contract.Requires(body != null);
Contract.Ensures(Contract.Result<CatchBlock>() != null);
return default(CatchBlock);
}
#endregion
Expand All @@ -53,6 +56,7 @@ public Expression Body
{
get
{
Contract.Ensures(Contract.Result<Expression>() != null);
return default(Expression);
}
}
Expand All @@ -69,6 +73,7 @@ public Type Test
{
get
{
Contract.Ensures(Contract.Result<Type>() != null);
return default(Type);
}
}
Expand Down
29 changes: 29 additions & 0 deletions ...Research/Contracts/System.Core.Contracts/System.Linq.Expressions.ConditionalExpression.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,5 +68,34 @@ public Expression Test
return default(Expression);
}
}

#if NETFRAMEWORK_4_0 || SILVERLIGHT_4_0 || SILVERLIGHT_5_0
//
// Summary:
// Creates a new expression that is like this one, but using the supplied children.
// If all of the children are the same, it will return this expression
//
// Parameters:
// test:
// The System.Linq.Expressions.ConditionalExpression.Test property of the result.
//
// ifTrue:
// The System.Linq.Expressions.ConditionalExpression.IfTrue property of the result.
//
// ifFalse:
// The System.Linq.Expressions.ConditionalExpression.IfFalse property of the result.
//
// Returns:
// This expression if no children changed, or an expression with the updated children.
[Pure]
public ConditionalExpression Update(Expression test, Expression ifTrue, Expression ifFalse)
{
Contract.Requires(test != null);
Contract.Requires(ifTrue != null);
Contract.Requires(ifFalse != null);
Contract.Ensures(Contract.Result<ConditionalExpression>() != null);
return default(ConditionalExpression);
}
#endif
}
}
1 change: 1 addition & 0 deletions ...t.Research/Contracts/System.Core.Contracts/System.Linq.Expressions.DebugInfoExpression.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ public SymbolDocumentInfo Document
{
get
{
Contract.Ensures(Contract.Result<SymbolDocumentInfo>() != null);
return default(SymbolDocumentInfo);
}
}
Expand Down
23 changes: 15 additions & 8 deletions ...oft.Research/Contracts/System.Core.Contracts/System.Linq.Expressions.DynamicExpression.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,13 @@
#if NETFRAMEWORK_4_0 || SILVERLIGHT_4_0 || SILVERLIGHT_5_0
// File System.Linq.Expressions.DynamicExpression.cs
// Automatically generated contract file.
using System;
using System.Collections.Generic;
using System.Collections.ObjectModel;
using System.Diagnostics.Contracts;
using System.IO;
using System.Runtime.CompilerServices;
using System.Text;
using System.Diagnostics.Contracts;
using System;

// Disable the "this variable is not used" warning as every field would imply it.
#pragma warning disable 0414
Expand Down Expand Up @@ -49,34 +51,39 @@ protected internal override Expression Accept (ExpressionVisitor visitor)
return default(Expression);
}

[Pure]
public DynamicExpression Update (IEnumerable<Expression> arguments)
{
Copy link
Contributor

@SergeyTeplyakov SergeyTeplyakov Jul 25, 2015

Choose a reason for hiding this comment

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

Does it mean that arguments could be null?

Copy link
Contributor

@SergeyTeplyakov SergeyTeplyakov Jul 25, 2015

Choose a reason for hiding this comment

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

Yep, it seems that argument could be null. This is a valid case.

Copy link
Contributor

@sharwell sharwell Jul 25, 2015

Choose a reason for hiding this comment

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

💭 Just because the implementation doesn't crash when arguments is null doesn't mean the contract needs to allow null.

Contract.Ensures(Contract.Result<DynamicExpression>() != null);
Copy link
Contributor

@sharwell sharwell Jul 25, 2015

Choose a reason for hiding this comment

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

💡 The current implementation does not require arguments to be non-null, but I'm not sure that was the intended behavior. It's not intuitive or mentioned in the documentation, and isn't consistent with other Update methods. Consider adding a requirement that arguments not be null.

In particular, the following statement in the documentation is not true if we allow arguments to be null.

If they are not equal, a new DynamicExpression instance is returned that is identical to the current instance except that the Arguments property is set to the value of parameter arguments.

Copy link
Contributor Author

@fedotovalex fedotovalex Jul 25, 2015

Choose a reason for hiding this comment

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

Yeah, this was a great source of confusion to me. It appears that as a general rule they promote nulls to empty collections, unless that collection is expected to be non-empty. If that's the case they require it be non-null.

There are many methods in Expression that don't require non-null on IEnumerable<> and params arrays for that reason. Should those be changed as well?

Copy link
Contributor

@sharwell sharwell Jul 25, 2015

Choose a reason for hiding this comment

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

There are many methods in Expression that don't require non-null on IEnumerable<> and params arrays for that reason. Should those be changed as well?

I'd like to discuss this after I finish reviewing it, and we can make a decision on whether to leave them all as-is or change them all.

Copy link
Contributor

@SergeyTeplyakov SergeyTeplyakov Jul 25, 2015

Choose a reason for hiding this comment

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

@sharwell I don't think we can and should add this precondition. You may find this code: http://referencesource.microsoft.com/#System.Core/Microsoft/Scripting/Ast/DynamicExpression.cs,710

This code means that null is an absolutely valid input.

Copy link
Contributor

@sharwell sharwell Jul 25, 2015

Choose a reason for hiding this comment

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

It appears that as a general rule they promote nulls to empty collections, unless that collection is expected to be non-empty. If that's the case they require it be non-null.

➡️ I think this is the answer. For construction and transformation, the library treats null collections as empty collections, which means null is valid anywhere that empty is. Since the fastest path through ToReadOnly for the case of an empty collection is when null is provided, it's safe to assume client code leverages this, especially in performance-sensitive code.

💭 In a world where System.Collections.Immutable preceded System.Linq.Expressions, it's likely that all of these parameters would be ImmutableArray<T> and would require non-default instances to express empty. But we don't live in that world. 😄

return default(DynamicExpression);
}
#endregion

#region Properties and indexers
public System.Collections.ObjectModel.ReadOnlyCollection<Expression> Arguments
public ReadOnlyCollection<Expression> Arguments
{
get
{
return default(System.Collections.ObjectModel.ReadOnlyCollection<Expression>);
Contract.Ensures(Contract.Result<ReadOnlyCollection<Expression>>() != null);
return default(ReadOnlyCollection<Expression>);
}
}

public System.Runtime.CompilerServices.CallSiteBinder Binder
public CallSiteBinder Binder
{
get
{
return default(System.Runtime.CompilerServices.CallSiteBinder);
Contract.Ensures(Contract.Result<CallSiteBinder>() != null);
return default(CallSiteBinder);
}
}

public System.Type DelegateType
public Type DelegateType
{
get
{
return default(System.Type);
Contract.Ensures(Contract.Result<Type>() != null);
return default(Type);
}
}

Expand Down
27 changes: 25 additions & 2 deletions Microsoft.Research/Contracts/System.Core.Contracts/System.Linq.Expressions.ElementInit.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,11 @@
// THE SOFTWARE IS PROVIDED *AS IS*, WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.

using System;
using System.Collections.Generic;
using System.Collections.ObjectModel;
using System.Diagnostics.Contracts;
using System.Reflection;
using System.Text;
using System.Diagnostics.Contracts;

namespace System.Linq.Expressions
{
Expand Down Expand Up @@ -56,9 +57,31 @@ public ReadOnlyCollection<Expression> Arguments
get
{
Contract.Ensures(Contract.Result<ReadOnlyCollection<Expression>>() != null);
Contract.Ensures(Contract.Result<ReadOnlyCollection<Expression>>().Count >= 1);
return default(ReadOnlyCollection<Expression>);
}
}

#if NETFRAMEWORK_4_0 || SILVERLIGHT_4_0 || SILVERLIGHT_5_0
//
// Summary:
// Creates a new expression that is like this one, but using the supplied children.
// If all of the children are the same, it will return this expression.
//
// Parameters:
// arguments:
// The System.Linq.Expressions.ElementInit.Arguments property of the result.
//
// Returns:
// This expression if no children are changed or an expression with the updated
// children.
[Pure]
public ElementInit Update(IEnumerable<Expression> arguments)
{
Contract.Requires(arguments != null);
Contract.Ensures(Contract.Result<ElementInit>() != null);
return default(ElementInit);
}
#endif
}
}
}
Loading