From b067a5b70d40d1d50f45a2511c96cf36b1e4748d Mon Sep 17 00:00:00 2001 From: Rikki Gibson Date: Tue, 14 May 2019 15:27:26 -0700 Subject: [PATCH 1/5] Warn on implicit copies for event assignment within readonly members --- .../Portable/Binder/Binder_Operators.cs | 2 + .../Semantics/ReadOnlyStructsTests.cs | 45 +++++++++++++++++++ 2 files changed, 47 insertions(+) diff --git a/src/Compilers/CSharp/Portable/Binder/Binder_Operators.cs b/src/Compilers/CSharp/Portable/Binder/Binder_Operators.cs index 43ef6a8fe0e9e..7dbb9af71354d 100644 --- a/src/Compilers/CSharp/Portable/Binder/Binder_Operators.cs +++ b/src/Compilers/CSharp/Portable/Binder/Binder_Operators.cs @@ -262,6 +262,8 @@ private BoundExpression BindEventAssignment(AssignmentExpressionSyntax node, Bou } else { + CheckImplicitThisCopyInReadOnlyMember(receiverOpt, method, diagnostics); + if (!this.IsAccessible(method, ref useSiteDiagnostics, this.GetAccessThroughType(receiverOpt))) { // CONSIDER: depending on the accessibility (e.g. if it's private), dev10 might just report the whole event bogus. diff --git a/src/Compilers/CSharp/Test/Semantic/Semantics/ReadOnlyStructsTests.cs b/src/Compilers/CSharp/Test/Semantic/Semantics/ReadOnlyStructsTests.cs index 9ee8453091f5c..213445ad8fdaf 100644 --- a/src/Compilers/CSharp/Test/Semantic/Semantics/ReadOnlyStructsTests.cs +++ b/src/Compilers/CSharp/Test/Semantic/Semantics/ReadOnlyStructsTests.cs @@ -661,6 +661,51 @@ static void Main() Diagnostic(ErrorCode.WRN_ImplicitCopyInReadOnlyMember, "P2").WithArguments("S.P2.get", "this").WithLocation(11, 17)); } + [Fact] + public void ReadOnlyMethod_CallEventAccessor() + { + var csharp = @" +using System; + +public struct S +{ + public readonly void M() + { + Console.WriteLine(E is null); + // should create local copy + E += () => {}; // warning + Console.WriteLine(E is null); + E -= () => {}; // warning + + // explicit local copy, no warning + var copy = this; + copy.E += () => {}; + Console.WriteLine(copy.E is null); + } + + public event Action E; + + static void Main() + { + var s = new S(); + s.M(); + } +} +"; + + var verifier = CompileAndVerify(csharp, expectedOutput: +@"True +True +False"); + verifier.VerifyDiagnostics( + // (10,13): warning CS8656: Call to non-readonly member 'S.E.add' from a 'readonly' member results in an implicit copy of 'this'. + // E += () => {}; // warning + Diagnostic(ErrorCode.WRN_ImplicitCopyInReadOnlyMember, "E").WithArguments("S.E.add", "this").WithLocation(10, 13), + // (11,13): warning CS8656: Call to non-readonly member 'S.E.remove' from a 'readonly' member results in an implicit copy of 'this'. + // E -= () => {}; // warning + Diagnostic(ErrorCode.WRN_ImplicitCopyInReadOnlyMember, "E").WithArguments("S.E.remove", "this").WithLocation(11, 13)); + } + [Fact] public void ReadOnlyMethod_CallSetAccessor() { From 85874fc93b488e008b7161c41856b48112978662 Mon Sep 17 00:00:00 2001 From: Rikki Gibson Date: Tue, 14 May 2019 15:58:23 -0700 Subject: [PATCH 2/5] Update readonly event tests --- .../Semantics/ReadOnlyStructsTests.cs | 33 +++++++++++++++++-- 1 file changed, 30 insertions(+), 3 deletions(-) diff --git a/src/Compilers/CSharp/Test/Semantic/Semantics/ReadOnlyStructsTests.cs b/src/Compilers/CSharp/Test/Semantic/Semantics/ReadOnlyStructsTests.cs index 213445ad8fdaf..89061a4757d73 100644 --- a/src/Compilers/CSharp/Test/Semantic/Semantics/ReadOnlyStructsTests.cs +++ b/src/Compilers/CSharp/Test/Semantic/Semantics/ReadOnlyStructsTests.cs @@ -701,9 +701,31 @@ static void Main() // (10,13): warning CS8656: Call to non-readonly member 'S.E.add' from a 'readonly' member results in an implicit copy of 'this'. // E += () => {}; // warning Diagnostic(ErrorCode.WRN_ImplicitCopyInReadOnlyMember, "E").WithArguments("S.E.add", "this").WithLocation(10, 13), - // (11,13): warning CS8656: Call to non-readonly member 'S.E.remove' from a 'readonly' member results in an implicit copy of 'this'. + // (12,13): warning CS8656: Call to non-readonly member 'S.E.remove' from a 'readonly' member results in an implicit copy of 'this'. // E -= () => {}; // warning - Diagnostic(ErrorCode.WRN_ImplicitCopyInReadOnlyMember, "E").WithArguments("S.E.remove", "this").WithLocation(11, 13)); + Diagnostic(ErrorCode.WRN_ImplicitCopyInReadOnlyMember, "E").WithArguments("S.E.remove", "this").WithLocation(12, 13)); + } + + [Fact] + public void ReadOnlyMethod_CallReadOnlyEventAccessor() + { + var csharp = @" +using System; + +public struct S +{ + public readonly void M() + { + E += () => {}; + E -= () => {}; + } + + public readonly event Action E { add {} remove {} } +} +"; + + var verifier = CreateCompilation(csharp); + verifier.VerifyDiagnostics(); } [Fact] @@ -824,9 +846,14 @@ void handler(EventArgs args) { } } } "; - // should warn about E += handler in warning wave (see https://github.com/dotnet/roslyn/issues/33968) var comp = CreateCompilation(csharp); comp.VerifyDiagnostics( + // (9,9): warning CS8656: Call to non-readonly member 'S.E.add' from a 'readonly' member results in an implicit copy of 'this'. + // E += handler; + Diagnostic(ErrorCode.WRN_ImplicitCopyInReadOnlyMember, "E").WithArguments("S.E.add", "this").WithLocation(9, 9), + // (10,9): warning CS8656: Call to non-readonly member 'S.E.remove' from a 'readonly' member results in an implicit copy of 'this'. + // E -= handler; + Diagnostic(ErrorCode.WRN_ImplicitCopyInReadOnlyMember, "E").WithArguments("S.E.remove", "this").WithLocation(10, 9), // (11,9): error CS1604: Cannot assign to 'E' because it is read-only // E = handler; Diagnostic(ErrorCode.ERR_AssgReadonlyLocal, "E").WithArguments("E").WithLocation(11, 9)); From 97d62fc9b3f8051c1e654f9e34052efb5f29ec14 Mon Sep 17 00:00:00 2001 From: Rikki Gibson Date: Thu, 16 May 2019 11:32:52 -0700 Subject: [PATCH 3/5] Fix indent --- .../Semantics/ReadOnlyStructsTests.cs | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/Compilers/CSharp/Test/Semantic/Semantics/ReadOnlyStructsTests.cs b/src/Compilers/CSharp/Test/Semantic/Semantics/ReadOnlyStructsTests.cs index 89061a4757d73..6113cb2ecb504 100644 --- a/src/Compilers/CSharp/Test/Semantic/Semantics/ReadOnlyStructsTests.cs +++ b/src/Compilers/CSharp/Test/Semantic/Semantics/ReadOnlyStructsTests.cs @@ -671,16 +671,16 @@ public struct S { public readonly void M() { - Console.WriteLine(E is null); - // should create local copy - E += () => {}; // warning - Console.WriteLine(E is null); - E -= () => {}; // warning - - // explicit local copy, no warning - var copy = this; - copy.E += () => {}; - Console.WriteLine(copy.E is null); + Console.WriteLine(E is null); + // should create local copy + E += () => {}; // warning + Console.WriteLine(E is null); + E -= () => {}; // warning + + // explicit local copy, no warning + var copy = this; + copy.E += () => {}; + Console.WriteLine(copy.E is null); } public event Action E; From 353948a9dedfd80b34af26896462055ae4e9ae32 Mon Sep 17 00:00:00 2001 From: Rikki Gibson Date: Fri, 17 May 2019 11:08:15 -0700 Subject: [PATCH 4/5] Group event tests together and test nested event assignment --- .../Semantics/ReadOnlyStructsTests.cs | 167 +++++++++++------- 1 file changed, 99 insertions(+), 68 deletions(-) diff --git a/src/Compilers/CSharp/Test/Semantic/Semantics/ReadOnlyStructsTests.cs b/src/Compilers/CSharp/Test/Semantic/Semantics/ReadOnlyStructsTests.cs index 6113cb2ecb504..4ec46e966c6ad 100644 --- a/src/Compilers/CSharp/Test/Semantic/Semantics/ReadOnlyStructsTests.cs +++ b/src/Compilers/CSharp/Test/Semantic/Semantics/ReadOnlyStructsTests.cs @@ -661,73 +661,6 @@ static void Main() Diagnostic(ErrorCode.WRN_ImplicitCopyInReadOnlyMember, "P2").WithArguments("S.P2.get", "this").WithLocation(11, 17)); } - [Fact] - public void ReadOnlyMethod_CallEventAccessor() - { - var csharp = @" -using System; - -public struct S -{ - public readonly void M() - { - Console.WriteLine(E is null); - // should create local copy - E += () => {}; // warning - Console.WriteLine(E is null); - E -= () => {}; // warning - - // explicit local copy, no warning - var copy = this; - copy.E += () => {}; - Console.WriteLine(copy.E is null); - } - - public event Action E; - - static void Main() - { - var s = new S(); - s.M(); - } -} -"; - - var verifier = CompileAndVerify(csharp, expectedOutput: -@"True -True -False"); - verifier.VerifyDiagnostics( - // (10,13): warning CS8656: Call to non-readonly member 'S.E.add' from a 'readonly' member results in an implicit copy of 'this'. - // E += () => {}; // warning - Diagnostic(ErrorCode.WRN_ImplicitCopyInReadOnlyMember, "E").WithArguments("S.E.add", "this").WithLocation(10, 13), - // (12,13): warning CS8656: Call to non-readonly member 'S.E.remove' from a 'readonly' member results in an implicit copy of 'this'. - // E -= () => {}; // warning - Diagnostic(ErrorCode.WRN_ImplicitCopyInReadOnlyMember, "E").WithArguments("S.E.remove", "this").WithLocation(12, 13)); - } - - [Fact] - public void ReadOnlyMethod_CallReadOnlyEventAccessor() - { - var csharp = @" -using System; - -public struct S -{ - public readonly void M() - { - E += () => {}; - E -= () => {}; - } - - public readonly event Action E { add {} remove {} } -} -"; - - var verifier = CreateCompilation(csharp); - verifier.VerifyDiagnostics(); - } - [Fact] public void ReadOnlyMethod_CallSetAccessor() { @@ -827,7 +760,105 @@ public readonly void M() } [Fact] - public void ReadOnlyMethod_AddRemoveEventHandlers() + public void ReadOnlyMethod_EventAssignment() + { + var csharp = @" +using System; + +public struct S +{ + public readonly void M() + { + Console.WriteLine(E is null); + // should create local copy + E += () => {}; // warning + Console.WriteLine(E is null); + E -= () => {}; // warning + + // explicit local copy, no warning + var copy = this; + copy.E += () => {}; + Console.WriteLine(copy.E is null); + } + + public event Action E; + + static void Main() + { + var s = new S(); + s.M(); + } +} +"; + + var verifier = CompileAndVerify(csharp, expectedOutput: +@"True +True +False"); + verifier.VerifyDiagnostics( + // (10,9): warning CS8656: Call to non-readonly member 'S.E.add' from a 'readonly' member results in an implicit copy of 'this'. + // E += () => {}; // warning + Diagnostic(ErrorCode.WRN_ImplicitCopyInReadOnlyMember, "E").WithArguments("S.E.add", "this").WithLocation(10, 9), + // (12,9): warning CS8656: Call to non-readonly member 'S.E.remove' from a 'readonly' member results in an implicit copy of 'this'. + // E -= () => {}; // warning + Diagnostic(ErrorCode.WRN_ImplicitCopyInReadOnlyMember, "E").WithArguments("S.E.remove", "this").WithLocation(12, 9)); + } + + [Fact] + public void ReadOnlyMethod_ReadOnlyEventAssignment() + { + var csharp = @" +using System; + +public struct S +{ + public readonly void M() + { + E += () => {}; + E -= () => {}; + } + + public readonly event Action E { add {} remove {} } +} +"; + + var verifier = CreateCompilation(csharp); + verifier.VerifyDiagnostics(); + } + + [Fact] + public void ReadOnlyMethod_Field_EventAssignment() + { + var csharp = @" +#pragma warning disable 0067 +using System; + +public struct S2 +{ + public event Action E; +} + +public struct S1 +{ + public S2 s2; + + public readonly void M() + { + s2.E += () => {}; + s2.E -= () => {}; + } + + public readonly event Action E { add {} remove {} } +} +"; + + // TODO: should warn in warning wave https://github.com/dotnet/roslyn/issues/33968 + var verifier = CreateCompilation(csharp); + verifier.VerifyDiagnostics(); + } + + [Fact] + public void ReadOnlyMethod_EventAssignment_Error() { var csharp = @" using System; From 3a71b0f39cb4cd2374ad75cb58aa7d47a2bd22b7 Mon Sep 17 00:00:00 2001 From: Rikki Gibson Date: Mon, 15 Jul 2019 17:20:13 -0700 Subject: [PATCH 5/5] Cleanup and add a few events in readonly structs tests --- .../Semantics/ReadOnlyStructsTests.cs | 50 ++++++++++++++++++- 1 file changed, 49 insertions(+), 1 deletion(-) diff --git a/src/Compilers/CSharp/Test/Semantic/Semantics/ReadOnlyStructsTests.cs b/src/Compilers/CSharp/Test/Semantic/Semantics/ReadOnlyStructsTests.cs index 4ec46e966c6ad..68655c7a450ee 100644 --- a/src/Compilers/CSharp/Test/Semantic/Semantics/ReadOnlyStructsTests.cs +++ b/src/Compilers/CSharp/Test/Semantic/Semantics/ReadOnlyStructsTests.cs @@ -804,6 +804,27 @@ static void Main() Diagnostic(ErrorCode.WRN_ImplicitCopyInReadOnlyMember, "E").WithArguments("S.E.remove", "this").WithLocation(12, 9)); } + [Fact] + public void ReadOnlyStruct_EventAssignment() + { + var csharp = @" +using System; + +public struct S +{ + public void M() + { + E += () => {}; + E -= () => {}; + } + + public event Action E { add {} remove {} } +} +"; + var comp = CreateCompilation(csharp); + comp.VerifyDiagnostics(); + } + [Fact] public void ReadOnlyMethod_ReadOnlyEventAssignment() { @@ -847,8 +868,35 @@ public readonly void M() s2.E += () => {}; s2.E -= () => {}; } +} +"; - public readonly event Action E { add {} remove {} } + // TODO: should warn in warning wave https://github.com/dotnet/roslyn/issues/33968 + var verifier = CreateCompilation(csharp); + verifier.VerifyDiagnostics(); + } + + [Fact] + public void ReadOnlyStruct_Field_EventAssignment() + { + var csharp = @" +#pragma warning disable 0067 +using System; + +public struct S2 +{ + public event Action E; +} + +public readonly struct S1 +{ + public readonly S2 s2; + + public void M() + { + s2.E += () => {}; + s2.E -= () => {}; + } } ";