- Notifications
You must be signed in to change notification settings - Fork481
(2/4) Override Stream ReadAsync/WriteAsync Analyzer#4726
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
NewellClark commentedJan 21, 2021
@mavasani Sorry about that. I've retargeted this PR to the correct branch. |
codecovbot commentedJan 21, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Codecov Report
@@ Coverage Diff @@## release/6.0.1xx #4726 +/- ##===================================================+ Coverage 95.56% 95.57% +0.01%=================================================== Files 1210 1212 +2 Lines 276014 277225 +1211 Branches 16724 16737 +13 ===================================================+ Hits 263785 264971 +1186- Misses 10007 10020 +13- Partials 2222 2234 +12 |
...Analyzers/Core/Microsoft.NetCore.Analyzers/Runtime/ProvideStreamMemoryBasedAsyncOverrides.cs OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
...Analyzers/Core/Microsoft.NetCore.Analyzers/Runtime/ProvideStreamMemoryBasedAsyncOverrides.cs OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
...Analyzers/Core/Microsoft.NetCore.Analyzers/Runtime/ProvideStreamMemoryBasedAsyncOverrides.csShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
...Analyzers/Core/Microsoft.NetCore.Analyzers/Runtime/ProvideStreamMemoryBasedAsyncOverrides.cs OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
| returncontainingType.GetMembers(methodName) | ||
| .SingleOrDefault(symbol=>symbolisIMethodSymbolm&&IsMatch(m,argumentTypes))asIMethodSymbol; | ||
| staticboolIsMatch(IMethodSymbolmethod,ITypeSymbol[]argumentTypes) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@Evangelink Was there a helper method that does this?
NewellClarkJan 22, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The helper method requires instances ofParameterInfo, which themselves requireINamedTypeSymbol instances. It was significantly shorter to write it out my way, which allows anyITypeSymbol to be used. I did originally write it out using the helper method, but it was significantly longer and more verbose.
…ideStreamMemoryBasedAsyncOverrides.csCo-authored-by: Youssef Victor <31348972+Youssef1313@users.noreply.github.com>
If there are multiple definitions of the same overridden method, the analyzer can crash due to uses of `SingleOrDefault` instead of `FirstOrDefault`.To fix, I refactored `GetOverload` and `GetSymbolForOverridingMethod` into immutable-array-returning methods, and simply used `FirstOrDefault` or `SingleOrDefault` at each call-site as appropriate.
| <comment>This call site is reachable on: 'windows' 10.0.2000 and later, and all other platforms</comment> | ||
| </data> | ||
| <dataname="ProvideStreamMemoryBasedAsyncOverridesDescription"xml:space="preserve"> | ||
| <value>The default implementation of '{2}' delegates to '{1}', which is innefficient as it requires an extra copy. It also results in extra allocations as it returns Task instead of ValueTask. Consider moving your implementation to '{2}' and have '{1}' delegate to '{2}'.</value> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Seems like you missed the{0} in this declaration.
NewellClarkJan 23, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I used the{0} in the message field, but not in the description.
Edit: I've changed the description so it includes the{0} argument.
| context.RegisterSymbolAction(AnalyzeNamedType,SymbolKind.NamedType); | ||
| voidAnalyzeNamedType(SymbolAnalysisContextcontext) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Except if I have missed something, this local func could be marked asstatic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It uses thesymbols struct produced by the call toTryGetRequiredSymbols(...) on line 55.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
My bad!
| returnDiagnostic.Create( | ||
| Rule, | ||
| violatingType.Locations[0], | ||
| violatingType.Locations.Skip(1), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Please use theCreateDiagnostic helper method.
| returnDiagnostic.Create( | |
| Rule, | |
| violatingType.Locations[0], | |
| violatingType.Locations.Skip(1), | |
| returnviolatingType.CreateDiagnostic( | |
| Rule, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I've fixed this in the latest commit.
| // We know that there are no compiler errors on streamType, so we use 'SingleOrDefault'. | ||
| varreadAsyncArrayMethod=GetOverloads(streamType,ReadAsyncName,byteArrayType,int32Type,int32Type,cancellationTokenType).SingleOrDefault(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I would still avoid theSingleOrDefault here as it makes it harder to debug if you end up with actually more than one item.
What's your view on this@mavasani ?
NewellClarkJan 23, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I figuredSingleOrDefault since we're looking up methods on framework types that are in metadata, and therefore could never contain compiler errors. Now that I think about it though, if this analyzer is running againstSystem.Private.CoreLib, these framework types could actually contain compiler errors, and we definitely don't want the analyzer to crash if that happens. I suppose we could detect whetherstreamType is in source code or metadata, but it's probably not worth the hassle. I'll change this toFirstOrDefault for now.
Edit: I've switched toFirstOrDefault
...Analyzers/Core/Microsoft.NetCore.Analyzers/Runtime/ProvideStreamMemoryBasedAsyncOverrides.csShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
| { | ||
| publicclassProvideStreamMemoryBasedAsyncOverridesTests | ||
| { | ||
| #region Reports Diagnostic |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
[cosmetic]@mavasani Shall we keep introducing these regions?
| stringcode=$@" | ||
| {CSUsings} | ||
| namespace Testopolis | ||
| {{ | ||
| public class {{|#0:FooStream|}} : Stream | ||
| {{ | ||
| {CSAbstractMembers} | ||
| {CSReadAsyncArray} | ||
| }} | ||
| }}"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
[cosmetic] I am fine if you keep it this way but I find the double curly braces hard to read.
| stringcode=$@" | |
| {CSUsings} | |
| namespaceTestopolis | |
| {{ | |
| publicclass{{|#0:FooStream|}}:Stream | |
| {{ | |
| {CSAbstractMembers} | |
| {CSReadAsyncArray} | |
| }} | |
| }}"; | |
| stringcode= @" | |
| " +CSUsings + @" | |
| namespaceTestopolis | |
| { | |
| publicclass{|#0:FooStream|}:Stream | |
| { | |
| " +CSAbstractMembers + @" | |
| " +CSReadAsyncArray + @" | |
| } | |
| }"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Going forward, I will do it your way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
NewellClark commentedJan 24, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
This should be linked toissue #33789. |
| varint32Type=compilation.GetSpecialType(SpecialType.System_Int32); | ||
| varbyteType=compilation.GetSpecialType(SpecialType.System_Byte); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@mavasani Can these be null under any case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yes, they can be null for broken compilation references. We should have a null check for these types as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@mavasani Fixed. Sorry I missed this earlier.
| VerifyVB.Diagnostic(Rule) | ||
| .WithLocation(0) | ||
| .WithLocation(1) | ||
| .WithArguments("River",displayArrayMethod,displayMemoryMethod) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This is very confusing. Is this supposed to be two diagnostics?
I believe that only one diagnostic is passed to the test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The location that is underlined when this diagnostic is reported is the name of the class in the class declaration. If the class is a partial class, all partial declarations need to be underlined. That is what this test is checking: that when you have multiple partial declarations in the same file, all partial declarations will be underlined for a single violation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@NewellClark If I understand correctly, you're passing only one diagnostic. When you chainWithLocation calls, only the latest one takes effect.
I believe if you removed the chain and kept the latest WithLocation call only, the test will still pass. Could you confirm that locally on your machine?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Just tested it. The test fails if we remove any of theWithLocation calls, and the error message reports the diagnostics at the removed locations as the unexpected diagnostics.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@NewellClark Good to know! Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@Youssef1313 by the way, would it be possible to have this pull request linked toissue 33789?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@NewellClark The issue won't be auto-closed when the PR is merged. But a Microsoft employee will close it when he merges your PR.
| .WithLocation(0) | ||
| .WithLocation(1) | ||
| .WithLocation(2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Same comment here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Same as my previous comment, but this time we are checking whether all locations are reported when the partials are in separate files.
...zers/UnitTests/Microsoft.NetCore.Analyzers/Runtime/ProvideStreamMemoryAsyncOverridesTests.cs OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
...zers/UnitTests/Microsoft.NetCore.Analyzers/Runtime/ProvideStreamMemoryAsyncOverridesTests.csShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
...zers/UnitTests/Microsoft.NetCore.Analyzers/Runtime/ProvideStreamMemoryAsyncOverridesTests.csShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
| <comment>This call site is reachable on: 'windows' 10.0.2000 and later, and all other platforms</comment> | ||
| </data> | ||
| <dataname="ProvideStreamMemoryBasedAsyncOverridesDescription"xml:space="preserve"> | ||
| <value>The default implementation of '{2}' delegates to '{1}', which is innefficient as it requires an extra copy. It also results in extra allocations as it returns Task instead of ValueTask. Consider moving {0}'s implementation to '{2}' and have '{1}' delegate to '{2}'.</value> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| <value>The default implementation of '{2}' delegates to '{1}', which is innefficient as it requires an extra copy. It also results in extra allocations as it returns Task instead of ValueTask. Consider moving {0}'s implementation to '{2}' and have '{1}' delegate to '{2}'.</value> | |
| <value>The default implementation of '{2}' delegates to '{1}', which is innefficient as it requires an extra copy. It also results in extra allocations as it returns'Task' instead of'ValueTask'. Consider moving {0}'s implementation to '{2}' and have '{1}' delegate to '{2}'.</value> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Actually, you cannot have format arguments{0},{1}, etc. in description string, it is a static description per-descriptor, not something instantiated per-diagnostic. You should instead follow what@carlossanlop did in a similar analyzer and use more generic description string that is applicable for all violations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Fixed.
| context.RegisterSymbolAction(AnalyzeNamedType,SymbolKind.NamedType); | ||
| voidAnalyzeNamedType(SymbolAnalysisContextcontext) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| context.RegisterSymbolAction(AnalyzeNamedType,SymbolKind.NamedType); | |
| voidAnalyzeNamedType(SymbolAnalysisContextcontext) | |
| context.RegisterSymbolAction(AnalyzeNamedType,SymbolKind.NamedType); | |
| return; | |
| // Local functions | |
| voidAnalyzeNamedType(SymbolAnalysisContextcontext) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Fixed
| Rule ID | Missing Help Link | Title | | ||
| --------|-------------------|-------| | ||
| CA1840 |<https://docs.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1840> | Provide memory-based overrides of async methods when subclassing 'Stream' | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
ID has already been used, please choose a new one.
NewellClarkApr 20, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Changed ID to ca1844
mavasani left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Overall looks great. Please retarget to preview4 branch.
- Change ID to CA1842
mavasani commentedMay 17, 2021
@NewellClark Sorry for not getting back to this PR review sooner, but unfortunately even preview4 is now locked. Can you please retarget tohttps://github.com/dotnet/roslyn-analyzers/tree/release/6.0.1xx? Promise this one is the last branch retarget request. Again, very sorry for this additional work. Can you also address the remaining review comments? I should be able to merge once both of these are taken care of. |
Fix possible crash when primitive types can't be loaded
NewellClark commentedMay 17, 2021
@mavasani Fixed. Should I retarget for all my other analyzer PRs that are targeted to preview4? |
mavasani commentedMay 17, 2021
Yes please. preview4 branch is now locked. |
| returnviolatingType.CreateDiagnostic( | ||
| Rule, | ||
| violatingType.ToDisplayString(SymbolDisplayFormat.MinimallyQualifiedFormat), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We normally use just the type/method.Name in diagnostic messages.MinimallyQualifiedFormat actually produces a qualified name, which lead to very long names.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@mavasani Fixed. I've changed the message so it makes sense with simple names (the message was originally worded with the assumption that the arguments of each overload would be displayed).
mavasani left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
LGTM.
I can merge once you change the diagnostic messages to use simple names instead of ToDisplayString.
- Remove usages of `MinimallyQualifiedFormat`- Change message so it makes sense with simple names
mavasani left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thanks!
mavasani commentedMay 18, 2021
@NewellClark Can you please help us in creating documentation PRs for all the new .NET6 analyzers that you have contributed? Please see the item 4. inhttps://github.com/dotnet/roslyn-analyzers/blob/main/GuidelinesForNewRules.md. Also tagging@gewarren as we now have 8 new undocumented CA rules which have been implemented in the |
NewellClark commentedMay 18, 2021
Absolutely. I'm on it. |
Uh oh!
There was an error while loading.Please reload this page.
Fixesissue #33789.
I ran the analyzer against dotnet/runtime and found several violations:
DeflateManagedStream, which will have no extra copies. Edit: I've memorifiedDeflateManagedStream(PR).ReadAsync/WriteAsyncare only ever called fromWebSocketBase, which only calls the array-based override. Unfortunately,WebSocketHttpListenerDuplexStreamalso calls back intoWebSocket, and it passes the array it was given into multiple different methods onWebSocket, and some methods onWebSocketBuffer. Providing memory-based overrides would require almost a complete rewrite ofWebSocketBufferas well as changes to code that consumes it. Since nothing currently calls the memory-based overrides onWebSocketHttpListenerDuplexStream, I think we should strongly consider suppressing the warning.Note that I'm using
CA1840as the diagnostic ID because I usedCA1839inanother analyzer that has yet to be merged in. Not sure how that type of situation is typically handled.