Add readonly to struct methods (#8656)

In recent dogfood VS builds we've gotten many IDE0251 suggestions, some warnings escalated to errors in StringTools. Follow many of those suggestions, but bump the rule down to suggestion always, like the related IDE0250.

.editorconfig

@@ -390,8 +390,11 @@ dotnet_diagnostic.IDE0241.severity = suggestion
 # Struct can be made 'readonly'
 dotnet_diagnostic.IDE0250.severity = suggestion
 
+# Struct methods can be made 'readonly'
+dotnet_diagnostic.IDE0251.severity = suggestion
+
 # Null check can be simplified
 dotnet_diagnostic.IDE0270.severity = suggestion
 
 # naming rule violation
-dotnet_diagnostic.IDE1006.severity = suggestion
+dotnet_diagnostic.IDE1006.severity = suggestion

src/Build/Collections/ConvertingEnumerable.cs

@@ -82,7 +82,7 @@ namespace Microsoft.Build.Collections
             /// <summary>
             /// Get the current element, converted
             /// </summary>
-            public TTo2 Current
+            public readonly TTo2 Current
             {
                 get
                 {

src/Build/Construction/ProjectElementContainer.cs

@@ -742,7 +742,7 @@ namespace Microsoft.Build.Construction
             /// <summary>
             /// Get enumerator
             /// </summary>
-            public IEnumerator<ProjectElement> GetEnumerator()
+            public readonly IEnumerator<ProjectElement> GetEnumerator()
             {
                 return _enumerator;
             }
@@ -808,7 +808,7 @@ namespace Microsoft.Build.Construction
                 /// <summary>
                 /// Dispose. Do nothing.
                 /// </summary>
-                public void Dispose()
+                public readonly void Dispose()
                 {
                 }
 

src/Build/Definition/Project.cs

@@ -2558,7 +2558,7 @@ namespace Microsoft.Build.Evaluation
                     };
                 }
 
-                public void AccumulateInformationFromRemoveItemSpec(EvaluationItemSpec removeSpec)
+                public readonly void AccumulateInformationFromRemoveItemSpec(EvaluationItemSpec removeSpec)
                 {
                     IEnumerable<string> removeSpecFragmentStrings = removeSpec.FlattenFragmentsAsStrings();
                     var removeGlob = removeSpec.ToMSBuildGlob();

src/Build/Definition/ToolsetReader.cs

@@ -817,7 +817,7 @@ namespace Microsoft.Build.Evaluation
         /// <summary>
         /// Returns the corresponding property name - eg. "$(MSBuildExtensionsPath32)"
         /// </summary>
-        public string MSBuildPropertyName => String.Format($"$({StringRepresentation})");
+        public readonly string MSBuildPropertyName => String.Format($"$({StringRepresentation})");
 
         /// <summary>
         /// Tries to find a reference to MSBuildExtensionsPath* property in the given string

src/Build/Evaluation/Expander.cs

@@ -210,7 +210,7 @@ namespace Microsoft.Build.Evaluation
             /// concatenation of the string representation of the values, each additionally subjected
             /// to file path adjustment.
             /// </returns>
-            public object GetResult()
+            public readonly object GetResult()
             {
                 CheckDisposed();
                 if (_firstObject != null)
@@ -235,7 +235,7 @@ namespace Microsoft.Build.Evaluation
             /// <summary>
             /// Throws <see cref="ObjectDisposedException"/> if this concatenator is already disposed.
             /// </summary>
-            private void CheckDisposed() =>
+            private readonly void CheckDisposed() =>
                 ErrorUtilities.VerifyThrowObjectDisposed(!_disposed, nameof(SpanBasedConcatenator));
 
             /// <summary>
@@ -3142,7 +3142,7 @@ namespace Microsoft.Build.Evaluation
             /// </summary>
             public UsedUninitializedProperties UsedUninitializedProperties { get; set; }
 
-            internal Function<T> Build()
+            internal readonly Function<T> Build()
             {
                 return new Function<T>(
                     ReceiverType,

src/Build/Evaluation/ItemsAndMetadataPair.cs

@@ -40,7 +40,7 @@ namespace Microsoft.Build.Evaluation
         /// </summary>
         internal HashSet<string> Items
         {
-            get
+            readonly get
             {
                 return _items;
             }
@@ -58,7 +58,7 @@ namespace Microsoft.Build.Evaluation
         /// </summary>
         internal Dictionary<string, MetadataReference> Metadata
         {
-            get
+            readonly get
             {
                 return _metadata;
             }

src/Build/Evaluation/LazyItemEvaluator.cs

@@ -135,7 +135,7 @@ namespace Microsoft.Build.Evaluation
                 _normalizedItemValue = normalizedItemValue;
             }
 
-            public ItemData Clone(IItemFactory<I, I> itemFactory, ProjectItemElement initialItemElementForFactory)
+            public readonly ItemData Clone(IItemFactory<I, I> itemFactory, ProjectItemElement initialItemElementForFactory)
             {
                 // setting the factory's item element to the original item element that produced the item
                 // otherwise you get weird things like items that appear to have been produced by update elements

src/Build/Graph/ProjectGraph.cs

@@ -766,7 +766,7 @@ namespace Microsoft.Build.Graph
 
             public ImmutableList<string> RequestedTargets { get; }
 
-            public bool Equals(ProjectGraphBuildRequest other)
+            public readonly bool Equals(ProjectGraphBuildRequest other)
             {
                 if (Node != other.Node
                     || RequestedTargets.Count != other.RequestedTargets.Count)
@@ -786,12 +786,12 @@ namespace Microsoft.Build.Graph
                 return true;
             }
 
-            public override bool Equals(object obj)
+            public override readonly bool Equals(object obj)
             {
                 return !(obj is null) && obj is ProjectGraphBuildRequest graphNodeWithTargets && Equals(graphNodeWithTargets);
             }
 
-            public override int GetHashCode()
+            public override readonly int GetHashCode()
             {
                 unchecked
                 {

src/Build/Graph/ProjectGraphEntryPoint.cs

@@ -61,7 +61,7 @@ namespace Microsoft.Build.Graph
             }
         }
 
-        internal IEnumerable<ProjectGraphEntryPoint> AsEnumerable()
+        internal readonly IEnumerable<ProjectGraphEntryPoint> AsEnumerable()
         {
             yield return this;
         }

src/Build/Xml/ProjectXmlUtilities.XmlElementChildIterator.cs

@@ -63,7 +63,7 @@ namespace Microsoft.Build.Internal
                 return this;
             }
 
-            public XmlElementWithLocation Current
+            public readonly XmlElementWithLocation Current
             {
                 get
                 {
@@ -73,7 +73,7 @@ namespace Microsoft.Build.Internal
                 }
             }
 
-            private XmlElementWithLocation GetNextNode(XmlNode child)
+            private readonly XmlElementWithLocation GetNextNode(XmlNode child)
             {
                 while (child != null)
                 {

src/Deprecated/Engine/ItemsAndProperties/ExpressionShredder.cs

@@ -425,13 +425,13 @@ namespace Microsoft.Build.BuildEngine
 
         internal Hashtable Items
         {
-            get { return items; }
+            readonly get { return items; }
             set { items = value; }
         }
 
         internal Dictionary<string, MetadataReference> Metadata
         {
-            get { return metadata; }
+            readonly get { return metadata; }
             set { metadata = value; }
         }
     }

src/Framework/BuildEngineResult.cs

@@ -42,7 +42,7 @@ namespace Microsoft.Build.Framework
         /// <summary>
         /// Did the build pass or fail. True means the build succeeded, False means the build failed.
         /// </summary>
-        public bool Result
+        public readonly bool Result
         {
             get
             {

src/Framework/NativeMethods.cs

@@ -345,7 +345,7 @@ internal static class NativeMethods
         public UIntPtr UniqueProcessId;
         public UIntPtr InheritedFromUniqueProcessId;
 
-        public uint Size
+        public readonly uint Size
         {
             get
             {

src/Framework/Profiler/EvaluationLocation.cs

@@ -101,7 +101,7 @@ namespace Microsoft.Build.Framework.Profiler
         public EvaluationLocationKind Kind { get; }
 
         /// <nodoc/>
-        public bool IsEvaluationPass => File == null;
+        public readonly bool IsEvaluationPass => File == null;
 
         /// <nodoc/>
         public static EvaluationLocation CreateLocationForCondition(long? parentId, EvaluationPass evaluationPass, string evaluationDescription, string file,
@@ -183,7 +183,7 @@ namespace Microsoft.Build.Framework.Profiler
         public static EvaluationLocation EmptyLocation { get; } = CreateEmptyLocation();
 
         /// <nodoc/>
-        public EvaluationLocation WithEvaluationPass(EvaluationPass evaluationPass, string passDescription = null)
+        public readonly EvaluationLocation WithEvaluationPass(EvaluationPass evaluationPass, string passDescription = null)
         {
             return new EvaluationLocation(this.Id, evaluationPass, passDescription ?? PassDefaultDescription[evaluationPass],
                 this.File, this.Line, this.ElementName, this.ElementDescription, this.Kind);

src/Shared/FileMatcher.cs

@@ -796,7 +796,7 @@ namespace Microsoft.Build.Shared
             /// <summary>
             /// True if a SearchData.DirectoryPattern is specified but we have not descended into a matching directory.
             /// </summary>
-            public bool IsLookingForMatchingDirectory => (SearchData.DirectoryPattern != null && !IsInsideMatchingDirectory);
+            public readonly bool IsLookingForMatchingDirectory => (SearchData.DirectoryPattern != null && !IsInsideMatchingDirectory);
         }
 
         /// <summary>

src/Shared/FileSystem/WindowsNative.cs

@@ -141,7 +141,7 @@ namespace Microsoft.Build.Shared.FileSystem
             /// <summary>
             /// Indicates if enumeration succeeded.
             /// </summary>
-            public bool Succeeded
+            public readonly bool Succeeded
             {
                 get { return Status == EnumerateDirectoryStatus.Success; }
             }
@@ -153,7 +153,7 @@ namespace Microsoft.Build.Shared.FileSystem
             /// <remarks>
             /// This is a good <c>default:</c> case when switching on every possible <see cref="EnumerateDirectoryStatus"/>
             /// </remarks>
-            public NativeWin32Exception ThrowForUnknownError()
+            public readonly NativeWin32Exception ThrowForUnknownError()
             {
                 Debug.Assert(Status == EnumerateDirectoryStatus.UnknownError);
                 throw CreateExceptionForError();
@@ -173,7 +173,7 @@ namespace Microsoft.Build.Shared.FileSystem
             /// <summary>
             /// Creates (but does not throw) an exception for this result. The result must not be successful.
             /// </summary>
-            public NativeWin32Exception CreateExceptionForError()
+            public readonly NativeWin32Exception CreateExceptionForError()
             {
                 Debug.Assert(Status != EnumerateDirectoryStatus.Success);
                 if (Status == EnumerateDirectoryStatus.UnknownError)

src/Shared/Pair.cs

@@ -44,7 +44,7 @@ namespace Microsoft.Build.Shared
         /// <summary>
         /// Key
         /// </summary>
-        internal TKey Key
+        internal readonly TKey Key
         {
             get { return _key; }
         }
@@ -52,7 +52,7 @@ namespace Microsoft.Build.Shared
         /// <summary>
         /// Value
         /// </summary>
-        internal TValue Value
+        internal readonly TValue Value
         {
             get { return _value; }
         }

src/StringTools/InternableString.Simple.cs

@@ -57,7 +57,7 @@ namespace Microsoft.NET.StringTools
             /// <summary>
             /// Returns the current character.
             /// </summary>
-            public char Current => (_string._builder == null ? _string.FirstString[_charIndex] : _string._builder[_charIndex]);
+            public readonly char Current => (_string._builder == null ? _string.FirstString[_charIndex] : _string._builder[_charIndex]);
 
             /// <summary>
             /// Moves to the next character.
@@ -88,7 +88,7 @@ namespace Microsoft.NET.StringTools
         /// <summary>
         /// A convenience getter to ensure that we always operate on a non-null string.
         /// </summary>
-        private string FirstString => _firstString ?? string.Empty;
+        private readonly string FirstString => _firstString ?? string.Empty;
 
         /// <summary>
         /// Constructs a new InternableString wrapping the given string.
@@ -116,13 +116,13 @@ namespace Microsoft.NET.StringTools
         /// <summary>
         /// Gets the length of the string.
         /// </summary>
-        public int Length => (_builder == null ? FirstString.Length : _builder.Length);
+        public readonly int Length => (_builder == null ? FirstString.Length : _builder.Length);
 
         /// <summary>
         /// Creates a new enumerator for enumerating characters in this string. Does not allocate.
         /// </summary>
         /// <returns>The enumerator.</returns>
-        public Enumerator GetEnumerator()
+        public readonly Enumerator GetEnumerator()
         {
             return new Enumerator(this);
         }
@@ -132,7 +132,7 @@ namespace Microsoft.NET.StringTools
         /// </summary>
         /// <param name="other">Another string.</param>
         /// <returns>True if this string is equal to <paramref name="other"/>.</returns>
-        public bool Equals(string other)
+        public readonly bool Equals(string other)
         {
             if (other.Length != Length)
             {
@@ -163,7 +163,7 @@ namespace Microsoft.NET.StringTools
         /// System.String in which case the original string is returned.
         /// </summary>
         /// <returns>The string.</returns>
-        public string ExpensiveConvertToString()
+        public readonly string ExpensiveConvertToString()
         {
             // Special case: if we hold just one string, we can directly return it.
             if (_firstString != null)
@@ -178,9 +178,9 @@ namespace Microsoft.NET.StringTools
         /// </summary>
         /// <param name="str">The string to compare to.</param>
         /// <returns>True is this instance wraps the given string.</returns>
-        public bool ReferenceEquals(string str)
+        public readonly bool ReferenceEquals(string str)
         {
-            return Object.ReferenceEquals(str, _firstString);
+            return ReferenceEquals(str, _firstString);
         }
 
         /// <summary>
@@ -198,7 +198,7 @@ namespace Microsoft.NET.StringTools
         /// Implements the simple yet very decently performing djb2 hash function (xor version).
         /// </summary>
         /// <returns>A stable hashcode of the string represented by this instance.</returns>
-        public override int GetHashCode()
+        public override readonly int GetHashCode()
         {
             uint hash = (5381 << 16) + 5381;
             bool isOddIndex = false;

src/StringTools/InternableString.cs

@@ -43,7 +43,7 @@ namespace Microsoft.NET.StringTools
             /// <summary>
             /// Returns the current character.
             /// </summary>
-            public ref readonly char Current
+            public readonly ref readonly char Current
             {
                 get
                 {
@@ -178,7 +178,7 @@ namespace Microsoft.NET.StringTools
         /// </summary>
         /// <param name="other">Another string.</param>
         /// <returns>True if this string is equal to <paramref name="other"/>.</returns>
-        public bool Equals(string other)
+        public readonly bool Equals(string other)
         {
             if (other.Length != Length)
             {
@@ -210,7 +210,7 @@ namespace Microsoft.NET.StringTools
         /// System.String in which case the original string is returned.
         /// </summary>
         /// <returns>The string.</returns>
-        public unsafe string ExpensiveConvertToString()
+        public readonly unsafe string ExpensiveConvertToString()
         {
             if (Length == 0)
             {
@@ -268,7 +268,7 @@ namespace Microsoft.NET.StringTools
 
                 // The invariant that Length is the sum of span lengths is critical in this unsafe method.
                 // Violating it may lead to memory corruption and, since this code tends to run under a lock,
-                // to hangs caused by the lock getting orphaned. Attempt to detect that and throw now, 
+                // to hangs caused by the lock getting orphaned. Attempt to detect that and throw now,
                 // before the corruption causes further problems.
                 if (destPtr != resultPtr + Length)
                 {
@@ -283,7 +283,7 @@ namespace Microsoft.NET.StringTools
         /// </summary>
         /// <param name="str">The string to compare to.</param>
         /// <returns>True is this instance wraps the given string.</returns>
-        public bool ReferenceEquals(string str)
+        public readonly bool ReferenceEquals(string str)
         {
             if (_inlineSpan.Length == Length)
             {
@@ -317,7 +317,7 @@ namespace Microsoft.NET.StringTools
         /// characters that feed into the same operation but straddle multiple spans. Note that it must return the same value for
         /// a given string regardless of how it's split into spans (e.g. { "AB" } and { "A", "B" } have the same hash code).
         /// </remarks>
-        public override unsafe int GetHashCode()
+        public override readonly unsafe int GetHashCode()
         {
             uint hash = (5381 << 16) + 5381;
             bool hashedOddNumberOfCharacters = false;

src/StringTools/SpanBasedStringBuilder.Simple.cs

@@ -35,7 +35,7 @@ namespace Microsoft.NET.StringTools
             /// <summary>
             /// Returns the current character.
             /// </summary>
-            public char Current => _builder[_charIndex];
+            public readonly char Current => _builder[_charIndex];
 
             /// <summary>
             /// Moves to the next character.

src/Tasks/AssemblyDependency/AssemblyNameReference.cs

@@ -19,7 +19,7 @@ namespace Microsoft.Build.Tasks
         /// <summary>
         /// Display as string.
         /// </summary>
-        public override string ToString()
+        public override readonly string ToString()
         {
             return assemblyName + ", " + reference;
         }
@@ -27,7 +27,7 @@ namespace Microsoft.Build.Tasks
         /// <summary>
         /// Compare by assembly name.
         /// </summary>
-        public int CompareTo(AssemblyNameReference other)
+        public readonly int CompareTo(AssemblyNameReference other)
         {
             return assemblyName.CompareTo(other.assemblyName);
         }

src/Tasks/ComDependencyWalker.cs

@@ -66,7 +66,7 @@ namespace Microsoft.Build.Tasks
                 this.index = index;
             }
 
-            public override string ToString()
+            public override readonly string ToString()
             {
                 return string.Format(CultureInfo.InvariantCulture, "{0}.{1}.{2}.{3}:{4}",
                     this.guid, this.wMajorVerNum,