-
Notifications
You must be signed in to change notification settings - Fork 150
Update System.Threading.Tasks.Task for NET4.5/4.6 #296
Conversation
Adds missing .NET 4.5 API Task.FromResult<TResult>, and adds new .NET 4.6 APIs FromCanceled, FromCanceled<TResult>, FromException, FromException<TResult> and CompletedTask.
Any other suggestions? |
Getting this error now when running buildCC.bat, but no idea why:
Do I need to define those properties and the enum in the Contracts project? |
Yes, you need to uncomment these properties in the Task class. I see the properties are there, but they were commented out, probably they did not seem relevant for the contracts assembly before (but now they are). You will also need to add the TaskStatus enum. |
OK, seems to be compiling now. Thanks for the pointers, @fedotovalex. |
I should probably add actual contracts for |
Contract.Ensures(!Contract.Result<Task<TResult>>().IsCanceled); | ||
Contract.Ensures(Contract.Result<Task<TResult>>().IsCompleted); | ||
Contract.Ensures(!Contract.Result<Task<TResult>>().IsFaulted); | ||
Contract.Ensures(Contract.Result<Task<TResult>>().Status == TaskStatus.RanToCompletion); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious would it be enough to leave just one postcondition that Status == TaskStatus.RanToCompletion
and than express (via postconditions) relationship between Status
and IsCanceled
, IsCompleted
and IsFaulted
?
I'm fine with this approach but proposed one could be slightly easier to read, because all those postconditions:
+ Contract.Ensures(Contract.Result<Task<TResult>>().Exception == null);
+ Contract.Ensures(!Contract.Result<Task<TResult>>().IsCanceled);
+ Contract.Ensures(Contract.Result<Task<TResult>>().IsCompleted);
+ Contract.Ensures(!Contract.Result<Task<TResult>>().IsFaulted);
Are just redundant.
Add some minor comments, but everything is LGTM. |
Thanks. I'll take a shot at addressing your comments early next week. |
Contract.Ensures(Contract.Result<Task>().IsCompleted); | ||
Contract.Ensures(!Contract.Result<Task>().IsFaulted); | ||
Contract.Ensures(Contract.Result<Task>().Status == TaskStatus.Canceled); | ||
Contract.EnsuresOnThrow<ArgumentOutOfRangeException>(true, "Cancellation has not been requested for cancellationToken; its IsCancellationRequested property is false."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the purpose of this kinds of contracts? It never fails at runtime and it seems that it has no value for static analyzer. Is it some kind of documentation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe it actually does help static analyzer. Imagine you have a method
public void HandleCompletedTask(Task task)
{
Contract.Requires(task.IsCompleted);
...
}
With the contracts above, you should be able to write
HandleCompletedTask(Task.FromCanceled(cancellationToken));
and the static checker would not complain. Without the contract, you'll have to add an explicit Assume to keep the static checker quiet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought @hubuk's comment was specifically on the EnsuresOnThrow
, in which case I have no idea what it's for, but I've added it purely to be consistent with existing contracts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, my question was about
Contract.EnsuresOnThrow<ArgumentOutOfRangeException>(true, "Cancellation has not been requested for cancellationToken; its IsCancellationRequested property is false.");
Never saw contracts like this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are quite a handful already.
Update System.Threading.Tasks.Task for NET4.5/4.6
@yaakov-h This PR broke the build. I'm going to revert it to fix the master. |
@SergeyTeplyakov It seems as this is not the only one PR that required fixing. There are other problems related to enabling contracts for .Net 4.6. Looking into it right now. |
Odd, the build appeared to work... I'd guess it's because I only tested this building on top of master, not on top of #291 (I think? Hard to remember) so |
Task.FromResult<TResult>
Task.FromCanceled
Task.FromCanceled<TResult>
Task.FromException
Task.FromException<TResult>
Task.CompletedTask