Partial results of running LOC8 to identify dead code. #17630

zebmason · 2017-03-08T17:56:42Z

You'll want to review these changes to ensure the code identified as dead is actually dead. Some of it looks like it is there for debug purposes.

The tool used to find the "dead" code is currently under development with a log at https://github.com/zebmason/LOC8. More work is needed on the tool to avoid false positives. If you don't find this useful let me know and I'll train the tool on other code.

…ant to actually delete this code but wrap with #if DEBUG, etc.

dnfclas · 2017-03-08T17:56:46Z

@zebmason,
Thanks for your contribution.
To ensure that the project team has proper rights to use your work, please complete the Contribution License Agreement at https://cla2.dotnetfoundation.org.

It will cover your contributions to all Microsoft-managed open source projects.
Thanks,
.NET Foundation Pull Request Bot

jasonmalinowski · 2017-03-08T19:13:13Z

src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerImageReference.cs

@@ -65,27 +65,5 @@ public override object Id
                return _id;
            }
        }
-
-        private string GetDebuggerDisplay()


@zebmason This particular one is being used: the DebuggerDisplay attribute on the class will make this one matter.

jasonmalinowski

I did see one false positive here.

jasonmalinowski · 2017-03-08T19:14:31Z

Tagging @dotnet/roslyn-compiler, @dotnet/roslyn-ide and @dotnet/roslyn-analysis since some of these do look quite legitimate.

heejaechang · 2017-03-08T20:17:01Z

@mavasani can you take a look?

mavasani · 2017-03-08T21:34:04Z

src/Compilers/Core/Portable/DiagnosticAnalyzer/CompilationWithAnalyzers.cs

@@ -137,13 +137,6 @@ private CompilationWithAnalyzers(Compilation compilation, ImmutableArray<Diagnos
            _executingCompilationOrNonConcurrentTreeTask = null;
        }

-        private void AddExceptionDiagnostic(Exception exception, DiagnosticAnalyzer analyzer, Diagnostic diagnostic)


So this seems to have uncovered a regression. This method should be hooked up in the constructor of CompilationWithAnalyzers and invoked when there is an exception from any analyzer. It seems this got regressed when we moved to AnalyzerDriver v2 in Dev14 Update3. Can you please retain this method and file a separate tracking issue to fix this and assign it to me?

tannergooding · 2017-03-10T18:43:45Z

test windows_debug_vs-integration_prtest please

tannergooding · 2017-03-10T18:43:51Z

test windows_release_vs-integration_prtest please

jcouv · 2017-03-10T18:57:16Z

src/Compilers/Core/CodeAnalysisTest/Resources.Designer.cs

-        /// <summary>
-        ///   Looks up a localized resource of type System.Byte[].
-        /// </summary>
-        internal static byte[] defaultWin32Manifest {


I don't see the corresponding change to resx file (which generates this cs file).

heejaechang · 2017-03-10T21:57:20Z

src/Features/Core/Portable/Diagnostics/AnalyzerHelper.cs

@@ -29,7 +29,6 @@ internal static class AnalyzerHelper

        // Shared with Compiler
        internal const string AnalyzerExceptionDiagnosticId = "AD0001";
-        internal const string AnalyzerDriverExceptionDiagnosticId = "AD0002";


this should be left since it is supposed to be in sync with the one defined in compiler side.

# Conflicts: # src/Features/CSharp/Portable/InlineDeclaration/CSharpInlineDeclarationCodeFixProvider.cs

jcouv · 2017-03-13T23:30:13Z

src/Compilers/CSharp/Portable/Binder/Binder.cs

@@ -776,66 +776,5 @@ internal BoundStatement WrapWithVariablesAndLocalFunctionsIfAny(CSharpSyntaxNode
                                  ImmutableArray.Create(statement))
            { WasCompilerGenerated = true };
        }
-
-        internal string Dump()


This is not dead code. We use it for debugging.

I was being a bit ruthless. A lot of the code which appears designed to be run in the debugger don't have #if DEBUG guards, perhaps they should?

No. We still run things in the debugger when in release mode. Now, maybe we should introduce an attribute to call out that this is intentional and is not dead code. Then your tool could skip those guys.

Cyrus

Thanks for the feedback, the tool currently ignores build flavours and thus defines. Based on your comments I need to modify it so that it uses a tool specific build flavour in which all the relevant defines are specified.

Cheers

Zeb

jcouv · 2017-03-13T23:30:26Z

src/Compilers/CSharp/Portable/Binder/Binder_Deconstruct.cs

@@ -379,14 +379,6 @@ internal static void FreeDeconstructionVariables(ArrayBuilder<DeconstructionVari

                variables.Free();
            }
-            private string GetDebuggerDisplay()


jcouv · 2017-03-13T23:31:33Z

src/Compilers/CSharp/Portable/Parser/Directives.cs

@@ -62,14 +62,6 @@ public bool IncrementallyEquivalent(Directive other)
            }
        }

-        // Can't be private as it's called by DirectiveStack in its GetDebuggerDisplay()
-        internal string GetDebuggerDisplay()


None of the GetDebbuggerDisplay methods are dead code.

CyrusNajmabadi · 2017-03-13T23:34:40Z

src/EditorFeatures/CSharp/NavigationBar/CSharpNavigationBarItemService.cs

@@ -346,12 +346,6 @@ private static void ValidateSpanFromBounds(ITextSnapshot snapshot, int start, in
            Contract.Requires(start >= 0 && end <= snapshot.Length && start <= end);
        }

-        [Conditional("DEBUG")]
-        private static void ValidateSpan(ITextSnapshot snapshot, int start, int length)


Are you testing with debug built?

Yes I built with debug, the analysis tool used needs a little work on it to select build flavours to ensure that when it uses Roslyn to create the semantic model that all the correct options are set.

CyrusNajmabadi · 2017-03-13T23:35:23Z

src/EditorFeatures/CSharpTest/KeywordHighlighting/IfStatementHighlighterTests.cs

@@ -131,28 +131,6 @@ public void Foo()
 }");
        }

-        private const string Code3 = @"


these worry me. t hey seem liek missed tests.

CyrusNajmabadi · 2017-03-13T23:37:13Z

-18k lines is massive. We're going to need this broken up. perhaps it can be done so by grouping into folders.

jaredpar

All of the GetDebuggerDisplay removals are false positives. They are referenced on the type attributes and used at debug time. Can't delete those.

jaredpar · 2017-03-13T23:54:11Z

src/Compilers/CSharp/Portable/Binder/ImportChain.cs

-        {
-            return $"{Imports.GetDebuggerDisplay()} ^ {ParentOpt?.GetHashCode() ?? 0}";
-        }
-


This is not dead code. It's referenced in the type attribute and used at debug time. Same for every other GetDebuggerDisplay that is referenced.

CyrusNajmabadi · 2017-03-13T23:56:46Z

src/Compilers/CSharp/Portable/CSharpResources.Designer.cs

@@ -98,14026 +98,317 @@ internal class CSharpResources {
        }

        /// <summary>
-        ///   Looks up a localized string similar to Elements cannot be null..


This file has like 13k deletes. Is that really supposed to be happening? Also, we may want to just have a Pr that is the designer resources cleanup.

zebmason · 2017-03-14T00:10:19Z

I've got the number of false positives produced by my tool down to less than 1700, still a lot to wade through and I can see some extra tests that I need to write. I've been a bit more brutal than before and deleted lots of code which appears to exist for use within the debugger - where it is marked as debug I've added #if DEBUG guards.

Obviously there are several possible reasons for the code that I've identified as being dead, examples being

It is there for debugging purposes
I didn't compile with the correct option to activate that code
It flags a regression
The code is partially completed and a subsequent commit will ensure that the dead code gets used
It wasn't tidied up after a commit

My tool uses Roslyn to parse the C# AST so I thought I might contribute something back by way of thanks. Hopefully the noise is minimal and there are some useful issues to address. I'll be quiet for a while now as I doubt that I've missed many members that my tool classifies as dead whilst I endeavour to productionise it for shipping.

heejaechang · 2017-03-14T00:18:15Z

@zebmason can this tool written as diagnostic analyzer? that probably will make dead code to be removed much sooner.

zebmason · 2017-03-14T08:33:09Z

@heejaechang the tool is currently written as a console application which is added as a pre-build step so that the warnings are printed to the output window to be clicked. It was my intention that once stabilised I turn it into a VS Extension and then sell it through VS Marketplace, however that is not currently a possibility. Apart from using Roslyn to parse C# (VB needs to be worked on) it uses libclang to parse C++ and its main purpose is for reviewing Lack of Cohesion problems - the dead code checking was only intended as a side function and I've been using it to verify that my C# parsing is correct.

Incidentally I've used it to refactor my own C# and C++ code - given that I was the only one to write that code it was embarrassing to find dead code and interesting to see where my design choices had reduced cohesion. On the latter point I've actually chopped up some classes so they make more sense and are more flexible.

CyrusNajmabadi · 2022-06-03T17:19:22Z

Closing out. It's not feasible to take this in in the current state (esp with all the conflicts now). This is definitely an interesting thing to look at, but future PRs will have to be much smaller in scope (or broken into clear and easy commits to look at) to ensure that this is safe.

Partial results of running LOC8 to identify dead code. Note may not w…

d82fb6a

…ant to actually delete this code but wrap with #if DEBUG, etc.

dnfclas added cla-required and removed cla-required labels Mar 8, 2017

jasonmalinowski reviewed Mar 8, 2017

View reviewed changes

jasonmalinowski requested changes Mar 8, 2017

View reviewed changes

mavasani reviewed Mar 8, 2017

View reviewed changes

Merge branch 'master' into LOC8

acab4de

jcouv reviewed Mar 10, 2017

View reviewed changes

heejaechang reviewed Mar 10, 2017

View reviewed changes

zebmason added 4 commits March 12, 2017 18:26

Remove more dead code

11130fb

Merge branch 'master' into LOC8

26eb292

Remove more dead code

b7a99e0

Merge branch 'master' into LOC8

6bb28eb

# Conflicts: # src/Features/CSharp/Portable/InlineDeclaration/CSharpInlineDeclarationCodeFixProvider.cs

jcouv reviewed Mar 13, 2017

View reviewed changes

CyrusNajmabadi reviewed Mar 13, 2017

View reviewed changes

jaredpar requested changes Mar 13, 2017

View reviewed changes

CyrusNajmabadi reviewed Mar 13, 2017

View reviewed changes

zebmason mentioned this pull request Mar 27, 2017

Remove deadcode dotnet/msbuild#1905

Closed

zebmason mentioned this pull request Apr 14, 2017

Comment out apparently dead code in src\System.Private.Xml\System.Pri… dotnet/corefx#18395

Closed

jongalloway removed the cla-signed label Oct 31, 2017

dotnet deleted a comment from dnfclas Nov 1, 2017

jcouv added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Mar 31, 2018

Base automatically changed from master to main March 3, 2021 23:51

jinujoseph requested review from a team as code owners March 3, 2021 23:51

RikkiGibson mentioned this pull request Apr 20, 2022

Remove unused method Binder.CheckViability<TSymbol>() #60865

Merged

CyrusNajmabadi closed this Jun 3, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Partial results of running LOC8 to identify dead code. #17630

Partial results of running LOC8 to identify dead code. #17630

zebmason commented Mar 8, 2017

dnfclas commented Mar 8, 2017

jasonmalinowski Mar 8, 2017

jasonmalinowski left a comment

jasonmalinowski commented Mar 8, 2017

heejaechang commented Mar 8, 2017

mavasani Mar 8, 2017

tannergooding commented Mar 10, 2017

tannergooding commented Mar 10, 2017

jcouv Mar 10, 2017

heejaechang Mar 10, 2017

jcouv Mar 13, 2017

zebmason Mar 13, 2017

CyrusNajmabadi Mar 13, 2017

zebmason Mar 15, 2017

jcouv Mar 13, 2017

jcouv Mar 13, 2017

CyrusNajmabadi Mar 13, 2017

zebmason Mar 13, 2017

CyrusNajmabadi Mar 13, 2017

CyrusNajmabadi commented Mar 13, 2017

jaredpar left a comment

jaredpar Mar 13, 2017

CyrusNajmabadi Mar 13, 2017

zebmason commented Mar 14, 2017

heejaechang commented Mar 14, 2017

zebmason commented Mar 14, 2017

CyrusNajmabadi commented Jun 3, 2022

Partial results of running LOC8 to identify dead code. #17630

Partial results of running LOC8 to identify dead code. #17630

Conversation

zebmason commented Mar 8, 2017

dnfclas commented Mar 8, 2017

Choose a reason for hiding this comment

jasonmalinowski left a comment

Choose a reason for hiding this comment

jasonmalinowski commented Mar 8, 2017

heejaechang commented Mar 8, 2017

Choose a reason for hiding this comment

tannergooding commented Mar 10, 2017

tannergooding commented Mar 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi commented Mar 13, 2017

jaredpar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zebmason commented Mar 14, 2017

heejaechang commented Mar 14, 2017

zebmason commented Mar 14, 2017

CyrusNajmabadi commented Jun 3, 2022