-
Notifications
You must be signed in to change notification settings - Fork 2
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
[AK1005] Must close sender in async lambda expression argument #87
[AK1005] Must close sender in async lambda expression argument #87
Conversation
…-in-ReceiveAsync # Conflicts: # src/Akka.Analyzers/Context/Core/Actor/BaseActorContext.cs # src/Akka.Analyzers/Utility/RuleDescriptors.cs
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.
One very minor change suggested, a couple of questions
id: "AK1005", | ||
title: "Must close over `Sender` or `Self`", | ||
category: AnalysisCategory.ActorDesign, | ||
defaultSeverity: DiagnosticSeverity.Error, |
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'd reduce this to a warning for now, in case this rule doesn't work as expected in the wild. We can always raise the severity later.
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.
Sounds good, I'll fix it now.
ReceiveAsync<string>(async str => { | ||
var sender = Sender; | ||
await api.AsyncLambda(async () => { | ||
sender.Tell(new Message(str)); |
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.
Just so I'm clear, it's the async
lambda running inside this local function that makes accessing Context.Sender
an issue, not the await
-ing inside the ReceiveAsync
itself, right?
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.
And I know this is a passing case with a closure et al, but I just wanted to be clear since I'm working from the top down of the diff.
ReceiveAsync<string>(async str => | ||
{ | ||
await api.AsyncLambda(async () => { | ||
Context.Sender.Tell(new Message(str)); |
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.
Got it, yep I can see why this would be a problem.
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.
Analyzer test cases look good and make sense.
var akkaActor = akkaContext.AkkaCore.Actor; | ||
|
||
// The class declaration must implements or inherits `Akka.Actor.ActorBase` | ||
if (!classDeclaration.IsDerivedOrImplements(semanticModel, akkaActor.ActorBaseType!)) |
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
} | ||
} | ||
|
||
private sealed class BlockVisitor: CSharpSyntaxVisitor<List<Diagnostic>> |
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 is this visitor for?
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 are 2 visitors:
- one is for collecting all lambda expression that are being passed in as an argument to a method invocation,
- the other is a visitor that enumerates the body of said lambda to look for the use of
Sender
,Self
,Context.Sender
, andContext.Self
.
The two needs to be separated because they need to be executed in step
public INamedTypeSymbol? TaskType { get; } | ||
} | ||
|
||
public sealed class SystemThreadingTasksContext: ISystemThreadingTasksContext |
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
return false; | ||
|
||
// Property is equal to `ActorBase.Sender` or `IActorContext.Sender` | ||
return ReferenceEquals(akkaContext.Actor.ActorBase.Sender, propertySymbol) || |
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
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
Fixes #71
Changes