Fix S3168: Rule should not raise FP with UWP event args

[Portikus]

Description

Hi there,

If you subscribe the Windows.UI.Xaml.Application.Suspending event and declare the handler method as async void the analyser suggest to change the return type to Task instead of void which is impossible for event handler.

Repro steps

I created a small repo solution with an UWP app. In the App.xaml.cs you can see the warning in row 84.

Expected behavior

No S3168 warning on an UWP event handler if they have the async keyword.

Actual behavior

A S3168 warning on an UWP event handler if they have the async keyword.

Known workarounds

No workaround known.

Related information

• SonarC# Version: 6.3.0.2862
• Visual Studio Version: 15.3.0

[valhristov]

Hi @Portikus, thanks for the feedback! This is indeed a false positive, but I am afraid that it would be rather difficult to avoid (unless we hardcode the name of the event). When checking if you can return Task from an async method, we explicitly check if the method has an EventHandler signature and we do not raise an issue. However, the Suspending event does not have a regular event handler signature, because its second argument is not an EventArgs descendant but an interface, hence we raise. Such events should be rare (I even did not knew that there is an event handler in the .NET Framework that is not using the standard method signature) and we would recommend marking this issue as False Positive in your SonarQube (if you use connected mode), or suppressing it with an SuppressMessage attribute.
It seems that our rule S3242 incorrectly suggests to change the EventArgs from the method signature to an interface, then S3168 cannot detect if the method is an event handler and raises an issue.

[Portikus]

I guess there are more event handler like this in the UWP world.
I don't like it to supress warnings but if you say it is the only way I have to deal with it.

What about checking if the secound parameter ends with 'EventArgs'?

[valhristov]

@Portikus I wrote too quickly, it seems this issue is a result of S3168 raising a false positive, am I right? So it seems that if we make S3168 not raise on event handler methods this should not appear.

[Portikus]

Even if I don't use the interface ISuspendingEventArgs type but the concrete SuspendingEventArgs I get the same false positive. If it is that what you mean.

[valhristov]

Hi @Portikus, I see that the SuspendingEventArgs (and many other UWP event args) does not inherit from EventArgs, thus we cannot recognize that the method is an event handler and we raise issue... I guess we can safely ignore methods that look like event handler (e.g. Foo(object sender, TArgs args)) where the second parameter's class name ends with EventArgs. What do you think?

[Portikus]

I guess we can safely ignore methods that look like event handler (e.g. Foo(object sender, TArgs args)) where the second parameter's class name ends with EventArgs. What do you think?

I think this should be very helpfull otherwise the rule is unusable in UWP projects.
Or is there any chance that you can recognize that this method is assigned like that: someDelegate+=IssueRaisingMethod?

[Evangelink]

@Portikus Sadly we cannot rely on detecting the += because it might be into a different project. But I have a good idea to get rid of this problem (stay tuned for PR).