Skip to content
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.

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

Support for ValueTask #1658

Closed
thomaslevesque opened this issue Oct 14, 2019 · 7 comments · Fixed by #1666
Closed

Support for ValueTask #1658

thomaslevesque opened this issue Oct 14, 2019 · 7 comments · Fixed by #1666
Assignees
Milestone

Comments

@thomaslevesque
Copy link
Member

Currently FakeItEasy has special support for Task and Task<T>:

  • Dummy creation strategy: return a completed Task, or Task<T> whose result is a dummy T
  • Special overload for configuring the return value of a method that returns Task<T>, where the user can just pass the task's value instead of the task itself.

We should consider supporting ValueTask and ValueTask<T> as well. But it might not be as simple as for Task and Task<T>...

  • ValueTask<T> is only available in .NET Core (all versions) and .NET Standard 2.1
  • ValueTask is only available in .NET Core 2.1+ and .NET Standard 2.1
  • BUT there's a NuGet package supported on .NET Standard 1.0+ that exposes both types

So, if we add support for ValueTask only in a new .NET Standard 2.1 target, projects that target TFMs not compatible with .NET Standard 2.1 and use the package won't benefit from it. A possible approach would be to offer the Returns overloads for ValueTask in a separate package, and support dummy creation dynamically (like we did for ValueTuple)

Also, ValueTask[<T>] doesn't work exactly the same as Task[<T>], there are a few subtleties. For instance, you can't await the same ValueTask[<T>] more than once, which will have to be taken into account for dummy creation (we'll have to return a new one for each call).

@blairconrad
Copy link
Member

These optimizations and improvements in the libraries are not making things easy on us!
As you suggest, we can implement the two facets separately (even in different release versions, if we had to).

I think supporting Dummy creation via reflection for all versions makes sense.
By

A possible approach would be to offer the Returns overloads for ValueTask in a separate package

do you mean something like FakeItEasy.ValueTask.Extensions (or whatever; I did not spend long on that name) with dependencies on FakeItEasy and System.Threading.Tasks.Extensions? Could do. I assume we'd target .NET Standard 1.6 and 2.0 only and supply built-in overloads in our .NET Standard 2.1+-targeting libs?

@blairconrad
Copy link
Member

blairconrad commented Oct 14, 2019

Also, ValueTask[<T>] doesn't work exactly the same as Task[<T>], there are a few subtleties. For instance, you can't await the same ValueTask[<T>] more than once, which will have to be taken into account for dummy creation (we'll have to return a new one for each call).

I don't quite follow this. We typically return a new instance when we create a Dummy. Or are you suggesting that auto-properties are configured to return a new instance each time, in contrast with the current behaviour for all other types? I'm uneasy with this. I can see why it might be helpful, but it would be yet another special case that users have to keep in their heads. In general, I think having a ValueTask as a property would be a mistake…

@thomaslevesque
Copy link
Member Author

By

A possible approach would be to offer the Returns overloads for ValueTask in a separate package

do you mean something like FakeItEasy.ValueTask.Extensions (or whatever; I did not spend long on that name) with dependencies on FakeItEasy and System.Threading.Tasks.Extensions? Could do.

Yes.

I assume we'd target .NET Standard 1.6 and 2.0 only and supply built-in overloads in our .NET Standard 2.1+-targeting libs?

I hadn't thought of including the overloads in the .NET Standard 2.1 targets, but yes, it would make sense. My only worry is that if someone uses the extension package in a .NET Core 2.0 project, then upgrade to .NET Core 3.0, they will have the extension method twice, which will cause an ambiguity...

Also, ValueTask doesn't work exactly the same as Task, there are a few subtleties. For instance, you can't await the same ValueTask more than once, which will have to be taken into account for dummy creation (we'll have to return a new one for each call).

I don't quite follow this. We typically return a new instance when we create a Dummy. Or are you suggesting that auto-properties are configured to return a new instance each time, in contrast with the current behaviour for all other types? I'm uneasy with this. I can see why it might be helpful, but it would be yet another special case that users have to keep in their heads. In general, I think having a ValueTask as a property would be a mistake…

Currently, if you do this:

A.CallTo(() => foo.BarAsync()).Returns(42); // actually returns a `Task<int>` whose value is 42

The same task is returned for every call. I think it might not work for ValueTask: we need to return a new ValueTask every time (with the same value). So the ValueTask overload would probably look like this:

public static IAfterCallConfiguredWithOutAndRefParametersConfiguration<IReturnValueConfiguration<ValueTask<T>>> Returns<T>(
    this IReturnValueConfiguration<ValueTask<T>> configuration, T value)
{
    returns configuration.ReturnsLazily(() => new ValueTask<T>(value));
}

instead of

public static IAfterCallConfiguredWithOutAndRefParametersConfiguration<IReturnValueConfiguration<ValueTask<T>>> Returns<T>(
    this IReturnValueConfiguration<ValueTask<T>> configuration, T value)
{
    var task = new ValueTask<T>(value);
    returns configuration.Returns(task);
}

@thomaslevesque
Copy link
Member Author

thomaslevesque commented Oct 15, 2019

The same task is returned for every call.

Ah, it looks like my assumption was wrong... in fact we lazily create a new task every time. Just ignore me, we can do exactly the same for ValueTask

Also, synchronously completed ValueTasks (that just contain a value) can be awaited multiple times, apparently. So there's no problem here after all.

@blairconrad
Copy link
Member

My only worry is that if someone uses the extension package in a .NET Core 2.0 project, then upgrade to .NET Core 3.0, they will have the extension method twice, which will cause an ambiguity...

I'm strangely unconcerned about this. Would they not also all of a sudden have conflicting ValueTask types? It seems this situation will arise more and more, the way Microsoft is choosing to distribute new functionality. Besides, I think a new error after upgrade that says "this thing you used to just get from a FakeItEasy.ValueTask.Extensions package is now in the Extensions package and the main FakeItEasy package" should be easy to resolve.

However, this might help us define the scope of any FakeItEasy.ValueTask.Extensions package. I was wondering after I typed that whether we shouldn't have chunkier packages, rather than releasing many small ones (assuming there might be more), and I'm now thinking that many small packages is preferable—I don't think we should make guesses about the .NET versions that will end up adding or removing various features.

@thomaslevesque
Copy link
Member Author

Would they not also all of a sudden have conflicting ValueTask types?

No, because the netcoreapp3.0/netstandard2.1 TFMs of this package don't contain the ValueTask type.

But yes, the problem is fairly easy to fix. And we could even avoid the error entirely, by multitargeting the package. The netcoreapp3.0/netstandard2.1 TFMs would contain nothing, since FakeItEasy would already define the extension methods for these TFMs. We could consider including a target that causes a warning (like we did for FakeItEasy.Analyzer) to indicate the package is no longer necessary.

@thomaslevesque
Copy link
Member Author

This change has been released as part of FakeItEasy 5.4.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants