-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Merge Features/async main #19436
Merge Features/async main #19436
Conversation
* enable main methods to be asynchronous * add cref to documentation * fix doc comment syntax * remove message from resx * move comments * add async int test * add more test * re-add errors for pre-71 compilers * add tests for previous versions of csharp * add feature detection * Disallow ref returning Main method The Main method cannot have a ref return. closes #17923 * add ref tests, add prototype comment * change prototype comment
…eatures/async-main
* Add markdown feature documents * Update testing plan
…eatures/async-main
* attempt to get main entrypoints working * async main codegen * remove garbage file * fix vb tests * add some comments * moduleBeingBuilt can sometimes be null apparently * fix style nits * more whitespace fixes * address feedback * automated style fixer * move most work out into the constructor * protect against null in error condition * add comment * address cston review * autoformat * use cast * check module being built * add another object cast * address review * check for old cases where async methods called Main also exist * more suggestions * autoformat * mostly cleanup * use shorter Count and Single * remove superfluous diagnostics add * check exit code / revert to old entrypoint finding code * remove language version check in entrypoint compiler * fix debug.assert * switch to assert equal * codereview * add more tests; change conditions for test execution * fix up emit methods * narrow down error location * ammend GetAwaitableExpressionInfo to return a bound call to GetAwaiter().GetResult() * more comments * find entrypoint methods uses the async binder * autoformat * use diagnostics from async binder * relocate tests * fix suggestions for review 8346 * add back no-async tests with better location * only allow named types * remove empty file; remove null checks from helper * forgot to add csproj file * emit into warning diagnostics instead of regular diagnostics * autoformat * remove unused file from merge * use diagnostic locations * move some methods around, split some tests up * fix non-returning async tests, remove bool return from check availability * add params test * style and expected output fix * switch back to null because default emits warnings * add partial method tests * fix locations for unix * better partial syntax tests * use expected length of 0 * use default length of 0 for expected output * check implementation symbol
* simplify partial method return type syntax finder tests * Bug fixes and API use changes * address first review * better comment and compiler trait on class * fix tests, pull entrypoint check out into local function * unify diagnostics for intvoid or task mains * add more tests, change error text * address review * finish tests * be explicit on language version in tests * add another test, fix typo, add diagnostics even when they aren't likely to exist * rewrite error->warning code, add more cases to tests * change test language versions * formatting on comments and adding WithLocation to tests * fix formatting for some tests * fix some formatting and added withlocation * fix withlocation
* remove remaining prototype comments, add extension method tests * fix old test, address review feedback * ReturnType implemented on each subclass * added more tests on non-async task returning mains * add obsolete tests * autoformat * add better tests for successful task overrides
@dotnet-bot test windows_release_vs-integration_prtest please |
@dotnet/roslyn-infrastructure VSI failure: https://ci.dot.net/job/dotnet_roslyn/job/master/job/windows_release_vs-integration_prtest/4010/ |
I still need signoffs for this merge |
Ping @MeiChin-Tsai for approval |
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.
LGTM, pending test results for required tests.
* `static async Task Main(...)` -> `static void $EntrypointMain(...)` | ||
* `static async Task<int> Main(...)` -> `static int $EntrypointMain(...)` | ||
* The parameters between the user-defined main and the generated main should match exactly. | ||
* The body of the generated main should be `return Main(args...).GetAwaiter().GetResult();` |
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.
❓ Was there consideration of cancellation for this feature? For example, see the following:
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'm not necessarily expecting the synchronization context to be set up, but hooking up cancellation and passing that to the asynchronous method has proven very valuable for us.
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.
The primary goal was basically to allow using await
in the Main.
Cancellation was discussed, but unlike async/await
, cancellation is not something that is directly supported by the language right now.
If there is enough desire to make cancellation a concept in the language, a feature like this might not be a good place to start.
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.
Changing the parameter list for Main was never considered, no.
|
||
## Technical Details | ||
|
||
* The compiler must recognize `Task` and `Task<int>` as valid entrypoint return types in addition to `void` and `int`. |
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 about ValueTask<int>
? (I'm expecting the answer to be "It's left for future" but it would still be good to be clear on it.)
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.
"It's left for future".
Some design work was done specifically to not prevent supporting Task-likes in the future if needed.
For example - the return type is inferred from the shape of GetAwaiter and not from the T
in the Task<T>
.
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.
@sharwell What Vlad said. Since GetResult on tasklikes isn't required to block, letting async main use them is something that we'll have to think more about.
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 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.
Btw, @TyOverby, you may want to record links to relevant LDM notes to your feature doc, once they get posted.
Please fill in the template and answer all the questions from Sam. approved. |
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.
LGTM
Customer scenario
The customer wants to write applications that are almost entirely asynchronous, but could not
make async main methods.
Workarounds, if any
Write your own shim.
Risk
Medium
Performance impact
Low impact because additional code is only run on a single method (the selected async main method)
Is this a regression from a previous update?
No