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

Fix S3168 FN: Async implementation of non-async interfaces #9696

Closed
qhris opened this issue Nov 22, 2024 · 4 comments
Closed

Fix S3168 FN: Async implementation of non-async interfaces #9696

qhris opened this issue Nov 22, 2024 · 4 comments

Comments

@qhris
Copy link

qhris commented Nov 22, 2024

Description

Async implementations of non-async interface methods are not properly reported as an issue.

Repro steps

Minimal reproduction code:

public interface ITestReproduction
{
    void TestMethod();
}

public class TestReproduction : ITestReproduction
{
    async void TestMethod() // FN: Not reported as an issue.
    {
        await Task.Delay(1000);
    }
}

Expected behavior

The above code should report an issue with the async void implementation of the interface.

Actual behavior

Nothing is reported.

Known workarounds

Nothing.

Related information

  • C# 12
  • .NET 8.0
  • Rider 2024.3 with SonarLint 10.12.0.79769
  • Operating System Linux
@Tim-Pohlmann
Copy link
Contributor

Hi @qhris!
This case is excluded because it cannot be fixed without changing the interface or making the method non-async. I will update rspec to clarify this.
Let me know if you need more clarification.

@Tim-Pohlmann
Copy link
Contributor

Here is the rspec PR:
SonarSource/rspec#4547

@qhris
Copy link
Author

qhris commented Nov 28, 2024

Seems a bit strange to me, the fix is pretty clear to me: make the method synchronous. @Tim-Pohlmann

The entire async void really needs to be reported regardless in my opinion.
It is a pretty nasty footgun which can bring down the entire application (and did in our case, by a segfault).

Quoting from Microsoft here:

Async void methods have different error-handling semantics. When an exception is thrown out of an async Task or async Task method, that exception is captured and placed on the Task object. With async void methods, there is no Task object, so any exceptions thrown out of an async void method will be raised directly on the SynchronizationContext that was active when the async void method started.

Should this be a separate rule then? Catching stuff like this is what we want to use the sonar scanner for in the first place.

@Tim-Pohlmann
Copy link
Contributor

@qhris That is a Very valid point; thanks for your input!
I created an issue for a new rule idea. Please review it and let me know if this is what you are looking for.
Thanks for challenging our decisions; this is how we grow from user feedback and improve our products to better suit your needs!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants