Enable IDE0060 (Remove unused parameter) analyzer #67527
Conversation
|
Tagging subscribers to this area: @dotnet/area-meta Issue Detailsnull
|
marek-safar
force-pushed
the
ide0060
branch
3 times, most recently
from
e0e389d to
b055821
Compare
|
You're killing me, Marek :) |
stephentoub
left a comment
stephentoub
left a comment
There was a problem hiding this comment.
I skimmed the first half. Can't spend more time on it this moment.
| // This method is identical to Type.GetTypeFromCLSID. Since it's interop specific, we expose it | ||
| // on Marshal for more consistent API surface. | ||
| internal static Type? GetTypeFromCLSID(Guid clsid, string? server, bool throwOnError) | ||
| internal static Type? GetTypeFromCLSID(Guid clsid, string? server, bool *throwOnError) |
There was a problem hiding this comment.
What is this asterisk?
Also, why the suppression rather than removing the parameter?
| // This is a fail-fast function used by the runtime as a last resort that will terminate the process with | ||
| // as little effort as possible. No guarantee is made about the semantics of this fail-fast. | ||
| internal static void FallbackFailFast(RhFailFastReason reason, object unhandledException) | ||
| internal static void FallbackFailFast(RhFailFastReason _ /*reason*/, object _1 /*unhandledException*/) |
There was a problem hiding this comment.
Why change the parameter names? Does the analyzer ignore underscore-prefixed names?
There was a problem hiding this comment.
Yes, that's documented syntax the analyzer understands https://docs.microsoft.com/bs-latn-ba/dotnet/fundamentals/code-analysis/style-rules/ide0060#overview
| { | ||
| public static partial class ThreadPool | ||
| { | ||
| [Conditional("unnecessary")] |
There was a problem hiding this comment.
We should come up with some standard compilation-constant we use in such situations, rather than just "unnecessary".
There was a problem hiding this comment.
What value do you suggest ?
| [LibraryImport(Libraries.OpenLdap, EntryPoint = "ber_get_stringal")] | ||
| private static partial int ber_get_stringal(SafeBerHandle berElement, ref IntPtr value); | ||
|
|
||
| #pragma warning disable IDE0060 |
There was a problem hiding this comment.
On the native side of things, we use the equivalent of discards rather than suppressions, e.g.
// V and v are not supported on Unix yet.
_ = berElement;
_ = value;
_ = tag;Should we standardize on doing the same thing in managed, rather than suppressing IDE0060 (and any other analyzers that might flag the same thing)?
There was a problem hiding this comment.
I'm not sure if the compiler will be able to optimize this out in all cases.
| [UnsupportedOSPlatform("tvos")] | ||
| internal static class ContextFlagsAdapterPal | ||
| { | ||
| #pragma warning disable IDE0060 |
There was a problem hiding this comment.
Maybe we should instead update IDE0060 to recognize the pattern where the whole body is throw new PNSE(...) and not warn on such methods? The developer obviously doesn't intend to use the method at all in such cases, nevermind its arguments.
There was a problem hiding this comment.
That sounds like a reasonable suggestion for analyzer improvement
| /// </summary> | ||
| /// <param name="ldapHandle">The connection handle to the LDAP server.</param> | ||
| /// <param name="flags">Flags that control the interaction used to retrieve any necessary Sasl authentication parameters</param> | ||
| /// <param name="_"></param> |
There was a problem hiding this comment.
This makes the approach of using "_" for unused parameters even stranger.
| /// <param name="interactPtr">Pointer to the challenge we need to resolve</param> | ||
| /// <returns></returns> | ||
| internal static int SaslInteractionProcedure(IntPtr ldapHandle, uint flags, IntPtr defaultsPtr, IntPtr interactPtr) | ||
| internal static int SaslInteractionProcedure(IntPtr ldapHandle, uint _, IntPtr defaultsPtr, IntPtr interactPtr) |
There was a problem hiding this comment.
Other places you used a comment with the parameter name.
| #pragma warning disable IDE0060 | ||
| internal static Task? GetAddrInfoAsync(string hostName, bool justAddresses, AddressFamily family, CancellationToken cancellationToken) => | ||
| throw new NotSupportedException(); | ||
| #pragma warning restore IDE0060 |
There was a problem hiding this comment.
Another example where fixing IDE0060 to ignore always-throw methods would be beneficial. I don't think there should be controversy about it, but if there is, we could make it an opt-in thing, and we'd opt-in in the editorconfig.
| [UnsupportedOSPlatform("tvos")] | ||
| internal static partial class NegotiateStreamPal | ||
| { | ||
| #pragma warning disable IDE0060 |
There was a problem hiding this comment.
In general if there are places where we surround the whole body with disable/restore, I think we should just disable it at the top of the file
| int headerSize, | ||
| int trailerSize, | ||
| int _ /*headerSize*/, | ||
| int _1 /*trailerSize*/, |
There was a problem hiding this comment.
These _, _1, _2, etc. parameters just make the source harder to read, make things harder to understand in IntelliSense, etc. As noted earlier, I'd prefer if we were on a plan to use discards.
|
This pull request has been automatically marked |
ghost
closed this
|
Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it. |
ghost
locked as resolved and limited conversation to collaborators
ghost
locked as resolved and limited conversation to collaborators
3 participants
No description provided.