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.

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

Add [RequireStaticDelegate] annotations #6174

Merged
merged 4 commits into from
Feb 17, 2024

Conversation

Susko3
Copy link
Member

@Susko3 Susko3 commented Feb 4, 2024

Adds [RequireStaticDelegate] annotation to places where there is an Action<T> + T data overload and the intent is to avoid capture.

Adding this to InvokeOnDisposal is a no-brainer, but I'm not entirely sure about adding to Scheduler and other places. So this is an RFC of sorts.

There are valid use cases of non-static delegates, so I'm thinking it would be useful to apply the attribute, but change the inspection severity to a hint -- let the programmer know that context is captured.
For cases where we want to raise an error for capturing context (eg. in InvokeOnDisposal), [RequireStaticDelegate(IsError = true)] can be used. This will force an error regardless of the configured severity. Unless the severity is configured as DO_NOT_SHOW, in which case the error is silenced.

image

Example of setting LambdaShouldNotCaptureContext severity to hint. If this is considered too noisy, then we don't apply the attribute in Scheduler, etc.

Inspections that can be configured

LambdaExpressionMustBeStatic

Active when the lambda doesn't capture context, but isn't static.

image

I think this is best set to hint (defaults to suggestion).

LambdaShouldNotCaptureContext

Active when the lambda captures context (eg. variables, this):

image

Including when a method is passed and this is implicitly captured:

image

@smoogipoo
Copy link
Contributor

smoogipoo commented Feb 4, 2024

I like this and I started going down this path with the recent osu-side issues.

I think we should use IsError liberally, including the existing cases in this PR.

@bdach
Copy link
Collaborator

bdach commented Feb 5, 2024

I'm not entirely sure about adding to Scheduler and other places

Shouldn't be applied to scheduler at any level higher than a hint at most I think. Using Scheduler.AddOnce() for instance like that has semantics implications.

Also I assume this is going to require game-side changes? Where is that diff?

@smoogipoo smoogipoo enabled auto-merge February 17, 2024 09:27
@smoogipoo smoogipoo merged commit 5d98a51 into ppy:master Feb 17, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants