-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Review unused locals across corefx #30457
Comments
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. |
@bartonjs I went through the S.S.C ones, I'm not sure about one of them: The DSA Should there be checks that it is DSS1 - DSS4 magic values? |
Seems reasonable to at least assert it :) |
I'll take the System.Data.Common. PR is ready: dotnet/corefx#41962 |
PR on removing unused variables on System.Data.OleDb is submitted: dotnet/corefx#42485 |
* emove unused locals in System.Data.OleDband keep some logic using discard. Partially fix issue #39962 * Update src/System.Data.OleDb/src/OleDbDataReader.cs update as suggested (OleDbDataReader) Co-Authored-By: Stephen Toub <[email protected]> * Update src/System.Data.OleDb/src/OleDbTransaction.cs Done update as suggested Co-Authored-By: Stephen Toub <[email protected]> * Update src/System.Data.OleDb/src/RowBinding.cs Done update as suggested (RowDataBinding) Co-Authored-By: Stephen Toub <[email protected]> * Update src/System.Data.OleDb/src/RowBinding.cs Done as suggested Co-Authored-By: Stephen Toub <[email protected]> * Update src/System.Data.OleDb/src/SafeHandles.cs Done updating as suggested (SafeHandles) Co-Authored-By: Stephen Toub <[email protected]>
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? |
All issues are going to be transferred in bulk. |
@stephentoub 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. |
Submitted #32896 for System.IO.Ports. |
src/libraries/System.Net.Requests/src/System/Net/FtpControlStream.cs, remove unused variable 'request' in L414. Partially Fix dotnet#30457
@stephentoub |
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! |
Closing the issue once all the PRs are merged seems fine to me. |
related issue is dotnet#30457
Remove unused locals and private methods in System.ServiceProcess.ServiceController. This fixes a part of dotnet#30457.
Remove unused locals and private methods in System.ServiceProcess.ServiceController. This fixes a part of #30457.
dotnet#30457 In line 292, remove the unused variable 'length'. Thanks.
#30457 In line 292, remove the unused variable 'length'. Thanks.
issue #30457 remove unused local variable dwErrorCode at line 248
* remove unused local issue #30457 remove unused local variable fullPath at line 415 * Update src/libraries/System.Security.Cryptography.X509Certificates/src/System/Security/Cryptography/X509Certificates/X509Certificate2.cs Co-authored-by: Kevin Gosse <[email protected]> Co-authored-by: Kevin Gosse <[email protected]>
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! |
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! |
@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? |
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. |
Ah I missed that aspect |
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
The text was updated successfully, but these errors were encountered: