Fix IEventAssignmentExpression #20960

genlu · 2017-07-18T22:30:48Z

Replaced the event symbol in IEventAssignmentExpression with IEventReferenceExpression

    /// <summary>
    /// Represents a binding of an event.
    /// </summary>
    /// <remarks>
    /// This interface is reserved for implementation by its associated APIs. We reserve the right to
    /// change it in the future.
    /// </remarks>
    public interface IEventAssignmentExpression : IOperation
    {
        /// <summary>
        /// Reference to the event being bound.
        /// </summary>
        IEventReferenceExpression EventReference { get; }
 
        /// <summary>
        /// Handler supplied for the event.
        /// </summary>
        IOperation HandlerValue { get; }
 
        /// <summary>
        /// True for adding a binding, false for removing one.
        /// </summary>
        bool Adds { get; }
    }

Note that for C#, the bound node doesn't contain the syntax for the event reference, even though it was provided during binding. Because of this, the syntax of entire event assignment expression is used for the syntax event reference operation, just so I can get the interface change done first. To address this issue, we probably need to at least change BoundEventAssignmentOperator.

@dotnet/analyzer-ioperation @cston @jcouv

cston · 2017-07-19T05:34:48Z

src/Compilers/CSharp/Portable/Operations/CSharpOperationFactory.cs

+            {
+                IEventSymbol @event = boundEventAssignmentOperator.Event;
+                Lazy<IOperation> instance = new Lazy<IOperation>(() => Create(boundEventAssignmentOperator.Event.IsStatic ? null : boundEventAssignmentOperator.ReceiverOpt));
+                ISymbol member = boundEventAssignmentOperator.Event;


member is not used.

cston · 2017-07-19T05:35:50Z

src/Compilers/CSharp/Portable/Operations/CSharpOperationFactory.cs

+                Lazy<IOperation> instance = new Lazy<IOperation>(() => Create(boundEventAssignmentOperator.Event.IsStatic ? null : boundEventAssignmentOperator.ReceiverOpt));
+                ISymbol member = boundEventAssignmentOperator.Event;
+                // BoundEventAssignmentOperator doesn't hold on to all the data from the provided BoundEventAccess during binding.
+                // Based on the implementation of those two bound node types, the following data can be retieved w/o changing BoundEventAssignmentOperator:


retrieved?

cston · 2017-07-19T05:49:27Z

src/Compilers/VisualBasic/Portable/Operations/VisualBasicOperationFactory_Methods.vb

            Return New EventAssignmentExpression(
-                [event], instance, Create(statement.Handler), adds:=False, syntax:=statement.Syntax, type:=Nothing, constantValue:=Nothing)
+                eventReference, Create(statement.Handler), adds:=False, syntax:=statement.Syntax, type:=Nothing, constantValue:=Nothing)
        End Function


These two methods look identical. Can they be replaced by a single GetAddOrRemoveHandlerStatementExpression?

cston · 2017-07-19T05:52:14Z

...Compilers/VisualBasic/Test/Semantic/IOperation/IOperationTests_IEventAssignmentExpression.vb

+
+    Sub Add(receiver As TestClass)
+        AddHandler TestEvent, AddressOf M'BIND:"AddHandler TestEvent, AddressOf M"
+    End Sub


receiver is not used in these tests. #Resolved

mavasani · 2017-07-19T13:52:32Z

Because of this, the syntax of entire event assignment expression is used for the syntax event reference operation, just so I can get the interface change done first. To address this issue, we probably need to at least change BoundEventAssignmentOperator

I think we should do this change in this PR. Is it non-trivial to change the bound node?

mavasani · 2017-07-19T13:55:56Z

src/Compilers/CSharp/Portable/Operations/CSharpOperationFactory.cs

+                //  1. The type of BoundEventAccess is the type of the event symbol.
+                //  2. the constant value of BoundEventAccess is always null.
+                // However, we can't reliably get the syntax node for the BoundEventAccess w/o changing BoundEventAssignmentOperator, so the syntax of entire
+                // BoundEventAssignmentOperator is used instead.


Can we cast the syntax to AssignmentOperatorSyntax and use the Left/Target field of this syntax node? This way the correct syntax will be used, and we can optimize the implementation as per your comments later. Also, please file a bug for this TODO.

If the syntax from left can be guaranteed to be correct, then I guess we can do w/o changing bound node after all.

A closer look suggests that the syntax can only be AssignmentOperatorSyntax for a BoundEventAssignmentOperator. There's no need to change bound node then.

mavasani · 2017-07-19T13:57:17Z

src/Compilers/CSharp/Portable/Operations/CSharpOperationFactory.cs

+            Lazy<IEventReferenceExpression> eventReference = new Lazy<IEventReferenceExpression>(() =>
+            {
+                IEventSymbol @event = boundEventAssignmentOperator.Event;
+                Lazy<IOperation> instance = new Lazy<IOperation>(() => Create(boundEventAssignmentOperator.Event.IsStatic ? null : boundEventAssignmentOperator.ReceiverOpt));


Do we need the conditional expression here, i.e. is there a case where event is static but ReceiverOpt is non-null?

Good point, will fix.

Actually, it seems the conditional is necessary, since the receiver will not be null for static event reference:
http://source.roslyn.io/#Microsoft.CodeAnalysis.CSharp/Binder/Binder_Expressions.cs,5809

genlu · 2017-07-19T18:13:11Z

@mavasani I think there's a few options:

The simplest way to fix the syntax of newly added EventReference is to add a field in BoundEventAssignmentOperator just for the syntax of BoundEventAccess.
similar to the one above, we can capture the entire BoundEventAccess, but keep binding/lowering process unchanged.
This is the approach I prefer, we can "un-lower" the BoundEventAssignmentOperator so it contains BoundEventAccess when exposed to us, and modify rewriter to do the lowering logic currently resides in binder. But I'm not sure if this would affect other things, like perf, etc.

genlu · 2017-07-24T23:05:06Z

ping... @mavasani @cston @jcouv

cston · 2017-07-24T23:22:19Z

src/Compilers/VisualBasic/Portable/Operations/VisualBasicOperationFactory_Methods.vb

-            Dim instance = If([event] Is Nothing OrElse [event].IsStatic, Nothing, If(eventAccess IsNot Nothing, Create(eventAccess.ReceiverOpt), Nothing))
-
+            Dim eventReference = If(eventAccess Is Nothing, Nothing, CreateBoundEventAccessOperation(eventAccess))
+            Dim adds = TypeOf statement Is BoundAddHandlerStatement


Dim adds = statement.Kind = BoundKind.AddHandlerStatement

cston · 2017-07-24T23:25:25Z

src/Test/Utilities/Portable/Diagnostics/OperationTestAnalyzer.cs

                     {
-                         if (eventAssignment.EventInstance == null && eventAssignment.HasErrors(operationContext.Compilation, operationContext.CancellationToken))
+                         if (eventAssignment.EventReference?.Instance == null && eventAssignment.HasErrors(operationContext.Compilation, operationContext.CancellationToken))
                         {
                             // report inside after checking for null to make sure it does't crash.


Typo: doesn't

cston · 2017-07-24T23:29:16Z

src/Test/Utilities/Portable/Diagnostics/OperationTestAnalyzer.cs

                     {
-                         if (eventAssignment.EventInstance == null && eventAssignment.HasErrors(operationContext.Compilation, operationContext.CancellationToken))
+                         if (eventAssignment.EventReference?.Instance == null && eventAssignment.HasErrors(operationContext.Compilation, operationContext.CancellationToken))


When are both EventReference?.Event == null and EventReference?.Instance == null? Is that only when EventReference == null? If so, consider checking that instead.

genlu · 2017-07-28T19:57:04Z

src/Compilers/CSharp/Test/Semantic/IOperation/IOperationTests_IForLoopStatement.cs

@@ -1247,24 +1247,24 @@ public void Next()
 string expectedOperationTree = @"
 IForLoopStatement (LoopKind.For) (OperationKind.LoopStatement) (Syntax: 'for (d.Init ... }')
  Condition: IUnaryOperatorExpression (UnaryOperationKind.DynamicTrue) (OperationKind.UnaryOperatorExpression, Type: System.Boolean) (Syntax: 'd.Done')
-      Operand: IOperation:  (OperationKind.None) (Syntax: 'd.Done')


@jinujoseph This seems to be caused by two PRs merged around the same time w/o running tests on up-to-date code?

genlu · 2017-07-28T19:58:09Z

Ping @mavasani @dotnet/roslyn-compiler

cston · 2017-07-28T20:10:02Z

src/Compilers/CSharp/Portable/Operations/CSharpOperationFactory.cs

+                //  2. the constant value of BoundEventAccess is always null.
+                //  3. the syntax of the boundEventAssignmentOperator is always AssignmentExpressionSyntax, so the syntax for the event reference would be the LHS of the assignment.
+                IEventSymbol @event = boundEventAssignmentOperator.Event;
+                Lazy<IOperation> instance = new Lazy<IOperation>(() => Create(boundEventAssignmentOperator.Event.IsStatic ? null : boundEventAssignmentOperator.ReceiverOpt));


We should include the receiver in the static case as well, so the consumer can walk the entire expression even in an error scenario.

Good point. I have made the change for both C# and VB (and I noticed in test that adding handler to static event through an instance is not an error in VB.

As I just mentioned offline, making this change causes us to return a None operation as the instance derived from a BoundTypeExpression as BoundEventAssignmentOperator.ReceiverOpt. If we want to expose as much information as possible in erroneous case, we will need to accurately check for error when creating the EventReferenceOperation plus modify the interface definition to make it clear that instance could be non null in this case. We probably discuss this during our design meeting.

Meanwhile, I 'm gonna revert this change (but keep those tests) to make the behavior consistent with other related operations, e.g.:
https://github.com/dotnet/roslyn/blob/features/ioperation/src/Compilers/CSharp/Portable/Operations/CSharpOperationFactory.cs#L306

Is this OK?

Yes, we should have consistent behavior for related operations. It sounds reasonable to revert the change for now while we discuss the overall approach.

By the way, there's already an issue tracking the BoundTypeExpression issue:
#8909

cston · 2017-07-31T21:40:27Z

src/Compilers/CSharp/Test/Semantic/IOperation/IOperationTests_IEventAssignmentExpression.cs

+        }
+
+        [Fact]
+        public void AddEventHandler_AssignToStaticEventOnInstance()


For completeness, please test a reference to a instance event without a receiver, and test the same two cases in VB.

…ore tests

cston · 2017-08-01T21:16:22Z

LGTM

…ntAssignment

mavasani · 2017-08-01T22:03:48Z

src/Compilers/CSharp/Portable/Operations/CSharpOperationFactory.cs

-            IEventSymbol @event = boundEventAssignmentOperator.Event;
-            Lazy<IOperation> eventInstance = new Lazy<IOperation>(() => Create(boundEventAssignmentOperator.Event.IsStatic ? null : boundEventAssignmentOperator.ReceiverOpt));
+            SyntaxNode syntax = boundEventAssignmentOperator.Syntax;
+            Lazy<IEventReferenceExpression> eventReference = new Lazy<IEventReferenceExpression>(() =>


Can you refactor this into a separate method CreateBoundEventReferenceExpressionOperation?

genlu · 2017-08-02T00:42:03Z

Can I get another review from @dotnet/roslyn-compiler?

jcouv · 2017-08-02T21:16:01Z

Looking now.

jcouv · 2017-08-02T21:23:49Z

src/Compilers/CSharp/Test/Semantic/IOperation/IOperationTests_IEventAssignmentExpression.cs

+
+namespace Microsoft.CodeAnalysis.CSharp.UnitTests
+{
+    public partial class IOperationTests : SemanticModelTestBase


There should be a trait to tag IOperation tests, if there isn't one already.
Something like [CompilerTrait(CompilerFeature.Tuples)]

@dotnet/analyzer-ioperation Do we have this already? I don't see it in existing tests. I can create one in a separate PR if not.

No we don't ...there is a issue tracking this #20652
We can handle that separately

jcouv

LGTM
If you don't have a trait for IOperations tests. Please add one in future PR. Thanks

…ntAssignment

genlu · 2017-08-03T16:42:10Z

@dotnet/roslyn-infrastructure Linux builds are failing with following error, any ideas?
https://ci.dot.net/job/dotnet_roslyn/job/features_ioperation/job/ubuntu_14_debug_prtest/385/

I see the --nocache was removed from groovy script a couple days ago, why is it still being add here?

9:25:16 + ./cibuild.sh --nocache --debug
09:25:16 Runs our integration suite on Linux
09:25:16 usage: cibuild.sh [options]
09:25:16 
09:25:16 Options
09:25:16   --debug               Build Debug (default)
09:25:16   --release             Build Release
09:25:16   --cleanrun            Clean the project before building
09:25:16   --skiptest            Do not run tests
09:25:16   --skipcommitprinting  Do not print commit information
09:25:17 Build step 'Execute shell' marked build as failure

jasonmalinowski · 2017-08-03T20:37:27Z

@brettfo Is this something we just need to take another merge for?

…ntAssignment

dnfclas added the cla-already-signed label Jul 18, 2017

cston reviewed Jul 19, 2017

View reviewed changes

mavasani reviewed Jul 19, 2017

View reviewed changes

genlu force-pushed the FixEventAssignment branch from 856028a to 4a418b8 Compare July 21, 2017 19:06

cston reviewed Jul 24, 2017

View reviewed changes

Pilchie added the Area-Analyzers label Jul 25, 2017

genlu added 5 commits July 28, 2017 12:39

Fix IEventAssignmentExpression

5aca5d7

Fix syntax node for the EventReference in IEventAssignmentOperation

6a1c967

Fix tests after rebase

a9117b7

Address review comments

d1fe56b

Fix a failed test

0e0e4ef

genlu force-pushed the FixEventAssignment branch from 04d9246 to 0e0e4ef Compare July 28, 2017 19:55

genlu commented Jul 28, 2017

View reviewed changes

cston reviewed Jul 28, 2017

View reviewed changes

Expose receiver for static event assignemt

d500d52

cston reviewed Jul 31, 2017

View reviewed changes

heejaechang mentioned this pull request Aug 1, 2017

added parent property to IOperation #20964

Merged

Reverted the change for receiver in static event assignment and add m…

f8bf025

…ore tests

Merge remote-tracking branch 'dotnet/features/ioperation' into FixEve…

601987b

…ntAssignment

mavasani approved these changes Aug 1, 2017

View reviewed changes

extract method for creating event reference

ab14b77

jcouv reviewed Aug 2, 2017

View reviewed changes

jcouv approved these changes Aug 2, 2017

View reviewed changes

genlu added 2 commits August 2, 2017 16:28

Merge remote-tracking branch 'dotnet/features/ioperation' into FixEve…

3fbbebd

…ntAssignment

Resolve merge conflict

0f17f9e

genlu force-pushed the FixEventAssignment branch from 7fa7b67 to 0f17f9e Compare August 3, 2017 01:10

genlu added 4 commits August 8, 2017 10:56

Merge remote-tracking branch 'dotnet/features/ioperation' into FixEve…

4485c1d

…ntAssignment

Add compiler trait for IOperation tests

7c0283c

Merge remote-tracking branch 'dotnet/features/ioperation' into FixEve…

f995261

…ntAssignment

Fix test failure

08016e4

genlu merged commit 77260eb into dotnet:features/ioperation Aug 8, 2017

genlu deleted the FixEventAssignment branch August 8, 2017 23:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix IEventAssignmentExpression #20960

Fix IEventAssignmentExpression #20960

genlu commented Jul 18, 2017

cston Jul 19, 2017

cston Jul 19, 2017

cston Jul 19, 2017

cston Jul 19, 2017 •

edited by jcouv

Loading

mavasani commented Jul 19, 2017

mavasani Jul 19, 2017

genlu Jul 19, 2017

genlu Jul 21, 2017

mavasani Jul 19, 2017

genlu Jul 19, 2017

genlu Jul 21, 2017

genlu commented Jul 19, 2017

genlu commented Jul 24, 2017

cston Jul 24, 2017

cston Jul 24, 2017

cston Jul 24, 2017

genlu Jul 28, 2017

genlu commented Jul 28, 2017

cston Jul 28, 2017

genlu Jul 31, 2017

genlu Aug 1, 2017

cston Aug 1, 2017 •

edited

Loading

genlu Aug 1, 2017 •

edited

Loading

cston Jul 31, 2017

cston commented Aug 1, 2017

mavasani Aug 1, 2017

genlu commented Aug 2, 2017

jcouv commented Aug 2, 2017

jcouv Aug 2, 2017

genlu Aug 2, 2017

jinujoseph Aug 2, 2017 •

edited

Loading

jcouv left a comment

genlu commented Aug 3, 2017 •

edited

Loading

jasonmalinowski commented Aug 3, 2017

Fix IEventAssignmentExpression #20960

Fix IEventAssignmentExpression #20960

Conversation

genlu commented Jul 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston Jul 19, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

mavasani commented Jul 19, 2017

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

Choose a reason for hiding this comment

genlu commented Jul 19, 2017

genlu commented Jul 24, 2017

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

genlu commented Jul 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston Aug 1, 2017 • edited Loading

Choose a reason for hiding this comment

genlu Aug 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston commented Aug 1, 2017

Choose a reason for hiding this comment

genlu commented Aug 2, 2017

jcouv commented Aug 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jinujoseph Aug 2, 2017 • edited Loading

Choose a reason for hiding this comment

jcouv left a comment

Choose a reason for hiding this comment

genlu commented Aug 3, 2017 • edited Loading

jasonmalinowski commented Aug 3, 2017

cston Jul 19, 2017 •

edited by jcouv

Loading

cston Aug 1, 2017 •

edited

Loading

genlu Aug 1, 2017 •

edited

Loading

jinujoseph Aug 2, 2017 •

edited

Loading

genlu commented Aug 3, 2017 •

edited

Loading