Use non-void type for `BoundAwaitOperator` in VB statement #67822

jcouv · 2023-04-14T17:18:23Z

I looked for places that use BoundAwaitOperator.Type. The main place that is affected is in AsyncRewriter.AsyncMethodToClassRewriter.Await.vb and it is still correct and covered (Task and Task<T> involve awaiters with GetResult() methods that return void and T respectively).

jcouv · 2023-04-17T21:37:20Z

@dotnet/roslyn-compiler for review. Thanks

AlekseyTs · 2023-04-20T13:31:29Z

Use proper type

Consider to be more specific, "proper" doesn't provide enough clarity, in my opinion.

In reply to: 1516336775

AlekseyTs · 2023-04-20T13:38:37Z

src/Compilers/VisualBasic/Portable/Binding/Binder_Expressions.vb

@@ -4909,7 +4901,7 @@ lElseClause:
            Return New BoundAwaitOperator(node, operand,
                                          awaitableInstancePlaceholder, getAwaiter,
                                          awaiterInstancePlaceholder, isCompleted, getResult,
-                                          resultType, hasErrors)
+                                          type:=getResult.Type, hasErrors)


getResult.Type

It will be good to have a test for a scenario when we are awaiting a Task rather than a Task(Of T). #Closed

Also it would be good to test the effect of this change on an await in a Sub expression lambda.

a scenario when we are awaiting a Task rather than a Task(Of T).

I think TestAwaitExpression (first test in that file) covers that.

an await in a Sub expression lambda.

Can you clarify what you'd like this test to verify? Just the IOperation node for the await in statement in lambda, or something else?

Can you clarify what you'd like this test to verify? Just the IOperation node for the await in statement in lambda, or something else?

Emit, execution, optionally IOperation.

AlekseyTs · 2023-04-20T13:40:18Z

...isualBasic/Portable/Lowering/AsyncRewriter/AsyncRewriter.AsyncMethodToClassRewriter.Await.vb

@@ -2,16 +2,10 @@
 ' The .NET Foundation licenses this file to you under the MIT license.
 ' See the LICENSE file in the project root for more information.

-Imports System.Collections.Generic


It looks like this file doesn't have meaningful changes. Please revert. #Closed

AlekseyTs · 2023-04-20T13:41:04Z

...isualBasic/Portable/Lowering/AsyncRewriter/AsyncRewriter.AsyncMethodToClassRewriter.Await.vb

@@ -99,7 +93,6 @@ Namespace Microsoft.CodeAnalysis.VisualBasic
                                                    Me.F.AssignmentExpression(Me.F.Local(resultTemp, True), rewrittenGetResult),
                                                    clearAwaiterTemp,
                                                    Me.F.Local(resultTemp, False))
-
                Else


The empty line before Else is intentional. #Closed

AlekseyTs · 2023-04-20T13:44:23Z

Done with review pass (commit 1)

jcouv · 2023-04-20T16:06:09Z

@dotnet/roslyn-compiler for another review. Thanks

AlekseyTs · 2023-04-21T12:15:45Z

src/Compilers/VisualBasic/Test/IOperation/IOperation/IOperationTests_IAwaitExpression.vb

+]]>.Value
+
+            Dim expectedDiagnostics = <![CDATA[
+BC30491: Expression does not produce a value.


Is there a benefit in having this error?

I think we actually want to verify that we can emit and run the code.

This scenario hasn't changed. We already produce an error today, so I'm not sure what you're asking.

This scenario hasn't changed. We already produce an error today, so I'm not sure what you're asking.

I am asking why you are testing this error scenario. An action cannot be awaited, and it really doesn't matter what is in the lambda that was used to create the Action. The error doesn't test anything related to the change you are making.

So, unless you have a specific reason to test this error scenario in connection to the change that you are making, consider getting rid of the error and converting the test into a success form (build/execute) similar to the test below.

AlekseyTs · 2023-04-21T12:19:32Z

src/Compilers/VisualBasic/Test/IOperation/IOperation/IOperationTests_IAwaitExpression.vb

+    Public Async Function M() as Task
+        Dim lambda As Action = Async Sub()
+                                 Await M2()'BIND:"Await M2()"
+                               End Sub


This isn't an expression lambda though. And expression lambda would look like

Async Sub() Await M2()

Let's keep this scenario too. #Closed

Let's keep this scenario too.

Here I was referring to the body of the lambda rather than to the error.

AlekseyTs · 2023-04-21T12:21:53Z

src/Compilers/VisualBasic/Test/IOperation/IOperation/IOperationTests_IAwaitExpression.vb

+                                 Await M2()'BIND:"Await M2()"
+                               End Sub
+
+        Await lambda()


Await

Let's drop Await here and Async from the M #Closed

AlekseyTs · 2023-04-21T12:26:31Z

Done with review pass (commit 4)

AlekseyTs

LGTM (commit 5)

jcouv · 2023-04-21T19:21:54Z

@dotnet/roslyn-compiler for second review. Thanks

jcouv · 2023-04-24T23:01:04Z

@dotnet/roslyn-compiler for a second review. Thanks

jcouv force-pushed the await-iop branch from 8ddca49 to ced14ad Compare April 14, 2023 17:18

dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Apr 14, 2023

jcouv self-assigned this Apr 14, 2023

jcouv removed the untriaged Issues and PRs which have not yet been triaged by a lead label Apr 14, 2023

jcouv added this to the 17.7 milestone Apr 14, 2023

jcouv added Language-VB Feature - IOperation IOperation labels Apr 14, 2023

Use proper type for BoundAwaitOperator in VB statement

2221e47

jcouv force-pushed the await-iop branch from ced14ad to 2221e47 Compare April 15, 2023 06:27

jcouv marked this pull request as ready for review April 15, 2023 08:00

jcouv requested a review from a team as a code owner April 15, 2023 08:00

AlekseyTs reviewed Apr 20, 2023

View reviewed changes

jcouv changed the title ~~Use proper type for BoundAwaitOperator in VB statement~~ Use non-void type for BoundAwaitOperator in VB statement Apr 20, 2023

jcouv added 3 commits April 20, 2023 09:16

Address feedback

a14d48c

Consume Async Sub lambda

4536f7f

Tweak test name

7093420

jcouv requested a review from AlekseyTs April 20, 2023 16:38

AlekseyTs reviewed Apr 21, 2023

View reviewed changes

Add running test with expression lambda

74ca5c1

AlekseyTs approved these changes Apr 21, 2023

View reviewed changes

tweak test

5b5839f

333fred approved these changes Apr 25, 2023

View reviewed changes

jcouv merged commit 1546e8e into dotnet:main Apr 25, 2023

jcouv deleted the await-iop branch April 25, 2023 00:16

ghost modified the milestones: 17.7, Next Apr 25, 2023

allisonchou mentioned this pull request Apr 25, 2023

[Automated] PRs inserted in VS build main-33625.187 #67961

Closed

dibarbet modified the milestones: Next, 17.7 P1 Apr 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use non-void type for `BoundAwaitOperator` in VB statement #67822

Use non-void type for `BoundAwaitOperator` in VB statement #67822

jcouv commented Apr 14, 2023 •

edited

Loading

jcouv commented Apr 17, 2023

AlekseyTs commented Apr 20, 2023 •

edited by jcouv

Loading

AlekseyTs Apr 20, 2023 •

edited

Loading

AlekseyTs Apr 20, 2023

jcouv Apr 20, 2023

AlekseyTs Apr 20, 2023

AlekseyTs Apr 20, 2023 •

edited

Loading

AlekseyTs Apr 20, 2023 •

edited

Loading

AlekseyTs commented Apr 20, 2023

jcouv commented Apr 20, 2023

AlekseyTs Apr 21, 2023

AlekseyTs Apr 21, 2023

jcouv Apr 21, 2023

AlekseyTs Apr 21, 2023

AlekseyTs Apr 21, 2023

AlekseyTs Apr 21, 2023 •

edited

Loading

AlekseyTs Apr 21, 2023

AlekseyTs Apr 21, 2023 •

edited

Loading

AlekseyTs commented Apr 21, 2023

AlekseyTs left a comment

jcouv commented Apr 21, 2023

jcouv commented Apr 24, 2023

Use non-void type for BoundAwaitOperator in VB statement #67822

Use non-void type for BoundAwaitOperator in VB statement #67822

Conversation

jcouv commented Apr 14, 2023 • edited Loading

jcouv commented Apr 17, 2023

AlekseyTs commented Apr 20, 2023 • edited by jcouv Loading

AlekseyTs Apr 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Apr 20, 2023 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Apr 20, 2023 • edited Loading

Choose a reason for hiding this comment

AlekseyTs commented Apr 20, 2023

jcouv commented Apr 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Apr 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Apr 21, 2023 • edited Loading

Choose a reason for hiding this comment

AlekseyTs commented Apr 21, 2023

AlekseyTs left a comment

Choose a reason for hiding this comment

jcouv commented Apr 21, 2023

jcouv commented Apr 24, 2023

Use non-void type for `BoundAwaitOperator` in VB statement #67822

Use non-void type for `BoundAwaitOperator` in VB statement #67822

jcouv commented Apr 14, 2023 •

edited

Loading

AlekseyTs commented Apr 20, 2023 •

edited by jcouv

Loading

AlekseyTs Apr 20, 2023 •

edited

Loading

AlekseyTs Apr 20, 2023 •

edited

Loading

AlekseyTs Apr 20, 2023 •

edited

Loading

AlekseyTs Apr 21, 2023 •

edited

Loading

AlekseyTs Apr 21, 2023 •

edited

Loading