feat: Add ValueTask support

[MarcelRoozekrans]

• Added ValueTask overloads
• Added TestCases for the new ValueTask overloads
• Refactored tests to be more consistent
• Added missing test cases for current extensions

[cportwood]

Thanks @MarcelRoozekrans for getting on top of that! I got tickets for KCDC last minute so I got delayed. I had started on that this weekend.

[cportwood]

Do we wish to use the Default implementation of DefaultAwait used for Tasks?

public static ConfiguredValueTaskAwaitable<T> DefaultAwait<T>(this ValueTask<T> task) => task.ConfigureAwait(Result.DefaultConfigureAwait);

Unfortunately the implementation for ValueTask.ConfigureAwait() is not the same as Task.ConfigureAwait() which means in a value task you may not be returned to the same synchronization context.

dotnet/runtime#71313

[cportwood]

It does look like the synchronization context issue is on the backlog to be corrected. There is no due date for it as of yet though.

Maybe we keep this on hold until it is fixed in .net?

[vkhorikov]

Thank you for this work. There are a couple of earlier smaller PRs that I've merged to master. Would you please rebase this PR?

Also, I'm wondering if we could code-gen all the ValueTask counterparts.

[vkhorikov]

@cportwood Regarding the sync context -- my understanding was that .NET Core (and therefore .NET 5 & 6) don't have a sync context anymore. Is that correct? Also, AFAIK, ValueTask is only supported by .NET Core (not the "classic" .NET Framework e.g 4.5). If so, we might not need DefaultAwait for ValueTask.

[cportwood]

@vkhorikov, thank you for that response. I had looked at that. I was not sure if that would have any affect on a dependency written in .Net Framework. Thinking through how this library works I don't think there is any way that this library would conflict with a dependency as there is no reflection being done to invoke anything so I feel your assumptions should be valid. I agree that removing the DefaultAwait and I was going to suggest code generation for Task and ValueTask as well anyway. We could get that feature and a refactor in at the same time?

[MarcelRoozekrans]

@vkhorikov Rebased as requested and removed the DefaultAwait for the ValueTask extensions. Because I agree with that form Net5 onwards the synchronisationContext has been removed.
Although it might be a consideration that we leave the decision on the synchronizationContext to the host project. Having the DefaultAwait implemented will not hurt the execution in anyway.

Regarding the source code generation, it crossed my mind but I’m to unfamiliar with it.

[vkhorikov]

Thanks @MarcelRoozekrans , merging this PR, will publish it shortly as v2.33.0.

Let's keep the explicit version for now. Will explore code generation as a separate feature.

Although it might be a consideration that we leave the decision on the synchronizationContext to the host project

We should definitely keep it for regular Tasks because those can be used in .NET Framework 4.5 and 4.0 (which this library also supports), but no need to keep them in code that targets later versions.

[vkhorikov]

@MarcelRoozekrans Looks that TapIf methods were accidentally deleted ( #436 ). I've brought them back, could you please refactor them as well?

[MarcelRoozekrans]

@vkhorikov Sure don't know what happend here but seems that I lost them. Openend new PR with the refactor if the missing tests.

[vkhorikov]

Thank you. Will publish the refactorings as a minor version update shortly.

[hankovich]

@vkhorikov @MarcelRoozekrans Please note ValueTask support introduces breaking changes. After switching from version 2.31.1 to version 2.33.2, I got a lot of compilation errors.

I've detected two types of them:

1. Result.Try

error CS0121: The call is ambiguous between the following methods or properties: 'Result.Try(Func<Task>, Func<Exception, string>)' and 'Result.Try(Func<ValueTask>, Func<Exception, string>)'

error CS0121: The call is ambiguous between the following methods or properties: 'Result.Try<T, E>(Func<Task<T>>, Func<Exception, E>)' and 'Result.Try<T, E>(Func<ValueTask<T>>, Func<Exception, E>)'

in the following code

public Task<Result<Profile>> GetProfileAsync() =>
    Result.Try(async () =>
    {
        await Task.Yield();
        
        return new Profile();
    });

public Task<Result<Profile, Error>> GetProfileWithErrorAsync() =>
    Result.Try(async () =>
    {
        await Task.Yield();
        
        return new Profile();
    }, _ => new Error());

2. Result.Bind

error CS0121: The call is ambiguous between the following methods or properties: 'AsyncResultExtensionsRightOperand.Bind<T, K, E>(Result<T, E>, Func<T, Task<Result<K, E>>>)' and 'AsyncResultExtensionsRightOperand.Bind<T, K, E>(Result<T, E>, Func<T, ValueTask<Result<K, E>>>)'

in the following code

Result<T1, E> result = Result.Success<T1, E>(new());

Task<Result<T2, E>> GetTask() => Task.FromResult(Result.Success<T2, E>(new()));

await result
    .Bind(async _ => await GetTask());

class T1
{

}

class T2
{

}

class E
{

}

But it looks like all async lambdas in the existing async right extensions are broken.

[vkhorikov]

@hankovich Thanks for posting the code, I've added it as failing tests (AmbiguityTests).

@hankovich @MarcelRoozekrans I see 2 possible fixes here:

1. Remove all XXX.Task.Right.cs versions in favor of XXX.ValueTask.Right.cs -- May lead to other potential compatibility issues, though I'm not 100% sure

2. Remove all XXX.ValueTask.Right.cs versions

What do you think?

[MarcelRoozekrans]

@vkhorikov and @hankovich instead of having an inline fucntio you could make a external one with a specific signatue. This was it chooses the right overload. the async/await syntactic sugar applies to Task and ValueTask.

  public Task<Result<Profile>> GetProfileAsync() => Result.Try(Func);

  private static async Task<Profile> Func()
  {
      await Task.Yield();
      return new object();
  }

We could also sort it by @ProphetLamb solution in splitting up the library.

[vkhorikov]

I'd like to avoid breaking changes as much as possible. Another option is to rename ValueTask counterparts into XXXValueTask() , e.g. BindValueTask().

[MarcelRoozekrans]

I'd like to avoid breaking changes as much as possible. Another option is to rename ValueTask counterparts into XXXValueTask() , e.g. BindValueTask().

Renaming is also a good options, only for the right methods then ?? or would you likt to have them all renamed ?

[vkhorikov]

Let's rename all ValueTask methods for consistency sake.

[cportwood]

I like the rename option as well because I personally like to quickly see when something goes asynchronous in functional styles. Not so much for when I am writing but maintaining other's code.

[MarcelRoozekrans]

@vkhorikov Need to find some time though to execute it quite a busy schedule today.

[vkhorikov]

@MarcelRoozekrans No worries, it can wait a day or two. I can also do this myself over this weekend.

[hankovich]

instead of having an inline fucntio you could make a external one with a specific signatue. This was it chooses the right overload. the async/await syntactic sugar applies to Task and ValueTask.

Thanks, I understand why this code doesn't compile anymore. My concern is not how to change it so it compiles again, but why it doesn't compile anymore after updating the minor package version.

Moreover I personally like lambdas and I use them everywhere. I don't think that the library should force users to use method groups instead of lambdas.

---

I don't generally think that

Remove all XXX.Task.Right.cs versions in favor of XXX.ValueTask.Right.cs

is user-friendly enough, because it basically means users will be forced to use ValueTask everywhere. And such decition btw will break the following code:

Result<int> result = default;

Task<Result<int>> FooAsync() => default;

await result
    .Map(async i => i) // any async lambda
    .Bind(FooAsync) // there are no extensions with ValueTask as left and Task as right

---

@vkhorikov I think it would be nice to move all ValueTask extensions to a separate namespace with the existing well-known names.
Then all existing code will compile as is. And if someone really needs all these extensions, they can enable them by adding an extra using. And deal with async lambdas ambiguity.

The only issue I see about the described approach is that we will loose instance methods (like Result.Try) for ValueTask.

---

I don't really think we should rename methods to XxxValueTask or so, because for me one of the key concepts of CSharpFunctionalExtensions is ubiquitous language. We use the same well-known methods like Map or Bind for Result/Result<T>/Result<T, TError>/ Task<Result>/Task<Result<T>>/Task<Result<T, TError>> and it makes everything exteremely readable. I'm a big fan of Async suffixes for Task-like-returning-type-methods, but even I think that we should not introduce MapAsync and BindAsync, because it will break the readability and the existing simplicity of changing signatures and refactoring.

With XxxValueTask naming all these extensions don't seem to be first class sitizens of the library.

[vkhorikov]

I like the approach with a separate namespace.

The only issue I see about the described approach is that we will loose instance methods (like Result.Try) for ValueTask.

@hankovich Could you elaborate why? EDIT: Ah, because they are part of the Result class, I see.

[vkhorikov]

@MarcelRoozekrans @cportwood If you are fine with the approach with the namespaces, I suggest we go with that.

[hankovich]

@vkhorikov

Could you elaborate why?

My bad, I meant not instance methods, but methods of the Result type itself.

EDIT: Ah, because they are part of the Result class, I see.

Yeah, exactly.

I've checked and it seems Result.Try is the single such method. Probably we can create a new type with static Try method and ValueTask support. Or just skip Try for ValueTasks for now.

[hankovich]

Ah, C# really misses higher-ordered generics in it's type system. It's weird that we have to copy all extensions twice to support ValueTask.

It'll be definitely nice just to have an additional TTask where TTask: <>, awaitable generic parameter in all extensions to be both Task and ValueTask plugable.
One day, maybe.

[vkhorikov]

One day, maybe.

I'm skeptical about this, given that the proposal is 5 years old and there are some other useful and much less complicated proposals with regards to generics that sit on the shelve still as well. E.g: dotnet/csharplang#1239

[MarcelRoozekrans]

@MarcelRoozekrans @cportwood If you are fine with the approach with the namespaces, I suggest we go with that.

I'm fine with he namespace this is also inline wit the proposal from @ProphetLamb #435

[cportwood]

@MarcelRoozekrans I am a bit unclear. Are you saying to use namespace changes to handle conflicts for async methods? If that is the case how would you allow Try and the ValueTask version of Try distinguish themselves? It is highly possible that I may be misunderstanding. If you are differentiating by namespace if you have to use both in the same page which is certainly a likely situation wouldn't you still run into the same issue?

[cportwood]

I do find the way generics are handled in .Net is inadequate for a lot of use cases especially concerning async methods. I find myself having to write a lot of boilerplate to handle this issue.

[cportwood]

@MarcelRoozekrans A generic code generation package to annotate a method or class for async copies would be an extremely useful package to solve this issue in the future. I'm sure it would take a bit to get all use cases but I would use it in a number of projects. That is certainly something I may look at as a project in the future.

[MarcelRoozekrans]

Ok came up with an other solution, we could make the the parameters named differently this way we can distinguish the different overoads..

But leaving them out will let you heve uncompilable code again. But maybe in combination of the different namespacing this could be a solution ?

    public void Test(Func<Task> task)    {    }
    public void Test(Func<ValueTask> valueTask)    {    }

    Test(task: async () => await Task.CompletedTask);
    Test(valueTask: async () => await Task.CompletedTask);

[cportwood]

   public void Test(Action action) { }
   public Task Test(Func<Task> task)    {    }
   public ValueTask Test(Func<ValueTask> valueTask) { }    

The above code works.

   public Maybe<T> Test<T>(Func<T> func) { }
   public Task<Maybe<T>> Test<T>(Func<Task<T>> task)    {    }
   public ValueTask<Maybe<T>> Test<T>(Func<ValueTask<T>> valueTask) { }    

The above code doesn't work as Func<T> and Func<Task<T>> are ambiguous.

As long as there is no need for a generic Func<T> in a given scenario.

[cportwood]

@MarcelRoozekrans Wait, are you saying to use the param name to differentiate?

[MarcelRoozekrans]

@MarcelRoozekrans Wait, are you saying to use the param name to differentiate?

Yeah, just some random idea thought I put it into the mix :)

[cportwood]

@MarcelRoozekrans That may work? Trying to think through if it would cause any issues with existing code. We would need to ensure to add a lot of comments to the code but it would allow for us to retain the Async postfix standard. I don't think Microsoft thought that part through extremely well when they introduced ValueTask.

[cportwood]

@MarcelRoozekrans add the following test.

   
    public void Test(Func<Task> task)    {    }
    public void Test(Func<ValueTask> valueTask)    {    }

    Test(async () => await Task.CompletedTask);
    Test(task: async () => await Task.CompletedTask);
    Test(valueTask: async () => await Task.CompletedTask);
        

[cportwood]

@MarcelRoozekrans If that works than we probably wouldn't need to specify the param name if we postfix Async for async extensions.

[cportwood]

@MarcelRoozekrans I may have made a bad assumption. After some testing I a possibility may be that ValueTasks can only be awaited once then they return undefined. I my assumption was based on an assumption I had made in a past issue that I misremembered.

[vkhorikov]

I don't think different param names is a solution here because it doesn't solve the backward compatibility issue (you'll have to add param names to your existing code to fix the compilation errors).

[MarcelRoozekrans]

@vkhorikov we could out the new valuetask in a separate namespace this way the kib becames backwards compatible again. Only if you want to use the task and valuetask in the same file you need to add the Param name.

[vkhorikov]

@MarcelRoozekrans Ah, I see what you mean. Not sure how valuable this would be, though.

[vkhorikov]

I was trying to introduce of a new CSharpFunctionalExtensions.ValueTasks namespace, but the compiler really gives me a hard time here. It doesn't like extension methods with the same name in different namespaces and can't resolve them unless I specify a fully-qualified name. For example in Map_ValueTask_Right_executes_on_success_returns_new_success:

This code doesn't compile:
Result<K> actual = await result.Map(ValueTask_Func_K)

And I have to use this code instead:
Result<K> actual = await ValueTasks.AsyncResultExtensionsRightOperand.Map(result, ValueTask_Func_K);

@MarcelRoozekrans Would you like to give it a try? If not, I suggest going with renaming all the ValueTask-related methods to XXXValueTask, as in MapValueTask.

@hankovich

[MarcelRoozekrans]

@vkhorikov created a new PR #440 with my proposal.

[vkhorikov]

I've removed Result.Try overloads with value tasks. With this, all issues with backward compatibility should be resolved now. @hankovich could you please try v2.33.3?

[hankovich]

@vkhorikov It works. Well, at least it compiles :)

Thanks for the fix!

[hankovich]

And I have to use this code instead:
Result actual = await ValueTasks.AsyncResultExtensionsRightOperand.Map(result, ValueTask_Func_K);

You can use something like

using Result = CSharpFunctionalExtensions.Result;

_ = Result.Try(() => 5).Map(i => new ValueTask<int>(i));