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

zebmason · 2017-04-14T20:24:50Z

…vate.Xml.sln

N.B. This is purely for code review purposes. Let me know if this is useful as I did a rather large commit for Roslyn using the same tool - dotnet/roslyn#17630

…vate.Xml.sln N.B. This is purely for code review

stephentoub · 2017-04-14T21:14:18Z

src/System.Runtime.Serialization.Xml/tests/SerializationTypes.cs

@@ -1150,7 +1150,7 @@ public class DerivedTypeWithDifferentOverrides : BaseType

        new public string Name3 { get; set; }

-        new internal string Name4 { get; set; }
+//        new internal string Name4 { get; set; }


I expect this is the likely cause of all of the build failures.

stephentoub · 2017-04-14T21:14:55Z

src/System.Private.Xml/tests/Xslt/XslTransformApi/DataHelper.cs

+//    private String DefaultFunction()
+//    {
+//        return "Default Function";
+//    }


Typically functions like this are used via reflection. Do we know they're not being used?

Unless there is code coverage anything accessed via reflection will be missed

stephentoub · 2017-04-14T21:15:27Z

src/System.Private.Xml/tests/XmlSerializer/XmlSerializerTests.cs

-        Assert.True(Enumerable.SequenceEqual(value.HexBinaryContent, actual.HexBinaryContent), "Actual HexBinaryContent was not as expected.");
-    }
+//    [Fact]
+//    static void Xml_TypeWithTypesHavingCustomFormatter()


Looks like this test should have been public and should be changed to be public rather than being deleted.

stephentoub · 2017-04-14T21:16:53Z

src/System.Private.Xml/src/System/Xml/XPath/Internal/XPathArrayIterator.cs

@@ -85,6 +85,6 @@ public override IEnumerator GetEnumerator()
            return list.GetEnumerator();
        }

-        private object debuggerDisplayProxy { get { return index < 1 ? null : (object)new XPathNavigator.DebuggerDisplayProxy(Current); } }
+//        private object debuggerDisplayProxy { get { return index < 1 ? null : (object)new XPathNavigator.DebuggerDisplayProxy(Current); } }


This is being used, by the DebuggerDisplay attribute. Please be very careful about deleting things like this. We should really have tests that validate such attributes, as we do in other projects.

cc: @sepidehms

Found similar issues when I deleted 18.5k lines of Roslyn which is why I flagged the branch as for review only. The other issues with Roslyn were regression (code should have been called), partial implementation (specifically of tests), non-conditional compilation.

stephentoub · 2017-04-14T21:17:32Z

src/System.Private.Xml/src/System/Xml/Serialization/XmlSerializer.cs

-                return Mode == SerializationMode.ReflectionOnly || Mode == SerializationMode.ReflectionAsBackup;
-            }
-        }
+//        private static bool ReflectionMethodEnabled


The name of this suggests it might be used by code somewhere via reflection. Do we know it's not?

stephentoub · 2017-04-14T21:19:30Z

Thanks, @zebmason. I'll let @krwq, @sepidehms, and @shmao comment on whether this is something we'd like to do. In general, we're happy to see dead code removed, but if we do go ahead with it, we'd want to see it deleted rather than just commented out. We also need to be very careful about the deletions; some of them are actually not dead, and deleting them will result in problems, e.g. private members used by a debugger via reflection for debugger display attributes.

danmoseley · 2017-04-14T22:55:08Z

@zebmason with any change it woudl also be good to build a few configurations locally (eg at least windows) before putting a PR up. :)

karelz · 2017-04-15T00:26:36Z

@zebmason if you want to help us clean up some dead code, we have a tracking issue with action items here: #17905

Also please check out #17619 how best to help. Our contribution guidance is on our main page.
Don't hesitate to ask if you're not sure about something, or if you need help. We'll be happy to help!
Thanks for your contribution!

zebmason · 2017-04-15T07:54:57Z

Thanks I'll try to get this cleaned up next week. Unfortunately I've been nursing my build machine back to health over the last fortnight but I'm still switching to safe mode too often which kills my wi-fi and thus a lot of the tests. I had noticed that at the time of forking that not all tests were passing so I was more cavalier than normal.

The tool I've written which detects the dead code is still producing too many false positives and has absolutely no idea when it comes to reflection. So the process is run - build/test - revert - build/test - revert...

zebmason · 2017-04-17T11:52:45Z

I've removed commenting out from a test that was erroneously removed by my tool
I've deleted all the code that was commented out so that a merge can take place
Now a Linux test fails which is confusing me

My motivation is the development of a tool that I'm currently working on. I'm happy to submit further changes to this project if you find that those changes are useful. Note that my build machine is still somewhat ill and thus a pull request may unfortunately cause tests to fail, which I will then try to address.

When I was requested to split a large pull request for msbuild (so that it could be dealt with within that teams process) I instead offered to licence the tool to Microsoft but that was declined. i.e. I have no desire to do re-do grunt work. If you would like a beta version of the tool this is possible.

stephentoub · 2017-04-17T13:09:19Z

Now a Linux test fails which is confusing me

This is unrelated to your change: #18431

stephentoub · 2017-04-17T13:10:11Z

src/System.Private.Xml/src/System/Xml/Dom/XPathNodeList.cs

@@ -1,4 +1,4 @@
-// Licensed to the .NET Foundation under one or more agreements.
+// Licensed to the .NET Foundation under one or more agreements.


What's changing here and other places where the first line is showing as modified? Is the file's BOM changing?

It could be, my code optionally re-writes the files to make the lines commented out (will change to deleted out during the week). I did copy some example C# code from a Microsoft help page on how to detect encoding but couldn't see anything about the BOM and wondered at the time what would happen. Suppose it could also be the line ending but obviously it happens in only one place. Time for a binary diff.

After processing the file is now pre-pended with EF BB BF. i.e. the BOM for UTF-8 which was not there before.

stephentoub · 2017-04-17T13:11:48Z

src/System.Private.Xml/src/System/Xml/Serialization/Compiler.cs

@@ -27,9 +27,6 @@ namespace System.Xml.Serialization

    internal class Compiler
    {
-#if !XMLSERIALIZERGENERATOR
-        private bool _debugEnabled = DiagnosticsSwitches.KeepTempFiles.Enabled;
-#endif


@krwq, @sepidehms, are there any builds where we don't set XMLSERIALIZERGENERATOR? I'm wondering if we could remove the define across the codebase.

The tool takes in a series of defines so I never set this hence why it was detected as unused.

@stephentoub XMLSERIALIZERGENERATOR is used when we build SGen on core, Microsoft.XmlSerializer.Generaton .

We don't set XMLSERIALIZERGENERATOR in any of the System.Private.Xml builds. Some of the Xml code is being referenced by Microsoft.XmlSerializer.Generator and that contract sets this define. In this case, looks like System.Private.Xml is no longer using this bool and it can be removed.

@sepidehms Thanks, I think this means that the code is shared between projects and thus my tool is operating in an unsafe manner and shouldn't have detected this. i.e. I have some work to do

stephentoub · 2017-04-17T13:12:43Z

src/System.Private.Xml/src/System/Xml/XPath/Internal/XPathArrayIterator.cs

@@ -84,7 +84,5 @@ public override IEnumerator GetEnumerator()
        {
            return list.GetEnumerator();
        }
-
-        private object debuggerDisplayProxy { get { return index < 1 ? null : (object)new XPathNavigator.DebuggerDisplayProxy(Current); } }


As I previously commented, this must not be deleted. It's being used by the debugger display attribute... see the attribute on the type declaration.

Sorry I meant to add to my main message that the merge should be done by someone who knows what they are doing, hence why I left in such things to ensure that it would flag as a bad merge

How does removing this flag it as a bad merge?

I'm unclear as to the purpose of this PR. Did you intend to get it into a state where we could simply hit the Merge button in GitHub? Or was your goal here just to highlight code that could potentially be deleted but expect this PR to be closed rather than merged?

Thanks.

The purpose of the PR was for a review rather than a merge as I don't have the background to determine whether something is truly unused. i.e. due to regressions, partially implemented code, used only in the debugger, etc.

So I expected someone with the appropriate competence to fork my branch, review and revert as appropriate then merge. Does that sound sensible?

@zebmason got it. I think probably the most likely way to take such changes would be for you to open an issue pointing at your tool. We can have a list of libraries with a checkbox for each, simliar to https://github.com/dotnet/corefx/issues/17905. Then anyone interested can try it out and clean up a PR for a library.

If your tool could read the existing ILLinkTrim.xml files that would make it easier as those could be used to help avoid false positives. if it makes sense, flags on the tool might make it easier to select modes that are more "reliable"

Speaking for the full time contributors on this project, we'd certainly like to remove dead code where it exists but after the recent cleanup of the worst cases it's not something we're likely to get to anytime soon. So setting up an issue for community folk to look at if interested would be best. They are likely to start there rather than volunteer to clean up a PR from someone else.

Does that sound reasonable?

Also as you see in the others issue it would be very handy if it could find dead shared code across assemblies as ILLink cannot.

BTW please don't take this as discouragement... it would certainly help the project to remove more dead code. We'd welcome help doing that.

danmoseley · 2017-04-22T05:35:46Z

I'm going to close as we don't want to continue with this PR as is. I certainly encourage contributions as suggested above. Thanks for this.

Comment out apparently dead code in src\System.Private.Xml\System.Pri…

965a541

…vate.Xml.sln N.B. This is purely for code review

dnfclas added the cla-already-signed label Apr 14, 2017

stephentoub reviewed Apr 14, 2017

View reviewed changes

karelz added the area-System.Xml label Apr 15, 2017

karelz assigned zebmason, krwq and sepidehkh Apr 15, 2017

Revert 2 changes which my build machine should have found to be buggy

6176881

stephentoub added the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Apr 15, 2017

zebmason added 2 commits April 17, 2017 11:30

Uncomment out test

41fe859

Delete commented out code

6a9116e

stephentoub removed the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Apr 17, 2017

stephentoub reviewed Apr 17, 2017

View reviewed changes

danmoseley closed this Apr 22, 2017

karelz modified the milestone: 2.0.0 Apr 23, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

zebmason commented Apr 14, 2017

stephentoub Apr 14, 2017

stephentoub Apr 14, 2017

zebmason Apr 15, 2017

stephentoub Apr 14, 2017

sepidehkh Apr 17, 2017

stephentoub Apr 14, 2017

krwq Apr 14, 2017

zebmason Apr 15, 2017

stephentoub Apr 14, 2017

stephentoub commented Apr 14, 2017

danmoseley commented Apr 14, 2017

karelz commented Apr 15, 2017

zebmason commented Apr 15, 2017

zebmason commented Apr 17, 2017

stephentoub commented Apr 17, 2017

stephentoub Apr 17, 2017

zebmason Apr 17, 2017

zebmason Apr 18, 2017

stephentoub Apr 17, 2017

zebmason Apr 17, 2017

shmao Apr 17, 2017

sepidehkh Apr 17, 2017 •

edited

Loading

zebmason Apr 17, 2017

stephentoub Apr 17, 2017

zebmason Apr 17, 2017

stephentoub Apr 17, 2017

zebmason Apr 17, 2017

danmoseley Apr 20, 2017

danmoseley Apr 20, 2017

danmoseley Apr 20, 2017

danmoseley commented Apr 22, 2017

		@@ -1,4 +1,4 @@
		// Licensed to the .NET Foundation under one or more agreements.
		// Licensed to the .NET Foundation under one or more agreements.

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

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

Conversation

zebmason commented Apr 14, 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

stephentoub commented Apr 14, 2017

danmoseley commented Apr 14, 2017

karelz commented Apr 15, 2017

zebmason commented Apr 15, 2017

zebmason commented Apr 17, 2017

stephentoub commented Apr 17, 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

sepidehkh Apr 17, 2017 • edited Loading

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

danmoseley commented Apr 22, 2017

sepidehkh Apr 17, 2017 •

edited

Loading