Adding IsConst modreq on ref readonly signatures #19658

OmarTawfik · 2017-05-19T07:50:34Z

Contributes to #17287

OmarTawfik · 2017-05-19T19:47:17Z

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

+        internal override bool IsConstModifierType(PEModuleSymbol moduleSymbol, TypeSymbol type)
+        {
+            return type.ToDisplayString() == WellKnownTypes.GetMetadataName(WellKnownType.System_Runtime_CompilerServices_IsConst);
+        }


Comparing symbols by full name is probably not be the best way to do it. Looking up a better alternative. #Closed

Consider a similar scenario like IsVolatileModfifierType. Can we re-use that approach here? #Resolved

Unfortunately, no. Unless there is something I'm missing, IsConst is not a special library type :(

In reply to: 117829472 [](ancestors = 117829472)

Perhaps the same approach as SymbolFactory.GetSystemTypeSymbol. #Resolved

we need a bug or a PROTOTYPE marker to track this

In reply to: 117561528 [](ancestors = 117561528)

This seems simple enough to fix now vs. needing a PROTOTYPE comment to fix later. #Resolved

Comparing symbols by full name is probably not be the best way to do it. Looking up a better alternative.

Given that this is not a SpecialType, I think name comparison is the only right way to do that. If performance is a concern we can compare type name and namespace names separately, then calling ToDisplayString won't be needed. #Closed

If going with name comparing, should definitely compare namespace/type name parts separately. I think ToDisplay will concat them every time

In reply to: 118063143 [](ancestors = 118063143)

Planning on fixing now.

In reply to: 117888944 [](ancestors = 117888944,117561528)

From what I tracked down, it ends up doing a lookup in NamespaceOrTypeSymbol based on string comparison as well. Will separate the comparison to namespace/type pair to improve performance.

In reply to: 117866143 [](ancestors = 117866143)

OmarTawfik · 2017-05-19T19:59:03Z

src/Compilers/Core/Portable/MetadataReader/MetadataDecoder.cs


-                for (;;)
+                customModifiers = DecodeModifiersOrThrow(ref signatureReader, out typeCode, acceptRequiredModifier: type =>


DecodeModifiersOrThrow [](start = 34, length = 22)

This was refactored since it shares the same behavior as DecodeModifiersOrThrow(), making use of the additional parameter. #Closed

OmarTawfik · 2017-05-22T18:27:09Z

cc @VSadov @dotnet/roslyn-compiler #Closed

jaredpar · 2017-05-22T19:39:57Z

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

                                                                      out _lazyParameters, alsoCopyParamsModifier: true);
                    }
                }
+                else if (_refKind == RefKind.RefReadOnly && (this.IsVirtual || this.IsAbstract))
+                {
+                    var isConst = withTypeParamsBinder.GetWellKnownType(WellKnownType.System_Runtime_CompilerServices_IsConst, diagnostics, syntax);


What if the type is missing? Consider that custom mscorlibs may not have this type. #ByDesign

Yes. I've a test that specifically looks for that. Check ProperErrorsArePropagatedIfMscorlibIsConstIsNotAvailable in IsConstModifierTests.cs.
The method Binder.GetWellKnownType() will report such errors to the passed diagnostics bag.

In reply to: 117828511 [](ancestors = 117828511)

Consider the name isConstType vs. isConst. The former is a bit more descriptive and seems more consistent with other names in the method. The latter can be misread as describing the mutable state of a value. #Resolved

jaredpar · 2017-05-22T19:40:37Z

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

@@ -48,6 +50,25 @@ internal abstract class SourceParameterSymbol : SourceParameterSymbolBase
                    identifier.Parent.GetLocation());
            }

+            if (shouldPlaceIsConstModifier && refKind == RefKind.RefReadOnly && (owner.IsVirtual || owner.IsAbstract))
+            {
+                var isConst = context.GetWellKnownType(WellKnownType.System_Runtime_CompilerServices_IsConst, declarationDiagnostics, syntax);


What if the type is missing? consider custom mscorlib may not have this type. #ByDesign

jaredpar · 2017-05-22T19:40:43Z

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

@@ -273,6 +273,14 @@ internal sealed class SourcePropertySymbol : PropertySymbol, IAttributeTargetSym
                    _lazyParameters = CustomModifierUtils.CopyParameterCustomModifiers(overriddenOrImplementedProperty.Parameters, _lazyParameters, alsoCopyParamsModifier: isOverride);
                }
            }
+            else if (_refKind == RefKind.RefReadOnly && (this.IsVirtual || this.IsAbstract))
+            {
+                var isConst = bodyBinder.GetWellKnownType(WellKnownType.System_Runtime_CompilerServices_IsConst, diagnostics, syntax);


Same #ByDesign

jaredpar · 2017-05-22T19:42:18Z

src/Compilers/VisualBasic/Portable/Symbols/Metadata/PE/SymbolFactory.vb

@@ -49,6 +49,11 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.Symbols.Metadata.PE
            Return False
        End Function

+        Friend Overrides Function IsConstModifierType(moduleSymbol As PEModuleSymbol, type As TypeSymbol) As Boolean
+            ' VB doesn't deal with ref-readonly parameters or return-types.
+            Return False


Don't understand why we are returning False here. Even if VB doesn't deal with these typse returning False here still seems wrong. If the function isn't implemented it should throw, not return the (potentially) wrong answer. #ByDesign

This means that if VB MetadataDecoder tries to read a symbol with a modreq of type IsConst, it will return False meaning it doesn't support it, and the symbol will be marked appropriately. Check IsVolatileModifierType() in the same file.

In reply to: 117828980 [](ancestors = 117828980)

If that's the case then we should consider renaming both of these methods. The name simply does not match up to the actual implementation. Consider this code

var isConst = IsConstModifierType(type);

99% of developers who look at that line are going to interpret this as determining if type is the const modifier. In reality it's detecting if a) it's the const modifier and b) the present language supports it. The 1% who will understand the behavior are those who realize the name of the method is wrong.
#Resolved

Will rename both to IsAcceptedXXXXModifierType

In reply to: 118021028 [](ancestors = 118021028)

jaredpar · 2017-05-22T19:44:47Z

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

+        internal override bool IsConstModifierType(PEModuleSymbol moduleSymbol, TypeSymbol type)
+        {
+            return type.ToDisplayString() == WellKnownTypes.GetMetadataName(WellKnownType.System_Runtime_CompilerServices_IsConst);
+        }


Consider a similar scenario like IsVolatileModfifierType. Can we re-use that approach here? #Resolved

OmarTawfik · 2017-05-22T20:42:34Z

src/Compilers/CSharp/Test/Emit/Emit/IsConstModifierTests.cs

+        }
+
+        [Fact]
+        public void ProperErrorsArePropagatedIfMscorlibIsConstIsNotAvailable()


ProperErrorsArePropagatedIfMscorlibIsConstIsNotAvailable [](start = 20, length = 56)

Note to self: duplicate tests for other symbol types and the same error. #Closed

VSadov · 2017-05-22T22:58:47Z

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

@@ -273,6 +273,14 @@ internal sealed class SourcePropertySymbol : PropertySymbol, IAttributeTargetSym
                    _lazyParameters = CustomModifierUtils.CopyParameterCustomModifiers(overriddenOrImplementedProperty.Parameters, _lazyParameters, alsoCopyParamsModifier: isOverride);
                }
            }
+            else if (_refKind == RefKind.RefReadOnly && (this.IsVirtual || this.IsAbstract))


does this work with interface implementations? explicit/implicit? #Resolved

I think we need some tests for interface implementations

In reply to: 117863982 [](ancestors = 117863982)

VSadov · 2017-05-23T03:12:42Z

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

@@ -249,6 +249,7 @@ private sealed class InvokeMethod : SourceDelegateMethodSymbol
                    binder, this, syntax.ParameterList, out arglistToken,
                    allowRefOrOut: true,
                    allowThis: false,
+                    shouldPlaceIsConstModifier: false,


I think we want "true" here. Otherwise old compiler may convert methods with regular "ref" to delegate types with "ref readonly". #Resolved

I think we want "true" here. Otherwise old compiler may convert methods with regular "ref" to delegate types with "ref readonly".

Please add VB test for this scenario. #Closed

VSadov · 2017-05-23T03:14:29Z

I am done with a review pass. Mostly good. I had some comments, that I think are actionable.

In reply to: 303181737 [](ancestors = 303181737)

cston · 2017-05-23T06:14:26Z

src/Compilers/Core/Portable/MetadataReader/MetadataDecoder.cs

+            {
+                // This cannot be placed on CustomModifiers, just RefCustomModifiers
+                throw new UnsupportedSignatureContent();
+            }


Is isConstFound needed or could the check above be replaced by else if (info.CustomModifiers.Length > 0)? Then the delegate to DecodeModifiersOrThrow can be simplified to avoid the closure class. Similar comment for isVolatileFound in DecodeFieldSignature. #Resolved

Will remove the need for capturing, but we still need that isConstFound value since we allow optional modifiers, just not required ones.

In reply to: 117907098 [](ancestors = 117907098)

cston · 2017-05-23T14:25:39Z

src/Compilers/VisualBasic/Test/Emit/Emit/EmitCustomModifiers.vb

@@ -107,5 +108,188 @@ End Class

            CompileAndVerify(comp, expectedOutput:=<![CDATA[2]]>)
        End Sub
+
+        <Fact>
+        Public Sub NonExtensibleReadOnlySignaturesAreRead()


Perhaps Virtual rather than Extensible, here and below. #Resolved

cston · 2017-05-23T14:53:16Z

src/Compilers/CSharp/Test/Emit/Emit/IsConstModifierTests.cs

+    public int this[ref readonly int x] => x;
+}";
+
+            var options = TestOptions.DebugDll.WithMetadataImportOptions(MetadataImportOptions.All);


Is MetadataImportOptions.All needed here, or in tests below? #Resolved

cston · 2017-05-23T14:58:33Z

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

                                                                      out _lazyParameters, alsoCopyParamsModifier: true);
                    }
                }
+                else if (_refKind == RefKind.RefReadOnly && (this.IsVirtual || this.IsAbstract))


Are we including IsConst on interface methods? #Resolved

Yes: virtual method parameters, returns, interface methods in any position and delegates. #Resolved

Parameters handle both parameters and returns. Will add tests for interfaces and delegates.

In reply to: 118018517 [](ancestors = 118018517)

Will add tests to both implicit and explicit interface implementations

In reply to: 118016179 [](ancestors = 118016179)

jaredpar · 2017-05-23T15:04:29Z

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

+        internal override bool IsConstModifierType(PEModuleSymbol moduleSymbol, TypeSymbol type)
+        {
+            return type.ToDisplayString() == WellKnownTypes.GetMetadataName(WellKnownType.System_Runtime_CompilerServices_IsConst);
+        }


This seems simple enough to fix now vs. needing a PROTOTYPE comment to fix later. #Resolved

jaredpar · 2017-05-23T15:05:18Z

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

@@ -20,7 +20,8 @@ internal static class ParameterHelpers
            out SyntaxToken arglistToken,
            DiagnosticBag diagnostics,
            bool allowRefOrOut,
-            bool allowThis)
+            bool allowThis,
+            bool shouldPlaceIsConstModifier)


Consider shoudEmit vs. shouldPlace. #Resolved

Consider shoudEmit vs. shouldPlace.

Perhaps simply "addIsConstModifier". #Closed

jaredpar · 2017-05-23T15:05:57Z

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

                                                                      out _lazyParameters, alsoCopyParamsModifier: true);
                    }
                }
+                else if (_refKind == RefKind.RefReadOnly && (this.IsVirtual || this.IsAbstract))


Yes: virtual method parameters, returns, interface methods in any position and delegates. #Resolved

jaredpar · 2017-05-23T15:09:59Z

src/Compilers/Core/Portable/MetadataReader/MetadataDecoder.cs

+                var isConst = IsConstModifierType(type);
+                isConstFound |= isConst;
+                return isConst;
+            });


This lambda is capturing hence is adding two allocations to each call to DecodeModifiersOrThrow. Is this a sensitive enough area of the code that we should consider a non-allocating solution? #Resolved

Will move to a version that has less allocations, also adding an extra overload for the less-traversed path.

In reply to: 118019729 [](ancestors = 118019729)

jaredpar · 2017-05-23T15:14:21Z

src/Compilers/VisualBasic/Portable/Symbols/Metadata/PE/SymbolFactory.vb

@@ -49,6 +49,11 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.Symbols.Metadata.PE
            Return False
        End Function

+        Friend Overrides Function IsConstModifierType(moduleSymbol As PEModuleSymbol, type As TypeSymbol) As Boolean
+            ' VB doesn't deal with ref-readonly parameters or return-types.
+            Return False


If that's the case then we should consider renaming both of these methods. The name simply does not match up to the actual implementation. Consider this code

var isConst = IsConstModifierType(type);

99% of developers who look at that line are going to interpret this as determining if type is the const modifier. In reality it's detecting if a) it's the const modifier and b) the present language supports it. The 1% who will understand the behavior are those who realize the name of the method is wrong.
#Resolved

AlekseyTs · 2017-05-23T17:18:40Z

src/Compilers/Core/Portable/MetadataReader/MetadataDecoder.cs

+        private ImmutableArray<ModifierInfo<TypeSymbol>> DecodeModifiersOrThrow(
+            ref BlobReader signatureReader,
+            out SignatureTypeCode typeCode,
+            Func<TypeSymbol, bool> acceptRequiredModifier = null)


Func<TypeSymbol, bool> acceptRequiredModifier = null [](start = 12, length = 52)

This feels too complicated. I think we can simply pas a boolean whether IsConst modifier should be accepted as a mod_req or not. #Closed

Rather than making changes to this function, I would instead modify DecodeParameterOrThrow to specially handle IsConst modifier in the manner DecodeFieldSignature used to do that for IsVolatile and would keep this function and DecodeFieldSignature unchanged.

In reply to: 118052362 [](ancestors = 118052362)

Will try to introduce in the next iteration a simpler version that doesn't use capturing lambdas. I don't like to duplicate the same logic in 3 places (DecodeModifiers, DecodeParameters, DecodeFields).

In reply to: 118055994 [](ancestors = 118055994,118052362)

AlekseyTs · 2017-05-23T17:26:54Z

src/Compilers/Core/Portable/MetadataReader/MetadataDecoder.cs

                info.CustomModifiers = DecodeModifiersOrThrow(ref signatureReader, out typeCode);
            }
+            else if (isConstFound)


else if (isConstFound) [](start = 12, length = 22)

We could simply check for presence of any required modifiers in info.CustomModifiers. #Closed

Will try to refactor it so that we don't need to do the extra enumeration.

In reply to: 118054332 [](ancestors = 118054332)

AlekseyTs · 2017-05-23T17:29:19Z

src/Compilers/Core/Portable/MetadataReader/MetadataDecoder.cs

                SignatureTypeCode typeCode;
-                ArrayBuilder<ModifierInfo<TypeSymbol>> customModifierBuilder = null;


ArrayBuilder<ModifierInfo> customModifierBuilder = null; [](start = 16, length = 68)

I would revert changes in this function, it doesn't look like we want any behavior change here. #Closed

Will try to introduce in the next iteration a simpler version that doesn't use capturing lambdas. I don't like to duplicate the same logic in 3 places (DecodeModifiers, DecodeParameters, DecodeFields).

In reply to: 118054960 [](ancestors = 118054960)

AlekseyTs · 2017-05-23T17:38:10Z

src/Compilers/Core/Portable/PEWriter/MetadataVisitor.cs

@@ -273,6 +273,7 @@ public virtual void Visit(IMethodDefinition method)
        {
            this.Visit(method.GetReturnValueAttributes(Context));
            this.Visit(method.ReturnValueCustomModifiers);
+            this.Visit(method.RefCustomModifiers);


this.Visit(method.RefCustomModifiers); [](start = 12, length = 38)

It feels like RefCustomModifiers should be visited before ReturnValueCustomModifiers. Also, it looks like there are two more places in ReferenceIndexerBase.cs that should be adjusted in a similar way. #Closed

It looks like there is one more place in ReferenceDependencyWalker.cs as well.

In reply to: 118057254 [](ancestors = 118057254)

AlekseyTs · 2017-05-23T17:50:07Z

src/Compilers/Core/Portable/PEWriter/MetadataVisitor.cs

@@ -412,6 +413,7 @@ public virtual void Visit(IParameterDefinition parameterDefinition)
            Debug.Assert((marshalling != null || !parameterDefinition.MarshallingDescriptor.IsDefaultOrEmpty) == parameterDefinition.IsMarshalledExplicitly);

            this.Visit(parameterDefinition.GetAttributes(Context));
+            this.Visit(parameterDefinition.RefCustomModifiers);


this.Visit(parameterDefinition.RefCustomModifiers); [](start = 12, length = 51)

It looks like there is one more place in ReferenceDependencyWalker.cs that should be adjusted in a similar way. And in public virtual void Visit(IParameterTypeInformation parameterTypeInformation) in this type as well. #Closed

AlekseyTs · 2017-05-23T17:56:34Z

src/Compilers/CSharp/Portable/Symbols/PropertySymbolExtensions.cs

@@ -63,7 +63,7 @@ public static bool HasRefOrOutParameter(this PropertySymbol property)
        {
            foreach (ParameterSymbol param in property.Parameters)
            {
-                if (param.RefKind != RefKind.None)
+                if (param.RefKind == RefKind.Ref || param.RefKind == RefKind.Out)


if (param.RefKind == RefKind.Ref || param.RefKind == RefKind.Out) [](start = 16, length = 65)

Did you add a test backing this change? Could you point to the test(s)? #Closed

Adding IsConstModReqIsConsumedInRefCustomModifiersPosition_Indexers_ReturnTypes. This change basically allows to pass ref-readonly arguments to indexer methods. Passing ordinary arguments is the same to properties as ref-readonly arguments.

In reply to: 118061963 [](ancestors = 118061963)

Adding IsConstModReqIsConsumedInRefCustomModifiersPosition_Indexers_ReturnTypes. This change basically allows to pass ref-readonly arguments to indexer methods. Passing ordinary arguments is the same to properties as ref-readonly arguments.

I couldn't find the test, what file it should be in?

In reply to: 118609191 [](ancestors = 118609191,118061963)

File IsConstModifierTests.cs in C# emit tests project. You can look forIsConstModReqIsConsumedInRefCustomModifiersPosition_Code_Indexers_ReturnTypes now.

In reply to: 119789809 [](ancestors = 119789809,118609191,118061963)

Correction: IsConstModReqIsConsumedInRefCustomModifiersPosition_Code_Indexers_Parameters*

In reply to: 120218968 [](ancestors = 120218968,119789809,118609191,118061963)

AlekseyTs · 2017-05-23T18:05:43Z

src/Compilers/VisualBasic/Test/Emit/Emit/EmitCustomModifiers.vb

@@ -107,5 +108,188 @@ End Class

            CompileAndVerify(comp, expectedOutput:=<![CDATA[2]]>)
        End Sub
+
+        <Fact>
+        Public Sub NonExtensibleReadOnlySignaturesAreRead()


Public Sub NonExtensibleReadOnlySignaturesAreRead() [](start = 8, length = 51)

Consider moving all these tests into a new file dedicated to ReadonlyRef feature. #Closed

AlekseyTs · 2017-05-23T18:07:51Z

src/Compilers/VisualBasic/Test/Emit/Emit/EmitCustomModifiers.vb

+            Dim reference = CreateCSharpCompilation("
+public class TestRef
+{
+    public void M(ref readonly int x)


public void M(ref readonly int x) [](start = 4, length = 33)

This test covers ref readonly parameter. Do we have a similar test for return? #Closed

AlekseyTs · 2017-05-23T18:09:30Z

src/Compilers/VisualBasic/Test/Emit/Emit/EmitCustomModifiers.vb

+Class Test
+    Shared Sub Main() 
+        Dim obj = New TestRef()
+        Dim value = obj.M()


Dim value = [](start = 8, length = 12)

Consider removing the local, leaving gust the method call. #Closed

AlekseyTs · 2017-05-23T18:10:50Z

src/Compilers/VisualBasic/Test/Emit/Emit/EmitCustomModifiers.vb

+public class TestRef
+{
+    private int value = 0;
+    public virtual ref readonly int P => ref value;


public virtual ref readonly int P => ref value; [](start = 4, length = 47)

Do we have a test for a non-virtual property? #Closed

AlekseyTs · 2017-06-02T06:06:24Z

        CompileAndVerify(code, additionalRefs: new[] { reference.EmitToImageReference() }, symbolValidator: module =>

Success scenarios should be executed and runtime behavior should be verified. Applies to all similar tests. #Closed

Refers to: src/Compilers/CSharp/Test/Emit/Emit/IsConstModifierTests.cs:2421 in 42a8f4c. [](commit_id = 42a8f4c, deletion_comment = False)

AlekseyTs · 2017-06-02T06:08:03Z

            var parameter = type.GetMethod("Parent.M").Parameters.Single();

Consider asserting that this method implements the interface. Applies to all similar tests. #Closed

Refers to: src/Compilers/CSharp/Test/Emit/Emit/IsConstModifierTests.cs:2564 in 42a8f4c. [](commit_id = 42a8f4c, deletion_comment = False)

AlekseyTs · 2017-06-02T06:13:22Z

    public void WhenImplementingParentWithModifiersCopyThem_Methods_ReturnTypes_ExplicitInterfaces()

I think for all "copy modifiers" tests it would be interesting to test flavor when compilation define their own IsConst type. Then we actually can observe the copy happening vs. we just added modifiers based on ref readonly usage. #Closed

Refers to: src/Compilers/CSharp/Test/Emit/Emit/IsConstModifierTests.cs:2708 in 42a8f4c. [](commit_id = 42a8f4c, deletion_comment = False)

AlekseyTs · 2017-06-02T06:18:00Z

    public void CreatingLambdasOfDelegatesWithModifiersCanBeExecuted_Parameters()

Similarly should test scenarios when both compilations define their own IsConst type. #Closed

Refers to: src/Compilers/CSharp/Test/Emit/Emit/IsConstModifierTests.cs:3257 in 42a8f4c. [](commit_id = 42a8f4c, deletion_comment = False)

AlekseyTs · 2017-06-02T06:35:42Z

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

-                                                          destinationMethod.ReturnsByRef ? constructedSourceMethod.RefCustomModifiers : ImmutableArray<CustomModifier>.Empty);
+            customModifiers = CustomModifiersTuple.Create(
+                constructedSourceMethod.ReturnTypeCustomModifiers,
+                destinationMethod.ReturnsByRef || destinationMethod.ReturnsByRefReadonly ? constructedSourceMethod.RefCustomModifiers : ImmutableArray<CustomModifier>.Empty);


destinationMethod.ReturnsByRef || destinationMethod.ReturnsByRefReadonly [](start = 16, length = 73)

Could we simply check if RefKind is None? #Closed

AlekseyTs · 2017-06-02T06:40:37Z

Done with review pass. #Closed

AlekseyTs · 2017-06-02T16:48:02Z

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

@@ -167,6 +167,7 @@ private void MethodChecks(MethodDeclarationSyntax syntax, Binder withTypeParamsB
                signatureBinder, this, syntax.ParameterList, out arglistToken,
                allowRefOrOut: true,
                allowThis: true,
+                addIsConstModifier: IsVirtual || IsAbstract,


IsVirtual || IsAbst [](start = 36, length = 19)

This should probably include checks for override and explicit implementation, those are virtual (in metadata terms) as well and the difference will be observable for cases when the target method cannot be found, etc. #Closed

Filed #20053 for that.

In reply to: 119902520 [](ancestors = 119902520)

AlekseyTs · 2017-06-02T16:48:31Z

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

@@ -1388,7 +1397,7 @@ private TypeSymbol ComputeType(Binder binder, BasePropertyDeclarationSyntax synt
        private ImmutableArray<ParameterSymbol> ComputeParameters(Binder binder, BasePropertyDeclarationSyntax syntax, DiagnosticBag diagnostics)
        {
            var parameterSyntaxOpt = GetParameterListSyntax(syntax);
-            var parameters = MakeParameters(binder, this, parameterSyntaxOpt, diagnostics);
+            var parameters = MakeParameters(binder, this, parameterSyntaxOpt, diagnostics, addIsConstModifier: IsVirtual || IsAbstract);


IsVirtual || IsAbstract [](start = 111, length = 23)

Same comment as for methods. #Closed

OmarTawfik · 2017-06-07T00:46:43Z

            var parameter = type.GetMethod("M").Parameters.Single();

This is not an explicit implementation. comment misplaced? If the method is virtual, we don't create explicit implementations. Only for non-virtuals. I modified the other tests.

In reply to: 305697978 [](ancestors = 305697978)

Refers to: src/Compilers/CSharp/Test/Emit/Emit/IsConstModifierTests.cs:2530 in 42a8f4c. [](commit_id = 42a8f4c, deletion_comment = False)

OmarTawfik · 2017-06-07T21:17:58Z

    public void WhenImplementingParentWithModifiersCopyThem_Methods_ReturnTypes_ExplicitInterfaces()

Please check the tests starting with UsingIsConstFromReferenceWhileHavingDuplicateInCompilation_*.
We generate the modreqs based on the available type because (in most cases, unless the interface was specificied explicitly) we don't know which interface this method might belong to.
In these cases, we will generate a proxy function that has the other type.
We can iterate over this behaviour (which I think applies globally, not for this PR only) in #20053

In reply to: 305699094 [](ancestors = 305699094)

Refers to: src/Compilers/CSharp/Test/Emit/Emit/IsConstModifierTests.cs:2708 in 42a8f4c. [](commit_id = 42a8f4c, deletion_comment = False)

OmarTawfik · 2017-06-08T18:26:11Z

@AlekseyTs comments addressed. #Closed

OmarTawfik · 2017-06-08T19:32:32Z

@cston @jaredpar @VSadov any other comments? #Closed

AlekseyTs · 2017-06-09T17:34:36Z

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

+                if (_refKind == RefKind.RefReadOnly)
+                {
+                    var isConstType = binder.GetWellKnownType(WellKnownType.System_Runtime_CompilerServices_IsConst, diagnostics, syntax.ReturnType);
+                    _refCustomModifiers = ImmutableArray.Create(CSharpCustomModifier.CreateRequired(isConstType));


_refCustomModifiers = ImmutableArray.Create(CSharpCustomModifier.CreateRequired(isConstType)); [](start = 20, length = 94)

It feels like we could simply reuse modifiers from the invoke. #Closed

AlekseyTs · 2017-06-09T17:38:09Z

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

@@ -342,15 +356,18 @@ internal override OneOrMany<SyntaxList<AttributeListSyntax>> GetReturnTypeAttrib

        private sealed class EndInvokeMethod : SourceDelegateMethodSymbol
        {
-            private readonly RefKind refKind;
+            private readonly RefKind _refKind;
+            private readonly ImmutableArray<CustomModifier> _refCustomModifiers;


private readonly ImmutableArray _refCustomModifiers; [](start = 12, length = 68)

Consider replacing two these members with a reference to invoke, then implementation of RefKind and RefCustomModifiers could simply delegate to the invoke. #Closed

AlekseyTs · 2017-06-09T18:10:12Z

        CompileAndVerify(code, additionalRefs: new[] { reference }, expectedOutput: "5");

I noticed that you cloned the entire unit-test in order to test this scenario against compilation reference. This feel like an overkill to me, both scenarios could easily be tested right here:

CompileAndVerify(code, additionalRefs: new[] { compilation1.ToMetedataReference() }, expectedOutput: "5");
CompileAndVerify(code, additionalRefs: new[] { compilation1.EmitToImageReference() }, expectedOutput: "5");

This isn't a blocker, but it causes an unnecessary code duplication and work duplication while tests are executed. #Closed

Refers to: src/Compilers/CSharp/Test/Emit/Emit/IsConstModifierTests.cs:38 in b32dc89. [](commit_id = b32dc89, deletion_comment = False)

AlekseyTs · 2017-06-09T18:30:42Z

        CompileAndVerify(code, additionalRefs: new[] { reference.EmitToImageReference() }, expectedOutput: "5");

Please add a call (applies to all similar tests)

CompileAndVerify(code, additionalRefs: new[] { reference.ToMetadataReference() }, expectedOutput: "5");
``` #Closed

---
Refers to: src/Compilers/CSharp/Test/Emit/Emit/IsConstModifierTests.cs:3848 in b32dc89. [](commit_id = b32dc89aaa354d3b23a18534f8b4bbe076c1b51a, deletion_comment = False)

AlekseyTs · 2017-06-09T19:04:07Z

Done with review pass. #Closed

VSadov · 2017-06-09T19:15:57Z

@OmarTawfik - I have no actionable comments on this. Except - Can we wait with merging this until after the ref-extension are merged in and there is a build that contains them?

This would be the first change that intentionally breaks "compatibility" with old compilers. It would be nice to have a builds before/after - just for comparison reasons.

OmarTawfik · 2017-06-13T21:02:31Z

        CompileAndVerify(code, additionalRefs: new[] { reference }, expectedOutput: "5");

Tried to isolate the root cause. If a test fails because of the difference between metadata/image references, then one of them should succeed.
But taking a look at the entirety of the tests added, it is going to be a few thousands of lines to maintain. Will refactor.

In reply to: 307460674 [](ancestors = 307460674)

Refers to: src/Compilers/CSharp/Test/Emit/Emit/IsConstModifierTests.cs:38 in b32dc89. [](commit_id = b32dc89, deletion_comment = False)

OmarTawfik · 2017-06-14T17:47:22Z

@AlekseyTs done with latest set of comments.

AlekseyTs

LGTM

OmarTawfik added Area-Compilers Feature - Readonly References Readonly References PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. labels May 19, 2017

dnfclas added the cla-already-signed label May 19, 2017

OmarTawfik commented May 19, 2017

View reviewed changes

Adding IsConst modreq on ref readonly signatures

e6b31c1

OmarTawfik removed the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label May 19, 2017

jaredpar reviewed May 22, 2017

View reviewed changes

OmarTawfik commented May 22, 2017

View reviewed changes

VSadov reviewed May 22, 2017

View reviewed changes

VSadov reviewed May 23, 2017

View reviewed changes

cston reviewed May 23, 2017

View reviewed changes

jaredpar reviewed May 23, 2017

View reviewed changes

AlekseyTs reviewed May 23, 2017

View reviewed changes

AlekseyTs reviewed Jun 2, 2017

View reviewed changes

OmarTawfik mentioned this pull request Jun 6, 2017

Symbols should construct their own modifiers appropriately before looking into overridden symbols #20053

Open

Responded to PR feedback

b32dc89

AlekseyTs reviewed Jun 9, 2017

View reviewed changes

jcouv modified the milestone: 15.6 Jun 12, 2017

More PR Comments

15c5a6c

AlekseyTs approved these changes Jun 14, 2017

View reviewed changes

OmarTawfik mentioned this pull request Jun 21, 2017

Replace IsConst modreq with InAttribute #20254

Merged

Resolve merge conflicts

743a3bc

VSadov merged commit 55d359d into dotnet:features/readonly-ref Jun 23, 2017

OmarTawfik deleted the isconst-modreqs branch July 19, 2017 18:25


		for (;;)
		customModifiers = DecodeModifiersOrThrow(ref signatureReader, out typeCode, acceptRequiredModifier: type =>

		SignatureTypeCode typeCode;
		ArrayBuilder<ModifierInfo<TypeSymbol>> customModifierBuilder = null;

Adding IsConst modreq on ref readonly signatures #19658

Adding IsConst modreq on ref readonly signatures #19658

Conversation

OmarTawfik commented May 19, 2017 • edited Loading

OmarTawfik May 19, 2017 • edited Loading

Choose a reason for hiding this comment

jaredpar May 22, 2017 • edited by OmarTawfik Loading

Choose a reason for hiding this comment

OmarTawfik May 22, 2017 • edited Loading

Choose a reason for hiding this comment

cston May 22, 2017 • edited by OmarTawfik Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaredpar May 23, 2017 • edited by OmarTawfik Loading

Choose a reason for hiding this comment

AlekseyTs May 23, 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

OmarTawfik May 19, 2017 • edited Loading

Choose a reason for hiding this comment

OmarTawfik commented May 22, 2017 • edited Loading

jaredpar May 22, 2017 • edited by OmarTawfik Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaredpar May 23, 2017 • edited by OmarTawfik Loading

Choose a reason for hiding this comment

jaredpar May 22, 2017 • edited by OmarTawfik Loading

Choose a reason for hiding this comment

jaredpar May 22, 2017 • edited by OmarTawfik Loading

Choose a reason for hiding this comment

jaredpar May 22, 2017 • edited by OmarTawfik Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaredpar May 23, 2017 • edited by OmarTawfik Loading

Choose a reason for hiding this comment

OmarTawfik May 24, 2017 • edited Loading

Choose a reason for hiding this comment

jaredpar May 22, 2017 • edited by OmarTawfik Loading

Choose a reason for hiding this comment

OmarTawfik May 22, 2017 • edited Loading

Choose a reason for hiding this comment

VSadov May 22, 2017 • edited by OmarTawfik Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VSadov May 23, 2017 • edited by OmarTawfik Loading

Choose a reason for hiding this comment

AlekseyTs May 23, 2017 • edited Loading

Choose a reason for hiding this comment

VSadov commented May 23, 2017

cston May 23, 2017 • edited by OmarTawfik Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston May 23, 2017 • edited by OmarTawfik Loading

Choose a reason for hiding this comment

cston May 23, 2017 • edited by OmarTawfik Loading

Choose a reason for hiding this comment

cston May 23, 2017 • edited by OmarTawfik Loading

Choose a reason for hiding this comment

jaredpar May 23, 2017 • edited by OmarTawfik Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaredpar May 23, 2017 • edited by OmarTawfik Loading

Choose a reason for hiding this comment

jaredpar May 23, 2017 • edited by OmarTawfik Loading

Choose a reason for hiding this comment

AlekseyTs May 23, 2017 • edited Loading

Choose a reason for hiding this comment

jaredpar May 23, 2017 • edited by OmarTawfik Loading

Choose a reason for hiding this comment

jaredpar May 23, 2017 • edited by OmarTawfik Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaredpar May 23, 2017 • edited by OmarTawfik Loading

Choose a reason for hiding this comment

AlekseyTs May 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OmarTawfik commented May 19, 2017 •

edited

Loading

OmarTawfik May 19, 2017 •

edited

Loading

jaredpar May 22, 2017 •

edited by OmarTawfik

Loading

OmarTawfik May 22, 2017 •

edited

Loading

cston May 22, 2017 •

edited by OmarTawfik

Loading

jaredpar May 23, 2017 •

edited by OmarTawfik

Loading

AlekseyTs May 23, 2017 •

edited

Loading

OmarTawfik May 19, 2017 •

edited

Loading

OmarTawfik commented May 22, 2017 •

edited

Loading

jaredpar May 22, 2017 •

edited by OmarTawfik

Loading

jaredpar May 23, 2017 •

edited by OmarTawfik

Loading

jaredpar May 22, 2017 •

edited by OmarTawfik

Loading

jaredpar May 22, 2017 •

edited by OmarTawfik

Loading

jaredpar May 22, 2017 •

edited by OmarTawfik

Loading

jaredpar May 23, 2017 •

edited by OmarTawfik

Loading

OmarTawfik May 24, 2017 •

edited

Loading

jaredpar May 22, 2017 •

edited by OmarTawfik

Loading

OmarTawfik May 22, 2017 •

edited

Loading

VSadov May 22, 2017 •

edited by OmarTawfik

Loading

VSadov May 23, 2017 •

edited by OmarTawfik

Loading

AlekseyTs May 23, 2017 •

edited

Loading

cston May 23, 2017 •

edited by OmarTawfik

Loading

cston May 23, 2017 •

edited by OmarTawfik

Loading

cston May 23, 2017 •

edited by OmarTawfik

Loading

cston May 23, 2017 •

edited by OmarTawfik

Loading

jaredpar May 23, 2017 •

edited by OmarTawfik

Loading

jaredpar May 23, 2017 •

edited by OmarTawfik

Loading

jaredpar May 23, 2017 •

edited by OmarTawfik

Loading

AlekseyTs May 23, 2017 •

edited

Loading

jaredpar May 23, 2017 •

edited by OmarTawfik

Loading

jaredpar May 23, 2017 •

edited by OmarTawfik

Loading

jaredpar May 23, 2017 •

edited by OmarTawfik

Loading

AlekseyTs May 23, 2017 •

edited

Loading

AlekseyTs May 23, 2017 •

edited

Loading

AlekseyTs May 23, 2017 •

edited

Loading

AlekseyTs May 23, 2017 •

edited

Loading

AlekseyTs May 23, 2017 •

edited

Loading

AlekseyTs May 23, 2017 •

edited

Loading

OmarTawfik May 25, 2017 •

edited

Loading

OmarTawfik Jun 5, 2017 •

edited

Loading

AlekseyTs May 23, 2017 •

edited

Loading

AlekseyTs May 23, 2017 •

edited

Loading

AlekseyTs May 23, 2017 •

edited

Loading

AlekseyTs May 23, 2017 •

edited

Loading

AlekseyTs commented Jun 2, 2017 •

edited

Loading

AlekseyTs commented Jun 2, 2017 •

edited

Loading

AlekseyTs commented Jun 2, 2017 •

edited

Loading

AlekseyTs commented Jun 2, 2017 •

edited

Loading

AlekseyTs Jun 2, 2017 •

edited

Loading

AlekseyTs commented Jun 2, 2017 •

edited by OmarTawfik

Loading

AlekseyTs Jun 2, 2017 •

edited

Loading

AlekseyTs Jun 2, 2017 •

edited

Loading

OmarTawfik commented Jun 7, 2017 •

edited

Loading

OmarTawfik commented Jun 8, 2017 •

edited

Loading

OmarTawfik commented Jun 8, 2017 •

edited

Loading

AlekseyTs Jun 9, 2017 •

edited

Loading