Ensure modreq must exist on PE symbols that C# compiler expects them to have (#24524)

* Ensure modreq must exist on PE symbols that C# compiler expects them to have

* PR feedback

* More tests + feedback

* Skip tests on mono due to bug

* Added Assert.True(IsOverride) for overrides

docs/compilers/CSharp/Compiler Breaking Changes - post VS2017.md

@@ -95,3 +95,5 @@ Example: `Func<int> f = default(TypedReference).GetHashCode; // new error CS0123
     }
     ```
     This is changed in 15.6 to now produce an error that the variable is not definitely assigned.
+
+- Visual Studio 2017 version 15.7: https://github.com/dotnet/roslyn/issues/19792 C# compiler will now reject [IsReadOnly] symbols that should have an [InAttribute] modreq, but don't.

src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEMethodSymbol.cs

@@ -583,7 +583,10 @@ private SignatureData LoadSignature()
                 var builder = ImmutableArray.CreateBuilder<ParameterSymbol>(count);
                 for (int i = 0; i < count; i++)
                 {
-                    builder.Add(PEParameterSymbol.Create(moduleSymbol, this, i, paramInfo[i + 1], out isBadParameter));
+                    builder.Add(PEParameterSymbol.Create(
+                        moduleSymbol, this, this.IsMetadataVirtual(), i,
+                        paramInfo[i + 1], isReturn: false, out isBadParameter));
+
                     if (isBadParameter)
                     {
                         makeBad = true;
@@ -605,7 +608,9 @@ private SignatureData LoadSignature()
 
             paramInfo[0].Type = returnType;
 
-            var returnParam = PEParameterSymbol.Create(moduleSymbol, this, 0, paramInfo[0], out isBadParameter);
+            var returnParam = PEParameterSymbol.Create(
+                moduleSymbol, this, this.IsMetadataVirtual(), 0,
+                paramInfo[0], isReturn: true, out isBadParameter);
 
             if (makeBad || isBadParameter)
             {

src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEParameterSymbol.cs

@@ -142,11 +142,16 @@ public bool TryGetWellKnownAttribute(WellKnownAttributeFlags flag, out bool valu
         internal static PEParameterSymbol Create(
             PEModuleSymbol moduleSymbol,
             PEMethodSymbol containingSymbol,
+            bool isContainingSymbolVirtual,
             int ordinal,
-            ParamInfo<TypeSymbol> parameter,
+            ParamInfo<TypeSymbol> parameterInfo,
+            bool isReturn,
             out bool isBad)
         {
-            return Create(moduleSymbol, containingSymbol, ordinal, parameter.IsByRef, parameter.RefCustomModifiers, parameter.Type, parameter.Handle, parameter.CustomModifiers, out isBad);
+            return Create(
+                moduleSymbol, containingSymbol, isContainingSymbolVirtual, ordinal,
+                parameterInfo.IsByRef, parameterInfo.RefCustomModifiers, parameterInfo.Type,
+                parameterInfo.Handle, parameterInfo.CustomModifiers, isReturn, out isBad);
         }
 
         /// <summary>
@@ -158,17 +163,21 @@ public bool TryGetWellKnownAttribute(WellKnownAttributeFlags flag, out bool valu
         /// <param name="handle">The property parameter doesn't have a name in metadata,
         /// so this is the handle of a corresponding accessor parameter, if there is one,
         /// or of the ParamInfo passed in, otherwise).</param>
+        /// <param name="parameterInfo" />
         /// <param name="isBad" />
-        /// <param name="parameter"></param>
         internal static PEParameterSymbol Create(
             PEModuleSymbol moduleSymbol,
             PEPropertySymbol containingSymbol,
+            bool isContainingSymbolVirtual,
             int ordinal,
             ParameterHandle handle,
-            ParamInfo<TypeSymbol> parameter,
+            ParamInfo<TypeSymbol> parameterInfo,
             out bool isBad)
         {
-            return Create(moduleSymbol, containingSymbol, ordinal, parameter.IsByRef, parameter.RefCustomModifiers, parameter.Type, handle, parameter.CustomModifiers, out isBad);
+            return Create(
+                moduleSymbol, containingSymbol, isContainingSymbolVirtual, ordinal,
+                parameterInfo.IsByRef, parameterInfo.RefCustomModifiers, parameterInfo.Type,
+                handle, parameterInfo.CustomModifiers, isReturn: false, out isBad);
         }
 
         private PEParameterSymbol(
@@ -266,20 +275,39 @@ private bool HasNameInMetadata
         private static PEParameterSymbol Create(
             PEModuleSymbol moduleSymbol,
             Symbol containingSymbol,
+            bool isContainingSymbolVirtual,
             int ordinal,
             bool isByRef,
             ImmutableArray<ModifierInfo<TypeSymbol>> refCustomModifiers,
             TypeSymbol type,
             ParameterHandle handle,
             ImmutableArray<ModifierInfo<TypeSymbol>> customModifiers,
+            bool isReturn,
             out bool isBad)
         {
-            if (customModifiers.IsDefaultOrEmpty && refCustomModifiers.IsDefaultOrEmpty)
+            PEParameterSymbol parameter = customModifiers.IsDefaultOrEmpty && refCustomModifiers.IsDefaultOrEmpty
+                ? new PEParameterSymbol(moduleSymbol, containingSymbol, ordinal, isByRef, type, handle, 0, out isBad)
+                : new PEParameterSymbolWithCustomModifiers(moduleSymbol, containingSymbol, ordinal, isByRef, refCustomModifiers, type, handle, customModifiers, out isBad);
+
+            bool hasInAttributeModifier = parameter.RefCustomModifiers.HasInAttributeModifier();
+
+            if (isReturn)
+            {
+                // A RefReadOnly return parameter should always have this modreq, and vice versa.
+                isBad |= (parameter.RefKind == RefKind.RefReadOnly) != hasInAttributeModifier;
+            }
+            else if (parameter.RefKind == RefKind.In)
+            {
+                // An in parameter should not have this modreq, unless the containing symbol was virtual or abstract.
+                isBad |= isContainingSymbolVirtual != hasInAttributeModifier;
+            }
+            else if (hasInAttributeModifier)
             {
-                return new PEParameterSymbol(moduleSymbol, containingSymbol, ordinal, isByRef, type, handle, 0, out isBad);
+                // This modreq should not exist on non-in parameters.
+                isBad = true;
             }
 
-            return new PEParameterSymbolWithCustomModifiers(moduleSymbol, containingSymbol, ordinal, isByRef, refCustomModifiers, type, handle, customModifiers, out isBad);
+            return parameter;
         }
 
         private sealed class PEParameterSymbolWithCustomModifiers : PEParameterSymbol
@@ -304,12 +332,6 @@ private sealed class PEParameterSymbolWithCustomModifiers : PEParameterSymbol
                 _customModifiers = CSharpCustomModifier.Convert(customModifiers);
                 _refCustomModifiers = CSharpCustomModifier.Convert(refCustomModifiers);
 
-                if (this.RefKind != RefKind.In && _refCustomModifiers.Any(modifier => !modifier.IsOptional && modifier.Modifier.IsWellKnownTypeInAttribute()))
-                {
-                    // The modreq is only accepted on RefReadOnly symbols
-                    isBad = true;
-                }
-
                 Debug.Assert(_refCustomModifiers.IsEmpty || isByRef);
             }
 

src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEPropertySymbol.cs

@@ -69,18 +69,14 @@ private enum Flags : byte
             var propertyParams = metadataDecoder.GetSignatureForProperty(handle, out callingConvention, out propEx);
             Debug.Assert(propertyParams.Length > 0);
 
-            var isBad = false;
             var returnInfo = propertyParams[0];
-            PEPropertySymbol result;
 
-            if (returnInfo.CustomModifiers.IsDefaultOrEmpty && returnInfo.RefCustomModifiers.IsDefaultOrEmpty)
-            {
-                result = new PEPropertySymbol(moduleSymbol, containingType, handle, getMethod, setMethod, 0, propertyParams, metadataDecoder);
-            }
-            else
-            {
-                result = new PEPropertySymbolWithCustomModifiers(moduleSymbol, containingType, handle, getMethod, setMethod, propertyParams, metadataDecoder, out isBad);
-            }
+            PEPropertySymbol result = returnInfo.CustomModifiers.IsDefaultOrEmpty && returnInfo.RefCustomModifiers.IsDefaultOrEmpty
+                ? new PEPropertySymbol(moduleSymbol, containingType, handle, getMethod, setMethod, 0, propertyParams, metadataDecoder)
+                : new PEPropertySymbolWithCustomModifiers(moduleSymbol, containingType, handle, getMethod, setMethod, propertyParams, metadataDecoder);
+
+            // A property should always have this modreq, and vice versa.
+            var isBad = (result.RefKind == RefKind.In) != result.RefCustomModifiers.HasInAttributeModifier();
 
             if (propEx != null || isBad)
             {
@@ -133,7 +129,10 @@ private enum Flags : byte
             // use the parameter names from one of the indexers
             // NB: prefer setter names to getter names if both are present.
             bool isBad;
-            _parameters = GetParameters(moduleSymbol, this, propertyParams, setMethodParams ?? getMethodParams, out isBad);
+
+            _parameters = setMethodParams is null
+                ? GetParameters(moduleSymbol, this, propertyParams, getMethodParams, getMethod.IsMetadataVirtual(), out isBad)
+                : GetParameters(moduleSymbol, this, propertyParams, setMethodParams, setMethod.IsMetadataVirtual(), out isBad);
 
             if (getEx != null || setEx != null || mrEx != null || isBad)
             {
@@ -650,6 +649,7 @@ internal override bool MustCallMethodsDirectly
             PEPropertySymbol property,
             ParamInfo<TypeSymbol>[] propertyParams,
             ParamInfo<TypeSymbol>[] accessorParams,
+            bool isPropertyVirtual,
             out bool anyParameterIsBad)
         {
             anyParameterIsBad = false;
@@ -671,13 +671,15 @@ internal override bool MustCallMethodsDirectly
                 var paramHandle = i < numAccessorParams ? accessorParams[i].Handle : propertyParam.Handle;
                 var ordinal = i - 1;
                 bool isBad;
-                parameters[ordinal] = PEParameterSymbol.Create(moduleSymbol, property, ordinal, paramHandle, propertyParam, out isBad);
+                
+                parameters[ordinal] = PEParameterSymbol.Create(moduleSymbol, property, isPropertyVirtual, ordinal, paramHandle, propertyParam, out isBad);
 
                 if (isBad)
                 {
                     anyParameterIsBad = true;
                 }
             }
+
             return parameters.AsImmutableOrNull();
         }
 
@@ -732,18 +734,14 @@ private sealed class PEPropertySymbolWithCustomModifiers : PEPropertySymbol
                 PEMethodSymbol getMethod,
                 PEMethodSymbol setMethod,
                 ParamInfo<TypeSymbol>[] propertyParams,
-                MetadataDecoder metadataDecoder,
-                out bool isBad)
-                : base (moduleSymbol, containingType, handle, getMethod, setMethod,
+                MetadataDecoder metadataDecoder)
+                : base(moduleSymbol, containingType, handle, getMethod, setMethod,
                         propertyParams[0].CustomModifiers.NullToEmpty().Length + propertyParams[0].RefCustomModifiers.NullToEmpty().Length,
                         propertyParams, metadataDecoder)
             {
                 var returnInfo = propertyParams[0];
                 _typeCustomModifiers = CSharpCustomModifier.Convert(returnInfo.CustomModifiers);
                 _refCustomModifiers = CSharpCustomModifier.Convert(returnInfo.RefCustomModifiers);
-
-                // The modreq is only accepted on RefReadOnly symbols
-                isBad = this.RefKind != RefKind.RefReadOnly && _refCustomModifiers.Any(modifier => !modifier.IsOptional && modifier.Modifier.IsWellKnownTypeInAttribute());
             }
 
             public override ImmutableArray<CustomModifier> TypeCustomModifiers

src/Compilers/CSharp/Portable/Symbols/Source/CustomModifierUtils.cs

@@ -129,5 +129,10 @@ internal static ImmutableArray<ParameterSymbol> CopyParameterCustomModifiers(Imm
 
             return builder == null ? destinationParameters : builder.ToImmutableAndFree();
         }
+
+        internal static bool HasInAttributeModifier(this ImmutableArray<CustomModifier> modifiers)
+        {
+            return modifiers.Any(modifier => !modifier.IsOptional && modifier.Modifier.IsWellKnownTypeInAttribute());
+        }
     }
 }