VSTHRD114: fires even in the function is annotated as returning nullable Tasks

[jasonmalinowski]

Bug description

Consider this code:

https://github.com/dotnet/roslyn/blob/5f5c283ce3fbcdceb998d48aca4ed3c9792605e3/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Utilities/ValuesSources/WeaklyCachedRecoverableValueSource.cs#L141-L165

The "return null" at the end triggers the warning, which is a useful warning if this code wasn't null annotated. In this case however the function is marked as returning Task? and the compiler will warn if somebody tries to await it without checking for null first.

Expected behavior

If the function is explicitly Task? returning, we don't analyze the method since the developer has explicitly indicated they know this, and the compiler will help check. If you're not on C# 8 or the code hasn't been annotated, then absolutely keep warning like it does today.

Actual behavior

We're analyzing code unnecessarily.

• Version used:
• Application (if applicable):

Additional context

Add any other context about the problem here.

[AArnott]

We don't reference c# 8 compiler assemblies yet, so what you're asking would be difficult without dropping support for older toolsets.
Also, I doubt the scenario of returning a null task, regardless of nullable annotations. Besides being really odd, there's often no guarantee that the caller has nullable annotation warnings even turned on.

Do you think there's a legit case for ever returning null instead of a task?

[jasonmalinowski]

Do you think there's a legit case for ever returning null instead of a task?

Absolutely. There were others in the Roslyn codebase as well, for example this one:

https://github.com/dotnet/roslyn/blob/ef50fdec4a6a72545e01cdbaca18ada7740557c2/src/Compilers/Shared/BuildServerConnection.cs#L161

This case was a TryStartProcess() where the failure to start the task was an error return that had to be handled differently. Returning a completed task wouldn't have been correct because the caller wouldn't know what happened. The Try* pattern specifically I don't see a good alternative for.

We don't reference c# 8 compiler assemblies yet, so what you're asking would be difficult without dropping support for older toolsets.

Analyzers usually just use reflection in this case; since you just have to read a property on the interface in this case. But yes, that's not fun either way.

[sharwell]

Just hit this again porting a different library to use vs-threading

[AArnott]

At this point, depending on Roslyn assemblies that are new enough to include the nullable ref annotations should be tolerable.