Update vs-threading analyzers to 16.7.53#45628
Conversation
jasonmalinowski
left a comment
There was a problem hiding this comment.
There's one place in FileChangeTracker where the old/broken behavior may have been a feature. I can't quite tell. It depends on whether the old code would have actually thrown or whether AsyncLazy was handling that in some way.
I didn't review the compiler changes, this of course needs a review from @dotnet/roslyn-compiler.
There was a problem hiding this comment.
This probably should be throwing, since this isn't any more valid than the original code.
There was a problem hiding this comment.
The other methods also aren't throwing, so this is just a preservation of the original behavior.
There was a problem hiding this comment.
I think s_none is only ever used as a sentinel value and any attempt to actually call the AsyncLazy is potentially bad; if this would have thrown before we should replace the body with an explicit throw to maintain that invariant.
There was a problem hiding this comment.
It looks like this is used as a sentinel, although it wasn't necessarily intended to be used that way.
There was a problem hiding this comment.
The fact this doesn't blow up with a null is terrifying. :-/
5fa68e3 to
2f7223a
Compare
| End Sub | ||
|
|
||
| Private Function CompileNamespaceAsTask(symbol As NamespaceSymbol) As Task | ||
| Private Function CompileNamespaceAsync(symbol As NamespaceSymbol) As Task |
There was a problem hiding this comment.
📝 This rename is the same one we applied for C# in a previous PR
|
@dotnet/roslyn-compiler for reviews |
| End Try | ||
| End Function | ||
|
|
||
| <SuppressMessage("Style", "VSTHRD200:Use ""Async"" suffix for async methods", Justification:="'Async' refers to the language feature here.")> |
There was a problem hiding this comment.
Why an attribute rather than #Disable Warning?
There was a problem hiding this comment.
- It's one line instead of two (the
#Disable Warningwould still go on this line, but then it would require another line after the method header) - Easy to include the rationale
There was a problem hiding this comment.
The suppression is only needed in source, not from metadata.
In reply to: 452462415 [](ancestors = 452462415)
There was a problem hiding this comment.
SuppressMessage is a conditional attribute. I don't believe we are defining CODE_ANALYSIS in our production builds so both forms would be source-only suppressions.
| // Example: https://github.com/dotnet/roslyn/issues/24124 | ||
| #pragma warning disable VSTHRD114 // Avoid returning a null Task (False positive: https://github.com/microsoft/vs-threading/issues/637) | ||
| return null; | ||
| #pragma warning restore VSTHRD114 // Avoid returning a null Task |
There was a problem hiding this comment.
What is the value in running the analyzer on this code base? This code never executes in the context of VS. I don't see why we should be following VS threading rules here.
There was a problem hiding this comment.
This isn't a naming pattern violation though, it's a functional rule.
There was a problem hiding this comment.
We don't have to use this rule, but I believe we would generally agree it is correct and not specific to VS code. In most cases, if someone returns null from a Task-returning method, they actually meant to return Task.FromResult<T>(null).
Also, this bug will be fixed so the rule will not run on #nullable enable code (since it duplicates Nullable Reference Types there).
There was a problem hiding this comment.
This isn't a naming pattern violation though, it's a functional rule.
I got mixed up thinking this comment was a reply to #45628 (comment). Previous reply has been replaced by a new one.
This update brings the IOperation version of these analyzers, with two primary benefits: