-
Notifications
You must be signed in to change notification settings - Fork 303
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
feat: Add ValueTask support #433
feat: Add ValueTask support #433
Conversation
MarcelRoozekrans
commented
Aug 15, 2022
- Added ValueTask overloads
- Added TestCases for the new ValueTask overloads
- Refactored tests to be more consistent
- Added missing test cases for current extensions
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. |
Do we wish to use the Default implementation of DefaultAwait used for Tasks?
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. |
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? |
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 |
@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, |
@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? |
- Added ValueTask overloads - Added TestCases for the new ValueTask overloads - Refactored tests to be more consistent - Added missing test cases for current extensions
@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. Regarding the source code generation, it crossed my mind but I’m to unfamiliar with it. |
2c577dd
to
6d031f2
Compare
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.
We should definitely keep it for regular |
@MarcelRoozekrans Looks that |
@vkhorikov Sure don't know what happend here but seems that I lost them. Openend new PR with the refactor if the missing tests. |
Thank you. Will publish the refactorings as a minor version update shortly. |
@vkhorikov @MarcelRoozekrans Please note I've detected two types of them:
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());
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 |
@hankovich Thanks for posting the code, I've added it as failing tests ( @hankovich @MarcelRoozekrans I see 2 possible fixes here:
What do you think? |
@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 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. |
I'd like to avoid breaking changes as much as possible. Another option is to rename |
Renaming is also a good options, only for the right methods then ?? or would you likt to have them all renamed ? |
Let's rename all |
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. |
@vkhorikov Need to find some time though to execute it quite a busy schedule today. |
@MarcelRoozekrans No worries, it can wait a day or two. I can also do this myself over this weekend. |
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
is user-friendly enough, because it basically means users will be forced to use 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 The only issue I see about the described approach is that we will loose instance methods (like I don't really think we should rename methods to With |
I like the approach with a separate namespace.
@hankovich Could you elaborate why? EDIT: Ah, because they are part of the Result class, I see. |
@MarcelRoozekrans @cportwood If you are fine with the approach with the namespaces, I suggest we go with that. |
My bad, I meant not instance methods, but methods of the
Yeah, exactly. I've checked and it seems |
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 It'll be definitely nice just to have an additional |
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 |
I'm fine with he namespace this is also inline wit the proposal from @ProphetLamb #435 |
@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? |
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. |
@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. |
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); |
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. |
@MarcelRoozekrans Wait, are you saying to use the param name to differentiate? |
Yeah, just some random idea thought I put it into the mix :) |
@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. |
@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);
|
@MarcelRoozekrans If that works than we probably wouldn't need to specify the param name if we postfix Async for async extensions. |
@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. |
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). |
@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. |
@MarcelRoozekrans Ah, I see what you mean. Not sure how valuable this would be, though. |
I was trying to introduce of a new This code doesn't compile: And I have to use this code instead: @MarcelRoozekrans Would you like to give it a try? If not, I suggest going with renaming all the ValueTask-related methods to |
@vkhorikov created a new PR #440 with my proposal. |
I've removed |
… backward compatibility
… backward compatibility
… backward compatibility
@vkhorikov It works. Well, at least it compiles :) Thanks for the fix! |
You can use something like using Result = CSharpFunctionalExtensions.Result;
_ = Result.Try(() => 5).Map(i => new ValueTask<int>(i)); |