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

[fedotovalex]

This addresses #170. Adds contracts for:

• static methods in the Expression class
• Update methods in specific expression classes
• methods and properties in new classes added in .NET 4

[sharwell]

@fedotovalex This is great! 😄 Note that I'm hand reviewing every added contract for accuracy, so please do not rebase, squash, or amend any commits in this pull request (doing so will force me to start over). I have not found any mistakes yet, but even if I do please just correct them as new commits.

[SergeyTeplyakov]

Does it mean that arguments could be null?

[SergeyTeplyakov]

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

[sharwell]

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

[sharwell]

Review from Expression.cs:

• 💡 The requires lines in DebugInfo(SymbolDocumentInfo, int, int, int, int) might be improved by using startLine >= 1 instead of startLine > 0. In the editor extensions this drives home the fact that these are 1-based indexes. (All assuming of course that the editor extensions actually use the requirement in string form.)
• ❓ In Default(Type), what happens if type is null? It won't fail there, but will it fail when you try to compile the IL?
• 💡 Expression.Parameter (two overloads) can each require type != typeof(void).
• ❗ The requirement in Expression.Property(Expression, PropertyInfo, IEnumerable<Expression>) that instance != null is incorrect.
• ❗ The requirement in Expression.Property(Expression, Type, string) that expression != null is incorrect.
• ❗ The requirement in Expression.Property(Expression, PropertyInfo, Expression[]) that instance != null is incorrect.
• ❗ The requirement in Expression.Property(Expression, string, Expression[]) that instance != null is incorrect.
• 💡 Expression.Switch (6 overloads) can include a requirement that cases != null (could also require non-empty and non-null items if we are doing that)
• 💡 Expression.SwitchCase (2 overloads) can include a requirement that testValues != null (could also require non-empty and non-null items if we are doing that)
• 💡 Expression.TryCatch(Expression, CatchBlock[]) can require handlers != null (could also require non-empty and non-null if we are doing that)
• 💡 Expression.TryCatchFinally(Expression, Expression, CatchBlock[]) can require finally != null || handlers != null (could also require items in handlers to be non-null when handlers itself is not null)
• 💡 Expression.TryFault(Expression, Expression) can require fault != null
• 💡 Expression.TryFinally(Expression, Expression) can require finally != null

[sharwell]

Outstanding work overall @fedotovalex. You'll definitely need to update the items marked with ❗ since they are incorrect and the documentation explicitly covers the case. Let me know if you have any questions.

[fedotovalex]

@sharwell, thanks for the thorough review.

Most of the items from the Expression.cs list are done. Here are some comments:

• An attempt to compile an expression with Default(null) results in ArgumentNullException deep in the code when it tries to insert a null type value into a dictionary, so I added a precondition for type != null. Perhaps Microsoft guys can file an internal ticket to have it checked at runtime.
• I did not include a precondition for type != typeof(void) in Parameter overloads because it cannot be proven by the static checker in the simplest cases.

Contract.Assert(typeof(string) != typeof(void)); // CodeContracts: assert unproven

• In Expression.Property(Expression, string, Expression[]), expression != null is actually required as the expression is the only source of type information (http://referencesource.microsoft.com/#System.Core/Microsoft/Scripting/Ast/IndexExpression.cs,214, RequiresCanRead implies non-null).

Everything else on the list should be addressed.

[brettshearer]

Can we use PEX or similar to automatically generate tests for the framework, watch for exceptions and autogenerate contracts based on these?
We are trying to use ReviewBot internally to similar effect (but in our case, generating contracts in our own code).

From: Alex Fedotov [mailto:notifications@github.com]
Sent: Saturday, 25 July 2015 7:26 PM
To: Microsoft/CodeContracts
Subject: Re: [CodeContracts] Add missing contracts in System.Linq.Expressions.Expression and derived types (#171)

@sharwellhttps://github.com/sharwell, thanks for the thorough review.

Most of the items from the Expression.cs list are done. Here are some comments:

· An attempt to compile an expression with Default(null) results in ArgumentNullException deep in the code when it tries to insert a null type value into a dictionary, so I added a precondition for type != null. Perhaps Microsoft guys can file an internal ticket to have it checked at runtime.

· I did not include a precondition for type != typeof(void) in Parameter overloads because it cannot be proven by the static checker in the simplest cases.

Contract.Assert(typeof(string) != typeof(void)); // CodeContracts: assert unproven

• In Expression.Property(Expression, string, Expression[]), expression != null is actually required as the expression is the only source of type information (http://referencesource.microsoft.com/#System.Core/Microsoft/Scripting/Ast/IndexExpression.cs,214, RequiresCanRead implies non-null).

Everything else on the list should be addressed.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/171#issuecomment-124825756.

[sharwell]

In Expression.Property(Expression, string, Expression[]), expression != null is actually required as the expression is the only source of type information (http://referencesource.microsoft.com/#System.Core/Microsoft/Scripting/Ast/IndexExpression.cs,214, RequiresCanRead implies non-null).

I was referring to Expression.Property(Expression, Type, string).

Edit: I see you meant to say instance, not expression. For the overload you mentioned, the documentation for the parameter is incorrect. It should probably be updated to remove the suggestion that instance could be null.

[fedotovalex]

For the overload you mentioned, the documentation for the parameter is incorrect. It should probably be updated to remove the suggestion that instance could be null.

Okay, I updated the comment in the code and also submitted feedback on the corresponding MSDN page.

Let me know if there are any other issues I need to address before this PR can move forward.

[SergeyTeplyakov]

I think everything is looking good and feature is ready for merging.

Any objections? @fedotovalex, @sharwell?

[sharwell]

👍

[SergeyTeplyakov]

Merged. Thanks a lot, @sharwell, @fedotovalex!