Skip to content
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.

Sign up for GitHub

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

Fix IEventAssignmentExpression #20960

Merged
merged 15 commits into from
Aug 8, 2017
Merged

Fix IEventAssignmentExpression #20960

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
5aca5d7
Fix IEventAssignmentExpression
genlu Jul 18, 2017
6a1c967
Fix syntax node for the EventReference in IEventAssignmentOperation
genlu Jul 21, 2017
a9117b7
Fix tests after rebase
genlu Jul 21, 2017
d1fe56b
Address review comments
genlu Jul 28, 2017
0e0e4ef
Fix a failed test
genlu Jul 28, 2017
d500d52
Expose receiver for static event assignemt
genlu Jul 31, 2017
f8bf025
Reverted the change for receiver in static event assignment and add m…
genlu Aug 1, 2017
601987b
Merge remote-tracking branch 'dotnet/features/ioperation' into FixEve…
genlu Aug 1, 2017
ab14b77
extract method for creating event reference
genlu Aug 2, 2017
3fbbebd
Merge remote-tracking branch 'dotnet/features/ioperation' into FixEve…
genlu Aug 2, 2017
0f17f9e
Resolve merge conflict
genlu Aug 3, 2017
4485c1d
Merge remote-tracking branch 'dotnet/features/ioperation' into FixEve…
genlu Aug 8, 2017
7c0283c
Add compiler trait for IOperation tests
genlu Aug 8, 2017
f995261
Merge remote-tracking branch 'dotnet/features/ioperation' into FixEve…
genlu Aug 8, 2017
08016e4
Fix test failure
genlu Aug 8, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 17 additions & 4 deletions src/Compilers/CSharp/Portable/Operations/CSharpOperationFactory.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
using System.Linq;
using Microsoft.CodeAnalysis.CSharp;
using Microsoft.CodeAnalysis.CSharp.Symbols;
using Microsoft.CodeAnalysis.CSharp.Syntax;
using Microsoft.CodeAnalysis.PooledObjects;
using Roslyn.Utilities;

Expand Down Expand Up @@ -355,14 +356,26 @@ private IEventReferenceExpression CreateBoundEventAccessOperation(BoundEventAcce

private IEventAssignmentExpression CreateBoundEventAssignmentOperatorOperation(BoundEventAssignmentOperator boundEventAssignmentOperator)
{
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>(() =>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you refactor this into a separate method CreateBoundEventReferenceExpressionOperation?

{
// BoundEventAssignmentOperator doesn't hold on to BoundEventAccess provided during binding.
// Based on the implementation of those two bound node types, the following data can be retrieved w/o changing BoundEventAssignmentOperator:
// 1. the type of BoundEventAccess is the type of the event symbol.
// 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));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, will fix.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

@cston cston Aug 1, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@genlu genlu Aug 1, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

SyntaxNode eventAccessSyntax = ((AssignmentExpressionSyntax)syntax).Left;

return new LazyEventReferenceExpression(@event, instance, @event, eventAccessSyntax, @event.Type, ConvertToOptional(null));
});

Lazy<IOperation> handlerValue = new Lazy<IOperation>(() => Create(boundEventAssignmentOperator.Argument));
bool adds = boundEventAssignmentOperator.IsAddition;
SyntaxNode syntax = boundEventAssignmentOperator.Syntax;
ITypeSymbol type = boundEventAssignmentOperator.Type;
Optional<object> constantValue = ConvertToOptional(boundEventAssignmentOperator.ConstantValue);
return new LazyEventAssignmentExpression(@event, eventInstance, handlerValue, adds, syntax, type, constantValue);
return new LazyEventAssignmentExpression(eventReference, handlerValue, adds, syntax, type, constantValue);
}

private IParameterReferenceExpression CreateBoundParameterOperation(BoundParameter boundParameter)
Expand Down
19 changes: 8 additions & 11 deletions src/Compilers/CSharp/Test/Semantic/Diagnostics/OperationAnalyzerTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1125,7 +1125,7 @@ public override void M1()
);
}

[Fact(Skip = "https://github.com/dotnet/roslyn/issues/18839")]
[Fact]
public void EventAndMethodReferencesCSharp()
{
const string source = @"
Expand Down Expand Up @@ -1154,21 +1154,18 @@ private void Mumbler(object sender, System.EventArgs args)
CreateCompilationWithMscorlib45(source, parseOptions: TestOptions.RegularWithIOperationFeature)
.VerifyDiagnostics()
.VerifyAnalyzerDiagnostics(new DiagnosticAnalyzer[] { new MemberReferenceAnalyzer() }, null, null, false,
// Bug: we are missing diagnostics of "MethodBindingDescriptor" here. https://github.com/dotnet/roslyn/issues/20095
Diagnostic(MemberReferenceAnalyzer.HandlerAddedDescriptor.Id, "Mumble += new MumbleEventHandler(Mumbler)").WithLocation(10, 9),
// Bug: Missing a EventReferenceExpression here https://github.com/dotnet/roslyn/issues/8346
Diagnostic(MemberReferenceAnalyzer.MethodBindingDescriptor.Id, "Mumbler").WithLocation(10, 42),
Diagnostic(MemberReferenceAnalyzer.EventReferenceDescriptor.Id, "Mumble").WithLocation(10, 9),
Diagnostic(MemberReferenceAnalyzer.HandlerAddedDescriptor.Id, "Mumble += (s, a) => {}").WithLocation(11, 9),
// Bug: Missing a EventReferenceExpression here https://github.com/dotnet/roslyn/issues/8346
Diagnostic(MemberReferenceAnalyzer.EventReferenceDescriptor.Id, "Mumble").WithLocation(11, 9),
Diagnostic(MemberReferenceAnalyzer.HandlerAddedDescriptor.Id, "Mumble += new MumbleEventHandler((s, a) => {})").WithLocation(12, 9),
// Bug: Missing a EventReferenceExpression here https://github.com/dotnet/roslyn/issues/8346
Diagnostic(MemberReferenceAnalyzer.MethodBindingDescriptor.Id, "(s, a) => {}").WithLocation(12, 42), // Bug: this is not a method binding https://github.com/dotnet/roslyn/issues/8347
Diagnostic(MemberReferenceAnalyzer.EventReferenceDescriptor.Id, "Mumble").WithLocation(12, 9),
Diagnostic(MemberReferenceAnalyzer.EventReferenceDescriptor.Id, "Mumble").WithLocation(13, 9),
Diagnostic(MemberReferenceAnalyzer.EventReferenceDescriptor.Id, "Mumble").WithLocation(14, 20),
Diagnostic(MemberReferenceAnalyzer.MethodBindingDescriptor.Id, "Mumbler").WithLocation(15, 32),
Diagnostic(MemberReferenceAnalyzer.HandlerRemovedDescriptor.Id, "Mumble -= new MumbleEventHandler(Mumbler)").WithLocation(17, 9),
// Bug: Missing a EventReferenceExpression here https://github.com/dotnet/roslyn/issues/8346
Diagnostic(MemberReferenceAnalyzer.MethodBindingDescriptor.Id, "Mumbler").WithLocation(17, 42)
);
Diagnostic(MemberReferenceAnalyzer.EventReferenceDescriptor.Id, "Mumble").WithLocation(17, 9));
}

[Fact]
Expand Down Expand Up @@ -1485,11 +1482,11 @@ void Foo()
CreateCompilationWithMscorlib45(source, parseOptions: TestOptions.RegularWithIOperationFeature)
.VerifyDiagnostics(Diagnostic(ErrorCode.WRN_UnreferencedEvent, "E").WithArguments("D.E").WithLocation(6, 32))
.VerifyAnalyzerDiagnostics(new DiagnosticAnalyzer[] { new StaticMemberTestAnalyzer() }, null, null, false,
Diagnostic(StaticMemberTestAnalyzer.StaticMemberDescriptor.Id, "C.E += D.Method").WithLocation(23, 9),
Diagnostic(StaticMemberTestAnalyzer.StaticMemberDescriptor.Id, "C.E").WithLocation(23, 9),
Diagnostic(StaticMemberTestAnalyzer.StaticMemberDescriptor.Id, "D.Method").WithLocation(23, 16),
Diagnostic(StaticMemberTestAnalyzer.StaticMemberDescriptor.Id, "C.E").WithLocation(24, 9),
Diagnostic(StaticMemberTestAnalyzer.StaticMemberDescriptor.Id, "C.Bar()").WithLocation(25, 9),
Diagnostic(StaticMemberTestAnalyzer.StaticMemberDescriptor.Id, "D.E += () => { }").WithLocation(27, 9),
Diagnostic(StaticMemberTestAnalyzer.StaticMemberDescriptor.Id, "D.E").WithLocation(27, 9),
Diagnostic(StaticMemberTestAnalyzer.StaticMemberDescriptor.Id, "D.Field").WithLocation(28, 9),
Diagnostic(StaticMemberTestAnalyzer.StaticMemberDescriptor.Id, "D.Property").WithLocation(29, 17),
Diagnostic(StaticMemberTestAnalyzer.StaticMemberDescriptor.Id, "D.Method()").WithLocation(30, 9)
Expand Down
Loading