Skip to content
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

CA1823 Avoid unused private fields #7180

Merged
merged 9 commits into from
Jan 26, 2022
Merged
2 changes: 1 addition & 1 deletion eng/Common.globalconfig
Original file line number Diff line number Diff line change
Expand Up @@ -277,7 +277,7 @@ dotnet_diagnostic.CA1821.severity = warning
dotnet_diagnostic.CA1822.severity = none

# Avoid unused private fields
dotnet_diagnostic.CA1823.severity = suggestion
dotnet_diagnostic.CA1823.severity = warning

# Mark assemblies with NeutralResourcesLanguageAttribute
dotnet_diagnostic.CA1824.severity = warning
Expand Down
32 changes: 17 additions & 15 deletions src/Build/Collections/RetrievableEntryHashSet/HashSet.cs
Original file line number Diff line number Diff line change
Expand Up @@ -93,8 +93,10 @@ internal class RetrievableEntryHashSet<T> : ICollection<T>,
{
// store lower 31 bits of hash code
private const int Lower31BitMask = 0x7FFFFFFF;
#if NEVER
// cutoff point, above which we won't do stackallocs. This corresponds to 100 integers.
private const int StackAllocThreshold = 100;
#endif
// when constructing a hashset from an existing collection, it may contain duplicates,
// so this is used as the max acceptable excess ratio of capacity to count. Note that
// this is only used on the ctor and not to automatically shrink if the hashset has, e.g,
Expand All @@ -121,7 +123,7 @@ internal class RetrievableEntryHashSet<T> : ICollection<T>,
// temporary variable needed during deserialization
private SerializationInfo _siInfo;

#region Constructors
#region Constructors

public RetrievableEntryHashSet(IEqualityComparer<string> comparer)
{
Expand Down Expand Up @@ -204,7 +206,7 @@ protected RetrievableEntryHashSet(SerializationInfo info, StreamingContext conte
_siInfo = info;
}

#endregion
#endregion

// Convenience to minimise change to callers used to dictionaries
public ICollection<string> Keys
Expand All @@ -230,7 +232,7 @@ public ICollection<T> Values
get { return this; }
}

#region ICollection<T> methods
#region ICollection<T> methods

// Convenience to minimise change to callers used to dictionaries
internal T this[string name]
Expand Down Expand Up @@ -481,9 +483,9 @@ internal void MakeReadOnly()
_readOnly = true;
}

#endregion
#endregion

#region IEnumerable methods
#region IEnumerable methods

public Enumerator GetEnumerator()
{
Expand All @@ -508,9 +510,9 @@ IEnumerator IEnumerable.GetEnumerator()
return new Enumerator(this);
}

#endregion
#endregion

#region ISerializable methods
#region ISerializable methods

// [SecurityPermissionAttribute(SecurityAction.LinkDemand, Flags = SecurityPermissionFlag.SerializationFormatter)]
[SecurityCritical]
Expand All @@ -533,9 +535,9 @@ public virtual void GetObjectData(SerializationInfo info, StreamingContext conte
}
}

#endregion
#endregion

#region IDeserializationCallback methods
#region IDeserializationCallback methods

public virtual void OnDeserialization(Object sender)
{
Expand Down Expand Up @@ -580,9 +582,9 @@ public virtual void OnDeserialization(Object sender)
_siInfo = null;
}

#endregion
#endregion

#region HashSet methods
#region HashSet methods

/// <summary>
/// Add item to this HashSet.
Expand Down Expand Up @@ -630,7 +632,7 @@ public void UnionWith(IEnumerable<T> other)
}
}

#if NEVER
#if NEVER
/// <summary>
/// Takes the intersection of this set with other. Modifies this set.
///
Expand Down Expand Up @@ -1152,9 +1154,9 @@ public static IEqualityComparer<RetrievableEntryHashSet<T>> CreateSetComparer()
#endif
#endif

#endregion
#endregion

#region Helper methods
#region Helper methods

/// <summary>
/// Initializes buckets and slots arrays. Uses suggested capacity by finding next prime
Expand Down Expand Up @@ -1723,7 +1725,7 @@ private int InternalGetHashCode(string item)
return _comparer.GetHashCode(item) & Lower31BitMask;
}

#endregion
#endregion

// used for set checking operations (using enumerables) that rely on counting
internal struct ElementCount
Expand Down
6 changes: 0 additions & 6 deletions src/MSBuild.UnitTests/PerfLog_Tests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -26,12 +26,6 @@ namespace Microsoft.Build.UnitTests
{
public class PerfLogTests
{
#if USE_MSBUILD_DLL_EXTN
private const string MSBuildExeName = "MSBuild.dll";
#else
private const string MSBuildExeName = "MSBuild.exe";
#endif

private readonly ITestOutputHelper _output;

public PerfLogTests(ITestOutputHelper output)
Expand Down
21 changes: 0 additions & 21 deletions src/Shared/FileUtilities.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1089,27 +1089,6 @@ internal static string MakeRelative(string basePath, string path)
return StringBuilderCache.GetStringAndRelease(sb);
}

/// <summary>
/// Helper function to create an Uri object from path.
/// </summary>
/// <param name="path">path string</param>
/// <returns>uri object</returns>
private static Uri CreateUriFromPath(string path)
{
ErrorUtilities.VerifyThrowArgumentLength(path, nameof(path));

Uri pathUri;

// Try absolute first, then fall back on relative, otherwise it
// makes some absolute UNC paths like (\\foo\bar) relative ...
if (!Uri.TryCreate(path, UriKind.Absolute, out pathUri))
{
pathUri = new Uri(path, UriKind.Relative);
}

return pathUri;
}

/// <summary>
/// Normalizes the path if and only if it is longer than max path,
/// or would be if rooted by the current directory.
Expand Down
2 changes: 2 additions & 0 deletions src/Shared/UnitTests/EngineTestEnvironment.cs
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,9 @@ namespace Microsoft.Build.UnitTests
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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be safe to put

_ = new ResetDefaultBuildManager();

in the constructor and delete the field.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ladipro
The constructor is here:

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

private object _resetBuildManager = new ResetDefaultBuildManager();
#pragma warning restore CA1823 // Avoid unused private fields

private class ResetDefaultBuildManager
{
Expand Down
10 changes: 0 additions & 10 deletions src/Tasks/GenerateResource.cs
Original file line number Diff line number Diff line change
Expand Up @@ -926,17 +926,7 @@ public override bool Execute()
private static bool allowMOTW;

private const string CLSID_InternetSecurityManager = "7b8a2d94-0ac9-11d1-896c-00c04fb6bfc4";

private const uint ZoneLocalMachine = 0;

private const uint ZoneIntranet = 1;

private const uint ZoneTrusted = 2;

private const uint ZoneInternet = 3;

private const uint ZoneUntrusted = 4;

private static IInternetSecurityManager internetSecurityManager = null;

// Resources can have arbitrarily serialized objects in them which can execute arbitrary code
Expand Down
2 changes: 0 additions & 2 deletions src/Tasks/LockCheck.cs
Original file line number Diff line number Diff line change
Expand Up @@ -83,8 +83,6 @@ internal struct RM_UNIQUE_PROCESS
public FILETIME ProcessStartTime;
}

const int RM_INVALID_SESSION = -1;
const int RM_INVALID_PROCESS = -1;
const int CCH_RM_MAX_APP_NAME = 255;
const int CCH_RM_MAX_SVC_NAME = 63;
const int ERROR_SEM_TIMEOUT = 121;
Expand Down
2 changes: 1 addition & 1 deletion src/Tasks/ManifestUtil/SecurityUtil.cs
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,8 @@ public static class SecurityUtilities
#if !RUNTIME_TYPE_NETCORE
private const int Fx2MajorVersion = 2;
private const int Fx3MajorVersion = 3;
#endif
private static readonly Version s_dotNet40Version = new Version("4.0");
#endif
private static readonly Version s_dotNet45Version = new Version("4.5");

#if !RUNTIME_TYPE_NETCORE
Expand Down
3 changes: 2 additions & 1 deletion src/Tasks/ManifestUtil/Util.cs
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,10 @@ internal static class Util
internal static readonly string logPath = GetLogPath();
private static readonly char[] s_fileNameInvalidChars = { '\\', '/', ':', '*', '?', '"', '<', '>', '|' };
private static StreamWriter s_logFileWriter;
#if !RUNTIME_TYPE_NETCORE
// Major, Minor, Build and Revision of CLR v2.0
private static readonly int[] s_clrVersion2 = { 2, 0, 50727, 0 };
#if RUNTIME_TYPE_NETCORE
#else
// Major, Minor, Build and Revision of CLR v4.0
private static readonly int[] s_clrVersion4 = { 4, 0, 30319, 0 };
#endif
Expand Down
17 changes: 0 additions & 17 deletions src/Tasks/ManifestUtil/mansign2.cs
Original file line number Diff line number Diff line change
Expand Up @@ -281,20 +281,6 @@ private void init()
#endif
}

private static XmlElement FindIdElement(XmlElement context, string idValue)
{
if (context == null)
return null;

XmlElement idReference = context.SelectSingleNode("//*[@Id=\"" + idValue + "\"]") as XmlElement;
if (idReference != null)
return idReference;
idReference = context.SelectSingleNode("//*[@id=\"" + idValue + "\"]") as XmlElement;
if (idReference != null)
return idReference;
return context.SelectSingleNode("//*[@ID=\"" + idValue + "\"]") as XmlElement;
}

public override XmlElement GetIdElement(XmlDocument document, string idValue)
{
// We only care about Id references inside of the KeyInfo section
Expand All @@ -320,9 +306,6 @@ internal class SignedCmiManifest2
private const string Sha1SignatureMethodUri = @"http://www.w3.org/2000/09/xmldsig#rsa-sha1";
private const string Sha1DigestMethod = @"http://www.w3.org/2000/09/xmldsig#sha1";

private const string wintrustPolicyFlagsRegPath = "Software\\Microsoft\\Windows\\CurrentVersion\\WinTrust\\Trust Providers\\Software Publishing";
private const string wintrustPolicyFlagsRegName = "State";

private SignedCmiManifest2() { }

internal SignedCmiManifest2(XmlDocument manifestDom, bool useSha256)
Expand Down
12 changes: 7 additions & 5 deletions src/Tasks/NativeMethods.cs
Original file line number Diff line number Diff line change
Expand Up @@ -779,7 +779,9 @@ internal struct CRYPTOAPI_BLOB
#region PInvoke
private const string Crypt32DLL = "crypt32.dll";
private const string Advapi32DLL = "advapi32.dll";
#if !RUNTIME_TYPE_NETCORE
private const string MscoreeDLL = "mscoree.dll";
#endif

//------------------------------------------------------------------------------
// CreateHardLink
Expand Down Expand Up @@ -1127,9 +1129,9 @@ internal static extern int CreateAssemblyNameObject(
[DllImport(MscoreeDLL, SetLastError = true, CharSet = CharSet.Unicode)]
internal static extern uint GetFileVersion(String szFullPath, StringBuilder szBuffer, int cchBuffer, out uint dwLength);
#endif
#endregion
#endregion

#region Methods
#region Methods
#if FEATURE_HANDLEPROCESSCORRUPTEDSTATEEXCEPTIONS
/// <summary>
/// Given a pointer to a metadata blob, read the string parameter from it. Returns true if
Expand Down Expand Up @@ -1250,8 +1252,8 @@ internal static unsafe int CorSigUncompressData(IntPtr data, out int uncompresse

return count;
}
#endregion
#region InternalClass
#endregion
#region InternalClass
#if FEATURE_COM_INTEROP
/// <summary>
/// This class is a wrapper over the native GAC enumeration API.
Expand Down Expand Up @@ -1491,6 +1493,6 @@ public static string AssemblyPathFromStrongName(string strongName)
}
}
#endif
#endregion
#endregion
}
}
2 changes: 2 additions & 0 deletions src/Tasks/ResolveKeySource.cs
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,9 @@ namespace Microsoft.Build.Tasks
public class ResolveKeySource : TaskExtension
{
private const string pfxFileExtension = ".pfx";
#if !RUNTIME_TYPE_NETCORE
private const string pfxFileContainerPrefix = "VS_KEY_";
#endif

#region Properties

Expand Down
10 changes: 0 additions & 10 deletions src/Tasks/ResolveSDKReference.cs
Original file line number Diff line number Diff line change
Expand Up @@ -661,16 +661,6 @@ internal class SDKReference : IEquatable<SDKReference>
/// </summary>
private const string X64Arch = "X64";

/// <summary>
/// X86 architecture name
/// </summary>
private const string X86Arch = "X86";

/// <summary>
/// ARM architecture name
/// </summary>
private const string ARMArch = "ARM";

/// <summary>
/// ANY CPU architecture name
/// </summary>
Expand Down
5 changes: 0 additions & 5 deletions src/Tasks/ResourceHandling/MSBuildResXReader.cs
Original file line number Diff line number Diff line change
Expand Up @@ -72,13 +72,8 @@ private static void ParseAssemblyAlias(Dictionary<string,string> aliases, XEleme
// Consts from https://github.com/dotnet/winforms/blob/16b192389b377c647ab3d280130781ab1a9d3385/src/System.Windows.Forms/src/System/Resources/ResXResourceWriter.cs#L46-L63
private const string Beta2CompatSerializedObjectMimeType = "text/microsoft-urt/psuedoml-serialized/base64";
private const string CompatBinSerializedObjectMimeType = "text/microsoft-urt/binary-serialized/base64";
private const string CompatSoapSerializedObjectMimeType = "text/microsoft-urt/soap-serialized/base64";
private const string BinSerializedObjectMimeType = "application/x-microsoft.net.object.binary.base64";
private const string SoapSerializedObjectMimeType = "application/x-microsoft.net.object.soap.base64";
private const string DefaultSerializedObjectMimeType = BinSerializedObjectMimeType;
private const string ByteArraySerializedObjectMimeType = "application/x-microsoft.net.object.bytearray.base64";
private const string ResMimeType = "text/microsoft-resx";

private const string StringTypeNamePrefix = "System.String, mscorlib,";
private const string StringTypeName40 = "System.String, mscorlib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089";
private const string MemoryStreamTypeNamePrefix = "System.IO.MemoryStream, mscorlib,";
Expand Down