Enable IDE0060 (Remove unused parameter) analyzer by marek-safar · Pull Request #67527 · dotnet/runtime

marek-safar · 2022-04-04T07:33:34Z

No description provided.

ghost · 2022-04-04T07:33:50Z

Tagging subscribers to this area: @dotnet/area-meta
See info in area-owners.md if you want to be subscribed.

Issue Details

null

Author:	marek-safar
Assignees:	marek-safar
Labels:	`area-Meta`
Milestone:	-

stephentoub · 2022-04-04T13:07:02Z

You're killing me, Marek :)

stephentoub

I skimmed the first half. Can't spend more time on it this moment.

stephentoub · 2022-04-04T13:09:05Z

src/coreclr/System.Private.CoreLib/src/System/Runtime/InteropServices/Marshal.CoreCLR.cs

        // 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)


What is this asterisk?

Also, why the suppression rather than removing the parameter?

stephentoub · 2022-04-04T13:10:50Z

src/coreclr/nativeaot/Runtime.Base/src/System/Runtime/ExceptionHandling.cs

        // 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*/)


Why change the parameter names? Does the analyzer ignore underscore-prefixed names?

Yes, that's documented syntax the analyzer understands https://docs.microsoft.com/bs-latn-ba/dotnet/fundamentals/code-analysis/style-rules/ide0060#overview

stephentoub · 2022-04-04T13:13:57Z

src/coreclr/nativeaot/System.Private.CoreLib/src/System/Threading/ThreadPool.CoreRT.cs

 {
    public static partial class ThreadPool
    {
+        [Conditional("unnecessary")]


We should come up with some standard compilation-constant we use in such situations, rather than just "unnecessary".

What value do you suggest ?

stephentoub · 2022-04-04T13:19:18Z

src/libraries/Common/src/Interop/Linux/OpenLdap/Interop.Ber.cs

        [LibraryImport(Libraries.OpenLdap, EntryPoint = "ber_get_stringal")]
        private static partial int ber_get_stringal(SafeBerHandle berElement, ref IntPtr value);

+#pragma warning disable IDE0060


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)?

I'm not sure if the compiler will be able to optimize this out in all cases.

stephentoub · 2022-04-04T13:21:02Z

src/libraries/Common/src/System/Net/ContextFlagsAdapterPal.PlatformNotSupported.cs

    [UnsupportedOSPlatform("tvos")]
    internal static class ContextFlagsAdapterPal
    {
+#pragma warning disable IDE0060


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.

That sounds like a reasonable suggestion for analyzer improvement

stephentoub · 2022-04-04T14:18:25Z

....DirectoryServices.Protocols/src/System/DirectoryServices/Protocols/Interop/LdapPal.Linux.cs

        /// </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>


This makes the approach of using "_" for unused parameters even stranger.

stephentoub · 2022-04-04T14:18:40Z

....DirectoryServices.Protocols/src/System/DirectoryServices/Protocols/Interop/LdapPal.Linux.cs

        /// <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)


Other places you used a comment with the parameter name.

stephentoub · 2022-04-04T14:24:23Z

src/libraries/System.Net.NameResolution/src/System/Net/NameResolutionPal.Unix.cs

+#pragma warning disable IDE0060
        internal static Task? GetAddrInfoAsync(string hostName, bool justAddresses, AddressFamily family, CancellationToken cancellationToken) =>
            throw new NotSupportedException();
+#pragma warning restore IDE0060


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.

stephentoub · 2022-04-04T14:26:41Z

...aries/System.Net.Security/src/System/Net/Security/NegotiateStreamPal.PlatformNotSupported.cs

    [UnsupportedOSPlatform("tvos")]
    internal static partial class NegotiateStreamPal
    {
+#pragma warning disable IDE0060


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

stephentoub · 2022-04-04T14:27:46Z

src/libraries/System.Net.Security/src/System/Net/Security/SslStreamPal.Android.cs

-            int headerSize,
-            int trailerSize,
+            int _ /*headerSize*/,
+            int _1 /*trailerSize*/,


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.

ghost · 2022-04-23T03:01:05Z

This pull request has been automatically marked no-recent-activity because it has not had any activity for 14 days. It will be closed if no further activity occurs within 14 more days. Any new comment (by anyone, not necessarily the author) will remove no-recent-activity.

ghost · 2022-05-27T17:01:01Z

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.

ghost assigned marek-safar Apr 4, 2022

ghost added the area-Meta label Apr 4, 2022

marek-safar force-pushed the ide0060 branch 3 times, most recently from e0e389d to b055821 Compare April 4, 2022 09:11

Enable IDE0060 (Remove unused parameter) analyzer

8d36839

marek-safar force-pushed the ide0060 branch from b055821 to 8d36839 Compare April 4, 2022 09:54

stephentoub reviewed Apr 4, 2022

View reviewed changes

ghost added the no-recent-activity label Apr 23, 2022

ghost removed the no-recent-activity label Apr 27, 2022

ghost closed this May 27, 2022

ghost locked as resolved and limited conversation to collaborators Jun 26, 2022

dotnet unlocked this conversation Jul 22, 2022

marek-safar mentioned this pull request Jul 22, 2022

Enable IDE0060 (Remove unused parameter) analyzer #72667

Merged

ghost locked as resolved and limited conversation to collaborators Aug 21, 2022

This pull request was closed.

Conversation

marek-safar commented Apr 4, 2022

Uh oh!

ghost commented Apr 4, 2022

Uh oh!

stephentoub commented Apr 4, 2022

Uh oh!

stephentoub left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Apr 23, 2022

Uh oh!

ghost commented May 27, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants