CA1823 Avoid unused private fields by elachlan · Pull Request #7180 · dotnet/msbuild

elachlan · 2021-12-30T02:04:14Z

Relates to #7174

src/Build/Collections/RetrievableEntryHashSet/HashSet.cs

elachlan · 2021-12-30T02:44:40Z

I am not sure on the test failure. It seems unrelated.

Stack trace

Shouldly.ShouldAssertException : _httpListenerThreadException\n    should be null but was\nSystem.Net.HttpListenerException (0x80004005): The process cannot access the file because it is being used by another process\r\n   at System.Net.HttpListener.AddAllPrefixes()\r\n   at System.Net.HttpListener.Start()\r\n   at Microsoft.Build.UnitTests.Evaluation.Evaluator_Tests.HttpServerThread() in D:\a\1\s\src\Build.UnitTests\Evaluation\Evaluator_Tests.cs:line 4843
   at Shouldly.ShouldBeNullExtensions.ShouldBeNull[T](T actual, Func`1 customMessage)
   at Shouldly.ShouldBeNullExtensions.ShouldBeNull[T](T actual)
   at Microsoft.Build.UnitTests.Evaluation.Evaluator_Tests.VerifyDTDProcessingIsDisabled2() in D:\a\1\s\src\Build.UnitTests\Evaluation\Evaluator_Tests.cs:line 4288

Forgind · 2021-12-30T15:04:57Z

src/Shared/UnitTests/EngineTestEnvironment.cs

    public partial class TestEnvironment
    {
        // reset the default build manager and the state it might have accumulated from other tests
+#pragma warning disable CA1823 // Avoid unused private fields


I think you can just delete this? It looks legitimately unused.

I thought that too, but I think the constructor calls to create a new singleton. Which then resets some things. Its very strangely structured.

I tried deleting it and running tests, and it passed tests. Evidence that it isn't used but not definitive.

The constructor refers to SingletonField and by reflection to s_singletonInstance on BuildManager, but I didn't see a reference to _resetBuildManager.

I actually have no idea. I felt it erred on the side of caution and used a suppression instead of deleting all the code.

So what I think is happening, is that the TestEnvironment class is used in different tests. When it is instantiated, it makes a call to the DefaultBuildManager to clean-up the environment.

This seems strange, because I would have thought this would have been done on dispose for the TestEnvironment since it is utilised in a using pattern.

The comment explains it best:
// reset the default build manager and the state it might have accumulated from other tests

I get it. I guess the idea was that if the environment was corrupted (i.e., not used in a using pattern, there are bugs, etc.), someone might go down a rabbit hole trying to figure out why. Worse, it would only happen if the tests were run in a particular order. I might claim that this then hides potential serious problems, but I guess it's fine to leave it in.

It should be safe to put

_ = new ResetDefaultBuildManager();

in the constructor and delete the field.

@ladipro
The constructor is here:

msbuild/src/Shared/UnitTests/TestEnvironment.cs

Lines 58 to 63 in 597d77e

private TestEnvironment(ITestOutputHelper output)

{

Output = output;

_defaultTestDirectory = new Lazy<TransientTestFolder>(() => CreateFolder());

SetDefaultInvariant();

}

But if I add it here, it cannot find the class when in the Microsoft.Build.Framework.UnitTests project. I am not sure how to get around this. Maybe a #if but if its used somewhere else it would have to be updated. So its not an elegant solution.

I don't suppose EngineTestEnvironment wasn't included in Microsoft.Build.Framework.UnitTests by accident?

Ah, I see, let's leave as is then. Maybe the better way to structure the code would be to make the Engine TestEnvironment derive from the base TestEnvironment instead of using partial classes but definitely out of scope here. Apologies for the randomization.

src/MSBuild.UnitTests/PerfLog_Tests.cs

src/Build/Collections/RetrievableEntryHashSet/HashSet.cs

Forgind · 2021-12-30T15:10:34Z

I am not sure on the test failure. It seems unrelated.

Stack trace

Shouldly.ShouldAssertException : _httpListenerThreadException\n    should be null but was\nSystem.Net.HttpListenerException (0x80004005): The process cannot access the file because it is being used by another process\r\n   at System.Net.HttpListener.AddAllPrefixes()\r\n   at System.Net.HttpListener.Start()\r\n   at Microsoft.Build.UnitTests.Evaluation.Evaluator_Tests.HttpServerThread() in D:\a\1\s\src\Build.UnitTests\Evaluation\Evaluator_Tests.cs:line 4843
   at Shouldly.ShouldBeNullExtensions.ShouldBeNull[T](T actual, Func`1 customMessage)
   at Shouldly.ShouldBeNullExtensions.ShouldBeNull[T](T actual)
   at Microsoft.Build.UnitTests.Evaluation.Evaluator_Tests.VerifyDTDProcessingIsDisabled2() in D:\a\1\s\src\Build.UnitTests\Evaluation\Evaluator_Tests.cs:line 4288

It probably is—if it'll rerun if we push any more changes, or I can rerun it manually if not.

…d it to its own block. This way we avoid the Analyzer error and keeps the code logically grouped.

elachlan · 2021-12-30T23:55:53Z

@Forgind happy with these changes? When the team comes back they can have a look at the #if NEVER blocks and remove them if they want.

Forgind · 2021-12-31T01:04:11Z

@Forgind happy with these changes? When the team comes back they can have a look at the #if NEVER blocks and remove them if they want.

I'll be happy if we delete all the #if NEVER blocks 🙂 but if there's a good reason for them to exist, I'll accept it. Otherwise 👍

elachlan · 2022-01-10T22:28:54Z

So should I remove the #if NEVER blocks?

Forgind · 2022-01-10T23:50:30Z

So should I remove the #if NEVER blocks?

Sorry I got distracted—answer seems to be that we should avoid touching that file—and that was the point of the #if NEVER blocks. They aren't used, but if we need to merge in more changes from the original version of that file at some point, we want it as similar as possible without adding extra gunk to MSBuild assemblies. Adding #if NEVER means nothing unnecessary is added but also doesn't change the file very much. I'd suggest avoiding any changes to the file at all, sadly.

elachlan · 2022-01-10T23:54:02Z

So should I remove the #if NEVER blocks?

Sorry I got distracted—answer seems to be that we should avoid touching that file—and that was the point of the #if NEVER blocks. They aren't used, but if we need to merge in more changes from the original version of that file at some point, we want it as similar as possible without adding extra gunk to MSBuild assemblies. Adding #if NEVER means nothing unnecessary is added but also doesn't change the file very much. I'd suggest avoiding any changes to the file at all, sadly.

Not a problem. I have left it alone. Should we be okay for merge?

Forgind

LGTM. The changes to HashSet fall under "unnecessary methods and attributes if-deffed out," so I don't think the comment needs changing.

src/Tasks/ManifestUtil/Util.cs

ladipro

Some of the unused fields may serve the documentation purpose and the const ones have no run-time perf impact altogether. I believe enabling the rule is goodness, though. I've left one inline comment.

ladipro · 2022-01-19T11:06:14Z

src/Shared/UnitTests/EngineTestEnvironment.cs

    public partial class TestEnvironment
    {
        // reset the default build manager and the state it might have accumulated from other tests
+#pragma warning disable CA1823 // Avoid unused private fields


It should be safe to put

_ = new ResetDefaultBuildManager();

in the constructor and delete the field.

danmoseley · 2022-01-26T15:21:09Z

Not on the team, but I created those blocks/copied HashSet originally: I don't think they are worth keeping. They reflect the sources in .NET Framework, sources which no longer change. There have indeed been improvements to HashSet, but in .NET Core, where the code is changed enough there that diffing would not work well and changes would need to be ported by hand anyway.

danmoseley · 2022-01-26T17:00:13Z

@Forgind I opened #7340 for that suggestion.

CA1823 Avoid unused private fields

c8cf0d7

elachlan commented Dec 30, 2021

View reviewed changes

src/Build/Collections/RetrievableEntryHashSet/HashSet.cs Show resolved Hide resolved

Forgind reviewed Dec 30, 2021

View reviewed changes

elachlan added 2 commits December 31, 2021 07:32

CA1823 Remove MSBuildExeName from PerfLogTests due to analyzer

6d78c96

StackAllocThreshold is only used in the other #if NEVER blocks. So ad…

024a6d7

…d it to its own block. This way we avoid the Analyzer error and keeps the code logically grouped.

elachlan mentioned this pull request Jan 1, 2022

Change CodeAnalysis rules from 'Info' to 'Warning' after fixing all instances of the violations #7174

Open

elachlan added 5 commits January 8, 2022 10:28

Revert CodeAnalysis.ruleset

3d29bf9

merge master

f945884

enable warning on CA1823

8b42f67

Fix remaining occurrences of CA1823 violations

956e47c

Fix remaining occurrences of CA1823 violations

1d848f9

elachlan requested a review from Forgind January 18, 2022 22:56

Forgind approved these changes Jan 18, 2022

View reviewed changes

src/Tasks/ManifestUtil/Util.cs Outdated Show resolved Hide resolved

Change clrVersion ordering

4d1a1cd

ladipro reviewed Jan 19, 2022

View reviewed changes

ladipro approved these changes Jan 26, 2022

View reviewed changes

ladipro added the merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. label Jan 26, 2022

ladipro merged commit 96d7bbc into dotnet:main Jan 26, 2022

elachlan deleted the CA1823 branch January 26, 2022 21:45

	private TestEnvironment(ITestOutputHelper output)
	{
	Output = output;
	_defaultTestDirectory = new Lazy<TransientTestFolder>(() => CreateFolder());
	SetDefaultInvariant();
	}

Conversation

elachlan commented Dec 30, 2021

Uh oh!

Uh oh!

elachlan commented Dec 30, 2021

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!

Uh oh!

Uh oh!

Forgind commented Dec 30, 2021

Uh oh!

elachlan commented Dec 30, 2021

Uh oh!

Forgind commented Dec 31, 2021

Uh oh!

elachlan commented Jan 10, 2022

Uh oh!

Forgind commented Jan 10, 2022

Uh oh!

elachlan commented Jan 10, 2022

Uh oh!

Forgind left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ladipro left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danmoseley commented Jan 26, 2022

Uh oh!

danmoseley commented Jan 26, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants