dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
14.97k stars 4.65k forks source link

Review unused locals across corefx #30457

Closed stephentoub closed 3 years ago

stephentoub commented 5 years ago

Experimenting with various analyzers, I used SonarAnalyzers.CSharp to highlight all unused locals.

In dotnet/corefx#39956, I fixed some, but there remain a bunch across the repo. We could consider enabling the rule permanently, but there's enough noise that we may not want to. Even so, it'd probably be worth reviewing the remaining failures to ensure nothing important has slipped through.

D:\repos\corefx\src\Microsoft.Diagnostics.Tracing.EventSource.Redist\src\Microsoft.Diagnostics.Tracing.EventSource.Redist.csproj

D:\repos\corefx\src\System.Configuration.ConfigurationManager\src\System.Configuration.ConfigurationManager.csproj

D:\repos\corefx\src\System.Data.Common\src\System.Data.Common.csproj

D:\repos\corefx\src\System.Data.OleDb\src\System.Data.OleDb.csproj

D:\repos\corefx\src\System.Data.SqlClient\src\System.Data.SqlClient.csproj

D:\repos\corefx\src\System.DirectoryServices.AccountManagement\src\System.DirectoryServices.AccountManagement.csproj

D:\repos\corefx\src\System.DirectoryServices\src\System.DirectoryServices.csproj

D:\repos\corefx\src\System.Drawing.Common\src\System.Drawing.Common.csproj

D:\repos\corefx\src\System.IO.FileSystem.AccessControl\src\System.IO.FileSystem.AccessControl.csproj

D:\repos\corefx\src\System.IO.FileSystem.DriveInfo\src\System.IO.FileSystem.DriveInfo.csproj

D:\repos\corefx\src\System.IO.FileSystem\src\System.IO.FileSystem.csproj

D:\repos\corefx\src\System.IO.MemoryMappedFiles\src\System.IO.MemoryMappedFiles.csproj

D:\repos\corefx\src\System.IO.Ports\src\System.IO.Ports.csproj

D:\repos\corefx\src\System.Management\src\System.Management.csproj

D:\repos\corefx\src\System.Net.HttpListener\src\System.Net.HttpListener.csproj

D:\repos\corefx\src\System.Net.Requests\src\System.Net.Requests.csproj

D:\repos\corefx\src\System.Net.Security\src\System.Net.Security.csproj

D:\repos\corefx\src\System.Numerics.Tensors\src\System.Numerics.Tensors.csproj

D:\repos\corefx\src\System.Private.DataContractSerialization\src\System.Private.DataContractSerialization.csproj

D:\repos\corefx\src\System.Private.Xml\src\System.Private.Xml.csproj

D:\repos\corefx\src\System.Runtime.Extensions\src\System.Runtime.Extensions.csproj

D:\repos\corefx\src\System.Runtime.Serialization.Formatters\src\System.Runtime.Serialization.Formatters.csproj

D:\repos\corefx\src\System.Runtime.WindowsRuntime.UI.Xaml\src\System.Runtime.WindowsRuntime.UI.Xaml.csproj

D:\repos\corefx\src\System.Runtime.WindowsRuntime\src\System.Runtime.WindowsRuntime.csproj

D:\repos\corefx\src\System.Security.AccessControl\src\System.Security.AccessControl.csproj

D:\repos\corefx\src\System.Security.Cryptography.Algorithms\src\System.Security.Cryptography.Algorithms.csproj

D:\repos\corefx\src\System.Security.Cryptography.Cng\src\System.Security.Cryptography.Cng.csproj

D:\repos\corefx\src\System.Security.Cryptography.Csp\src\System.Security.Cryptography.Csp.csproj

D:\repos\corefx\src\System.Security.Cryptography.Encoding\src\System.Security.Cryptography.Encoding.csproj

D:\repos\corefx\src\System.Security.Cryptography.OpenSsl\src\System.Security.Cryptography.OpenSsl.csproj

D:\repos\corefx\src\System.Security.Cryptography.Pkcs\src\System.Security.Cryptography.Pkcs.csproj

D:\repos\corefx\src\System.Security.Cryptography.X509Certificates\src\System.Security.Cryptography.X509Certificates.csproj

D:\repos\corefx\src\System.Security.Cryptography.Xml\src\System.Security.Cryptography.Xml.csproj

D:\repos\corefx\src\System.Security.Principal.Windows\src\System.Security.Principal.Windows.csproj

D:\repos\corefx\src\System.ServiceModel.Syndication\src\System.ServiceModel.Syndication.csproj

D:\repos\corefx\src\System.ServiceProcess.ServiceController\src\System.ServiceProcess.ServiceController.csproj

danmoseley commented 5 years ago

Just curious does it affect the code gen?

stephentoub commented 5 years ago

Just curious does it affect the code gen?

It depends. If it's just about an unused local, then no, it shouldn't. But some of the cases I fixed in my PR were, for example, initializing unused locals with allocations, or invoking properties or methods or delegates to initialize them. Those require more analysis to validate they can be removed safely (in some cases the side effects are important), but when they can be removed, obviously they effect code gen.

vcsjones commented 4 years ago

@bartonjs I went through the S.S.C ones, I'm not sure about one of them: The DSA magic field isn't checked here:

https://github.com/dotnet/corefx/blob/b8048a1e2bd7ee776b4cb6df684db3a0b35886c1/src/System.Security.Cryptography.Csp/src/System/Security/Cryptography/CapiHelper.DSA.Shared.cs#L181

Should there be checks that it is DSS1 - DSS4 magic values?

bartonjs commented 4 years ago

Seems reasonable to at least assert it :)

eriawan commented 4 years ago

@stephentoub

I'll take the System.Data.Common. PR is ready: dotnet/corefx#41962

eriawan commented 4 years ago

@stephentoub

PR on removing unused variables on System.Data.OleDb is submitted: dotnet/corefx#42485

eriawan commented 4 years ago

@stephentoub

Now that we have dotnet/runtime repo, I think we have to transfer/move this issue to dotnet/runtime repo for more visibility and tracking this to .NET Core 5.0, because I also think the progress on this issue is valid for .NET Core 5.0 milestone, cmiiw.

Could we move/transfer this issue to https://github.com/dotnet/runtime repo?

stephentoub commented 4 years ago

All issues are going to be transferred in bulk.

eriawan commented 4 years ago

@stephentoub Thanks for the quick reply! 🙂 I'm glad to know we'll move the issues here to dotnet/runtime.

Just for tracking update, if you don't mind could you also update the finished/merged progress on System.Data.OleDb check marks? Thanks in advance.

alanisaac commented 4 years ago

@stephentoub

Submitted #32896 for System.IO.Ports.

handlespeed commented 4 years ago

@stephentoub Submitted #33619 for System.Net.Requests, thanks.

danmoseley commented 4 years ago

anyone interested in doing more?

eriawan commented 4 years ago

@danmosemsft I'll take the System.Runtime.Serialization.Formatters. PR will be submitted today.

Looks like some of the unused locals already removed:

  1. originalHolder in ObjectManager.cs by dotnet/corefx#42188
  2. objName in BinaryObjectReader.cs by dotnet/corefx#42188
  3. byteLength in BinaryFormatterWriter.cs by dotnet/corefx#42188

So I'll remove the remaining in System.Runtime.Serialization.Formatters, the genId.

vcsjones commented 4 years ago

For what it's worth, all of:

Are fixed, since these got flagged multiple times since they are included in multiple projects.

pviotti commented 4 years ago

howdy, it appears that

I've submitted #34975 to sort out files reported in System.Configuration.ConfigurationManager.csproj and System.DirectoryServices.csproj.

vivekbm commented 4 years ago

@danmosemsft - Does this still require work? Happy to taken it on.

danmoseley commented 4 years ago

@vivekbm yes, I think the checklist is roughly up to date, so you'd be welcome to. Eg., a PR for the IO ones, and/or a separate PR for the crypto ones?

MattKotsenas commented 4 years ago

Hey @stephentoub @danmosemsft ! With the PRs I opened Friday / today, all the issues listed in this issue have been addressed. How would you like to proceed?

Since keeping this rule clean isn't enforced I would assume new issues will / have crept in, but even so, my vote would be to close this issue to avoid accumulating mentions forever, and if desired open a new issue to track turning on enforcement.

If you disagree or have anything you'd like me to follow up on, please let me know. Thanks!

bartonjs commented 4 years ago

Closing the issue once all the PRs are merged seems fine to me.

MattKotsenas commented 3 years ago

Hi @bartonjs! I believe all the outstanding PRs for this are now merged; it would be great if we could either close this issue out, or update it with what's remaining.

If you have any questions, please let me know. Thanks!

stephentoub commented 3 years ago

Thanks, @MattKotsenas (and to all who contributed here). I just re-ran the analyzer over the repo, and while there were a few remaining occurrences, it was only a small number. I submitted a PR to fix the only impactful one (#42778). Since we're not currently planning on enabling this analyzer, we don't need to be 100% clean, and we can close the issue. Thanks!

danmoseley commented 3 years ago

@stephentoub I missed or don't see the discussion about enabling it. Is there any reason we can't safely "baseline" everything remaining by using a discard, and then enable the analyzer?

stephentoub commented 3 years ago

and then enable the analyzer?

It would mean pulling a 3rd-party analyzer into the repo, which means someone else's arbitrary code would run on every build on every dev's machine and in CI. We need to have a longer discussion before we do such a thing.

danmoseley commented 3 years ago

Ah I missed that aspect