From a514e140497a1de26f8f4ddc9286e4c50e565baf Mon Sep 17 00:00:00 2001 From: Rikki Gibson Date: Fri, 1 Mar 2019 13:16:47 -0800 Subject: [PATCH 1/7] Add MethodSymbol.IsReadOnly API --- .../AnonymousType.SynthesizedMethodBase.cs | 2 ++ src/Compilers/CSharp/Portable/Symbols/ErrorMethodSymbol.cs | 2 ++ .../CSharp/Portable/Symbols/Metadata/PE/PEMethodSymbol.cs | 3 +++ src/Compilers/CSharp/Portable/Symbols/MethodSymbol.cs | 7 +++++++ .../Portable/Symbols/ReducedExtensionMethodSymbol.cs | 3 +++ .../CSharp/Portable/Symbols/SignatureOnlyMethodSymbol.cs | 2 ++ .../CSharp/Portable/Symbols/Source/LambdaSymbol.cs | 2 ++ .../CSharp/Portable/Symbols/Source/LocalFunctionSymbol.cs | 2 ++ .../Portable/Symbols/Source/SourceMemberMethodSymbol.cs | 2 +- .../Symbols/Synthesized/SynthesizedEntryPointSymbol.cs | 2 ++ .../Symbols/Synthesized/SynthesizedGlobalMethodSymbol.cs | 2 ++ .../Symbols/Synthesized/SynthesizedInstanceMethodSymbol.cs | 2 ++ .../Synthesized/SynthesizedIntrinsicOperatorSymbol.cs | 2 ++ .../Symbols/Synthesized/SynthesizedStaticConstructor.cs | 2 ++ .../CSharp/Portable/Symbols/Wrapped/WrappedMethodSymbol.cs | 2 ++ .../CSharp/Test/Semantic/Semantics/ReadOnlyStructsTests.cs | 3 +-- .../Source/ExpressionCompiler/Symbols/EEMethodSymbol.cs | 2 ++ .../ExpressionCompiler/Symbols/PlaceholderMethodSymbol.cs | 2 ++ 18 files changed, 41 insertions(+), 3 deletions(-) diff --git a/src/Compilers/CSharp/Portable/Symbols/AnonymousTypes/SynthesizedSymbols/AnonymousType.SynthesizedMethodBase.cs b/src/Compilers/CSharp/Portable/Symbols/AnonymousTypes/SynthesizedSymbols/AnonymousType.SynthesizedMethodBase.cs index e22eee9aa202c..bc05f44a8c2c7 100644 --- a/src/Compilers/CSharp/Portable/Symbols/AnonymousTypes/SynthesizedSymbols/AnonymousType.SynthesizedMethodBase.cs +++ b/src/Compilers/CSharp/Portable/Symbols/AnonymousTypes/SynthesizedSymbols/AnonymousType.SynthesizedMethodBase.cs @@ -117,6 +117,8 @@ public sealed override ImmutableArray ExplicitInterfaceImplementat get { return ImmutableArray.Empty; } } + internal sealed override bool IsReadOnly => true; + public sealed override ImmutableArray RefCustomModifiers { get { return ImmutableArray.Empty; } diff --git a/src/Compilers/CSharp/Portable/Symbols/ErrorMethodSymbol.cs b/src/Compilers/CSharp/Portable/Symbols/ErrorMethodSymbol.cs index e76f5d85a21b1..3f27ab1cd02fb 100644 --- a/src/Compilers/CSharp/Portable/Symbols/ErrorMethodSymbol.cs +++ b/src/Compilers/CSharp/Portable/Symbols/ErrorMethodSymbol.cs @@ -124,6 +124,8 @@ public override ImmutableArray ExplicitInterfaceImplementations get { return ImmutableArray.Empty; } } + internal override bool IsReadOnly => false; + internal override int ParameterCount { get { return 0; } diff --git a/src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEMethodSymbol.cs b/src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEMethodSymbol.cs index 0904c4e1ca922..495d02a3247e6 100644 --- a/src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEMethodSymbol.cs +++ b/src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEMethodSymbol.cs @@ -1027,6 +1027,9 @@ public override ImmutableArray ExplicitInterfaceImplementations } } + // PROTOTYPE: need to round trip readonly attribute in metadata + internal override bool IsReadOnly => false; + public override string GetDocumentationCommentXml(CultureInfo preferredCulture = null, bool expandIncludes = false, CancellationToken cancellationToken = default(CancellationToken)) { return PEDocumentationCommentUtils.GetDocumentationComment(this, _containingType.ContainingPEModule, preferredCulture, cancellationToken, ref AccessUncommonFields()._lazyDocComment); diff --git a/src/Compilers/CSharp/Portable/Symbols/MethodSymbol.cs b/src/Compilers/CSharp/Portable/Symbols/MethodSymbol.cs index 1fe40eaab52cb..18f35ae3004f8 100644 --- a/src/Compilers/CSharp/Portable/Symbols/MethodSymbol.cs +++ b/src/Compilers/CSharp/Portable/Symbols/MethodSymbol.cs @@ -297,6 +297,13 @@ internal virtual bool IsExplicitInterfaceImplementation get { return ExplicitInterfaceImplementations.Any(); } } + // PROTOTYPE: this should become public API + /// + /// Indicates whether the method is readonly, i.e. + /// whether 'this' is readonly in the scope of the method. + /// + internal abstract bool IsReadOnly { get; } + /// /// Returns interface methods explicitly implemented by this method. /// diff --git a/src/Compilers/CSharp/Portable/Symbols/ReducedExtensionMethodSymbol.cs b/src/Compilers/CSharp/Portable/Symbols/ReducedExtensionMethodSymbol.cs index 08d47ff81fa3a..9adc825257f08 100644 --- a/src/Compilers/CSharp/Portable/Symbols/ReducedExtensionMethodSymbol.cs +++ b/src/Compilers/CSharp/Portable/Symbols/ReducedExtensionMethodSymbol.cs @@ -373,6 +373,9 @@ internal override bool IsExplicitInterfaceImplementation get { return false; } } + // extension method has no 'this' parameter + internal override bool IsReadOnly => false; + public override ImmutableArray ExplicitInterfaceImplementations { get { return ImmutableArray.Empty; } diff --git a/src/Compilers/CSharp/Portable/Symbols/SignatureOnlyMethodSymbol.cs b/src/Compilers/CSharp/Portable/Symbols/SignatureOnlyMethodSymbol.cs index f06a75b85abca..0be076d2fc839 100644 --- a/src/Compilers/CSharp/Portable/Symbols/SignatureOnlyMethodSymbol.cs +++ b/src/Compilers/CSharp/Portable/Symbols/SignatureOnlyMethodSymbol.cs @@ -143,6 +143,8 @@ internal override bool IsMetadataFinal } } + internal override bool IsReadOnly => false; + internal override int CalculateLocalSyntaxOffset(int localPosition, SyntaxTree localTree) { throw ExceptionUtilities.Unreachable; } #endregion diff --git a/src/Compilers/CSharp/Portable/Symbols/Source/LambdaSymbol.cs b/src/Compilers/CSharp/Portable/Symbols/Source/LambdaSymbol.cs index 8c5e11afb6635..17eeb52a5d49d 100644 --- a/src/Compilers/CSharp/Portable/Symbols/Source/LambdaSymbol.cs +++ b/src/Compilers/CSharp/Portable/Symbols/Source/LambdaSymbol.cs @@ -396,6 +396,8 @@ internal override bool GenerateDebugInfo get { return true; } } + internal override bool IsReadOnly => false; + public override ImmutableArray GetTypeParameterConstraintClauses(bool early) => ImmutableArray.Empty; internal override int CalculateLocalSyntaxOffset(int localPosition, SyntaxTree localTree) diff --git a/src/Compilers/CSharp/Portable/Symbols/Source/LocalFunctionSymbol.cs b/src/Compilers/CSharp/Portable/Symbols/Source/LocalFunctionSymbol.cs index 204f98601810e..62450dbb23bca 100644 --- a/src/Compilers/CSharp/Portable/Symbols/Source/LocalFunctionSymbol.cs +++ b/src/Compilers/CSharp/Portable/Symbols/Source/LocalFunctionSymbol.cs @@ -360,6 +360,8 @@ internal override TypeSymbol IteratorElementType internal bool IsExpressionBodied => _syntax.Body == null && _syntax.ExpressionBody != null; + internal override bool IsReadOnly => false; + public override DllImportData GetDllImportData() => null; internal override ImmutableArray GetAppliedConditionalSymbols() => ImmutableArray.Empty; diff --git a/src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberMethodSymbol.cs b/src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberMethodSymbol.cs index 07f84a46584dc..4a0c985db89d6 100644 --- a/src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberMethodSymbol.cs +++ b/src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberMethodSymbol.cs @@ -509,7 +509,7 @@ public sealed override bool IsAsync } } - internal bool IsReadOnly + internal override bool IsReadOnly { get { diff --git a/src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedEntryPointSymbol.cs b/src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedEntryPointSymbol.cs index 4535060e17872..ac0e5750d014e 100644 --- a/src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedEntryPointSymbol.cs +++ b/src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedEntryPointSymbol.cs @@ -208,6 +208,8 @@ public override ImmutableArray ExplicitInterfaceImplementations get { return ImmutableArray.Empty; } } + internal sealed override bool IsReadOnly => false; + internal sealed override bool IsMetadataNewSlot(bool ignoreInterfaceImplementationChanges = false) { return false; diff --git a/src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedGlobalMethodSymbol.cs b/src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedGlobalMethodSymbol.cs index 342844cb7227d..5474bde697d4d 100644 --- a/src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedGlobalMethodSymbol.cs +++ b/src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedGlobalMethodSymbol.cs @@ -300,6 +300,8 @@ public override ImmutableArray ExplicitInterfaceImplementations get { return ImmutableArray.Empty; } } + internal sealed override bool IsReadOnly => false; + internal override bool SynthesizesLoweredBoundBody { get { return true; } diff --git a/src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedInstanceMethodSymbol.cs b/src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedInstanceMethodSymbol.cs index deb83e727e26b..7ca0322631b23 100644 --- a/src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedInstanceMethodSymbol.cs +++ b/src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedInstanceMethodSymbol.cs @@ -56,5 +56,7 @@ internal override int CalculateLocalSyntaxOffset(int localPosition, SyntaxTree l { throw ExceptionUtilities.Unreachable; } + + internal override bool IsReadOnly => false; } } diff --git a/src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedIntrinsicOperatorSymbol.cs b/src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedIntrinsicOperatorSymbol.cs index 25e666c164f15..a4c33aeb9f59d 100644 --- a/src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedIntrinsicOperatorSymbol.cs +++ b/src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedIntrinsicOperatorSymbol.cs @@ -261,6 +261,8 @@ public override ImmutableArray ExplicitInterfaceImplementations } } + internal override bool IsReadOnly => true; + public override ImmutableArray RefCustomModifiers { get diff --git a/src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedStaticConstructor.cs b/src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedStaticConstructor.cs index 34c71ecf4f9fb..ce74c34ee2798 100644 --- a/src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedStaticConstructor.cs +++ b/src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedStaticConstructor.cs @@ -278,6 +278,8 @@ public override ImmutableArray ExplicitInterfaceImplementations } } + internal override bool IsReadOnly => false; + public sealed override bool IsImplicitlyDeclared { get diff --git a/src/Compilers/CSharp/Portable/Symbols/Wrapped/WrappedMethodSymbol.cs b/src/Compilers/CSharp/Portable/Symbols/Wrapped/WrappedMethodSymbol.cs index 095f94414ea6e..cb2b39b2fb194 100644 --- a/src/Compilers/CSharp/Portable/Symbols/Wrapped/WrappedMethodSymbol.cs +++ b/src/Compilers/CSharp/Portable/Symbols/Wrapped/WrappedMethodSymbol.cs @@ -320,5 +320,7 @@ internal override bool GenerateDebugInfo return UnderlyingMethod.GenerateDebugInfo; } } + + internal override bool IsReadOnly => UnderlyingMethod.IsReadOnly; } } diff --git a/src/Compilers/CSharp/Test/Semantic/Semantics/ReadOnlyStructsTests.cs b/src/Compilers/CSharp/Test/Semantic/Semantics/ReadOnlyStructsTests.cs index 43a5e0360b496..11cf063678750 100644 --- a/src/Compilers/CSharp/Test/Semantic/Semantics/ReadOnlyStructsTests.cs +++ b/src/Compilers/CSharp/Test/Semantic/Semantics/ReadOnlyStructsTests.cs @@ -268,8 +268,7 @@ public readonly int M() var comp = CreateCompilation(csharp); comp.VerifyDiagnostics(); - var method = (SourceMemberMethodSymbol)comp.GetMember("S").GetMember("M"); - // PROTOTYPE: add `public abstract bool IsReadOnly` to MethodSymbol and implement in subtypes? + var method = comp.GetMember("S").GetMember("M"); Assert.True(method.IsReadOnly); } diff --git a/src/ExpressionEvaluator/CSharp/Source/ExpressionCompiler/Symbols/EEMethodSymbol.cs b/src/ExpressionEvaluator/CSharp/Source/ExpressionCompiler/Symbols/EEMethodSymbol.cs index 540304a714c85..0a502721cb21d 100644 --- a/src/ExpressionEvaluator/CSharp/Source/ExpressionCompiler/Symbols/EEMethodSymbol.cs +++ b/src/ExpressionEvaluator/CSharp/Source/ExpressionCompiler/Symbols/EEMethodSymbol.cs @@ -405,6 +405,8 @@ public override bool IsExtern get { return false; } } + internal override bool IsReadOnly => false; + internal override ObsoleteAttributeData ObsoleteAttributeData { get { throw ExceptionUtilities.Unreachable; } diff --git a/src/ExpressionEvaluator/CSharp/Source/ExpressionCompiler/Symbols/PlaceholderMethodSymbol.cs b/src/ExpressionEvaluator/CSharp/Source/ExpressionCompiler/Symbols/PlaceholderMethodSymbol.cs index 731ab2f96dc1c..4eea69bf5e589 100644 --- a/src/ExpressionEvaluator/CSharp/Source/ExpressionCompiler/Symbols/PlaceholderMethodSymbol.cs +++ b/src/ExpressionEvaluator/CSharp/Source/ExpressionCompiler/Symbols/PlaceholderMethodSymbol.cs @@ -118,6 +118,8 @@ public override bool IsVirtual get { return false; } } + internal override bool IsReadOnly => false; + public override ImmutableArray Locations { get { return ImmutableArray.Empty; } From 2ae16a57b170625a6ea9ca518062dbad0b6bd771 Mon Sep 17 00:00:00 2001 From: Rikki Gibson Date: Thu, 7 Mar 2019 15:53:39 -0800 Subject: [PATCH 2/7] Update emitter to copy 'this' for non-readonly method calls in readonly methods --- .../Portable/Binder/Binder.ValueChecks.cs | 2 +- .../CSharp/Portable/CodeGen/EmitAddress.cs | 7 + .../CSharp/Portable/CodeGen/EmitExpression.cs | 2 +- .../Semantics/ReadOnlyStructsTests.cs | 306 ++++++++++++++++++ 4 files changed, 315 insertions(+), 2 deletions(-) diff --git a/src/Compilers/CSharp/Portable/Binder/Binder.ValueChecks.cs b/src/Compilers/CSharp/Portable/Binder/Binder.ValueChecks.cs index 4ce3903ef64ac..09564fe0265cc 100644 --- a/src/Compilers/CSharp/Portable/Binder/Binder.ValueChecks.cs +++ b/src/Compilers/CSharp/Portable/Binder/Binder.ValueChecks.cs @@ -2990,7 +2990,7 @@ internal static bool HasHome( return true; } - if (!IsAnyReadOnly(addressKind) && type.IsReadOnly) + if (!IsAnyReadOnly(addressKind) && (type.IsReadOnly || method.IsReadOnly)) { return method.MethodKind == MethodKind.Constructor; } diff --git a/src/Compilers/CSharp/Portable/CodeGen/EmitAddress.cs b/src/Compilers/CSharp/Portable/CodeGen/EmitAddress.cs index 3c41c99cab655..ce2f43744d014 100644 --- a/src/Compilers/CSharp/Portable/CodeGen/EmitAddress.cs +++ b/src/Compilers/CSharp/Portable/CodeGen/EmitAddress.cs @@ -64,6 +64,13 @@ private LocalDefinition EmitAddress(BoundExpression expression, AddressKind addr if (expression.Type.IsValueType) { + + if (addressKind == AddressKind.Writeable && _method.IsReadOnly) + { + // a readonly method is calling a non-readonly method, therefore we need to copy 'this' + goto default; + } + _builder.EmitLoadArgumentOpcode(0); } else diff --git a/src/Compilers/CSharp/Portable/CodeGen/EmitExpression.cs b/src/Compilers/CSharp/Portable/CodeGen/EmitExpression.cs index 36c3c097efdaf..edba7907bf673 100644 --- a/src/Compilers/CSharp/Portable/CodeGen/EmitExpression.cs +++ b/src/Compilers/CSharp/Portable/CodeGen/EmitExpression.cs @@ -1688,7 +1688,7 @@ private bool IsReadOnlyCall(MethodSymbol method, NamedTypeSymbol methodContainin { Debug.Assert(methodContainingType.IsVerifierValue(), "only struct calls can be readonly"); - if (methodContainingType.IsReadOnly && method.MethodKind != MethodKind.Constructor) + if ((method.IsReadOnly || methodContainingType.IsReadOnly) && method.MethodKind != MethodKind.Constructor) { return true; } diff --git a/src/Compilers/CSharp/Test/Semantic/Semantics/ReadOnlyStructsTests.cs b/src/Compilers/CSharp/Test/Semantic/Semantics/ReadOnlyStructsTests.cs index 11cf063678750..507ddb8b30cc8 100644 --- a/src/Compilers/CSharp/Test/Semantic/Semantics/ReadOnlyStructsTests.cs +++ b/src/Compilers/CSharp/Test/Semantic/Semantics/ReadOnlyStructsTests.cs @@ -154,6 +154,312 @@ interface I1 ); } + [Fact] + public void WriteableInstanceFields_ReadOnlyMethod() + { + var text = @" +public struct A +{ + public static int s; + + public int x; + + readonly void AssignField() + { + // error + this.x = 1; + + A a = default; + // OK + a.x = 3; + // OK + s = 5; + } +} +"; + // PROTOTYPE: should give ERR_AssgReadonlyLocal + CreateCompilation(text).VerifyDiagnostics(); + } + + [Fact] + public void ReadOnlyMethod_PassThisByRef() + { + var csharp = @" +public struct S +{ + public static void M1(ref S s) {} + public static void M2(in S s) {} + + public void M2() + { + M1(ref this); // ok + M2(in this); // ok + } + + public readonly void M3() + { + M1(ref this); // error + M2(in this); // ok + } +} +"; + var comp = CreateCompilation(csharp); + // PROTOTYPE: should give ERR_RefReadonlyLocal + comp.VerifyDiagnostics(); + } + + [Fact] + public void ReadOnlyMethod_PassFieldByRef() + { + var csharp = @" +public struct S +{ + public static int f1; + public int f2; + + public static void M1(ref int s) {} + public static void M2(in int s) {} + + public void M3() + { + M1(ref f1); // ok + M1(ref f2); // ok + M2(in f1); // ok + M2(in f2); // ok + } + + public readonly void M4() + { + M1(ref f1); // ok + M1(ref f2); // error + M2(in f1); // ok + M2(in f2); // ok + } +} +"; + var comp = CreateCompilation(csharp); + // PROTOTYPE: should give ERR_RefReadonlyLocal + comp.VerifyDiagnostics(); + } + + [Fact] + public void ReadOnlyMethod_CallReadOnlyMethod() + { + var csharp = @" +public struct S +{ + public int i; + public readonly int M1() => M2() + 1; + public readonly int M2() => i; +} +"; + var comp = CompileAndVerify(csharp); + comp.VerifyIL("S.M1", @" +{ + // Code size 9 (0x9) + .maxstack 2 + IL_0000: ldarg.0 + IL_0001: call ""int S.M2()"" + IL_0006: ldc.i4.1 + IL_0007: add + IL_0008: ret +} +"); + comp.VerifyDiagnostics(); + } + + [Fact] + public void ReadOnlyMethod_CallStaticMethod() + { + var csharp = @" +public struct S +{ + public static int i; + public readonly int M1() => M2() + 1; + public static int M2() => i; +} +"; + var comp = CompileAndVerify(csharp); + comp.VerifyDiagnostics(); + } + + [Fact] + public void ReadOnlyMethod_CallNormalMethod() + { + var csharp = @" +public struct S +{ + public int i; + + public readonly void M1() + { + // should create local copy + M2(); + System.Console.Write(i); + + // explicit local copy, no warning + var copy = this; + copy.M2(); + System.Console.Write(copy.i); + } + + void M2() + { + i = 23; + } + + static void Main() + { + var s = new S { i = 1 }; + s.M1(); + } +} +"; + + var verifier = CompileAndVerify(csharp, expectedOutput: "123"); + // PROTOTYPE: should warn about copying 'this' when calling M2 + verifier.VerifyDiagnostics(); + verifier.VerifyIL("S.M1", @" +{ + // Code size 51 (0x33) + .maxstack 1 + .locals init (S V_0, //copy + S V_1) + IL_0000: ldarg.0 + IL_0001: ldobj ""S"" + IL_0006: stloc.1 + IL_0007: ldloca.s V_1 + IL_0009: call ""void S.M2()"" + IL_000e: ldarg.0 + IL_000f: ldfld ""int S.i"" + IL_0014: call ""void System.Console.Write(int)"" + IL_0019: ldarg.0 + IL_001a: ldobj ""S"" + IL_001f: stloc.0 + IL_0020: ldloca.s V_0 + IL_0022: call ""void S.M2()"" + IL_0027: ldloc.0 + IL_0028: ldfld ""int S.i"" + IL_002d: call ""void System.Console.Write(int)"" + IL_0032: ret +}"); + } + + [Fact] + public void ReadOnlyAccessor_CallNormalMethod() + { + var csharp = @" +public struct S +{ + public int i; + + public int P + { + readonly get + { + // should create local copy + M(); + System.Console.Write(i); + + // explicit local copy, no warning + var copy = this; + copy.M(); + System.Console.Write(copy.i); + + return i; + } + } + + void M() + { + i = 23; + } + + static void Main() + { + var s = new S { i = 1 }; + _ = s.P; + } +} +"; + + var verifier = CompileAndVerify(csharp, expectedOutput: "123"); + // PROTOTYPE: should warn about copying 'this' when calling M + verifier.VerifyDiagnostics(); + } + + [Fact] + public void ReadOnlyStruct_CallNormalMethodOnField() + { + var csharp = @" +public readonly struct S1 +{ + public readonly S2 s2; + public void M1() + { + s2.M2(); + } +} + +public struct S2 +{ + public int i; + public void M2() + { + i = 42; + } + + static void Main() + { + var s1 = new S1(); + s1.M1(); + System.Console.Write(s1.s2.i); + } +} +"; + CompileAndVerify(csharp, expectedOutput: "0"); + } + + [Fact] + public void ReadOnlyMethod_CallNormalMethodOnField() + { + var csharp = @" +public struct S1 +{ + public S2 s2; + public readonly void M1() + { + // warn on local copy + s2.M2(); + System.Console.Write(s2.i); + + // no warning on explicit copy + var copy = s2; + copy.M2(); + System.Console.Write(copy.i); + } +} + +public struct S2 +{ + public int i; + public void M2() + { + i = 23; + } + + static void Main() + { + var s1 = new S1() { s2 = new S2 { i = 1 } }; + s1.M1(); + } +} +"; + var verifier = CompileAndVerify(csharp, expectedOutput: "123"); + // PROTOTYPE: should warn about calling M2 + verifier.VerifyDiagnostics(); + } + private static string ilreadonlyStructWithWriteableFieldIL = @" .class private auto ansi sealed beforefieldinit Microsoft.CodeAnalysis.EmbeddedAttribute extends [mscorlib]System.Attribute From 0a46e61d04090c870ae254085c6ad69b7ffc6dbe Mon Sep 17 00:00:00 2001 From: Rikki Gibson Date: Fri, 8 Mar 2019 12:47:24 -0800 Subject: [PATCH 3/7] Add tests for static methods with in parameters --- .../Semantics/ReadOnlyStructsTests.cs | 90 ++++++++++++++++++- 1 file changed, 89 insertions(+), 1 deletion(-) diff --git a/src/Compilers/CSharp/Test/Semantic/Semantics/ReadOnlyStructsTests.cs b/src/Compilers/CSharp/Test/Semantic/Semantics/ReadOnlyStructsTests.cs index 507ddb8b30cc8..d27b5689a0900 100644 --- a/src/Compilers/CSharp/Test/Semantic/Semantics/ReadOnlyStructsTests.cs +++ b/src/Compilers/CSharp/Test/Semantic/Semantics/ReadOnlyStructsTests.cs @@ -268,6 +268,32 @@ .maxstack 2 comp.VerifyDiagnostics(); } + [Fact] + public void InParam_CallReadOnlyMethod() + { + var csharp = @" +public struct S +{ + public int i; + public static int M1(in S s) => s.M2() + 1; + public readonly int M2() => i; +} +"; + var comp = CompileAndVerify(csharp); + comp.VerifyIL("S.M1", @" +{ + // Code size 9 (0x9) + .maxstack 2 + IL_0000: ldarg.0 + IL_0001: call ""int S.M2()"" + IL_0006: ldc.i4.1 + IL_0007: add + IL_0008: ret +} +"); + comp.VerifyDiagnostics(); + } + [Fact] public void ReadOnlyMethod_CallStaticMethod() { @@ -345,6 +371,68 @@ .locals init (S V_0, //copy }"); } + [Fact] + public void InMethod_CallNormalMethod() + { + var csharp = @" +public struct S +{ + public int i; + + public static void M1(in S s) + { + // should create local copy + s.M2(); + System.Console.Write(s.i); + + // explicit local copy, no warning + var copy = s; + copy.M2(); + System.Console.Write(copy.i); + } + + void M2() + { + i = 23; + } + + static void Main() + { + var s = new S { i = 1 }; + M1(in s); + } +} +"; + + var verifier = CompileAndVerify(csharp, expectedOutput: "123"); + // PROTOTYPE: should warn about copying 's' when calling M2 + verifier.VerifyDiagnostics(); + verifier.VerifyIL("S.M1", @" +{ + // Code size 51 (0x33) + .maxstack 1 + .locals init (S V_0, //copy + S V_1) + IL_0000: ldarg.0 + IL_0001: ldobj ""S"" + IL_0006: stloc.1 + IL_0007: ldloca.s V_1 + IL_0009: call ""void S.M2()"" + IL_000e: ldarg.0 + IL_000f: ldfld ""int S.i"" + IL_0014: call ""void System.Console.Write(int)"" + IL_0019: ldarg.0 + IL_001a: ldobj ""S"" + IL_001f: stloc.0 + IL_0020: ldloca.s V_0 + IL_0022: call ""void S.M2()"" + IL_0027: ldloc.0 + IL_0028: ldfld ""int S.i"" + IL_002d: call ""void System.Console.Write(int)"" + IL_0032: ret +}"); + } + [Fact] public void ReadOnlyAccessor_CallNormalMethod() { @@ -456,7 +544,7 @@ static void Main() } "; var verifier = CompileAndVerify(csharp, expectedOutput: "123"); - // PROTOTYPE: should warn about calling M2 + // should warn about calling s2.M2 in warning wave (see https://github.com/dotnet/roslyn/issues/33968) verifier.VerifyDiagnostics(); } From 7f66f9abef40273906fc2c0cbb12d448b9bab808 Mon Sep 17 00:00:00 2001 From: Rikki Gibson Date: Fri, 8 Mar 2019 14:45:35 -0800 Subject: [PATCH 4/7] Rely on HasHome instead of directly checking IsReadOnly and AddressKind --- src/Compilers/CSharp/Portable/CodeGen/EmitAddress.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Compilers/CSharp/Portable/CodeGen/EmitAddress.cs b/src/Compilers/CSharp/Portable/CodeGen/EmitAddress.cs index ce2f43744d014..2763b60348159 100644 --- a/src/Compilers/CSharp/Portable/CodeGen/EmitAddress.cs +++ b/src/Compilers/CSharp/Portable/CodeGen/EmitAddress.cs @@ -65,7 +65,7 @@ private LocalDefinition EmitAddress(BoundExpression expression, AddressKind addr if (expression.Type.IsValueType) { - if (addressKind == AddressKind.Writeable && _method.IsReadOnly) + if (!HasHome(expression, addressKind)) { // a readonly method is calling a non-readonly method, therefore we need to copy 'this' goto default; From f8e20257f6697dc85e0e5f60b4bbcea4d509befa Mon Sep 17 00:00:00 2001 From: Rikki Gibson Date: Fri, 8 Mar 2019 14:46:05 -0800 Subject: [PATCH 5/7] Move IL-verifying tests to CodeGenReadonlyStructTests.cs --- .../CodeGen/CodeGenReadonlyStructTests.cs | 176 ++++++++++++++++++ .../Semantics/ReadOnlyStructsTests.cs | 176 ------------------ 2 files changed, 176 insertions(+), 176 deletions(-) diff --git a/src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenReadonlyStructTests.cs b/src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenReadonlyStructTests.cs index 496bd4232fcfd..110f01892cacb 100644 --- a/src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenReadonlyStructTests.cs +++ b/src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenReadonlyStructTests.cs @@ -1307,5 +1307,181 @@ public static explicit operator Test(Span value) CompileAndVerify(comp, expectedOutput: "SpanOpCalled", verify: Verification.Fails); } + + [Fact] + public void ReadOnlyMethod_CallNormalMethod() + { + var csharp = @" +public struct S +{ + public int i; + + public readonly void M1() + { + // should create local copy + M2(); + System.Console.Write(i); + + // explicit local copy, no warning + var copy = this; + copy.M2(); + System.Console.Write(copy.i); + } + + void M2() + { + i = 23; + } + + static void Main() + { + var s = new S { i = 1 }; + s.M1(); + } +} +"; + + var verifier = CompileAndVerify(csharp, expectedOutput: "123"); + // PROTOTYPE: should warn about copying 'this' when calling M2 + verifier.VerifyDiagnostics(); + verifier.VerifyIL("S.M1", @" +{ + // Code size 51 (0x33) + .maxstack 1 + .locals init (S V_0, //copy + S V_1) + IL_0000: ldarg.0 + IL_0001: ldobj ""S"" + IL_0006: stloc.1 + IL_0007: ldloca.s V_1 + IL_0009: call ""void S.M2()"" + IL_000e: ldarg.0 + IL_000f: ldfld ""int S.i"" + IL_0014: call ""void System.Console.Write(int)"" + IL_0019: ldarg.0 + IL_001a: ldobj ""S"" + IL_001f: stloc.0 + IL_0020: ldloca.s V_0 + IL_0022: call ""void S.M2()"" + IL_0027: ldloc.0 + IL_0028: ldfld ""int S.i"" + IL_002d: call ""void System.Console.Write(int)"" + IL_0032: ret +}"); + } + + [Fact] + public void InMethod_CallNormalMethod() + { + var csharp = @" +public struct S +{ + public int i; + + public static void M1(in S s) + { + // should create local copy + s.M2(); + System.Console.Write(s.i); + + // explicit local copy, no warning + var copy = s; + copy.M2(); + System.Console.Write(copy.i); + } + + void M2() + { + i = 23; + } + + static void Main() + { + var s = new S { i = 1 }; + M1(in s); + } +} +"; + + var verifier = CompileAndVerify(csharp, expectedOutput: "123"); + // PROTOTYPE: should warn about copying 's' when calling M2 + verifier.VerifyDiagnostics(); + verifier.VerifyIL("S.M1", @" +{ + // Code size 51 (0x33) + .maxstack 1 + .locals init (S V_0, //copy + S V_1) + IL_0000: ldarg.0 + IL_0001: ldobj ""S"" + IL_0006: stloc.1 + IL_0007: ldloca.s V_1 + IL_0009: call ""void S.M2()"" + IL_000e: ldarg.0 + IL_000f: ldfld ""int S.i"" + IL_0014: call ""void System.Console.Write(int)"" + IL_0019: ldarg.0 + IL_001a: ldobj ""S"" + IL_001f: stloc.0 + IL_0020: ldloca.s V_0 + IL_0022: call ""void S.M2()"" + IL_0027: ldloc.0 + IL_0028: ldfld ""int S.i"" + IL_002d: call ""void System.Console.Write(int)"" + IL_0032: ret +}"); + } + + [Fact] + public void ReadOnlyMethod_CallReadOnlyMethod() + { + var csharp = @" +public struct S +{ + public int i; + public readonly int M1() => M2() + 1; + public readonly int M2() => i; +} +"; + var comp = CompileAndVerify(csharp); + comp.VerifyIL("S.M1", @" +{ + // Code size 9 (0x9) + .maxstack 2 + IL_0000: ldarg.0 + IL_0001: call ""int S.M2()"" + IL_0006: ldc.i4.1 + IL_0007: add + IL_0008: ret +} +"); + comp.VerifyDiagnostics(); + } + + [Fact] + public void InParam_CallReadOnlyMethod() + { + var csharp = @" +public struct S +{ + public int i; + public static int M1(in S s) => s.M2() + 1; + public readonly int M2() => i; +} +"; + var comp = CompileAndVerify(csharp); + comp.VerifyIL("S.M1", @" +{ + // Code size 9 (0x9) + .maxstack 2 + IL_0000: ldarg.0 + IL_0001: call ""int S.M2()"" + IL_0006: ldc.i4.1 + IL_0007: add + IL_0008: ret +} +"); + comp.VerifyDiagnostics(); + } } } diff --git a/src/Compilers/CSharp/Test/Semantic/Semantics/ReadOnlyStructsTests.cs b/src/Compilers/CSharp/Test/Semantic/Semantics/ReadOnlyStructsTests.cs index d27b5689a0900..0fe90c550457d 100644 --- a/src/Compilers/CSharp/Test/Semantic/Semantics/ReadOnlyStructsTests.cs +++ b/src/Compilers/CSharp/Test/Semantic/Semantics/ReadOnlyStructsTests.cs @@ -242,58 +242,6 @@ public readonly void M4() comp.VerifyDiagnostics(); } - [Fact] - public void ReadOnlyMethod_CallReadOnlyMethod() - { - var csharp = @" -public struct S -{ - public int i; - public readonly int M1() => M2() + 1; - public readonly int M2() => i; -} -"; - var comp = CompileAndVerify(csharp); - comp.VerifyIL("S.M1", @" -{ - // Code size 9 (0x9) - .maxstack 2 - IL_0000: ldarg.0 - IL_0001: call ""int S.M2()"" - IL_0006: ldc.i4.1 - IL_0007: add - IL_0008: ret -} -"); - comp.VerifyDiagnostics(); - } - - [Fact] - public void InParam_CallReadOnlyMethod() - { - var csharp = @" -public struct S -{ - public int i; - public static int M1(in S s) => s.M2() + 1; - public readonly int M2() => i; -} -"; - var comp = CompileAndVerify(csharp); - comp.VerifyIL("S.M1", @" -{ - // Code size 9 (0x9) - .maxstack 2 - IL_0000: ldarg.0 - IL_0001: call ""int S.M2()"" - IL_0006: ldc.i4.1 - IL_0007: add - IL_0008: ret -} -"); - comp.VerifyDiagnostics(); - } - [Fact] public void ReadOnlyMethod_CallStaticMethod() { @@ -309,130 +257,6 @@ public struct S comp.VerifyDiagnostics(); } - [Fact] - public void ReadOnlyMethod_CallNormalMethod() - { - var csharp = @" -public struct S -{ - public int i; - - public readonly void M1() - { - // should create local copy - M2(); - System.Console.Write(i); - - // explicit local copy, no warning - var copy = this; - copy.M2(); - System.Console.Write(copy.i); - } - - void M2() - { - i = 23; - } - - static void Main() - { - var s = new S { i = 1 }; - s.M1(); - } -} -"; - - var verifier = CompileAndVerify(csharp, expectedOutput: "123"); - // PROTOTYPE: should warn about copying 'this' when calling M2 - verifier.VerifyDiagnostics(); - verifier.VerifyIL("S.M1", @" -{ - // Code size 51 (0x33) - .maxstack 1 - .locals init (S V_0, //copy - S V_1) - IL_0000: ldarg.0 - IL_0001: ldobj ""S"" - IL_0006: stloc.1 - IL_0007: ldloca.s V_1 - IL_0009: call ""void S.M2()"" - IL_000e: ldarg.0 - IL_000f: ldfld ""int S.i"" - IL_0014: call ""void System.Console.Write(int)"" - IL_0019: ldarg.0 - IL_001a: ldobj ""S"" - IL_001f: stloc.0 - IL_0020: ldloca.s V_0 - IL_0022: call ""void S.M2()"" - IL_0027: ldloc.0 - IL_0028: ldfld ""int S.i"" - IL_002d: call ""void System.Console.Write(int)"" - IL_0032: ret -}"); - } - - [Fact] - public void InMethod_CallNormalMethod() - { - var csharp = @" -public struct S -{ - public int i; - - public static void M1(in S s) - { - // should create local copy - s.M2(); - System.Console.Write(s.i); - - // explicit local copy, no warning - var copy = s; - copy.M2(); - System.Console.Write(copy.i); - } - - void M2() - { - i = 23; - } - - static void Main() - { - var s = new S { i = 1 }; - M1(in s); - } -} -"; - - var verifier = CompileAndVerify(csharp, expectedOutput: "123"); - // PROTOTYPE: should warn about copying 's' when calling M2 - verifier.VerifyDiagnostics(); - verifier.VerifyIL("S.M1", @" -{ - // Code size 51 (0x33) - .maxstack 1 - .locals init (S V_0, //copy - S V_1) - IL_0000: ldarg.0 - IL_0001: ldobj ""S"" - IL_0006: stloc.1 - IL_0007: ldloca.s V_1 - IL_0009: call ""void S.M2()"" - IL_000e: ldarg.0 - IL_000f: ldfld ""int S.i"" - IL_0014: call ""void System.Console.Write(int)"" - IL_0019: ldarg.0 - IL_001a: ldobj ""S"" - IL_001f: stloc.0 - IL_0020: ldloca.s V_0 - IL_0022: call ""void S.M2()"" - IL_0027: ldloc.0 - IL_0028: ldfld ""int S.i"" - IL_002d: call ""void System.Console.Write(int)"" - IL_0032: ret -}"); - } - [Fact] public void ReadOnlyAccessor_CallNormalMethod() { From 8de22506cb822ec80350a528983b69831ff1f36d Mon Sep 17 00:00:00 2001 From: Rikki Gibson Date: Mon, 11 Mar 2019 11:09:35 -0700 Subject: [PATCH 6/7] Fixes from feedback --- .../CSharp/Portable/Binder/Binder.ValueChecks.cs | 2 +- .../CSharp/Portable/CodeGen/EmitExpression.cs | 2 +- .../AnonymousType.SynthesizedMethodBase.cs | 2 +- .../CSharp/Portable/Symbols/ErrorMethodSymbol.cs | 2 +- .../Portable/Symbols/Metadata/PE/PEMethodSymbol.cs | 2 +- .../CSharp/Portable/Symbols/MethodSymbol.cs | 12 ++++++++++-- .../Portable/Symbols/ReducedExtensionMethodSymbol.cs | 4 ++-- .../Portable/Symbols/SignatureOnlyMethodSymbol.cs | 2 +- .../CSharp/Portable/Symbols/Source/LambdaSymbol.cs | 2 +- .../Portable/Symbols/Source/LocalFunctionSymbol.cs | 2 +- .../Symbols/Source/SourceMemberMethodSymbol.cs | 2 +- .../Symbols/Source/SourceOrdinaryMethodSymbol.cs | 2 +- .../Symbols/Source/SourcePropertyAccessorSymbol.cs | 2 +- .../Synthesized/SynthesizedEntryPointSymbol.cs | 2 +- .../Synthesized/SynthesizedGlobalMethodSymbol.cs | 2 +- .../Synthesized/SynthesizedInstanceMethodSymbol.cs | 2 +- .../SynthesizedIntrinsicOperatorSymbol.cs | 2 +- .../Synthesized/SynthesizedStaticConstructor.cs | 2 +- .../Portable/Symbols/Wrapped/WrappedMethodSymbol.cs | 2 +- .../Test/Semantic/Semantics/ReadOnlyStructsTests.cs | 3 ++- .../ExpressionCompiler/Symbols/EEMethodSymbol.cs | 2 +- .../Symbols/PlaceholderMethodSymbol.cs | 2 +- 22 files changed, 33 insertions(+), 24 deletions(-) diff --git a/src/Compilers/CSharp/Portable/Binder/Binder.ValueChecks.cs b/src/Compilers/CSharp/Portable/Binder/Binder.ValueChecks.cs index 09564fe0265cc..e2ccb20af3983 100644 --- a/src/Compilers/CSharp/Portable/Binder/Binder.ValueChecks.cs +++ b/src/Compilers/CSharp/Portable/Binder/Binder.ValueChecks.cs @@ -2990,7 +2990,7 @@ internal static bool HasHome( return true; } - if (!IsAnyReadOnly(addressKind) && (type.IsReadOnly || method.IsReadOnly)) + if (!IsAnyReadOnly(addressKind) && method.IsEffectivelyReadOnly) { return method.MethodKind == MethodKind.Constructor; } diff --git a/src/Compilers/CSharp/Portable/CodeGen/EmitExpression.cs b/src/Compilers/CSharp/Portable/CodeGen/EmitExpression.cs index edba7907bf673..96601597a5159 100644 --- a/src/Compilers/CSharp/Portable/CodeGen/EmitExpression.cs +++ b/src/Compilers/CSharp/Portable/CodeGen/EmitExpression.cs @@ -1688,7 +1688,7 @@ private bool IsReadOnlyCall(MethodSymbol method, NamedTypeSymbol methodContainin { Debug.Assert(methodContainingType.IsVerifierValue(), "only struct calls can be readonly"); - if ((method.IsReadOnly || methodContainingType.IsReadOnly) && method.MethodKind != MethodKind.Constructor) + if (method.IsEffectivelyReadOnly && method.MethodKind != MethodKind.Constructor) { return true; } diff --git a/src/Compilers/CSharp/Portable/Symbols/AnonymousTypes/SynthesizedSymbols/AnonymousType.SynthesizedMethodBase.cs b/src/Compilers/CSharp/Portable/Symbols/AnonymousTypes/SynthesizedSymbols/AnonymousType.SynthesizedMethodBase.cs index bc05f44a8c2c7..c89f334283f2a 100644 --- a/src/Compilers/CSharp/Portable/Symbols/AnonymousTypes/SynthesizedSymbols/AnonymousType.SynthesizedMethodBase.cs +++ b/src/Compilers/CSharp/Portable/Symbols/AnonymousTypes/SynthesizedSymbols/AnonymousType.SynthesizedMethodBase.cs @@ -117,7 +117,7 @@ public sealed override ImmutableArray ExplicitInterfaceImplementat get { return ImmutableArray.Empty; } } - internal sealed override bool IsReadOnly => true; + internal sealed override bool IsDeclaredReadOnly => true; public sealed override ImmutableArray RefCustomModifiers { diff --git a/src/Compilers/CSharp/Portable/Symbols/ErrorMethodSymbol.cs b/src/Compilers/CSharp/Portable/Symbols/ErrorMethodSymbol.cs index 3f27ab1cd02fb..34e967237c388 100644 --- a/src/Compilers/CSharp/Portable/Symbols/ErrorMethodSymbol.cs +++ b/src/Compilers/CSharp/Portable/Symbols/ErrorMethodSymbol.cs @@ -124,7 +124,7 @@ public override ImmutableArray ExplicitInterfaceImplementations get { return ImmutableArray.Empty; } } - internal override bool IsReadOnly => false; + internal override bool IsDeclaredReadOnly => false; internal override int ParameterCount { diff --git a/src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEMethodSymbol.cs b/src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEMethodSymbol.cs index 495d02a3247e6..bca2d77cd1e14 100644 --- a/src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEMethodSymbol.cs +++ b/src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEMethodSymbol.cs @@ -1028,7 +1028,7 @@ public override ImmutableArray ExplicitInterfaceImplementations } // PROTOTYPE: need to round trip readonly attribute in metadata - internal override bool IsReadOnly => false; + internal override bool IsDeclaredReadOnly => false; public override string GetDocumentationCommentXml(CultureInfo preferredCulture = null, bool expandIncludes = false, CancellationToken cancellationToken = default(CancellationToken)) { diff --git a/src/Compilers/CSharp/Portable/Symbols/MethodSymbol.cs b/src/Compilers/CSharp/Portable/Symbols/MethodSymbol.cs index 18f35ae3004f8..edde621be0433 100644 --- a/src/Compilers/CSharp/Portable/Symbols/MethodSymbol.cs +++ b/src/Compilers/CSharp/Portable/Symbols/MethodSymbol.cs @@ -299,10 +299,18 @@ internal virtual bool IsExplicitInterfaceImplementation // PROTOTYPE: this should become public API /// - /// Indicates whether the method is readonly, i.e. + /// Indicates whether the method is declared readonly, i.e. /// whether 'this' is readonly in the scope of the method. + /// See also /// - internal abstract bool IsReadOnly { get; } + internal abstract bool IsDeclaredReadOnly { get; } + + // PROTOTYPE: this should become public API + /// + /// Indicates whether the method is effectively readonly, + /// by either the method or the containing type being marked readonly. + /// + internal bool IsEffectivelyReadOnly => IsDeclaredReadOnly || ContainingType?.IsReadOnly == true; /// /// Returns interface methods explicitly implemented by this method. diff --git a/src/Compilers/CSharp/Portable/Symbols/ReducedExtensionMethodSymbol.cs b/src/Compilers/CSharp/Portable/Symbols/ReducedExtensionMethodSymbol.cs index 9adc825257f08..fb734782936cc 100644 --- a/src/Compilers/CSharp/Portable/Symbols/ReducedExtensionMethodSymbol.cs +++ b/src/Compilers/CSharp/Portable/Symbols/ReducedExtensionMethodSymbol.cs @@ -373,8 +373,8 @@ internal override bool IsExplicitInterfaceImplementation get { return false; } } - // extension method has no 'this' parameter - internal override bool IsReadOnly => false; + // extension methods are static and therefore not readonly + internal override bool IsDeclaredReadOnly => false; public override ImmutableArray ExplicitInterfaceImplementations { diff --git a/src/Compilers/CSharp/Portable/Symbols/SignatureOnlyMethodSymbol.cs b/src/Compilers/CSharp/Portable/Symbols/SignatureOnlyMethodSymbol.cs index 0be076d2fc839..3cd6eade24142 100644 --- a/src/Compilers/CSharp/Portable/Symbols/SignatureOnlyMethodSymbol.cs +++ b/src/Compilers/CSharp/Portable/Symbols/SignatureOnlyMethodSymbol.cs @@ -143,7 +143,7 @@ internal override bool IsMetadataFinal } } - internal override bool IsReadOnly => false; + internal override bool IsDeclaredReadOnly => false; internal override int CalculateLocalSyntaxOffset(int localPosition, SyntaxTree localTree) { throw ExceptionUtilities.Unreachable; } diff --git a/src/Compilers/CSharp/Portable/Symbols/Source/LambdaSymbol.cs b/src/Compilers/CSharp/Portable/Symbols/Source/LambdaSymbol.cs index 17eeb52a5d49d..f26f2c5893af1 100644 --- a/src/Compilers/CSharp/Portable/Symbols/Source/LambdaSymbol.cs +++ b/src/Compilers/CSharp/Portable/Symbols/Source/LambdaSymbol.cs @@ -396,7 +396,7 @@ internal override bool GenerateDebugInfo get { return true; } } - internal override bool IsReadOnly => false; + internal override bool IsDeclaredReadOnly => false; public override ImmutableArray GetTypeParameterConstraintClauses(bool early) => ImmutableArray.Empty; diff --git a/src/Compilers/CSharp/Portable/Symbols/Source/LocalFunctionSymbol.cs b/src/Compilers/CSharp/Portable/Symbols/Source/LocalFunctionSymbol.cs index 62450dbb23bca..c3aa434cd98db 100644 --- a/src/Compilers/CSharp/Portable/Symbols/Source/LocalFunctionSymbol.cs +++ b/src/Compilers/CSharp/Portable/Symbols/Source/LocalFunctionSymbol.cs @@ -360,7 +360,7 @@ internal override TypeSymbol IteratorElementType internal bool IsExpressionBodied => _syntax.Body == null && _syntax.ExpressionBody != null; - internal override bool IsReadOnly => false; + internal override bool IsDeclaredReadOnly => false; public override DllImportData GetDllImportData() => null; diff --git a/src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberMethodSymbol.cs b/src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberMethodSymbol.cs index 4a0c985db89d6..3c9b536109304 100644 --- a/src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberMethodSymbol.cs +++ b/src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberMethodSymbol.cs @@ -509,7 +509,7 @@ public sealed override bool IsAsync } } - internal override bool IsReadOnly + internal override bool IsDeclaredReadOnly { get { diff --git a/src/Compilers/CSharp/Portable/Symbols/Source/SourceOrdinaryMethodSymbol.cs b/src/Compilers/CSharp/Portable/Symbols/Source/SourceOrdinaryMethodSymbol.cs index 99ff44e0f8789..81c9b1d53e67d 100644 --- a/src/Compilers/CSharp/Portable/Symbols/Source/SourceOrdinaryMethodSymbol.cs +++ b/src/Compilers/CSharp/Portable/Symbols/Source/SourceOrdinaryMethodSymbol.cs @@ -938,7 +938,7 @@ private void CheckModifiers(bool hasBody, Location location, DiagnosticBag diagn // The modifier '{0}' is not valid for this item diagnostics.Add(ErrorCode.ERR_BadMemberFlag, location, SyntaxFacts.GetText(SyntaxKind.VirtualKeyword)); } - else if (IsStatic && IsReadOnly) + else if (IsStatic && IsDeclaredReadOnly) { // The modifier '{0}' is not valid for this item diagnostics.Add(ErrorCode.ERR_BadMemberFlag, location, SyntaxFacts.GetText(SyntaxKind.ReadOnlyKeyword)); diff --git a/src/Compilers/CSharp/Portable/Symbols/Source/SourcePropertyAccessorSymbol.cs b/src/Compilers/CSharp/Portable/Symbols/Source/SourcePropertyAccessorSymbol.cs index 18f65e6d2e385..691a97d754f78 100644 --- a/src/Compilers/CSharp/Portable/Symbols/Source/SourcePropertyAccessorSymbol.cs +++ b/src/Compilers/CSharp/Portable/Symbols/Source/SourcePropertyAccessorSymbol.cs @@ -456,7 +456,7 @@ private void CheckModifiers(Location location, bool hasBody, bool isAutoProperty { diagnostics.Add(AccessCheck.GetProtectedMemberInSealedTypeError(ContainingType), location, this); } - else if (IsStatic && IsReadOnly && !_property.HasReadOnlyModifier) + else if (IsStatic && IsDeclaredReadOnly && !_property.HasReadOnlyModifier) { // The modifier '{0}' is not valid for this item diagnostics.Add(ErrorCode.ERR_BadMemberFlag, location, SyntaxFacts.GetText(SyntaxKind.ReadOnlyKeyword)); diff --git a/src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedEntryPointSymbol.cs b/src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedEntryPointSymbol.cs index ac0e5750d014e..30e92dd84b1c0 100644 --- a/src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedEntryPointSymbol.cs +++ b/src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedEntryPointSymbol.cs @@ -208,7 +208,7 @@ public override ImmutableArray ExplicitInterfaceImplementations get { return ImmutableArray.Empty; } } - internal sealed override bool IsReadOnly => false; + internal sealed override bool IsDeclaredReadOnly => false; internal sealed override bool IsMetadataNewSlot(bool ignoreInterfaceImplementationChanges = false) { diff --git a/src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedGlobalMethodSymbol.cs b/src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedGlobalMethodSymbol.cs index 5474bde697d4d..98a828cedf419 100644 --- a/src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedGlobalMethodSymbol.cs +++ b/src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedGlobalMethodSymbol.cs @@ -300,7 +300,7 @@ public override ImmutableArray ExplicitInterfaceImplementations get { return ImmutableArray.Empty; } } - internal sealed override bool IsReadOnly => false; + internal sealed override bool IsDeclaredReadOnly => false; internal override bool SynthesizesLoweredBoundBody { diff --git a/src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedInstanceMethodSymbol.cs b/src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedInstanceMethodSymbol.cs index 7ca0322631b23..89f515fcef776 100644 --- a/src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedInstanceMethodSymbol.cs +++ b/src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedInstanceMethodSymbol.cs @@ -57,6 +57,6 @@ internal override int CalculateLocalSyntaxOffset(int localPosition, SyntaxTree l throw ExceptionUtilities.Unreachable; } - internal override bool IsReadOnly => false; + internal override bool IsDeclaredReadOnly => false; } } diff --git a/src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedIntrinsicOperatorSymbol.cs b/src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedIntrinsicOperatorSymbol.cs index a4c33aeb9f59d..22fa2f25f3f38 100644 --- a/src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedIntrinsicOperatorSymbol.cs +++ b/src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedIntrinsicOperatorSymbol.cs @@ -261,7 +261,7 @@ public override ImmutableArray ExplicitInterfaceImplementations } } - internal override bool IsReadOnly => true; + internal override bool IsDeclaredReadOnly => true; public override ImmutableArray RefCustomModifiers { diff --git a/src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedStaticConstructor.cs b/src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedStaticConstructor.cs index ce74c34ee2798..9b77abd1ed0e7 100644 --- a/src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedStaticConstructor.cs +++ b/src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedStaticConstructor.cs @@ -278,7 +278,7 @@ public override ImmutableArray ExplicitInterfaceImplementations } } - internal override bool IsReadOnly => false; + internal override bool IsDeclaredReadOnly => false; public sealed override bool IsImplicitlyDeclared { diff --git a/src/Compilers/CSharp/Portable/Symbols/Wrapped/WrappedMethodSymbol.cs b/src/Compilers/CSharp/Portable/Symbols/Wrapped/WrappedMethodSymbol.cs index cb2b39b2fb194..ff4f6e08f44ad 100644 --- a/src/Compilers/CSharp/Portable/Symbols/Wrapped/WrappedMethodSymbol.cs +++ b/src/Compilers/CSharp/Portable/Symbols/Wrapped/WrappedMethodSymbol.cs @@ -321,6 +321,6 @@ internal override bool GenerateDebugInfo } } - internal override bool IsReadOnly => UnderlyingMethod.IsReadOnly; + internal override bool IsDeclaredReadOnly => UnderlyingMethod.IsDeclaredReadOnly; } } diff --git a/src/Compilers/CSharp/Test/Semantic/Semantics/ReadOnlyStructsTests.cs b/src/Compilers/CSharp/Test/Semantic/Semantics/ReadOnlyStructsTests.cs index 0fe90c550457d..60e54902d0429 100644 --- a/src/Compilers/CSharp/Test/Semantic/Semantics/ReadOnlyStructsTests.cs +++ b/src/Compilers/CSharp/Test/Semantic/Semantics/ReadOnlyStructsTests.cs @@ -487,7 +487,8 @@ public readonly int M() comp.VerifyDiagnostics(); var method = comp.GetMember("S").GetMember("M"); - Assert.True(method.IsReadOnly); + Assert.True(method.IsDeclaredReadOnly); + Assert.True(method.IsEffectivelyReadOnly); } [Fact] diff --git a/src/ExpressionEvaluator/CSharp/Source/ExpressionCompiler/Symbols/EEMethodSymbol.cs b/src/ExpressionEvaluator/CSharp/Source/ExpressionCompiler/Symbols/EEMethodSymbol.cs index 0a502721cb21d..3afdfdebb2d55 100644 --- a/src/ExpressionEvaluator/CSharp/Source/ExpressionCompiler/Symbols/EEMethodSymbol.cs +++ b/src/ExpressionEvaluator/CSharp/Source/ExpressionCompiler/Symbols/EEMethodSymbol.cs @@ -405,7 +405,7 @@ public override bool IsExtern get { return false; } } - internal override bool IsReadOnly => false; + internal override bool IsDeclaredReadOnly => false; internal override ObsoleteAttributeData ObsoleteAttributeData { diff --git a/src/ExpressionEvaluator/CSharp/Source/ExpressionCompiler/Symbols/PlaceholderMethodSymbol.cs b/src/ExpressionEvaluator/CSharp/Source/ExpressionCompiler/Symbols/PlaceholderMethodSymbol.cs index 4eea69bf5e589..89018bfab9213 100644 --- a/src/ExpressionEvaluator/CSharp/Source/ExpressionCompiler/Symbols/PlaceholderMethodSymbol.cs +++ b/src/ExpressionEvaluator/CSharp/Source/ExpressionCompiler/Symbols/PlaceholderMethodSymbol.cs @@ -118,7 +118,7 @@ public override bool IsVirtual get { return false; } } - internal override bool IsReadOnly => false; + internal override bool IsDeclaredReadOnly => false; public override ImmutableArray Locations { From fc91eba6b15a3c3466e7e294654ed2e51010c1b1 Mon Sep 17 00:00:00 2001 From: Rikki Gibson Date: Mon, 11 Mar 2019 13:25:45 -0700 Subject: [PATCH 7/7] A few more test cases and cleanup --- .../CodeGen/CodeGenReadonlyStructTests.cs | 72 +++++++++++++++++++ .../Semantics/ReadOnlyStructsTests.cs | 39 ++++++++-- 2 files changed, 107 insertions(+), 4 deletions(-) diff --git a/src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenReadonlyStructTests.cs b/src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenReadonlyStructTests.cs index 110f01892cacb..d3aa569e79ef2 100644 --- a/src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenReadonlyStructTests.cs +++ b/src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenReadonlyStructTests.cs @@ -1483,5 +1483,77 @@ .maxstack 2 "); comp.VerifyDiagnostics(); } + + [Fact] + public void ReadOnlyMethod_CallReadOnlyMethodOnField() + { + var csharp = @" +public struct S1 +{ + public readonly void M1() {} +} + +public struct S2 +{ + S1 s1; + + public readonly void M2() + { + s1.M1(); + } +} +"; + var comp = CompileAndVerify(csharp); + + comp.VerifyIL("S2.M2", @" +{ + // Code size 12 (0xc) + .maxstack 1 + IL_0000: ldarg.0 + IL_0001: ldflda ""S1 S2.s1"" + IL_0006: call ""void S1.M1()"" + IL_000b: ret +}"); + + comp.VerifyDiagnostics(); + } + + [Fact] + public void ReadOnlyMethod_CallNormalMethodOnField() + { + var csharp = @" +public struct S1 +{ + public void M1() {} +} + +public struct S2 +{ + S1 s1; + + public readonly void M2() + { + s1.M1(); + } +} +"; + var comp = CompileAndVerify(csharp); + + comp.VerifyIL("S2.M2", @" +{ + // Code size 15 (0xf) + .maxstack 1 + .locals init (S1 V_0) + IL_0000: ldarg.0 + IL_0001: ldfld ""S1 S2.s1"" + IL_0006: stloc.0 + IL_0007: ldloca.s V_0 + IL_0009: call ""void S1.M1()"" + IL_000e: ret +}"); + + // should warn about calling s2.M2 in warning wave (see https://github.com/dotnet/roslyn/issues/33968) + comp.VerifyDiagnostics(); + } } } diff --git a/src/Compilers/CSharp/Test/Semantic/Semantics/ReadOnlyStructsTests.cs b/src/Compilers/CSharp/Test/Semantic/Semantics/ReadOnlyStructsTests.cs index 60e54902d0429..4dc213dea2dc3 100644 --- a/src/Compilers/CSharp/Test/Semantic/Semantics/ReadOnlyStructsTests.cs +++ b/src/Compilers/CSharp/Test/Semantic/Semantics/ReadOnlyStructsTests.cs @@ -181,6 +181,38 @@ readonly void AssignField() CreateCompilation(text).VerifyDiagnostics(); } + [Fact] + public void ReadOnlyStruct_PassThisByRef() + { + var csharp = @" +public readonly struct S +{ + public static void M1(ref S s) {} + public static void M2(in S s) {} + + public void M3() + { + M1(ref this); // error + M2(in this); // ok + } + + public readonly void M4() + { + M1(ref this); // error + M2(in this); // ok + } +} +"; + var comp = CreateCompilation(csharp); + comp.VerifyDiagnostics( + // (9,16): error CS1605: Cannot use 'this' as a ref or out value because it is read-only + // M1(ref this); // error + Diagnostic(ErrorCode.ERR_RefReadonlyLocal, "this").WithArguments("this").WithLocation(9, 16), + // (15,16): error CS1605: Cannot use 'this' as a ref or out value because it is read-only + // M1(ref this); // error + Diagnostic(ErrorCode.ERR_RefReadonlyLocal, "this").WithArguments("this").WithLocation(15, 16)); + } + [Fact] public void ReadOnlyMethod_PassThisByRef() { @@ -190,13 +222,13 @@ public struct S public static void M1(ref S s) {} public static void M2(in S s) {} - public void M2() + public void M3() { M1(ref this); // ok M2(in this); // ok } - public readonly void M3() + public readonly void M4() { M1(ref this); // error M2(in this); // ok @@ -329,6 +361,7 @@ static void Main() } } "; + // should warn about calling s2.M2 in warning wave (see https://github.com/dotnet/roslyn/issues/33968) CompileAndVerify(csharp, expectedOutput: "0"); } @@ -341,11 +374,9 @@ public struct S1 public S2 s2; public readonly void M1() { - // warn on local copy s2.M2(); System.Console.Write(s2.i); - // no warning on explicit copy var copy = s2; copy.M2(); System.Console.Write(copy.i);