From 884827e35803e06735550997110178db034cf3b6 Mon Sep 17 00:00:00 2001 From: Youssef1313 Date: Wed, 6 Apr 2022 09:03:50 +0200 Subject: [PATCH 01/38] [semi-auto-props]: Support assigning properties from constructor --- src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs | 5 +---- .../LocalRewriter/LocalRewriter_AssignmentOperator.cs | 7 +++++-- .../Test/Semantic/Semantics/PropertyFieldKeywordTests.cs | 4 ++-- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs b/src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs index 65a0f047433a4..008fac03ec55d 100644 --- a/src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs +++ b/src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs @@ -1703,11 +1703,8 @@ private static bool IsPropertyAssignedThroughBackingField(BoundExpression receiv propertySymbol = propertySymbol.OriginalDefinition; } - // PROTOTYPE(semi-auto-props): TODO: Support assigning semi auto prop from constructors. - return propertySymbol is SourcePropertySymbolBase sourceProperty && - // PROTOTYPE(semi-auto-props): Consider `public int P { get => field; set; }` - sourceProperty.GetMethod is SourcePropertyAccessorSymbol { IsEquivalentToBackingFieldAccess: true } && + (sourceProperty.GetMethod is SourcePropertyAccessorSymbol { IsEquivalentToBackingFieldAccess: true } || sourceProperty.FieldKeywordBackingField is not null) && // To be assigned through backing field, either SetMethod is null, or it's equivalent to backing field write // PROTOTYPE(semi-auto-props): TODO: Do we need to use `GetOwnOrInheritedSetMethod` instead of `SetMethod`? // Legacy auto-properties are required to override all accessors. If this is not the case with semi auto props, we may need to use GetOwnOrInheritedSetMethod diff --git a/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_AssignmentOperator.cs b/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_AssignmentOperator.cs index 6a6d4099ffc1b..15673add875d8 100644 --- a/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_AssignmentOperator.cs +++ b/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_AssignmentOperator.cs @@ -301,11 +301,14 @@ private BoundExpression MakePropertyAssignment( if (setMethod is null) { var autoProp = (SourcePropertySymbolBase)property.OriginalDefinition; - Debug.Assert(autoProp.IsAutoPropertyWithGetAccessor, + Debug.Assert(autoProp.IsAutoPropertyWithGetAccessor || ( + autoProp.SetMethod is null or SourcePropertyAccessorSymbol { IsEquivalentToBackingFieldAccess: true } && + (autoProp.GetMethod is SourcePropertyAccessorSymbol { IsEquivalentToBackingFieldAccess: true } || autoProp.FieldKeywordBackingField is not null) + ), "only autoproperties can be assignable without having setters"); Debug.Assert(property.Equals(autoProp, TypeCompareKind.IgnoreNullableModifiersForReferenceTypes)); - var backingField = autoProp.BackingField; + var backingField = autoProp.BackingField ?? autoProp.FieldKeywordBackingField; Debug.Assert(backingField is not null); return _factory.AssignmentExpression( diff --git a/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs b/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs index 4712070312528..09d431bc0dc40 100644 --- a/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs +++ b/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs @@ -844,7 +844,7 @@ .property instance int32 P() Assert.Equal(0, accessorBindingData.NumberOfPerformedAccessorBinding); } - [Fact(Skip = "PROTOTYPE(semi-auto-props): Assigning in constructor is not yet supported.")] + [Fact] public void TestFieldOnlyGetter() { var comp = CreateCompilation(@" @@ -928,7 +928,7 @@ .property instance int32 P() } } // end of class C "); - Assert.Equal(0, accessorBindingData.NumberOfPerformedAccessorBinding); + Assert.Equal(1, accessorBindingData.NumberOfPerformedAccessorBinding); } // PROTOTYPE(semi-auto-props): All success scenarios should be executed, expected runtime behavior should be observed. From 92abdde7165ede9489f995f001f5edf0a47166e9 Mon Sep 17 00:00:00 2001 From: Youssef1313 Date: Wed, 6 Apr 2022 09:51:13 +0200 Subject: [PATCH 02/38] Update tests --- .../Semantics/PropertyFieldKeywordTests.cs | 89 +++++++++++++++++-- 1 file changed, 82 insertions(+), 7 deletions(-) diff --git a/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs b/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs index 09d431bc0dc40..40c3fda3a0f2e 100644 --- a/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs +++ b/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs @@ -323,7 +323,7 @@ .property instance int32 P() } // end of class C "); Assert.Empty(comp.GetTypeByMetadataName("C").GetMembers().OfType()); - Assert.Equal(0, accessorBindingData.NumberOfPerformedAccessorBinding); + Assert.Equal(1, accessorBindingData.NumberOfPerformedAccessorBinding); } [Fact] @@ -457,7 +457,7 @@ .property instance int32 P() } // end of class C "); Assert.Empty(comp.GetTypeByMetadataName("C").GetMembers().OfType()); - Assert.Equal(0, accessorBindingData.NumberOfPerformedAccessorBinding); + Assert.Equal(1, accessorBindingData.NumberOfPerformedAccessorBinding); } [Fact] @@ -550,7 +550,7 @@ .property instance int32 P() } // end of class C "); Assert.Empty(comp.GetTypeByMetadataName("C").GetMembers().OfType()); - Assert.Equal(0, accessorBindingData.NumberOfPerformedAccessorBinding); + Assert.Equal(1, accessorBindingData.NumberOfPerformedAccessorBinding); } [Theory, CombinatorialData] @@ -762,7 +762,7 @@ .property instance int32 P() "); var field = comp.GetTypeByMetadataName("C").GetMembers().OfType().Single(); Assert.Equal("System.Int32 C.field", field.ToTestDisplayString()); - Assert.Equal(0, accessorBindingData.NumberOfPerformedAccessorBinding); + Assert.Equal(1, accessorBindingData.NumberOfPerformedAccessorBinding); } [Fact] @@ -841,7 +841,7 @@ .property instance int32 P() "); var field = comp.GetTypeByMetadataName("C").GetMembers().OfType().Single(); Assert.Equal("System.Int32 C.field", field.ToTestDisplayString()); - Assert.Equal(0, accessorBindingData.NumberOfPerformedAccessorBinding); + Assert.Equal(1, accessorBindingData.NumberOfPerformedAccessorBinding); } [Fact] @@ -870,7 +870,7 @@ public static void Main() CompileAndVerify(comp, expectedOutput: "5"); VerifyTypeIL(comp, "C", @" - .class public auto ansi beforefieldinit C +.class public auto ansi beforefieldinit C extends [mscorlib]System.Object { // Fields @@ -931,6 +931,81 @@ .property instance int32 P() Assert.Equal(1, accessorBindingData.NumberOfPerformedAccessorBinding); } + [Fact] + public void TestAssigningFromConstructorNoAccessors() + { + var comp = CreateCompilation(@" +public class C +{ + public C() + { + P = 5; + } + + public int P { } + + public static void Main() + { + System.Console.WriteLine(new C().P); + } +} +", options: TestOptions.DebugExe); + var accessorBindingData = new SourcePropertySymbolBase.AccessorBindingData(); + comp.TestOnlyCompilationData = accessorBindingData; + + Assert.Empty(comp.GetTypeByMetadataName("C").GetMembers().OfType()); + comp.VerifyDiagnostics( + // (6,9): error CS0200: Property or indexer 'C.P' cannot be assigned to -- it is read only + // P = 5; + Diagnostic(ErrorCode.ERR_AssgReadonlyProp, "P").WithArguments("C.P").WithLocation(6, 9), + // (9,16): error CS0548: 'C.P': property or indexer must have at least one accessor + // public int P { } + Diagnostic(ErrorCode.ERR_PropertyWithNoAccessors, "P").WithArguments("C.P").WithLocation(9, 16), + // (13,34): error CS0154: The property or indexer 'C.P' cannot be used in this context because it lacks the get accessor + // System.Console.WriteLine(new C().P); + Diagnostic(ErrorCode.ERR_PropertyLacksGet, "new C().P").WithArguments("C.P").WithLocation(13, 34) + ); + Assert.Equal(0, accessorBindingData.NumberOfPerformedAccessorBinding); + } + + [Theory] + [InlineData("get;", 0)] + [InlineData("get; set;", 0)] + // [InlineData("set;")] PROTOTYPE(semi-auto-props): Not yet supported. + [InlineData("get => field;", 1)] + // [InlineData("get => field; set;")] PROTOTYPE(semi-auto-props): Not yet supported + public void TestAssigningFromConstructorThroughBackingField(string accessors, int bindingCount) + { + var comp = CreateCompilation($@" +public class C +{{ + public C() + {{ + P = 5; + }} + + public int P {{ {accessors} }} + + public static void Main() + {{ + System.Console.WriteLine(new C().P); + }} +}} +", options: TestOptions.DebugExe); + var accessorBindingData = new SourcePropertySymbolBase.AccessorBindingData(); + comp.TestOnlyCompilationData = accessorBindingData; + CompileAndVerify(comp, expectedOutput: "5").VerifyIL("C.P.get", @" +{ + // Code size 7 (0x7) + .maxstack 1 + IL_0000: ldarg.0 + IL_0001: ldfld ""int C.

k__BackingField"" + IL_0006: ret +} +"); + Assert.Equal(bindingCount, accessorBindingData.NumberOfPerformedAccessorBinding); + } + // PROTOTYPE(semi-auto-props): All success scenarios should be executed, expected runtime behavior should be observed. // This is waiting until we support assigning to readonly properties in constructor. [Fact] @@ -2376,7 +2451,7 @@ public Point(int x, int y) comp.TestOnlyCompilationData = data; Assert.Empty(comp.GetTypeByMetadataName("Point").GetMembers().OfType()); comp.VerifyDiagnostics(); - Assert.Equal(0, data.NumberOfPerformedAccessorBinding); + Assert.Equal(2, data.NumberOfPerformedAccessorBinding); } [Fact] From ea5a0fcc734834c4a39786548986a34c4e958995 Mon Sep 17 00:00:00 2001 From: Youssef1313 Date: Wed, 6 Apr 2022 10:37:49 +0200 Subject: [PATCH 03/38] Update tests --- .../Semantics/PropertyFieldKeywordTests.cs | 221 +++++++++++++++++- 1 file changed, 216 insertions(+), 5 deletions(-) diff --git a/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs b/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs index 40c3fda3a0f2e..5e0b0fcba9e2e 100644 --- a/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs +++ b/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs @@ -969,12 +969,12 @@ public static void Main() } [Theory] - [InlineData("get;", 0)] - [InlineData("get; set;", 0)] + [InlineData("get;", 0, false)] + [InlineData("get; set;", 0, true)] // [InlineData("set;")] PROTOTYPE(semi-auto-props): Not yet supported. - [InlineData("get => field;", 1)] + [InlineData("get => field;", 1, false)] // [InlineData("get => field; set;")] PROTOTYPE(semi-auto-props): Not yet supported - public void TestAssigningFromConstructorThroughBackingField(string accessors, int bindingCount) + public void TestAssigningFromConstructorThroughBackingField(string accessors, int bindingCount, bool callsSynthesizedSetter) { var comp = CreateCompilation($@" public class C @@ -994,6 +994,38 @@ public static void Main() ", options: TestOptions.DebugExe); var accessorBindingData = new SourcePropertySymbolBase.AccessorBindingData(); comp.TestOnlyCompilationData = accessorBindingData; + var expectedCtorIL = callsSynthesizedSetter switch + { + true => @" +{ + // Code size 17 (0x11) + .maxstack 2 + IL_0000: ldarg.0 + IL_0001: call ""object..ctor()"" + IL_0006: nop + IL_0007: nop + IL_0008: ldarg.0 + IL_0009: ldc.i4.5 + IL_000a: call ""void C.P.set"" + IL_000f: nop + IL_0010: ret +} +", + false => @" +{ + // Code size 16 (0x10) + .maxstack 2 + IL_0000: ldarg.0 + IL_0001: call ""object..ctor()"" + IL_0006: nop + IL_0007: nop + IL_0008: ldarg.0 + IL_0009: ldc.i4.5 + IL_000a: stfld ""int C.

k__BackingField"" + IL_000f: ret +} +", + }; CompileAndVerify(comp, expectedOutput: "5").VerifyIL("C.P.get", @" { // Code size 7 (0x7) @@ -1002,10 +1034,189 @@ .maxstack 1 IL_0001: ldfld ""int C.

k__BackingField"" IL_0006: ret } -"); +").VerifyIL("C..ctor", expectedCtorIL); Assert.Equal(bindingCount, accessorBindingData.NumberOfPerformedAccessorBinding); } + [Fact] + public void TestAssigningFromConstructorThroughSetterWithFieldKeyword_NoGetter() + { + var comp = CreateCompilation(@" +public class C +{ + public C() + { + P = 5; + } + + public int P { set => field = value * 2; } +} +"); + var accessorBindingData = new SourcePropertySymbolBase.AccessorBindingData(); + comp.TestOnlyCompilationData = accessorBindingData; + CompileAndVerify(comp).VerifyIL("C.P.set", @" +{ + // Code size 10 (0xa) + .maxstack 3 + IL_0000: ldarg.0 + IL_0001: ldarg.1 + IL_0002: ldc.i4.2 + IL_0003: mul + IL_0004: stfld ""int C.

k__BackingField"" + IL_0009: ret +} +").VerifyIL("C..ctor", @" +{ + // Code size 14 (0xe) + .maxstack 2 + IL_0000: ldarg.0 + IL_0001: call ""object..ctor()"" + IL_0006: ldarg.0 + IL_0007: ldc.i4.5 + IL_0008: call ""void C.P.set"" + IL_000d: ret +} +"); + Assert.Equal(1, accessorBindingData.NumberOfPerformedAccessorBinding); + } + + [Theory] + //[InlineData("get; set => field = value * 2;")] PROTOTYPE(semi-auto-props): Not yet supported. + [InlineData("get => field; set => field = value * 2;")] + public void TestAssigningFromConstructorThroughSetterWithFieldKeyword_HasGetter(string accessors) + { + var comp = CreateCompilation($@" +public class C +{{ + public C() + {{ + P = 5; + }} + + public int P {{ {accessors} }} + + public static void Main() + {{ + System.Console.WriteLine(new C().P); + }} +}} +", options: TestOptions.DebugExe); + var accessorBindingData = new SourcePropertySymbolBase.AccessorBindingData(); + comp.TestOnlyCompilationData = accessorBindingData; + CompileAndVerify(comp, expectedOutput: "10").VerifyIL("C.P.get", @" +{ + // Code size 7 (0x7) + .maxstack 1 + IL_0000: ldarg.0 + IL_0001: ldfld ""int C.

k__BackingField"" + IL_0006: ret +} +").VerifyIL("C..ctor", @" +{ + // Code size 17 (0x11) + .maxstack 2 + IL_0000: ldarg.0 + IL_0001: call ""object..ctor()"" + IL_0006: nop + IL_0007: nop + IL_0008: ldarg.0 + IL_0009: ldc.i4.5 + IL_000a: call ""void C.P.set"" + IL_000f: nop + IL_0010: ret +} +"); + Assert.Equal(1, accessorBindingData.NumberOfPerformedAccessorBinding); + } + + [Theory(Skip = "PROTOTYPE(semi-auto-props): Not supported yet.")] + [InlineData("get; set => _ = value;")] + [InlineData("set => _ = value;")] + [InlineData("get => field; set => _ = value;")] + public void TestAssigningFromConstructorThroughSetter_RegularSetter(string accessors) + { + var comp = CreateCompilation($@" +public class C +{{ + public C() + {{ + P = 5; + }} + + public int P {{ {accessors} }} + + public static void Main() + {{ + System.Console.WriteLine(new C().P); + }} +}} +", options: TestOptions.DebugExe); + var accessorBindingData = new SourcePropertySymbolBase.AccessorBindingData(); + comp.TestOnlyCompilationData = accessorBindingData; + CompileAndVerify(comp, expectedOutput: "0").VerifyIL("C.P.get", @" +").VerifyIL("C..ctor", @" +"); + Assert.Equal(1, accessorBindingData.NumberOfPerformedAccessorBinding); + } + + [Theory] + // [InlineData("get => 0; set;")] PROTOTYPE(semi-auto-props): Not yet supported. + [InlineData("get => 0; set => field = value;")] + public void TestAssigningFromConstructorThroughSetter_RegularGetter_CanAssignInCtor(string accessors) + { + var comp = CreateCompilation($@" +public class C +{{ + public C() + {{ + P = 5; + }} + + public int P {{ {accessors} }} + + public static void Main() + {{ + System.Console.WriteLine(new C().P); + }} +}} +", options: TestOptions.DebugExe); + var accessorBindingData = new SourcePropertySymbolBase.AccessorBindingData(); + comp.TestOnlyCompilationData = accessorBindingData; + comp.VerifyDiagnostics(); + CompileAndVerify(comp, expectedOutput: "0").VerifyIL("C.P.get", @" +{ + // Code size 2 (0x2) + .maxstack 1 + IL_0000: ldc.i4.0 + IL_0001: ret +} +").VerifyIL("C.P.set", @" +{ + // Code size 8 (0x8) + .maxstack 2 + IL_0000: ldarg.0 + IL_0001: ldarg.1 + IL_0002: stfld ""int C.

k__BackingField"" + IL_0007: ret +} +").VerifyIL("C..ctor", @" +{ + // Code size 17 (0x11) + .maxstack 2 + IL_0000: ldarg.0 + IL_0001: call ""object..ctor()"" + IL_0006: nop + IL_0007: nop + IL_0008: ldarg.0 + IL_0009: ldc.i4.5 + IL_000a: call ""void C.P.set"" + IL_000f: nop + IL_0010: ret +} +"); + Assert.Equal(1, accessorBindingData.NumberOfPerformedAccessorBinding); + } + // PROTOTYPE(semi-auto-props): All success scenarios should be executed, expected runtime behavior should be observed. // This is waiting until we support assigning to readonly properties in constructor. [Fact] From ef9ed78c7832be75efae70698456c6c309185c89 Mon Sep 17 00:00:00 2001 From: Youssef1313 Date: Wed, 6 Apr 2022 11:21:40 +0200 Subject: [PATCH 04/38] Update tests --- .../Semantics/PropertyFieldKeywordTests.cs | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs b/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs index 5e0b0fcba9e2e..f821a6a75764a 100644 --- a/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs +++ b/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs @@ -370,8 +370,8 @@ public MyAttribute(int i) { } Assert.Equal(0, accessorBindingData.NumberOfPerformedAccessorBinding); } - [Fact] - public void TestFieldIsShadowed_ReferencedFromRegularLocalFunction() + [Theory, CombinatorialData] + public void TestFieldIsShadowed_ReferencedFromRegularLocalFunction([CombinatorialValues("always", "never")] string runNullableAnalysis) { var comp = CreateCompilation(@" System.Console.WriteLine(new C().P); @@ -389,7 +389,7 @@ public int P } } } -"); +", parseOptions: TestOptions.RegularNext.WithFeature("run-nullable-analysis", runNullableAnalysis)); var accessorBindingData = new SourcePropertySymbolBase.AccessorBindingData(); comp.TestOnlyCompilationData = accessorBindingData; CompileAndVerify(comp, expectedOutput: "5"); @@ -460,8 +460,8 @@ .property instance int32 P() Assert.Equal(1, accessorBindingData.NumberOfPerformedAccessorBinding); } - [Fact] - public void TestFieldIsShadowed_ReferencedFromRegularLambda() + [Theory, CombinatorialData] + public void TestFieldIsShadowed_ReferencedFromRegularLambda([CombinatorialValues("always", "never")] string runNullableAnalysis) { var comp = CreateCompilation(@" System.Console.WriteLine(new C().P); @@ -477,7 +477,7 @@ public int P } } } -"); +", parseOptions: TestOptions.RegularNext.WithFeature("run-nullable-analysis", runNullableAnalysis)); var accessorBindingData = new SourcePropertySymbolBase.AccessorBindingData(); comp.TestOnlyCompilationData = accessorBindingData; CompileAndVerify(comp, expectedOutput: "5"); @@ -550,7 +550,7 @@ .property instance int32 P() } // end of class C "); Assert.Empty(comp.GetTypeByMetadataName("C").GetMembers().OfType()); - Assert.Equal(1, accessorBindingData.NumberOfPerformedAccessorBinding); + Assert.Equal(runNullableAnalysis == "always" ? 0 : 1, accessorBindingData.NumberOfPerformedAccessorBinding); } [Theory, CombinatorialData] @@ -765,8 +765,8 @@ .property instance int32 P() Assert.Equal(1, accessorBindingData.NumberOfPerformedAccessorBinding); } - [Fact] - public void TestFieldIsShadowedByField_ReferencedFromRegularLambda() + [Theory, CombinatorialData] + public void TestFieldIsShadowedByField_ReferencedFromRegularLambda([CombinatorialValues("always", "never")] string runNullableAnalysis) { var comp = CreateCompilation(@" System.Console.WriteLine(new C().P); @@ -783,7 +783,7 @@ public int P } } } -"); +", parseOptions: TestOptions.RegularNext.WithFeature("run-nullable-analysis", runNullableAnalysis)); var accessorBindingData = new SourcePropertySymbolBase.AccessorBindingData(); comp.TestOnlyCompilationData = accessorBindingData; CompileAndVerify(comp, expectedOutput: "5"); From 24acdbb0e87a01fd99b35f846a8fd1a61a65cdcf Mon Sep 17 00:00:00 2001 From: Youssef1313 Date: Wed, 6 Apr 2022 11:58:46 +0200 Subject: [PATCH 05/38] Fix cycle --- .../Portable/Binder/Binder_Statements.cs | 4 +- .../Semantics/PropertyFieldKeywordTests.cs | 62 ++++++++++++++++++- 2 files changed, 63 insertions(+), 3 deletions(-) diff --git a/src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs b/src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs index 008fac03ec55d..626141b33dd50 100644 --- a/src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs +++ b/src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs @@ -1703,14 +1703,14 @@ private static bool IsPropertyAssignedThroughBackingField(BoundExpression receiv propertySymbol = propertySymbol.OriginalDefinition; } - return propertySymbol is SourcePropertySymbolBase sourceProperty && + return propertySymbol is SourcePropertySymbolBase sourceProperty && + IsConstructorOrField(fromMember, isStatic: sourceProperty.IsStatic) && (sourceProperty.GetMethod is SourcePropertyAccessorSymbol { IsEquivalentToBackingFieldAccess: true } || sourceProperty.FieldKeywordBackingField is not null) && // To be assigned through backing field, either SetMethod is null, or it's equivalent to backing field write // PROTOTYPE(semi-auto-props): TODO: Do we need to use `GetOwnOrInheritedSetMethod` instead of `SetMethod`? // Legacy auto-properties are required to override all accessors. If this is not the case with semi auto props, we may need to use GetOwnOrInheritedSetMethod sourceProperty.SetMethod is null or SourcePropertyAccessorSymbol { IsEquivalentToBackingFieldAccess: true } && TypeSymbol.Equals(sourceProperty.ContainingType, fromMember.ContainingType, TypeCompareKind.ConsiderEverything2) && - IsConstructorOrField(fromMember, isStatic: sourceProperty.IsStatic) && (sourceProperty.IsStatic || receiver.Kind == BoundKind.ThisReference); } diff --git a/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs b/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs index f821a6a75764a..485f2c535dd04 100644 --- a/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs +++ b/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs @@ -2108,7 +2108,7 @@ public class C } [Fact] - public void AssignReadOnlyOnlyPropertyOutsideConstructor() + public void AssignReadOnlyOnlyPropertyOutsideConstructor_FieldAssignedFirst() { var comp = CreateCompilation(@" class Test @@ -2140,6 +2140,66 @@ int X Assert.Equal(0, accessorBindingData.NumberOfPerformedAccessorBinding); } + [Fact] + public void AssignReadOnlyOnlyPropertyOutsideConstructor_FieldAssignedAfterProperty() + { + var comp = CreateCompilation(@" +class Test +{ + int X + { + get + { + X = 3; + field = 3; + return 0; + } + } +} +"); + var accessorBindingData = new SourcePropertySymbolBase.AccessorBindingData(); + comp.TestOnlyCompilationData = accessorBindingData; + Assert.Empty(comp.GetTypeByMetadataName("Test").GetMembers().OfType()); + comp.VerifyDiagnostics( + // (8,13): error CS0200: Property or indexer 'Test.X' cannot be assigned to -- it is read only + // X = 3; + Diagnostic(ErrorCode.ERR_AssgReadonlyProp, "X").WithArguments("Test.X").WithLocation(8, 13), + // PROTOTYPE(semi-auto-props): + // Should the generated field not be readonly? + // (9,13): error CS0191: A readonly field cannot be assigned to (except in a constructor or init-only setter of the type in which the field is defined or a variable initializer) + // field = 3; + Diagnostic(ErrorCode.ERR_AssgReadonly, "field").WithLocation(9, 13) + ); + Assert.Equal(0, accessorBindingData.NumberOfPerformedAccessorBinding); + } + + [Fact] + public void AssignReadOnlyOnlyPropertyOutsideConstructor_FieldNotAssigned() + { + var comp = CreateCompilation(@" +class Test +{ + int X + { + get + { + X = 3; + return 0; + } + } +} +"); + var accessorBindingData = new SourcePropertySymbolBase.AccessorBindingData(); + comp.TestOnlyCompilationData = accessorBindingData; + Assert.Empty(comp.GetTypeByMetadataName("Test").GetMembers().OfType()); + comp.VerifyDiagnostics( + // (8,13): error CS0200: Property or indexer 'Test.X' cannot be assigned to -- it is read only + // X = 3; + Diagnostic(ErrorCode.ERR_AssgReadonlyProp, "X").WithArguments("Test.X").WithLocation(8, 13) + ); + Assert.Equal(0, accessorBindingData.NumberOfPerformedAccessorBinding); + } + [Fact(Skip = "PROTOTYPE(semi-auto-props): Assigning in constructor is not yet supported.")] public void AssignReadOnlyOnlyPropertyInConstructor() { From 0e1283ced7906197ee62f5d9129410af093a152d Mon Sep 17 00:00:00 2001 From: Youssef1313 Date: Wed, 6 Apr 2022 12:12:18 +0200 Subject: [PATCH 06/38] Unskip a test --- .../Test/Semantic/Semantics/PropertyFieldKeywordTests.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs b/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs index 485f2c535dd04..cbe63ca6a7b2f 100644 --- a/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs +++ b/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs @@ -2200,7 +2200,7 @@ int X Assert.Equal(0, accessorBindingData.NumberOfPerformedAccessorBinding); } - [Fact(Skip = "PROTOTYPE(semi-auto-props): Assigning in constructor is not yet supported.")] + [Fact] public void AssignReadOnlyOnlyPropertyInConstructor() { var comp = CreateCompilation(@" @@ -2270,7 +2270,7 @@ .property instance int32 X() } } // end of class Test "); - Assert.Equal(0, accessorBindingData.NumberOfPerformedAccessorBinding); + Assert.Equal(1, accessorBindingData.NumberOfPerformedAccessorBinding); } [Fact] From 60e3cc67cc53c9bbe98e9d78d6e6268f4ae772ed Mon Sep 17 00:00:00 2001 From: Youssef Victor Date: Wed, 6 Apr 2022 17:15:44 +0200 Subject: [PATCH 07/38] Fix formatting --- src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs b/src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs index 626141b33dd50..18d491b1f92c9 100644 --- a/src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs +++ b/src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs @@ -1703,7 +1703,7 @@ private static bool IsPropertyAssignedThroughBackingField(BoundExpression receiv propertySymbol = propertySymbol.OriginalDefinition; } - return propertySymbol is SourcePropertySymbolBase sourceProperty && + return propertySymbol is SourcePropertySymbolBase sourceProperty && IsConstructorOrField(fromMember, isStatic: sourceProperty.IsStatic) && (sourceProperty.GetMethod is SourcePropertyAccessorSymbol { IsEquivalentToBackingFieldAccess: true } || sourceProperty.FieldKeywordBackingField is not null) && // To be assigned through backing field, either SetMethod is null, or it's equivalent to backing field write From 310585f4a94d9bfcad11169ac9519f5a82987afa Mon Sep 17 00:00:00 2001 From: Youssef1313 Date: Fri, 8 Apr 2022 05:06:09 +0200 Subject: [PATCH 08/38] Update failing tests --- .../Semantics/PropertyFieldKeywordTests.cs | 22 +++++++++---------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs b/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs index 5a9b4e3c33d1a..2231118d69da1 100644 --- a/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs +++ b/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs @@ -261,8 +261,8 @@ public MyAttribute(int i) { } Assert.Equal(0, accessorBindingData.NumberOfPerformedAccessorBinding); } - [Fact] - public void TestFieldInLocalFunction_ShadowedByConstant() + [Theory, CombinatorialData] + public void TestFieldInLocalFunction_ShadowedByConstant([CombinatorialValues("always", "never")] string runNullableAnalysis) { var comp = CreateCompilation(@" System.Console.WriteLine(new C().P); @@ -290,7 +290,7 @@ public class MyAttribute : System.Attribute { public MyAttribute(int i) { } } -"); +", parseOptions: TestOptions.RegularNext.WithFeature("run-nullable-analysis", runNullableAnalysis)); var accessorBindingData = new SourcePropertySymbolBase.AccessorBindingData(); comp.TestOnlyCompilationData = accessorBindingData; CompileAndVerify(comp, expectedOutput: "10"); @@ -377,7 +377,7 @@ .property instance int32 P() } // end of class C "); Assert.Empty(comp.GetTypeByMetadataName("C").GetMembers().OfType()); - Assert.Equal(1, accessorBindingData.NumberOfPerformedAccessorBinding); + Assert.Equal(0, accessorBindingData.NumberOfPerformedAccessorBinding); } [Fact] @@ -511,7 +511,7 @@ .property instance int32 P() } // end of class C "); Assert.Empty(comp.GetTypeByMetadataName("C").GetMembers().OfType()); - Assert.Equal(1, accessorBindingData.NumberOfPerformedAccessorBinding); + Assert.Equal(0, accessorBindingData.NumberOfPerformedAccessorBinding); } [Theory, CombinatorialData] @@ -604,7 +604,7 @@ .property instance int32 P() } // end of class C "); Assert.Empty(comp.GetTypeByMetadataName("C").GetMembers().OfType()); - Assert.Equal(runNullableAnalysis == "always" ? 0 : 1, accessorBindingData.NumberOfPerformedAccessorBinding); + Assert.Equal(0, accessorBindingData.NumberOfPerformedAccessorBinding); } [Theory, CombinatorialData] @@ -740,8 +740,8 @@ public int P Assert.Equal(0, accessorBindingData.NumberOfPerformedAccessorBinding); } - [Fact] - public void TestFieldIsShadowedByField_ReferencedFromRegularLocalFunction() + [Theory, CombinatorialData] + public void TestFieldIsShadowedByField_ReferencedFromRegularLocalFunction([CombinatorialValues("always", "never")] string runNullableAnalysis) { var comp = CreateCompilation(@" System.Console.WriteLine(new C().P); @@ -760,7 +760,7 @@ public int P } } } -"); +", parseOptions: TestOptions.RegularNext.WithFeature("run-nullable-analysis", runNullableAnalysis)); var accessorBindingData = new SourcePropertySymbolBase.AccessorBindingData(); comp.TestOnlyCompilationData = accessorBindingData; CompileAndVerify(comp, expectedOutput: "5"); @@ -816,7 +816,7 @@ .property instance int32 P() "); var field = comp.GetTypeByMetadataName("C").GetMembers().OfType().Single(); Assert.Equal("System.Int32 C.field", field.ToTestDisplayString()); - Assert.Equal(1, accessorBindingData.NumberOfPerformedAccessorBinding); + Assert.Equal(0, accessorBindingData.NumberOfPerformedAccessorBinding); } [Fact] @@ -970,7 +970,7 @@ .property instance int32 P() "); var field = comp.GetTypeByMetadataName("C").GetMembers().OfType().Single(); Assert.Equal("System.Int32 C.field", field.ToTestDisplayString()); - Assert.Equal(1, accessorBindingData.NumberOfPerformedAccessorBinding); + Assert.Equal(0, accessorBindingData.NumberOfPerformedAccessorBinding); } [Theory] From 2b3703f468808ef332105e4592dfef23ec72fba1 Mon Sep 17 00:00:00 2001 From: Youssef1313 Date: Fri, 8 Apr 2022 07:04:25 +0200 Subject: [PATCH 09/38] Support auto-default struct in conjunction of semi-auto-prop --- .../FlowAnalysis/EmptyStructTypeCache.cs | 13 +- .../Semantics/PropertyFieldKeywordTests.cs | 159 +++++++++++++++++- 2 files changed, 162 insertions(+), 10 deletions(-) diff --git a/src/Compilers/CSharp/Portable/FlowAnalysis/EmptyStructTypeCache.cs b/src/Compilers/CSharp/Portable/FlowAnalysis/EmptyStructTypeCache.cs index ac63c36de73d3..f8fe5a3caf39f 100644 --- a/src/Compilers/CSharp/Portable/FlowAnalysis/EmptyStructTypeCache.cs +++ b/src/Compilers/CSharp/Portable/FlowAnalysis/EmptyStructTypeCache.cs @@ -215,12 +215,17 @@ private FieldSymbol GetActualField(Symbol member, NamedTypeSymbol type) return (!eventSymbol.HasAssociatedField || ShouldIgnoreStructField(eventSymbol, eventSymbol.Type)) ? null : eventSymbol.AssociatedField.AsMember(type); case SymbolKind.Property: // PROTOTYPE(semi-auto-props): Review other event associated field callers and see if we have to do anything special for properties. + // Backing field for semi auto props are not included in GetMembers. - // PROTOTYPE(semi-auto-props): This condition is always false after modifiying BackingField implementation. Figure out a - // test that needs this. - if (member is SourcePropertySymbol { BackingField: SynthesizedBackingFieldSymbol { IsCreatedForFieldKeyword: true } backingField }) + if (member is SourcePropertySymbol property) { - return backingField; + // PROTOTYPE(semi-auto-props): property.BackingField be 'IsCreatedForFieldKeyword' only when it has initializer. + // Add a test for that when initializers are supported. + var backingField = property.BackingField ?? property.FieldKeywordBackingField; + if (backingField is { IsCreatedForFieldKeyword: true }) + { + return backingField; + } } return null; } diff --git a/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs b/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs index 2231118d69da1..5b46f5c8faef8 100644 --- a/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs +++ b/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs @@ -217,6 +217,119 @@ .property instance int32 P() Assert.Equal(0, accessorBindingData.NumberOfPerformedAccessorBinding); } + [Fact] + public void TestPropertyNotAssignedInStructConstructor() + { + var comp = CreateCompilation(@" +var x = new C(); +x.P = 10; +x = new C(); + +public struct C +{ + public C() + { + System.Console.WriteLine(""In C..ctor: "" + P); + } + + public int P { get => field; set => field = value; } +} +", options: TestOptions.ReleaseExe.WithSpecificDiagnosticOptions(ReportStructInitializationWarnings)); + var accessorBindingData = new SourcePropertySymbolBase.AccessorBindingData(); + comp.TestOnlyCompilationData = accessorBindingData; + comp.VerifyDiagnostics( + // (10,51): warning CS9020: The 'this' object is read before all of its fields have been assigned, causing preceding implicit assignments of 'default' to non-explicitly assigned fields. + // System.Console.WriteLine("In C..ctor: " + P); + Diagnostic(ErrorCode.WRN_UseDefViolationThisSupportedVersion, "P").WithArguments("this").WithLocation(10, 51) + ); + CompileAndVerify(comp, expectedOutput: @"In C..ctor: 0 +In C..ctor: 0").VerifyIL("C..ctor", @" +{ + // Code size 37 (0x25) + .maxstack 2 + .locals init (int V_0) + IL_0000: ldarg.0 + IL_0001: ldc.i4.0 + IL_0002: stfld ""int C.

k__BackingField"" + IL_0007: ldstr ""In C..ctor: "" + IL_000c: ldarg.0 + IL_000d: call ""int C.P.get"" + IL_0012: stloc.0 + IL_0013: ldloca.s V_0 + IL_0015: call ""string int.ToString()"" + IL_001a: call ""string string.Concat(string, string)"" + IL_001f: call ""void System.Console.WriteLine(string)"" + IL_0024: ret +} +"); + Assert.Empty(comp.GetTypeByMetadataName("C").GetMembers().OfType()); + Assert.Equal(1, accessorBindingData.NumberOfPerformedAccessorBinding); + } + + [Fact] + public void TestPropertyIsAssignedInStructConstructor() + { + var comp = CreateCompilation(@" +var x = new C(); +x.P = 10; +x = new C(); + +public struct C +{ + public C() + { + System.Console.WriteLine(""In C..ctor before assignment: "" + P); + P = 5; + System.Console.WriteLine(""In C..ctor after assignment: "" + P); + } + + public int P { get => field; set => field = value; } +} +", options: TestOptions.ReleaseExe.WithSpecificDiagnosticOptions(ReportStructInitializationWarnings)); + var accessorBindingData = new SourcePropertySymbolBase.AccessorBindingData(); + comp.TestOnlyCompilationData = accessorBindingData; + comp.VerifyDiagnostics( + // (10,69): warning CS9020: The 'this' object is read before all of its fields have been assigned, causing preceding implicit assignments of 'default' to non-explicitly assigned fields. + // System.Console.WriteLine("In C..ctor before assignment: " + P); + Diagnostic(ErrorCode.WRN_UseDefViolationThisSupportedVersion, "P").WithArguments("this").WithLocation(10, 69) + ); + CompileAndVerify(comp, expectedOutput: @"In C..ctor before assignment: 0 +In C..ctor after assignment: 5 +In C..ctor before assignment: 0 +In C..ctor after assignment: 5").VerifyIL("C..ctor", @" +{ + // Code size 73 (0x49) + .maxstack 2 + .locals init (int V_0) + IL_0000: ldarg.0 + IL_0001: ldc.i4.0 + IL_0002: stfld ""int C.

k__BackingField"" + IL_0007: ldstr ""In C..ctor before assignment: "" + IL_000c: ldarg.0 + IL_000d: call ""int C.P.get"" + IL_0012: stloc.0 + IL_0013: ldloca.s V_0 + IL_0015: call ""string int.ToString()"" + IL_001a: call ""string string.Concat(string, string)"" + IL_001f: call ""void System.Console.WriteLine(string)"" + IL_0024: ldarg.0 + IL_0025: ldc.i4.5 + IL_0026: call ""void C.P.set"" + IL_002b: ldstr ""In C..ctor after assignment: "" + IL_0030: ldarg.0 + IL_0031: call ""int C.P.get"" + IL_0036: stloc.0 + IL_0037: ldloca.s V_0 + IL_0039: call ""string int.ToString()"" + IL_003e: call ""string string.Concat(string, string)"" + IL_0043: call ""void System.Console.WriteLine(string)"" + IL_0048: ret +} +"); + Assert.Empty(comp.GetTypeByMetadataName("C").GetMembers().OfType()); + Assert.Equal(1, accessorBindingData.NumberOfPerformedAccessorBinding); + } + [Fact] public void TestFieldInLocalFunction() { @@ -293,6 +406,12 @@ public MyAttribute(int i) { } ", parseOptions: TestOptions.RegularNext.WithFeature("run-nullable-analysis", runNullableAnalysis)); var accessorBindingData = new SourcePropertySymbolBase.AccessorBindingData(); comp.TestOnlyCompilationData = accessorBindingData; + comp.VerifyDiagnostics( + // Looks like an incorrect diagnostic. Tracked by https://github.com/dotnet/roslyn/issues/60645 + // (10,23): warning CS0219: The variable 'field' is assigned but its value is never used + // const int field = 5; + Diagnostic(ErrorCode.WRN_UnreferencedVarAssg, "field").WithArguments("field").WithLocation(10, 23) + ); CompileAndVerify(comp, expectedOutput: "10"); VerifyTypeIL(comp, "C", @" .class public auto ansi beforefieldinit C @@ -446,6 +565,7 @@ public int P ", parseOptions: TestOptions.RegularNext.WithFeature("run-nullable-analysis", runNullableAnalysis)); var accessorBindingData = new SourcePropertySymbolBase.AccessorBindingData(); comp.TestOnlyCompilationData = accessorBindingData; + comp.VerifyDiagnostics(); CompileAndVerify(comp, expectedOutput: "5"); VerifyTypeIL(comp, "C", @" .class public auto ansi beforefieldinit C @@ -534,6 +654,7 @@ public int P ", parseOptions: TestOptions.RegularNext.WithFeature("run-nullable-analysis", runNullableAnalysis)); var accessorBindingData = new SourcePropertySymbolBase.AccessorBindingData(); comp.TestOnlyCompilationData = accessorBindingData; + comp.VerifyDiagnostics(); CompileAndVerify(comp, expectedOutput: "5"); VerifyTypeIL(comp, "C", @" .class public auto ansi beforefieldinit C @@ -763,6 +884,7 @@ public int P ", parseOptions: TestOptions.RegularNext.WithFeature("run-nullable-analysis", runNullableAnalysis)); var accessorBindingData = new SourcePropertySymbolBase.AccessorBindingData(); comp.TestOnlyCompilationData = accessorBindingData; + comp.VerifyDiagnostics(); CompileAndVerify(comp, expectedOutput: "5"); VerifyTypeIL(comp, "C", @" .class public auto ansi beforefieldinit C @@ -915,6 +1037,7 @@ public int P ", parseOptions: TestOptions.RegularNext.WithFeature("run-nullable-analysis", runNullableAnalysis)); var accessorBindingData = new SourcePropertySymbolBase.AccessorBindingData(); comp.TestOnlyCompilationData = accessorBindingData; + comp.VerifyDiagnostics(); CompileAndVerify(comp, expectedOutput: "5"); VerifyTypeIL(comp, "C", @" .class public auto ansi beforefieldinit C @@ -997,6 +1120,19 @@ public string P "); var accessorBindingData = new SourcePropertySymbolBase.AccessorBindingData(); comp.TestOnlyCompilationData = accessorBindingData; + if (member == "private int field;") + { + comp.VerifyDiagnostics( + // (6,17): warning CS0649: Field 'C.field' is never assigned to, and will always have its default value 0 + // private int field; + Diagnostic(ErrorCode.WRN_UnassignedInternalField, "field").WithArguments("C.field", "0").WithLocation(6, 17) + ); + } + else + { + comp.VerifyDiagnostics(); + } + CompileAndVerify(comp, expectedOutput: "field"); Assert.Equal(0, accessorBindingData.NumberOfPerformedAccessorBinding); } @@ -1025,6 +1161,7 @@ public static void Main() Assert.Empty(comp.GetTypeByMetadataName("C").GetMembers().OfType()); + comp.VerifyDiagnostics(); CompileAndVerify(comp, expectedOutput: "5"); VerifyTypeIL(comp, "C", @" .class public auto ansi beforefieldinit C @@ -1183,6 +1320,7 @@ .maxstack 2 } ", }; + comp.VerifyDiagnostics(); CompileAndVerify(comp, expectedOutput: "5").VerifyIL("C.P.get", @" { // Code size 7 (0x7) @@ -1211,6 +1349,7 @@ public C() "); var accessorBindingData = new SourcePropertySymbolBase.AccessorBindingData(); comp.TestOnlyCompilationData = accessorBindingData; + comp.VerifyDiagnostics(); CompileAndVerify(comp).VerifyIL("C.P.set", @" { // Code size 10 (0xa) @@ -1260,6 +1399,7 @@ public static void Main() ", options: TestOptions.DebugExe); var accessorBindingData = new SourcePropertySymbolBase.AccessorBindingData(); comp.TestOnlyCompilationData = accessorBindingData; + comp.VerifyDiagnostics(); CompileAndVerify(comp, expectedOutput: "10").VerifyIL("C.P.get", @" { // Code size 7 (0x7) @@ -1310,6 +1450,7 @@ public static void Main() ", options: TestOptions.DebugExe); var accessorBindingData = new SourcePropertySymbolBase.AccessorBindingData(); comp.TestOnlyCompilationData = accessorBindingData; + comp.VerifyDiagnostics(); CompileAndVerify(comp, expectedOutput: "0").VerifyIL("C.P.get", @" ").VerifyIL("C..ctor", @" "); @@ -1813,7 +1954,7 @@ .property instance string P() Assert.Equal(0, accessorBindingData.NumberOfPerformedAccessorBinding); } - [Fact(Skip = "PROTOTYPE(semi-auto-props): Currently has extra diagnostics")] + [Fact] public void Test_ERR_AutoSetterCantBeReadOnly() { var comp = CreateCompilation(@" @@ -1827,7 +1968,7 @@ public struct S "); var accessorBindingData = new SourcePropertySymbolBase.AccessorBindingData(); comp.TestOnlyCompilationData = accessorBindingData; - Assert.Empty(comp.GetTypeByMetadataName("C").GetMembers().OfType()); + Assert.Equal("System.Int32 S.k__BackingField", comp.GetTypeByMetadataName("S").GetMembers().OfType().Single().ToTestDisplayString()); comp.VerifyDiagnostics( // (4,35): error CS8658: Auto-implemented 'set' accessor 'S.P1.set' cannot be marked 'readonly'. // public int P1 { get; readonly set; } // ERR_AutoSetterCantBeReadOnly @@ -1837,9 +1978,13 @@ public struct S Diagnostic(ErrorCode.ERR_AssgReadonlyLocal, "field").WithArguments("field").WithLocation(6, 51), // (7,42): error CS1604: Cannot assign to 'field' because it is read-only // public int P4 { get; readonly set => field = value; } // No ERR_AutoSetterCantBeReadOnly, but ERR_AssgReadonlyLocal - Diagnostic(ErrorCode.ERR_AssgReadonlyLocal, "field").WithArguments("field").WithLocation(7, 42) + Diagnostic(ErrorCode.ERR_AssgReadonlyLocal, "field").WithArguments("field").WithLocation(7, 42), + // PROTOTYPE(semi-auto-props): The following diagnostic shouldn't exist. It should go away when mixed scenarios are supported. + // (7,21): error CS0501: 'S.P4.get' must declare a body because it is not marked abstract, extern, or partial + // public int P4 { get; readonly set => field = value; } // No ERR_AutoSetterCantBeReadOnly, but ERR_AssgReadonlyLocal + Diagnostic(ErrorCode.ERR_ConcreteMissingBody, "get").WithArguments("S.P4.get").WithLocation(7, 21) ); - Assert.Equal(0, accessorBindingData.NumberOfPerformedAccessorBinding); + Assert.Equal(2, accessorBindingData.NumberOfPerformedAccessorBinding); } [Fact] @@ -3024,10 +3169,12 @@ void M3() Assert.Empty(comp.GetTypeByMetadataName("S_WithManualProperty").GetMembers().OfType()); Assert.Empty(comp.GetTypeByMetadataName("S_WithSemiAutoProperty").GetMembers().OfType()); comp.VerifyDiagnostics( - // PROTOTYPE(semi-auto-props): Do we expect a similar error for semi auto props? // (22,33): error CS0165: Use of unassigned local variable 's1' // S_WithAutoProperty s2 = s1; - Diagnostic(ErrorCode.ERR_UseDefViolation, "s1").WithArguments("s1").WithLocation(22, 33) + Diagnostic(ErrorCode.ERR_UseDefViolation, "s1").WithArguments("s1").WithLocation(22, 33), + // (34,37): error CS0165: Use of unassigned local variable 's1' + // S_WithSemiAutoProperty s2 = s1; + Diagnostic(ErrorCode.ERR_UseDefViolation, "s1").WithArguments("s1").WithLocation(34, 37) ); Assert.Equal(0, accessorBindingData.NumberOfPerformedAccessorBinding); From 2b81ca97de65f38bb3c0bde710e0e1a537c7aa6a Mon Sep 17 00:00:00 2001 From: Youssef1313 Date: Fri, 8 Apr 2022 10:53:27 +0200 Subject: [PATCH 10/38] Fix test --- .../Test/Semantic/Semantics/PropertyFieldKeywordTests.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs b/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs index 5b46f5c8faef8..633a712521579 100644 --- a/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs +++ b/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs @@ -3123,8 +3123,8 @@ public class C1 Assert.Equal(0, accessorBindingData.NumberOfPerformedAccessorBinding); } - [Fact] - public void InStruct() + [Theory, CombinatorialData] + public void InStruct([CombinatorialValues("always", "never")] string runNullableAnalysis) { var comp = CreateCompilation(@" struct S_WithAutoProperty @@ -3162,7 +3162,7 @@ void M3() S_WithSemiAutoProperty s2 = s1; } } -"); +", parseOptions: TestOptions.RegularNext.WithFeature("run-nullable-analysis", runNullableAnalysis)); var accessorBindingData = new SourcePropertySymbolBase.AccessorBindingData(); comp.TestOnlyCompilationData = accessorBindingData; Assert.Equal("System.Int32 S_WithAutoProperty.

k__BackingField", comp.GetTypeByMetadataName("S_WithAutoProperty").GetMembers().OfType().Single().ToTestDisplayString()); @@ -3177,7 +3177,7 @@ void M3() Diagnostic(ErrorCode.ERR_UseDefViolation, "s1").WithArguments("s1").WithLocation(34, 37) ); - Assert.Equal(0, accessorBindingData.NumberOfPerformedAccessorBinding); + Assert.Equal(runNullableAnalysis == "always" ? 0 : 1, accessorBindingData.NumberOfPerformedAccessorBinding); } [Fact] From 67964c7e7d364062db8b4afe19012978f98631b6 Mon Sep 17 00:00:00 2001 From: Youssef1313 Date: Fri, 8 Apr 2022 14:19:31 +0200 Subject: [PATCH 11/38] Remove assert --- .../CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs b/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs index 633a712521579..83170a4a3cadd 100644 --- a/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs +++ b/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs @@ -3176,8 +3176,6 @@ void M3() // S_WithSemiAutoProperty s2 = s1; Diagnostic(ErrorCode.ERR_UseDefViolation, "s1").WithArguments("s1").WithLocation(34, 37) ); - - Assert.Equal(runNullableAnalysis == "always" ? 0 : 1, accessorBindingData.NumberOfPerformedAccessorBinding); } [Fact] From ba5e9514314242dd6e1de2bb2d887f28dc335edb Mon Sep 17 00:00:00 2001 From: Youssef1313 Date: Sat, 16 Apr 2022 23:19:22 +0200 Subject: [PATCH 12/38] Address some review comments --- .../Portable/Binder/Binder_Statements.cs | 6 +- .../FlowAnalysis/EmptyStructTypeCache.cs | 6 +- .../LocalRewriter_AssignmentOperator.cs | 6 +- .../Semantics/PropertyFieldKeywordTests.cs | 98 +++++++++++-------- 4 files changed, 63 insertions(+), 53 deletions(-) diff --git a/src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs b/src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs index 37055bf3ad6b9..d53a248cafef9 100644 --- a/src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs +++ b/src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs @@ -1704,14 +1704,14 @@ private static bool IsPropertyAssignedThroughBackingField(BoundExpression receiv } return propertySymbol is SourcePropertySymbolBase sourceProperty && + (sourceProperty.IsStatic || receiver.Kind == BoundKind.ThisReference) && IsConstructorOrField(fromMember, isStatic: sourceProperty.IsStatic) && - (sourceProperty.GetMethod is SourcePropertyAccessorSymbol { IsEquivalentToBackingFieldAccess: true } || sourceProperty.FieldKeywordBackingField is not null) && + TypeSymbol.Equals(sourceProperty.ContainingType, fromMember.ContainingType, TypeCompareKind.ConsiderEverything2) && // To be assigned through backing field, either SetMethod is null, or it's equivalent to backing field write // PROTOTYPE(semi-auto-props): TODO: Do we need to use `GetOwnOrInheritedSetMethod` instead of `SetMethod`? // Legacy auto-properties are required to override all accessors. If this is not the case with semi auto props, we may need to use GetOwnOrInheritedSetMethod sourceProperty.SetMethod is null or SourcePropertyAccessorSymbol { IsEquivalentToBackingFieldAccess: true } && - TypeSymbol.Equals(sourceProperty.ContainingType, fromMember.ContainingType, TypeCompareKind.ConsiderEverything2) && - (sourceProperty.IsStatic || receiver.Kind == BoundKind.ThisReference); + (sourceProperty.GetMethod is SourcePropertyAccessorSymbol { IsEquivalentToBackingFieldAccess: true } || sourceProperty.FieldKeywordBackingField is not null); } private static bool IsConstructorOrField(Symbol member, bool isStatic) diff --git a/src/Compilers/CSharp/Portable/FlowAnalysis/EmptyStructTypeCache.cs b/src/Compilers/CSharp/Portable/FlowAnalysis/EmptyStructTypeCache.cs index f8fe5a3caf39f..fd9acc2ddd414 100644 --- a/src/Compilers/CSharp/Portable/FlowAnalysis/EmptyStructTypeCache.cs +++ b/src/Compilers/CSharp/Portable/FlowAnalysis/EmptyStructTypeCache.cs @@ -219,10 +219,8 @@ private FieldSymbol GetActualField(Symbol member, NamedTypeSymbol type) // Backing field for semi auto props are not included in GetMembers. if (member is SourcePropertySymbol property) { - // PROTOTYPE(semi-auto-props): property.BackingField be 'IsCreatedForFieldKeyword' only when it has initializer. - // Add a test for that when initializers are supported. - var backingField = property.BackingField ?? property.FieldKeywordBackingField; - if (backingField is { IsCreatedForFieldKeyword: true }) + // PROTOTYPE(semi-auto-props): Add definite assignment tests involving initializer, including error scenarios where a FieldKeywordBackingField is still created. + if (property.FieldKeywordBackingField is { } backingField) { return backingField; } diff --git a/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_AssignmentOperator.cs b/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_AssignmentOperator.cs index 15673add875d8..4832e50aff89e 100644 --- a/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_AssignmentOperator.cs +++ b/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_AssignmentOperator.cs @@ -301,10 +301,8 @@ private BoundExpression MakePropertyAssignment( if (setMethod is null) { var autoProp = (SourcePropertySymbolBase)property.OriginalDefinition; - Debug.Assert(autoProp.IsAutoPropertyWithGetAccessor || ( - autoProp.SetMethod is null or SourcePropertyAccessorSymbol { IsEquivalentToBackingFieldAccess: true } && - (autoProp.GetMethod is SourcePropertyAccessorSymbol { IsEquivalentToBackingFieldAccess: true } || autoProp.FieldKeywordBackingField is not null) - ), + Debug.Assert(autoProp.SetMethod is null && + (autoProp.GetMethod is SourcePropertyAccessorSymbol { IsEquivalentToBackingFieldAccess: true } || autoProp.FieldKeywordBackingField is not null), "only autoproperties can be assignable without having setters"); Debug.Assert(property.Equals(autoProp, TypeCompareKind.IgnoreNullableModifiersForReferenceTypes)); diff --git a/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs b/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs index 83170a4a3cadd..a0ef1441b2450 100644 --- a/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs +++ b/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs @@ -374,8 +374,8 @@ public MyAttribute(int i) { } Assert.Equal(0, accessorBindingData.NumberOfPerformedAccessorBinding); } - [Theory, CombinatorialData] - public void TestFieldInLocalFunction_ShadowedByConstant([CombinatorialValues("always", "never")] string runNullableAnalysis) + [Fact] + public void TestFieldInLocalFunction_ShadowedByConstant() { var comp = CreateCompilation(@" System.Console.WriteLine(new C().P); @@ -403,7 +403,7 @@ public class MyAttribute : System.Attribute { public MyAttribute(int i) { } } -", parseOptions: TestOptions.RegularNext.WithFeature("run-nullable-analysis", runNullableAnalysis)); +"); var accessorBindingData = new SourcePropertySymbolBase.AccessorBindingData(); comp.TestOnlyCompilationData = accessorBindingData; comp.VerifyDiagnostics( @@ -1107,7 +1107,9 @@ public void TestNameOfField_FieldIsMember(string member) public class C {{ +#pragma warning disable CS0649 // Field 'C.field' is never assigned to, and will always have its default value 0 {member} +#pragma warning restore CS0649 public string P {{ @@ -1120,18 +1122,7 @@ public string P "); var accessorBindingData = new SourcePropertySymbolBase.AccessorBindingData(); comp.TestOnlyCompilationData = accessorBindingData; - if (member == "private int field;") - { - comp.VerifyDiagnostics( - // (6,17): warning CS0649: Field 'C.field' is never assigned to, and will always have its default value 0 - // private int field; - Diagnostic(ErrorCode.WRN_UnassignedInternalField, "field").WithArguments("C.field", "0").WithLocation(6, 17) - ); - } - else - { - comp.VerifyDiagnostics(); - } + comp.VerifyDiagnostics(); CompileAndVerify(comp, expectedOutput: "field"); Assert.Equal(0, accessorBindingData.NumberOfPerformedAccessorBinding); @@ -1237,13 +1228,8 @@ public C() } public int P { } - - public static void Main() - { - System.Console.WriteLine(new C().P); - } } -", options: TestOptions.DebugExe); +"); var accessorBindingData = new SourcePropertySymbolBase.AccessorBindingData(); comp.TestOnlyCompilationData = accessorBindingData; @@ -1254,10 +1240,7 @@ public static void Main() Diagnostic(ErrorCode.ERR_AssgReadonlyProp, "P").WithArguments("C.P").WithLocation(6, 9), // (9,16): error CS0548: 'C.P': property or indexer must have at least one accessor // public int P { } - Diagnostic(ErrorCode.ERR_PropertyWithNoAccessors, "P").WithArguments("C.P").WithLocation(9, 16), - // (13,34): error CS0154: The property or indexer 'C.P' cannot be used in this context because it lacks the get accessor - // System.Console.WriteLine(new C().P); - Diagnostic(ErrorCode.ERR_PropertyLacksGet, "new C().P").WithArguments("C.P").WithLocation(13, 34) + Diagnostic(ErrorCode.ERR_PropertyWithNoAccessors, "P").WithArguments("C.P").WithLocation(9, 16) ); Assert.Equal(0, accessorBindingData.NumberOfPerformedAccessorBinding); } @@ -1288,9 +1271,11 @@ public static void Main() ", options: TestOptions.DebugExe); var accessorBindingData = new SourcePropertySymbolBase.AccessorBindingData(); comp.TestOnlyCompilationData = accessorBindingData; - var expectedCtorIL = callsSynthesizedSetter switch + string expectedCtorIL; + if (callsSynthesizedSetter) { - true => @" + + expectedCtorIL = @" { // Code size 17 (0x11) .maxstack 2 @@ -1304,8 +1289,11 @@ .maxstack 2 IL_000f: nop IL_0010: ret } -", - false => @" +"; + } + else + { + expectedCtorIL = @" { // Code size 16 (0x10) .maxstack 2 @@ -1318,8 +1306,9 @@ .maxstack 2 IL_000a: stfld ""int C.

k__BackingField"" IL_000f: ret } -", - }; +"; + } + comp.VerifyDiagnostics(); CompileAndVerify(comp, expectedOutput: "5").VerifyIL("C.P.get", @" { @@ -1373,7 +1362,7 @@ .maxstack 2 IL_000d: ret } "); - Assert.Equal(1, accessorBindingData.NumberOfPerformedAccessorBinding); + Assert.Equal(0, accessorBindingData.NumberOfPerformedAccessorBinding); } [Theory] @@ -1423,7 +1412,7 @@ .maxstack 2 IL_0010: ret } "); - Assert.Equal(1, accessorBindingData.NumberOfPerformedAccessorBinding); + Assert.Equal(0, accessorBindingData.NumberOfPerformedAccessorBinding); } [Theory(Skip = "PROTOTYPE(semi-auto-props): Not supported yet.")] @@ -1512,7 +1501,7 @@ .maxstack 2 IL_0010: ret } "); - Assert.Equal(1, accessorBindingData.NumberOfPerformedAccessorBinding); + Assert.Equal(0, accessorBindingData.NumberOfPerformedAccessorBinding); } // PROTOTYPE(semi-auto-props): All success scenarios should be executed, expected runtime behavior should be observed. @@ -3024,7 +3013,7 @@ public Point(int x, int y) comp.TestOnlyCompilationData = data; Assert.Empty(comp.GetTypeByMetadataName("Point").GetMembers().OfType()); comp.VerifyDiagnostics(); - Assert.Equal(2, data.NumberOfPerformedAccessorBinding); + Assert.Equal(0, data.NumberOfPerformedAccessorBinding); } [Fact] @@ -3124,9 +3113,9 @@ public class C1 } [Theory, CombinatorialData] - public void InStruct([CombinatorialValues("always", "never")] string runNullableAnalysis) + public void InStruct([CombinatorialValues("always", "never")] string runNullableAnalysis, bool structTreeFirst) { - var comp = CreateCompilation(@" + var source1 = @" struct S_WithAutoProperty { public int P { get; set; } @@ -3141,7 +3130,9 @@ struct S_WithSemiAutoProperty { public int P { get => field; set => field = value; } } +"; + var source2 = @" class C { void M1() @@ -3162,20 +3153,43 @@ void M3() S_WithSemiAutoProperty s2 = s1; } } -", parseOptions: TestOptions.RegularNext.WithFeature("run-nullable-analysis", runNullableAnalysis)); +"; + var comp = CreateCompilation(new[] { source1, source2 }, parseOptions: TestOptions.RegularNext.WithFeature("run-nullable-analysis", runNullableAnalysis)); var accessorBindingData = new SourcePropertySymbolBase.AccessorBindingData(); comp.TestOnlyCompilationData = accessorBindingData; Assert.Equal("System.Int32 S_WithAutoProperty.

k__BackingField", comp.GetTypeByMetadataName("S_WithAutoProperty").GetMembers().OfType().Single().ToTestDisplayString()); Assert.Empty(comp.GetTypeByMetadataName("S_WithManualProperty").GetMembers().OfType()); Assert.Empty(comp.GetTypeByMetadataName("S_WithSemiAutoProperty").GetMembers().OfType()); + + if (structTreeFirst) + { + comp.GetDiagnosticsForSyntaxTree(CompilationStage.Compile, comp.SyntaxTrees[0], filterSpanWithinTree: null, includeEarlierStages: true).Verify(); + } + + comp.GetDiagnosticsForSyntaxTree(CompilationStage.Compile, comp.SyntaxTrees[1], filterSpanWithinTree: null, includeEarlierStages: true).Verify( + // (7,33): error CS0165: Use of unassigned local variable 's1' + // S_WithAutoProperty s2 = s1; + Diagnostic(ErrorCode.ERR_UseDefViolation, "s1").WithArguments("s1").WithLocation(7, 33), + // (19,37): error CS0165: Use of unassigned local variable 's1' + // S_WithSemiAutoProperty s2 = s1; + Diagnostic(ErrorCode.ERR_UseDefViolation, "s1").WithArguments("s1").WithLocation(19, 37) + ); + + if (!structTreeFirst) + { + comp.GetDiagnosticsForSyntaxTree(CompilationStage.Compile, comp.SyntaxTrees[0], filterSpanWithinTree: null, includeEarlierStages: true).Verify(); + } + comp.VerifyDiagnostics( - // (22,33): error CS0165: Use of unassigned local variable 's1' + // (7,33): error CS0165: Use of unassigned local variable 's1' // S_WithAutoProperty s2 = s1; - Diagnostic(ErrorCode.ERR_UseDefViolation, "s1").WithArguments("s1").WithLocation(22, 33), - // (34,37): error CS0165: Use of unassigned local variable 's1' + Diagnostic(ErrorCode.ERR_UseDefViolation, "s1").WithArguments("s1").WithLocation(7, 33), + // (19,37): error CS0165: Use of unassigned local variable 's1' // S_WithSemiAutoProperty s2 = s1; - Diagnostic(ErrorCode.ERR_UseDefViolation, "s1").WithArguments("s1").WithLocation(34, 37) + Diagnostic(ErrorCode.ERR_UseDefViolation, "s1").WithArguments("s1").WithLocation(19, 37) ); + + Assert.Equal(structTreeFirst ? 0 : 1, accessorBindingData.NumberOfPerformedAccessorBinding); } [Fact] From 6925606e75bdf75795c8a590fc13d3fa94744fee Mon Sep 17 00:00:00 2001 From: Youssef1313 Date: Sun, 17 Apr 2022 06:03:22 +0200 Subject: [PATCH 13/38] Reorder conditions --- src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs b/src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs index d53a248cafef9..754da469dd5ef 100644 --- a/src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs +++ b/src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs @@ -1704,9 +1704,9 @@ private static bool IsPropertyAssignedThroughBackingField(BoundExpression receiv } return propertySymbol is SourcePropertySymbolBase sourceProperty && - (sourceProperty.IsStatic || receiver.Kind == BoundKind.ThisReference) && IsConstructorOrField(fromMember, isStatic: sourceProperty.IsStatic) && TypeSymbol.Equals(sourceProperty.ContainingType, fromMember.ContainingType, TypeCompareKind.ConsiderEverything2) && + (sourceProperty.IsStatic || receiver.Kind == BoundKind.ThisReference) && // To be assigned through backing field, either SetMethod is null, or it's equivalent to backing field write // PROTOTYPE(semi-auto-props): TODO: Do we need to use `GetOwnOrInheritedSetMethod` instead of `SetMethod`? // Legacy auto-properties are required to override all accessors. If this is not the case with semi auto props, we may need to use GetOwnOrInheritedSetMethod From 6ff3c8d69c370c2a2f0860c10516f92b6d2521e8 Mon Sep 17 00:00:00 2001 From: Youssef1313 Date: Sun, 17 Apr 2022 18:41:55 +0200 Subject: [PATCH 14/38] Chain VerifyDiagnostics call --- .../Semantics/PropertyFieldKeywordTests.cs | 63 +++++++------------ 1 file changed, 23 insertions(+), 40 deletions(-) diff --git a/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs b/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs index a0ef1441b2450..bab28c47af2cf 100644 --- a/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs +++ b/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs @@ -237,11 +237,6 @@ public C() ", options: TestOptions.ReleaseExe.WithSpecificDiagnosticOptions(ReportStructInitializationWarnings)); var accessorBindingData = new SourcePropertySymbolBase.AccessorBindingData(); comp.TestOnlyCompilationData = accessorBindingData; - comp.VerifyDiagnostics( - // (10,51): warning CS9020: The 'this' object is read before all of its fields have been assigned, causing preceding implicit assignments of 'default' to non-explicitly assigned fields. - // System.Console.WriteLine("In C..ctor: " + P); - Diagnostic(ErrorCode.WRN_UseDefViolationThisSupportedVersion, "P").WithArguments("this").WithLocation(10, 51) - ); CompileAndVerify(comp, expectedOutput: @"In C..ctor: 0 In C..ctor: 0").VerifyIL("C..ctor", @" { @@ -261,7 +256,11 @@ .locals init (int V_0) IL_001f: call ""void System.Console.WriteLine(string)"" IL_0024: ret } -"); +").VerifyDiagnostics( + // (10,51): warning CS9020: The 'this' object is read before all of its fields have been assigned, causing preceding implicit assignments of 'default' to non-explicitly assigned fields. + // System.Console.WriteLine("In C..ctor: " + P); + Diagnostic(ErrorCode.WRN_UseDefViolationThisSupportedVersion, "P").WithArguments("this").WithLocation(10, 51) + ); Assert.Empty(comp.GetTypeByMetadataName("C").GetMembers().OfType()); Assert.Equal(1, accessorBindingData.NumberOfPerformedAccessorBinding); } @@ -288,11 +287,6 @@ public C() ", options: TestOptions.ReleaseExe.WithSpecificDiagnosticOptions(ReportStructInitializationWarnings)); var accessorBindingData = new SourcePropertySymbolBase.AccessorBindingData(); comp.TestOnlyCompilationData = accessorBindingData; - comp.VerifyDiagnostics( - // (10,69): warning CS9020: The 'this' object is read before all of its fields have been assigned, causing preceding implicit assignments of 'default' to non-explicitly assigned fields. - // System.Console.WriteLine("In C..ctor before assignment: " + P); - Diagnostic(ErrorCode.WRN_UseDefViolationThisSupportedVersion, "P").WithArguments("this").WithLocation(10, 69) - ); CompileAndVerify(comp, expectedOutput: @"In C..ctor before assignment: 0 In C..ctor after assignment: 5 In C..ctor before assignment: 0 @@ -325,7 +319,11 @@ .locals init (int V_0) IL_0043: call ""void System.Console.WriteLine(string)"" IL_0048: ret } -"); +").VerifyDiagnostics( + // (10,69): warning CS9020: The 'this' object is read before all of its fields have been assigned, causing preceding implicit assignments of 'default' to non-explicitly assigned fields. + // System.Console.WriteLine("In C..ctor before assignment: " + P); + Diagnostic(ErrorCode.WRN_UseDefViolationThisSupportedVersion, "P").WithArguments("this").WithLocation(10, 69) + ); Assert.Empty(comp.GetTypeByMetadataName("C").GetMembers().OfType()); Assert.Equal(1, accessorBindingData.NumberOfPerformedAccessorBinding); } @@ -406,13 +404,12 @@ public MyAttribute(int i) { } "); var accessorBindingData = new SourcePropertySymbolBase.AccessorBindingData(); comp.TestOnlyCompilationData = accessorBindingData; - comp.VerifyDiagnostics( + CompileAndVerify(comp, expectedOutput: "10").VerifyDiagnostics( // Looks like an incorrect diagnostic. Tracked by https://github.com/dotnet/roslyn/issues/60645 // (10,23): warning CS0219: The variable 'field' is assigned but its value is never used // const int field = 5; Diagnostic(ErrorCode.WRN_UnreferencedVarAssg, "field").WithArguments("field").WithLocation(10, 23) ); - CompileAndVerify(comp, expectedOutput: "10"); VerifyTypeIL(comp, "C", @" .class public auto ansi beforefieldinit C extends [mscorlib]System.Object @@ -565,8 +562,7 @@ public int P ", parseOptions: TestOptions.RegularNext.WithFeature("run-nullable-analysis", runNullableAnalysis)); var accessorBindingData = new SourcePropertySymbolBase.AccessorBindingData(); comp.TestOnlyCompilationData = accessorBindingData; - comp.VerifyDiagnostics(); - CompileAndVerify(comp, expectedOutput: "5"); + CompileAndVerify(comp, expectedOutput: "5").VerifyDiagnostics(); VerifyTypeIL(comp, "C", @" .class public auto ansi beforefieldinit C extends [mscorlib]System.Object @@ -654,8 +650,7 @@ public int P ", parseOptions: TestOptions.RegularNext.WithFeature("run-nullable-analysis", runNullableAnalysis)); var accessorBindingData = new SourcePropertySymbolBase.AccessorBindingData(); comp.TestOnlyCompilationData = accessorBindingData; - comp.VerifyDiagnostics(); - CompileAndVerify(comp, expectedOutput: "5"); + CompileAndVerify(comp, expectedOutput: "5").VerifyDiagnostics(); VerifyTypeIL(comp, "C", @" .class public auto ansi beforefieldinit C extends [mscorlib]System.Object @@ -884,8 +879,7 @@ public int P ", parseOptions: TestOptions.RegularNext.WithFeature("run-nullable-analysis", runNullableAnalysis)); var accessorBindingData = new SourcePropertySymbolBase.AccessorBindingData(); comp.TestOnlyCompilationData = accessorBindingData; - comp.VerifyDiagnostics(); - CompileAndVerify(comp, expectedOutput: "5"); + CompileAndVerify(comp, expectedOutput: "5").VerifyDiagnostics(); VerifyTypeIL(comp, "C", @" .class public auto ansi beforefieldinit C extends [mscorlib]System.Object @@ -1037,8 +1031,7 @@ public int P ", parseOptions: TestOptions.RegularNext.WithFeature("run-nullable-analysis", runNullableAnalysis)); var accessorBindingData = new SourcePropertySymbolBase.AccessorBindingData(); comp.TestOnlyCompilationData = accessorBindingData; - comp.VerifyDiagnostics(); - CompileAndVerify(comp, expectedOutput: "5"); + CompileAndVerify(comp, expectedOutput: "5").VerifyDiagnostics(); VerifyTypeIL(comp, "C", @" .class public auto ansi beforefieldinit C extends [mscorlib]System.Object @@ -1122,9 +1115,7 @@ public string P "); var accessorBindingData = new SourcePropertySymbolBase.AccessorBindingData(); comp.TestOnlyCompilationData = accessorBindingData; - comp.VerifyDiagnostics(); - - CompileAndVerify(comp, expectedOutput: "field"); + CompileAndVerify(comp, expectedOutput: "field").VerifyDiagnostics(); Assert.Equal(0, accessorBindingData.NumberOfPerformedAccessorBinding); } @@ -1151,9 +1142,7 @@ public static void Main() comp.TestOnlyCompilationData = accessorBindingData; Assert.Empty(comp.GetTypeByMetadataName("C").GetMembers().OfType()); - - comp.VerifyDiagnostics(); - CompileAndVerify(comp, expectedOutput: "5"); + CompileAndVerify(comp, expectedOutput: "5").VerifyDiagnostics(); VerifyTypeIL(comp, "C", @" .class public auto ansi beforefieldinit C extends [mscorlib]System.Object @@ -1309,8 +1298,7 @@ .maxstack 2 "; } - comp.VerifyDiagnostics(); - CompileAndVerify(comp, expectedOutput: "5").VerifyIL("C.P.get", @" + CompileAndVerify(comp, expectedOutput: "5").VerifyDiagnostics().VerifyIL("C.P.get", @" { // Code size 7 (0x7) .maxstack 1 @@ -1338,8 +1326,7 @@ public C() "); var accessorBindingData = new SourcePropertySymbolBase.AccessorBindingData(); comp.TestOnlyCompilationData = accessorBindingData; - comp.VerifyDiagnostics(); - CompileAndVerify(comp).VerifyIL("C.P.set", @" + CompileAndVerify(comp).VerifyDiagnostics().VerifyIL("C.P.set", @" { // Code size 10 (0xa) .maxstack 3 @@ -1388,8 +1375,7 @@ public static void Main() ", options: TestOptions.DebugExe); var accessorBindingData = new SourcePropertySymbolBase.AccessorBindingData(); comp.TestOnlyCompilationData = accessorBindingData; - comp.VerifyDiagnostics(); - CompileAndVerify(comp, expectedOutput: "10").VerifyIL("C.P.get", @" + CompileAndVerify(comp, expectedOutput: "10").VerifyDiagnostics().VerifyIL("C.P.get", @" { // Code size 7 (0x7) .maxstack 1 @@ -1439,8 +1425,7 @@ public static void Main() ", options: TestOptions.DebugExe); var accessorBindingData = new SourcePropertySymbolBase.AccessorBindingData(); comp.TestOnlyCompilationData = accessorBindingData; - comp.VerifyDiagnostics(); - CompileAndVerify(comp, expectedOutput: "0").VerifyIL("C.P.get", @" + CompileAndVerify(comp, expectedOutput: "0").VerifyDiagnostics().VerifyIL("C.P.get", @" ").VerifyIL("C..ctor", @" "); Assert.Equal(1, accessorBindingData.NumberOfPerformedAccessorBinding); @@ -1469,8 +1454,7 @@ public static void Main() ", options: TestOptions.DebugExe); var accessorBindingData = new SourcePropertySymbolBase.AccessorBindingData(); comp.TestOnlyCompilationData = accessorBindingData; - comp.VerifyDiagnostics(); - CompileAndVerify(comp, expectedOutput: "0").VerifyIL("C.P.get", @" + CompileAndVerify(comp, expectedOutput: "0").VerifyDiagnostics().VerifyIL("C.P.get", @" { // Code size 2 (0x2) .maxstack 1 @@ -2516,8 +2500,7 @@ int X var accessorBindingData = new SourcePropertySymbolBase.AccessorBindingData(); comp.TestOnlyCompilationData = accessorBindingData; Assert.Empty(comp.GetTypeByMetadataName("Test").GetMembers().OfType()); - comp.VerifyDiagnostics(); - CompileAndVerify(comp, expectedOutput: "3"); + CompileAndVerify(comp, expectedOutput: "3").VerifyDiagnostics(); VerifyTypeIL(comp, "Test", @" .class private auto ansi beforefieldinit Test extends [mscorlib]System.Object From d1208ddef48df53c4b118bdda5e7789aede04c4a Mon Sep 17 00:00:00 2001 From: Youssef1313 Date: Sun, 17 Apr 2022 19:10:51 +0200 Subject: [PATCH 15/38] Test struct --- .../Semantics/PropertyFieldKeywordTests.cs | 65 +++++++++++++------ 1 file changed, 44 insertions(+), 21 deletions(-) diff --git a/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs b/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs index bab28c47af2cf..112552d8df8c2 100644 --- a/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs +++ b/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs @@ -1310,34 +1310,42 @@ .maxstack 1 Assert.Equal(bindingCount, accessorBindingData.NumberOfPerformedAccessorBinding); } - [Fact] - public void TestAssigningFromConstructorThroughSetterWithFieldKeyword_NoGetter() + [Theory] + [InlineData("class")] + [InlineData("struct")] + public void TestAssigningFromConstructorThroughSetterWithFieldKeyword_NoGetter(string type) { - var comp = CreateCompilation(@" -public class C -{ + var comp = CreateCompilation($@" +public {type} C +{{ public C() - { + {{ P = 5; - } + }} - public int P { set => field = value * 2; } -} + public int P {{ set => field = value * 2; }} +}} "); - var accessorBindingData = new SourcePropertySymbolBase.AccessorBindingData(); - comp.TestOnlyCompilationData = accessorBindingData; - CompileAndVerify(comp).VerifyDiagnostics().VerifyIL("C.P.set", @" + string ctorExpectedIL; + if (type == "struct") + { + ctorExpectedIL = @" { - // Code size 10 (0xa) - .maxstack 3 + // Code size 15 (0xf) + .maxstack 2 IL_0000: ldarg.0 - IL_0001: ldarg.1 - IL_0002: ldc.i4.2 - IL_0003: mul - IL_0004: stfld ""int C.

k__BackingField"" - IL_0009: ret + IL_0001: ldc.i4.0 + IL_0002: stfld ""int C.

k__BackingField"" + IL_0007: ldarg.0 + IL_0008: ldc.i4.5 + IL_0009: call ""void C.P.set"" + IL_000e: ret } -").VerifyIL("C..ctor", @" +"; + } + else + { + ctorExpectedIL = @" { // Code size 14 (0xe) .maxstack 2 @@ -1348,7 +1356,22 @@ .maxstack 2 IL_0008: call ""void C.P.set"" IL_000d: ret } -"); +"; + } + var accessorBindingData = new SourcePropertySymbolBase.AccessorBindingData(); + comp.TestOnlyCompilationData = accessorBindingData; + CompileAndVerify(comp).VerifyDiagnostics().VerifyIL("C.P.set", @" +{ + // Code size 10 (0xa) + .maxstack 3 + IL_0000: ldarg.0 + IL_0001: ldarg.1 + IL_0002: ldc.i4.2 + IL_0003: mul + IL_0004: stfld ""int C.

k__BackingField"" + IL_0009: ret +} +").VerifyIL("C..ctor", ctorExpectedIL); Assert.Equal(0, accessorBindingData.NumberOfPerformedAccessorBinding); } From a80af8aac7a830c211be8397cd15106b90603345 Mon Sep 17 00:00:00 2001 From: Youssef1313 Date: Sun, 17 Apr 2022 19:14:15 +0200 Subject: [PATCH 16/38] Update assert --- .../CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs b/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs index 112552d8df8c2..0959a6b1a132a 100644 --- a/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs +++ b/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs @@ -1372,7 +1372,7 @@ .maxstack 3 IL_0009: ret } ").VerifyIL("C..ctor", ctorExpectedIL); - Assert.Equal(0, accessorBindingData.NumberOfPerformedAccessorBinding); + Assert.Equal(type == "struct" ? 1 : 0, accessorBindingData.NumberOfPerformedAccessorBinding); } [Theory] From 6119c22ed11295dfb92c48b2a4e0ac5beb56892f Mon Sep 17 00:00:00 2001 From: Youssef1313 Date: Tue, 19 Apr 2022 16:57:27 +0200 Subject: [PATCH 17/38] Address feedback --- .../Portable/Compiler/MethodCompiler.cs | 30 ++++++++++++ .../LocalRewriter_AssignmentOperator.cs | 3 +- .../Semantics/PropertyFieldKeywordTests.cs | 48 +++++++++---------- 3 files changed, 55 insertions(+), 26 deletions(-) diff --git a/src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs b/src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs index fb2bdfeff118b..a77b1c1cb974b 100644 --- a/src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs +++ b/src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs @@ -471,6 +471,25 @@ private void CompileNamedType(NamedTypeSymbol containingType) } var members = containingType.GetMembers(); + + // We first compile the accessors to avoid extra bindings for 'field' keyword. + for (int memberOrdinal = 0; memberOrdinal < members.Length; memberOrdinal++) + { + var member = members[memberOrdinal]; + + //When a filter is supplied, limit the compilation of members passing the filter. + if (!PassesFilter(_filterOpt, member) || + member is not SourcePropertyAccessorSymbol accessor) + { + continue; + } + + Debug.Assert(member.Kind == SymbolKind.Method); + Binder.ProcessedFieldInitializers processedInitializers = default; + CompileMethod(accessor, memberOrdinal, ref processedInitializers, synthesizedSubmissionFields, compilationState); + } + + // Then we compile everything, excluding the accessors we already compiled in the loop above. for (int memberOrdinal = 0; memberOrdinal < members.Length; memberOrdinal++) { var member = members[memberOrdinal]; @@ -490,6 +509,17 @@ private void CompileNamedType(NamedTypeSymbol containingType) case SymbolKind.Method: { MethodSymbol method = (MethodSymbol)member; + if (method is SourcePropertyAccessorSymbol) + { + // The loop for compiling accessors was written with these valid assumptions. + // If this requirement has changed, the loop above may need to be modified (e.g, if partial accessors was allowed in future) + Debug.Assert(!method.IsScriptConstructor); + Debug.Assert((object)method != scriptEntryPoint); + Debug.Assert(!IsFieldLikeEventAccessor(method)); + Debug.Assert(!method.IsPartialDefinition()); + continue; + } + if (method.IsScriptConstructor) { Debug.Assert(scriptCtorOrdinal == -1); diff --git a/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_AssignmentOperator.cs b/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_AssignmentOperator.cs index 4832e50aff89e..ec662e09fa14c 100644 --- a/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_AssignmentOperator.cs +++ b/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_AssignmentOperator.cs @@ -301,8 +301,7 @@ private BoundExpression MakePropertyAssignment( if (setMethod is null) { var autoProp = (SourcePropertySymbolBase)property.OriginalDefinition; - Debug.Assert(autoProp.SetMethod is null && - (autoProp.GetMethod is SourcePropertyAccessorSymbol { IsEquivalentToBackingFieldAccess: true } || autoProp.FieldKeywordBackingField is not null), + Debug.Assert(autoProp.GetMethod is SourcePropertyAccessorSymbol { IsEquivalentToBackingFieldAccess: true } || autoProp.FieldKeywordBackingField is not null, "only autoproperties can be assignable without having setters"); Debug.Assert(property.Equals(autoProp, TypeCompareKind.IgnoreNullableModifiersForReferenceTypes)); diff --git a/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs b/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs index 0959a6b1a132a..903101026236c 100644 --- a/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs +++ b/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs @@ -262,7 +262,7 @@ .locals init (int V_0) Diagnostic(ErrorCode.WRN_UseDefViolationThisSupportedVersion, "P").WithArguments("this").WithLocation(10, 51) ); Assert.Empty(comp.GetTypeByMetadataName("C").GetMembers().OfType()); - Assert.Equal(1, accessorBindingData.NumberOfPerformedAccessorBinding); + Assert.Equal(0, accessorBindingData.NumberOfPerformedAccessorBinding); } [Fact] @@ -325,7 +325,7 @@ .locals init (int V_0) Diagnostic(ErrorCode.WRN_UseDefViolationThisSupportedVersion, "P").WithArguments("this").WithLocation(10, 69) ); Assert.Empty(comp.GetTypeByMetadataName("C").GetMembers().OfType()); - Assert.Equal(1, accessorBindingData.NumberOfPerformedAccessorBinding); + Assert.Equal(0, accessorBindingData.NumberOfPerformedAccessorBinding); } [Fact] @@ -540,8 +540,8 @@ public MyAttribute(int i) { } Assert.Equal(0, accessorBindingData.NumberOfPerformedAccessorBinding); } - [Theory, CombinatorialData] - public void TestFieldIsShadowed_ReferencedFromRegularLocalFunction([CombinatorialValues("always", "never")] string runNullableAnalysis) + [Fact] + public void TestFieldIsShadowed_ReferencedFromRegularLocalFunction() { var comp = CreateCompilation(@" System.Console.WriteLine(new C().P); @@ -559,7 +559,7 @@ public int P } } } -", parseOptions: TestOptions.RegularNext.WithFeature("run-nullable-analysis", runNullableAnalysis)); +"); var accessorBindingData = new SourcePropertySymbolBase.AccessorBindingData(); comp.TestOnlyCompilationData = accessorBindingData; CompileAndVerify(comp, expectedOutput: "5").VerifyDiagnostics(); @@ -630,8 +630,8 @@ .property instance int32 P() Assert.Equal(0, accessorBindingData.NumberOfPerformedAccessorBinding); } - [Theory, CombinatorialData] - public void TestFieldIsShadowed_ReferencedFromRegularLambda([CombinatorialValues("always", "never")] string runNullableAnalysis) + [Fact] + public void TestFieldIsShadowed_ReferencedFromRegularLambda() { var comp = CreateCompilation(@" System.Console.WriteLine(new C().P); @@ -647,7 +647,7 @@ public int P } } } -", parseOptions: TestOptions.RegularNext.WithFeature("run-nullable-analysis", runNullableAnalysis)); +"); var accessorBindingData = new SourcePropertySymbolBase.AccessorBindingData(); comp.TestOnlyCompilationData = accessorBindingData; CompileAndVerify(comp, expectedOutput: "5").VerifyDiagnostics(); @@ -856,8 +856,8 @@ public int P Assert.Equal(0, accessorBindingData.NumberOfPerformedAccessorBinding); } - [Theory, CombinatorialData] - public void TestFieldIsShadowedByField_ReferencedFromRegularLocalFunction([CombinatorialValues("always", "never")] string runNullableAnalysis) + [Fact] + public void TestFieldIsShadowedByField_ReferencedFromRegularLocalFunction() { var comp = CreateCompilation(@" System.Console.WriteLine(new C().P); @@ -876,7 +876,7 @@ public int P } } } -", parseOptions: TestOptions.RegularNext.WithFeature("run-nullable-analysis", runNullableAnalysis)); +"); var accessorBindingData = new SourcePropertySymbolBase.AccessorBindingData(); comp.TestOnlyCompilationData = accessorBindingData; CompileAndVerify(comp, expectedOutput: "5").VerifyDiagnostics(); @@ -1010,8 +1010,8 @@ .property instance int32 P() Assert.Equal(0, accessorBindingData.NumberOfPerformedAccessorBinding); } - [Theory, CombinatorialData] - public void TestFieldIsShadowedByField_ReferencedFromRegularLambda([CombinatorialValues("always", "never")] string runNullableAnalysis) + [Fact] + public void TestFieldIsShadowedByField_ReferencedFromRegularLambda() { var comp = CreateCompilation(@" System.Console.WriteLine(new C().P); @@ -1028,7 +1028,7 @@ public int P } } } -", parseOptions: TestOptions.RegularNext.WithFeature("run-nullable-analysis", runNullableAnalysis)); +"); var accessorBindingData = new SourcePropertySymbolBase.AccessorBindingData(); comp.TestOnlyCompilationData = accessorBindingData; CompileAndVerify(comp, expectedOutput: "5").VerifyDiagnostics(); @@ -1202,7 +1202,7 @@ .property instance int32 P() } } // end of class C "); - Assert.Equal(1, accessorBindingData.NumberOfPerformedAccessorBinding); + Assert.Equal(0, accessorBindingData.NumberOfPerformedAccessorBinding); } [Fact] @@ -1235,12 +1235,12 @@ public int P { } } [Theory] - [InlineData("get;", 0, false)] - [InlineData("get; set;", 0, true)] + [InlineData("get;", false)] + [InlineData("get; set;", true)] // [InlineData("set;")] PROTOTYPE(semi-auto-props): Not yet supported. - [InlineData("get => field;", 1, false)] + [InlineData("get => field;", false)] // [InlineData("get => field; set;")] PROTOTYPE(semi-auto-props): Not yet supported - public void TestAssigningFromConstructorThroughBackingField(string accessors, int bindingCount, bool callsSynthesizedSetter) + public void TestAssigningFromConstructorThroughBackingField(string accessors, bool callsSynthesizedSetter) { var comp = CreateCompilation($@" public class C @@ -1307,7 +1307,7 @@ .maxstack 1 IL_0006: ret } ").VerifyIL("C..ctor", expectedCtorIL); - Assert.Equal(bindingCount, accessorBindingData.NumberOfPerformedAccessorBinding); + Assert.Equal(0, accessorBindingData.NumberOfPerformedAccessorBinding); } [Theory] @@ -1372,7 +1372,7 @@ .maxstack 3 IL_0009: ret } ").VerifyIL("C..ctor", ctorExpectedIL); - Assert.Equal(type == "struct" ? 1 : 0, accessorBindingData.NumberOfPerformedAccessorBinding); + Assert.Equal(0, accessorBindingData.NumberOfPerformedAccessorBinding); } [Theory] @@ -2567,7 +2567,7 @@ .property instance int32 X() } } // end of class Test "); - Assert.Equal(1, accessorBindingData.NumberOfPerformedAccessorBinding); + Assert.Equal(0, accessorBindingData.NumberOfPerformedAccessorBinding); } [Fact] @@ -3119,7 +3119,7 @@ public class C1 } [Theory, CombinatorialData] - public void InStruct([CombinatorialValues("always", "never")] string runNullableAnalysis, bool structTreeFirst) + public void InStruct(bool structTreeFirst) { var source1 = @" struct S_WithAutoProperty @@ -3160,7 +3160,7 @@ void M3() } } "; - var comp = CreateCompilation(new[] { source1, source2 }, parseOptions: TestOptions.RegularNext.WithFeature("run-nullable-analysis", runNullableAnalysis)); + var comp = CreateCompilation(new[] { source1, source2 }); var accessorBindingData = new SourcePropertySymbolBase.AccessorBindingData(); comp.TestOnlyCompilationData = accessorBindingData; Assert.Equal("System.Int32 S_WithAutoProperty.

k__BackingField", comp.GetTypeByMetadataName("S_WithAutoProperty").GetMembers().OfType().Single().ToTestDisplayString()); From 1e62ba73b107e09e71c23f6c97d1416f579e836f Mon Sep 17 00:00:00 2001 From: Youssef1313 Date: Fri, 22 Apr 2022 10:00:09 +0200 Subject: [PATCH 18/38] Address part of review --- .../Portable/Symbols/BaseTypeAnalysis.cs | 20 +- .../Source/SourceMemberContainerSymbol.cs | 16 +- .../Semantics/PropertyFieldKeywordTests.cs | 174 +++++++++++++++++- 3 files changed, 201 insertions(+), 9 deletions(-) diff --git a/src/Compilers/CSharp/Portable/Symbols/BaseTypeAnalysis.cs b/src/Compilers/CSharp/Portable/Symbols/BaseTypeAnalysis.cs index 5918d1d848ee6..9e05319f5fe86 100644 --- a/src/Compilers/CSharp/Portable/Symbols/BaseTypeAnalysis.cs +++ b/src/Compilers/CSharp/Portable/Symbols/BaseTypeAnalysis.cs @@ -92,9 +92,23 @@ private static void StructDependsClosure(NamedTypeSymbol type, HashSet p { foreach (var member in type.GetMembersUnordered()) { - var field = member as FieldSymbol; - var fieldType = field?.NonPointerType(); - if (fieldType is null || fieldType.TypeKind != TypeKind.Struct || field.IsStatic) + FieldSymbol field; + if (member is SourcePropertySymbolBase { IsStatic: false, FieldKeywordBackingField: { } fieldKeywordField }) + { + field = fieldKeywordField; + } + else if (member.Kind == SymbolKind.Field && !member.IsStatic) + { + field = (FieldSymbol)member; + } + else + { + continue; + } + + Debug.Assert(!field.IsStatic); + var fieldType = field.NonPointerType(); + if (fieldType.TypeKind != TypeKind.Struct) { continue; } diff --git a/src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol.cs b/src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol.cs index 65cd6833c686c..c5fdf5ee37f29 100644 --- a/src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol.cs +++ b/src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol.cs @@ -2163,16 +2163,22 @@ private bool HasStructCircularity(BindingDiagnosticBag diagnostics) { foreach (var member in valuesByName) { - if (member.Kind != SymbolKind.Field) + FieldSymbol field; + if (member is SourcePropertySymbolBase { IsStatic: false, FieldKeywordBackingField: { } fieldKeywordField }) { - // NOTE: don't have to check field-like events, because they can't have struct types. - continue; + field = fieldKeywordField; } - var field = (FieldSymbol)member; - if (field.IsStatic) + else if (member.Kind == SymbolKind.Field && !member.IsStatic) { + field = (FieldSymbol)member; + } + else + { + // NOTE: don't have to check field-like events, because they can't have struct types. continue; } + + Debug.Assert(!field.IsStatic); var type = field.NonPointerType(); if (((object)type != null) && (type.TypeKind == TypeKind.Struct) && diff --git a/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs b/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs index 903101026236c..b100a8e723204 100644 --- a/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs +++ b/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs @@ -266,7 +266,7 @@ .locals init (int V_0) } [Fact] - public void TestPropertyIsAssignedInStructConstructor() + public void TestPropertyIsReadThenAssignedInStructConstructor() { var comp = CreateCompilation(@" var x = new C(); @@ -323,6 +323,130 @@ .locals init (int V_0) // (10,69): warning CS9020: The 'this' object is read before all of its fields have been assigned, causing preceding implicit assignments of 'default' to non-explicitly assigned fields. // System.Console.WriteLine("In C..ctor before assignment: " + P); Diagnostic(ErrorCode.WRN_UseDefViolationThisSupportedVersion, "P").WithArguments("this").WithLocation(10, 69) + ); + Assert.Empty(comp.GetTypeByMetadataName("C").GetMembers().OfType()); + Assert.Equal(1, accessorBindingData.NumberOfPerformedAccessorBinding); + } + + [Fact] + public void TestPropertyIsReadThenAssignedInStructConstructor_ReadOnlyProperty() + { + var comp = CreateCompilation(@" +var x = new C(); +System.Console.Write(x.P); + +public struct C +{ + public C() + { + P = 5; + } + + public int P { get => field; } +} +", options: TestOptions.ReleaseExe.WithSpecificDiagnosticOptions(ReportStructInitializationWarnings)); + var accessorBindingData = new SourcePropertySymbolBase.AccessorBindingData(); + comp.TestOnlyCompilationData = accessorBindingData; + CompileAndVerify(comp, expectedOutput: "5").VerifyIL("C..ctor", @" +{ + // Code size 15 (0xf) + .maxstack 2 + IL_0000: ldarg.0 + IL_0001: ldc.i4.0 + IL_0002: stfld ""int C.

k__BackingField"" + IL_0007: ldarg.0 + IL_0008: ldc.i4.5 + IL_0009: stfld ""int C.

k__BackingField"" + IL_000e: ret +} +").VerifyDiagnostics( + // (9,9): warning CS9020: The 'this' object is read before all of its fields have been assigned, causing preceding implicit assignments of 'default' to non-explicitly assigned fields. + // P = 5; + Diagnostic(ErrorCode.WRN_UseDefViolationThisSupportedVersion, "P").WithArguments("this").WithLocation(9, 9) + ); + Assert.Empty(comp.GetTypeByMetadataName("C").GetMembers().OfType()); + Assert.Equal(0, accessorBindingData.NumberOfPerformedAccessorBinding); + } + + [Fact] + public void TestPropertyIsAssignedInStructConstructor_PropertyHasSetter() + { + var comp = CreateCompilation(@" +var x = new C(); +System.Console.Write(x.P + "" ""); // 5 +x.P = 10; +System.Console.Write(x.P + "" ""); // 10 +x = new C(); +System.Console.Write(x.P); // 5 + +public struct C +{ + public C() + { + P = 5; + } + + public int P { get => field; set => field = value; } +} +", options: TestOptions.ReleaseExe.WithSpecificDiagnosticOptions(ReportStructInitializationWarnings)); + var accessorBindingData = new SourcePropertySymbolBase.AccessorBindingData(); + comp.TestOnlyCompilationData = accessorBindingData; + CompileAndVerify(comp, expectedOutput: "5 10 5").VerifyIL("C..ctor", @" +{ + // Code size 15 (0xf) + .maxstack 2 + IL_0000: ldarg.0 + IL_0001: ldc.i4.0 + IL_0002: stfld ""int C.

k__BackingField"" + IL_0007: ldarg.0 + IL_0008: ldc.i4.5 + IL_0009: call ""void C.P.set"" + IL_000e: ret +} +").VerifyDiagnostics( + // (13,9): warning CS9020: The 'this' object is read before all of its fields have been assigned, causing preceding implicit assignments of 'default' to non-explicitly assigned fields. + // P = 5; + Diagnostic(ErrorCode.WRN_UseDefViolationThisSupportedVersion, "P").WithArguments("this").WithLocation(13, 9) + ); + Assert.Empty(comp.GetTypeByMetadataName("C").GetMembers().OfType()); + Assert.Equal(0, accessorBindingData.NumberOfPerformedAccessorBinding); + } + + [Fact] + public void TestPropertyIsAssignedInStructConstructor_ReadOnlyProperty() + { + var comp = CreateCompilation(@" +var x = new C(); +System.Console.Write(x.P); + +public struct C +{ + public C() + { + P = 5; + } + + public int P { get => field; } +} +", options: TestOptions.ReleaseExe.WithSpecificDiagnosticOptions(ReportStructInitializationWarnings)); + var accessorBindingData = new SourcePropertySymbolBase.AccessorBindingData(); + comp.TestOnlyCompilationData = accessorBindingData; + CompileAndVerify(comp, expectedOutput: "5").VerifyIL("C..ctor", @" +{ + // Code size 15 (0xf) + .maxstack 2 + IL_0000: ldarg.0 + IL_0001: ldc.i4.0 + IL_0002: stfld ""int C.

k__BackingField"" + IL_0007: ldarg.0 + IL_0008: ldc.i4.5 + IL_0009: stfld ""int C.

k__BackingField"" + IL_000e: ret +} +").VerifyDiagnostics( + // (9,9): warning CS9020: The 'this' object is read before all of its fields have been assigned, causing preceding implicit assignments of 'default' to non-explicitly assigned fields. + // P = 5; + Diagnostic(ErrorCode.WRN_UseDefViolationThisSupportedVersion, "P").WithArguments("this").WithLocation(9, 9) ); Assert.Empty(comp.GetTypeByMetadataName("C").GetMembers().OfType()); Assert.Equal(0, accessorBindingData.NumberOfPerformedAccessorBinding); @@ -3242,6 +3366,54 @@ public S(int arg) Assert.Equal(0, accessorBindingData.NumberOfPerformedAccessorBinding); } + [Theory, CombinatorialData] + public void InStruct_Cycle(bool firstIsSemi, bool secondIsSemi) + { + var firstAccessor = firstIsSemi ? "get => field;" : "get;"; + var secondAccessor = secondIsSemi ? "get => field;" : "get;"; + var comp = CreateCompilation($@" +struct S1 +{{ + public S2 P {{ {firstAccessor} }} +}} + +struct S2 +{{ + public S1 P {{ {secondAccessor} }} +}} +"); + var accessorBindingData = new SourcePropertySymbolBase.AccessorBindingData(); + comp.TestOnlyCompilationData = accessorBindingData; + comp.VerifyDiagnostics( + // (4,15): error CS0523: Struct member 'S1.P' of type 'S2' causes a cycle in the struct layout + // public S2 P { get; } + Diagnostic(ErrorCode.ERR_StructLayoutCycle, "P").WithArguments("S1.P", "S2").WithLocation(4, 15), + // (9,15): error CS0523: Struct member 'S2.P' of type 'S1' causes a cycle in the struct layout + // public S1 P { get; } + Diagnostic(ErrorCode.ERR_StructLayoutCycle, "P").WithArguments("S2.P", "S1").WithLocation(9, 15) + ); + Assert.Equal(0, accessorBindingData.NumberOfPerformedAccessorBinding); + } + + [Fact] + public void InStruct_SelfCycle() + { + var comp = CreateCompilation(@" +struct S +{ + public S P { get => field; } +} +"); + var accessorBindingData = new SourcePropertySymbolBase.AccessorBindingData(); + comp.TestOnlyCompilationData = accessorBindingData; + comp.VerifyDiagnostics( + // (4,14): error CS0523: Struct member 'S.P' of type 'S' causes a cycle in the struct layout + // public S P { get => field; } + Diagnostic(ErrorCode.ERR_StructLayoutCycle, "P").WithArguments("S.P", "S").WithLocation(4, 14) + ); + Assert.Equal(0, accessorBindingData.NumberOfPerformedAccessorBinding); + } + [Theory] [InlineData(SpeculativeBindingOption.BindAsExpression, "never")] [InlineData(SpeculativeBindingOption.BindAsExpression, "always")] From 28e32eb96ea117a831a46836cf4ae387af0d01eb Mon Sep 17 00:00:00 2001 From: Youssef1313 Date: Fri, 22 Apr 2022 10:51:05 +0200 Subject: [PATCH 19/38] More review comments --- .../Portable/Compiler/MethodCompiler.cs | 34 +++++----- .../Semantics/PropertyFieldKeywordTests.cs | 64 +++++++++++-------- 2 files changed, 55 insertions(+), 43 deletions(-) diff --git a/src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs b/src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs index a77b1c1cb974b..7b37561a6da14 100644 --- a/src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs +++ b/src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs @@ -489,6 +489,23 @@ private void CompileNamedType(NamedTypeSymbol containingType) CompileMethod(accessor, memberOrdinal, ref processedInitializers, synthesizedSubmissionFields, compilationState); } + foreach (var member in members) + { + if (member is SourcePropertySymbolBase property) + { + // PROTOTYPE(semi-auto-props): This can be optimized by checking for field keyword syntactically first. + // If we don't have field keyword, we can safely ignore the filter check. + // This will require more tests. + var getMethod = property.GetMethod; + var setMethod = property.SetMethod; + if ((getMethod is null || PassesFilter(_filterOpt, getMethod)) && + (setMethod is null || PassesFilter(_filterOpt, setMethod))) + { + property.MarkBackingFieldAsCalculated(); + } + } + } + // Then we compile everything, excluding the accessors we already compiled in the loop above. for (int memberOrdinal = 0; memberOrdinal < members.Length; memberOrdinal++) { @@ -604,23 +621,6 @@ private void CompileNamedType(NamedTypeSymbol containingType) } } - foreach (var member in members) - { - if (member is SourcePropertySymbolBase property) - { - // PROTOTYPE(semi-auto-props): This can be optimized by checking for field keyword syntactically first. - // If we don't have field keyword, we can safely ignore the filter check. - // This will require more tests. - var getMethod = property.GetMethod; - var setMethod = property.SetMethod; - if ((getMethod is null || PassesFilter(_filterOpt, getMethod)) && - (setMethod is null || PassesFilter(_filterOpt, setMethod))) - { - property.MarkBackingFieldAsCalculated(); - } - } - } - Debug.Assert(containingType.IsScriptClass == (scriptCtorOrdinal >= 0)); // process additional anonymous type members diff --git a/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs b/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs index b100a8e723204..517b48603dae7 100644 --- a/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs +++ b/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs @@ -221,12 +221,15 @@ .property instance int32 P() public void TestPropertyNotAssignedInStructConstructor() { var comp = CreateCompilation(@" -var x = new C(); -x.P = 10; -x = new C(); - public struct C { + public static void Main() + { + var x = new C(); + x.P = 10; + x = new C(); + } + public C() { System.Console.WriteLine(""In C..ctor: "" + P); @@ -257,9 +260,9 @@ .locals init (int V_0) IL_0024: ret } ").VerifyDiagnostics( - // (10,51): warning CS9020: The 'this' object is read before all of its fields have been assigned, causing preceding implicit assignments of 'default' to non-explicitly assigned fields. + // (13,51): warning CS9020: The 'this' object is read before all of its fields have been assigned, causing preceding implicit assignments of 'default' to non-explicitly assigned fields. // System.Console.WriteLine("In C..ctor: " + P); - Diagnostic(ErrorCode.WRN_UseDefViolationThisSupportedVersion, "P").WithArguments("this").WithLocation(10, 51) + Diagnostic(ErrorCode.WRN_UseDefViolationThisSupportedVersion, "P").WithArguments("this").WithLocation(13, 51) ); Assert.Empty(comp.GetTypeByMetadataName("C").GetMembers().OfType()); Assert.Equal(0, accessorBindingData.NumberOfPerformedAccessorBinding); @@ -328,15 +331,18 @@ .locals init (int V_0) Assert.Equal(1, accessorBindingData.NumberOfPerformedAccessorBinding); } - [Fact] + [Fact] // PROTOTYPE(semi-auto-props): Add test with semi-colon setter when mixed scenarios are supported. public void TestPropertyIsReadThenAssignedInStructConstructor_ReadOnlyProperty() { var comp = CreateCompilation(@" -var x = new C(); -System.Console.Write(x.P); - public struct C { + public static void Main() + { + var x = new C(); + System.Console.Write(x.P); + } + public C() { P = 5; @@ -360,9 +366,9 @@ .maxstack 2 IL_000e: ret } ").VerifyDiagnostics( - // (9,9): warning CS9020: The 'this' object is read before all of its fields have been assigned, causing preceding implicit assignments of 'default' to non-explicitly assigned fields. + // (12,9): warning CS9020: The 'this' object is read before all of its fields have been assigned, causing preceding implicit assignments of 'default' to non-explicitly assigned fields. // P = 5; - Diagnostic(ErrorCode.WRN_UseDefViolationThisSupportedVersion, "P").WithArguments("this").WithLocation(9, 9) + Diagnostic(ErrorCode.WRN_UseDefViolationThisSupportedVersion, "P").WithArguments("this").WithLocation(12, 9) ); Assert.Empty(comp.GetTypeByMetadataName("C").GetMembers().OfType()); Assert.Equal(0, accessorBindingData.NumberOfPerformedAccessorBinding); @@ -372,15 +378,18 @@ .maxstack 2 public void TestPropertyIsAssignedInStructConstructor_PropertyHasSetter() { var comp = CreateCompilation(@" -var x = new C(); -System.Console.Write(x.P + "" ""); // 5 -x.P = 10; -System.Console.Write(x.P + "" ""); // 10 -x = new C(); -System.Console.Write(x.P); // 5 - public struct C { + public static void Main() + { + var x = new C(); + System.Console.Write(x.P + "" ""); // 5 + x.P = 10; + System.Console.Write(x.P + "" ""); // 10 + x = new C(); + System.Console.Write(x.P); // 5 + } + public C() { P = 5; @@ -404,9 +413,9 @@ .maxstack 2 IL_000e: ret } ").VerifyDiagnostics( - // (13,9): warning CS9020: The 'this' object is read before all of its fields have been assigned, causing preceding implicit assignments of 'default' to non-explicitly assigned fields. + // (16,9): warning CS9020: The 'this' object is read before all of its fields have been assigned, causing preceding implicit assignments of 'default' to non-explicitly assigned fields. // P = 5; - Diagnostic(ErrorCode.WRN_UseDefViolationThisSupportedVersion, "P").WithArguments("this").WithLocation(13, 9) + Diagnostic(ErrorCode.WRN_UseDefViolationThisSupportedVersion, "P").WithArguments("this").WithLocation(16, 9) ); Assert.Empty(comp.GetTypeByMetadataName("C").GetMembers().OfType()); Assert.Equal(0, accessorBindingData.NumberOfPerformedAccessorBinding); @@ -416,11 +425,14 @@ .maxstack 2 public void TestPropertyIsAssignedInStructConstructor_ReadOnlyProperty() { var comp = CreateCompilation(@" -var x = new C(); -System.Console.Write(x.P); - public struct C { + public static void Main() + { + var x = new C(); + System.Console.Write(x.P); + } + public C() { P = 5; @@ -444,9 +456,9 @@ .maxstack 2 IL_000e: ret } ").VerifyDiagnostics( - // (9,9): warning CS9020: The 'this' object is read before all of its fields have been assigned, causing preceding implicit assignments of 'default' to non-explicitly assigned fields. + // (12,9): warning CS9020: The 'this' object is read before all of its fields have been assigned, causing preceding implicit assignments of 'default' to non-explicitly assigned fields. // P = 5; - Diagnostic(ErrorCode.WRN_UseDefViolationThisSupportedVersion, "P").WithArguments("this").WithLocation(9, 9) + Diagnostic(ErrorCode.WRN_UseDefViolationThisSupportedVersion, "P").WithArguments("this").WithLocation(12, 9) ); Assert.Empty(comp.GetTypeByMetadataName("C").GetMembers().OfType()); Assert.Equal(0, accessorBindingData.NumberOfPerformedAccessorBinding); From 29322b998a15b0568de6cdd786f1e3cc3f69f05a Mon Sep 17 00:00:00 2001 From: Youssef1313 Date: Sat, 23 Apr 2022 18:06:57 +0200 Subject: [PATCH 20/38] Address small part of feedback --- .../Portable/Compiler/MethodCompiler.cs | 4 +- .../Semantics/PropertyFieldKeywordTests.cs | 255 +++++++++++++++--- 2 files changed, 226 insertions(+), 33 deletions(-) diff --git a/src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs b/src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs index 7b37561a6da14..c83be932c29fa 100644 --- a/src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs +++ b/src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs @@ -479,7 +479,7 @@ private void CompileNamedType(NamedTypeSymbol containingType) //When a filter is supplied, limit the compilation of members passing the filter. if (!PassesFilter(_filterOpt, member) || - member is not SourcePropertyAccessorSymbol accessor) + member is not MethodSymbol { MethodKind: MethodKind.PropertyGet or MethodKind.PropertySet } accessor) { continue; } @@ -526,7 +526,7 @@ private void CompileNamedType(NamedTypeSymbol containingType) case SymbolKind.Method: { MethodSymbol method = (MethodSymbol)member; - if (method is SourcePropertyAccessorSymbol) + if (method.MethodKind is MethodKind.PropertyGet or MethodKind.PropertySet) { // The loop for compiling accessors was written with these valid assumptions. // If this requirement has changed, the loop above may need to be modified (e.g, if partial accessors was allowed in future) diff --git a/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs b/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs index 517b48603dae7..54a510fafd727 100644 --- a/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs +++ b/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs @@ -265,19 +265,22 @@ .locals init (int V_0) Diagnostic(ErrorCode.WRN_UseDefViolationThisSupportedVersion, "P").WithArguments("this").WithLocation(13, 51) ); Assert.Empty(comp.GetTypeByMetadataName("C").GetMembers().OfType()); - Assert.Equal(0, accessorBindingData.NumberOfPerformedAccessorBinding); + Assert.Equal(1, accessorBindingData.NumberOfPerformedAccessorBinding); } [Fact] public void TestPropertyIsReadThenAssignedInStructConstructor() { var comp = CreateCompilation(@" -var x = new C(); -x.P = 10; -x = new C(); - public struct C { + public static void Main() + { + var x = new C(); + x.P = 10; + x = new C(); + } + public C() { System.Console.WriteLine(""In C..ctor before assignment: "" + P); @@ -323,9 +326,9 @@ .locals init (int V_0) IL_0048: ret } ").VerifyDiagnostics( - // (10,69): warning CS9020: The 'this' object is read before all of its fields have been assigned, causing preceding implicit assignments of 'default' to non-explicitly assigned fields. + // (13,69): warning CS9020: The 'this' object is read before all of its fields have been assigned, causing preceding implicit assignments of 'default' to non-explicitly assigned fields. // System.Console.WriteLine("In C..ctor before assignment: " + P); - Diagnostic(ErrorCode.WRN_UseDefViolationThisSupportedVersion, "P").WithArguments("this").WithLocation(10, 69) + Diagnostic(ErrorCode.WRN_UseDefViolationThisSupportedVersion, "P").WithArguments("this").WithLocation(13, 69) ); Assert.Empty(comp.GetTypeByMetadataName("C").GetMembers().OfType()); Assert.Equal(1, accessorBindingData.NumberOfPerformedAccessorBinding); @@ -345,7 +348,9 @@ public static void Main() public C() { + System.Console.WriteLine(""In C..ctor before assignment: "" + P); P = 5; + System.Console.WriteLine(""In C..ctor after assignment: "" + P); } public int P { get => field; } @@ -353,25 +358,44 @@ public C() ", options: TestOptions.ReleaseExe.WithSpecificDiagnosticOptions(ReportStructInitializationWarnings)); var accessorBindingData = new SourcePropertySymbolBase.AccessorBindingData(); comp.TestOnlyCompilationData = accessorBindingData; - CompileAndVerify(comp, expectedOutput: "5").VerifyIL("C..ctor", @" + CompileAndVerify(comp, expectedOutput: @"In C..ctor before assignment: 0 +In C..ctor after assignment: 5 +5").VerifyIL("C..ctor", @" { - // Code size 15 (0xf) - .maxstack 2 - IL_0000: ldarg.0 - IL_0001: ldc.i4.0 - IL_0002: stfld ""int C.

k__BackingField"" - IL_0007: ldarg.0 - IL_0008: ldc.i4.5 - IL_0009: stfld ""int C.

k__BackingField"" - IL_000e: ret + // Code size 73 (0x49) + .maxstack 2 + .locals init (int V_0) + IL_0000: ldarg.0 + IL_0001: ldc.i4.0 + IL_0002: stfld ""int C.

k__BackingField"" + IL_0007: ldstr ""In C..ctor before assignment: "" + IL_000c: ldarg.0 + IL_000d: call ""int C.P.get"" + IL_0012: stloc.0 + IL_0013: ldloca.s V_0 + IL_0015: call ""string int.ToString()"" + IL_001a: call ""string string.Concat(string, string)"" + IL_001f: call ""void System.Console.WriteLine(string)"" + IL_0024: ldarg.0 + IL_0025: ldc.i4.5 + IL_0026: stfld ""int C.

k__BackingField"" + IL_002b: ldstr ""In C..ctor after assignment: "" + IL_0030: ldarg.0 + IL_0031: call ""int C.P.get"" + IL_0036: stloc.0 + IL_0037: ldloca.s V_0 + IL_0039: call ""string int.ToString()"" + IL_003e: call ""string string.Concat(string, string)"" + IL_0043: call ""void System.Console.WriteLine(string)"" + IL_0048: ret } ").VerifyDiagnostics( - // (12,9): warning CS9020: The 'this' object is read before all of its fields have been assigned, causing preceding implicit assignments of 'default' to non-explicitly assigned fields. - // P = 5; - Diagnostic(ErrorCode.WRN_UseDefViolationThisSupportedVersion, "P").WithArguments("this").WithLocation(12, 9) + // (12,69): warning CS9020: The 'this' object is read before all of its fields have been assigned, causing preceding implicit assignments of 'default' to non-explicitly assigned fields. + // System.Console.WriteLine("In C..ctor before assignment: " + P); + Diagnostic(ErrorCode.WRN_UseDefViolationThisSupportedVersion, "P").WithArguments("this").WithLocation(12, 69) ); Assert.Empty(comp.GetTypeByMetadataName("C").GetMembers().OfType()); - Assert.Equal(0, accessorBindingData.NumberOfPerformedAccessorBinding); + Assert.Equal(1, accessorBindingData.NumberOfPerformedAccessorBinding); } [Fact] @@ -418,7 +442,7 @@ .maxstack 2 Diagnostic(ErrorCode.WRN_UseDefViolationThisSupportedVersion, "P").WithArguments("this").WithLocation(16, 9) ); Assert.Empty(comp.GetTypeByMetadataName("C").GetMembers().OfType()); - Assert.Equal(0, accessorBindingData.NumberOfPerformedAccessorBinding); + Assert.Equal(1, accessorBindingData.NumberOfPerformedAccessorBinding); } [Fact] @@ -461,7 +485,7 @@ .maxstack 2 Diagnostic(ErrorCode.WRN_UseDefViolationThisSupportedVersion, "P").WithArguments("this").WithLocation(12, 9) ); Assert.Empty(comp.GetTypeByMetadataName("C").GetMembers().OfType()); - Assert.Equal(0, accessorBindingData.NumberOfPerformedAccessorBinding); + Assert.Equal(1, accessorBindingData.NumberOfPerformedAccessorBinding); } [Fact] @@ -1508,7 +1532,7 @@ .maxstack 3 IL_0009: ret } ").VerifyIL("C..ctor", ctorExpectedIL); - Assert.Equal(0, accessorBindingData.NumberOfPerformedAccessorBinding); + Assert.Equal(type == "struct" ? 1 : 0, accessorBindingData.NumberOfPerformedAccessorBinding); } [Theory] @@ -3072,7 +3096,7 @@ public ref struct S2 comp.VerifyDiagnostics( // PROTOTYPE(semi-auto-props): This should have ERR_FieldAutoPropCantBeByRefLike ); - Assert.Equal(0, accessorBindingData.NumberOfPerformedAccessorBinding); + Assert.Equal(2, accessorBindingData.NumberOfPerformedAccessorBinding); } [Fact] @@ -3092,7 +3116,7 @@ public struct S // public readonly string P { set => field = value; } Diagnostic(ErrorCode.ERR_AssgReadonlyLocal, "field").WithArguments("field").WithLocation(4, 39) ); - Assert.Equal(0, accessorBindingData.NumberOfPerformedAccessorBinding); + Assert.Equal(1, accessorBindingData.NumberOfPerformedAccessorBinding); } [Fact] @@ -3115,7 +3139,7 @@ public readonly struct S // public string P { set => field = value; } Diagnostic(ErrorCode.ERR_AssgReadonlyLocal, "field").WithArguments("field").WithLocation(4, 30) ); - Assert.Equal(0, accessorBindingData.NumberOfPerformedAccessorBinding); + Assert.Equal(1, accessorBindingData.NumberOfPerformedAccessorBinding); } [Fact] @@ -3254,6 +3278,36 @@ public class C1 Assert.Equal(0, accessorBindingData.NumberOfPerformedAccessorBinding); } + [Fact] + public void FieldLocal_PropertyAssignedInConstructor() + { + var comp = CreateCompilation(@" +public class C +{ + public C() + { + P = 10; + } + + public int P + { + get + { + int field = 0; + return field; + } + } +}"); + var accessorBindingData = new SourcePropertySymbolBase.AccessorBindingData(); + comp.TestOnlyCompilationData = accessorBindingData; + comp.VerifyDiagnostics( + // (6,9): error CS0200: Property or indexer 'C.P' cannot be assigned to -- it is read only + // P = 10; + Diagnostic(ErrorCode.ERR_AssgReadonlyProp, "P").WithArguments("C.P").WithLocation(6, 9) + ); + Assert.Equal(0, accessorBindingData.NumberOfPerformedAccessorBinding); + } + [Theory, CombinatorialData] public void InStruct(bool structTreeFirst) { @@ -3331,7 +3385,146 @@ void M3() Diagnostic(ErrorCode.ERR_UseDefViolation, "s1").WithArguments("s1").WithLocation(19, 37) ); - Assert.Equal(structTreeFirst ? 0 : 1, accessorBindingData.NumberOfPerformedAccessorBinding); + Assert.Equal(1, accessorBindingData.NumberOfPerformedAccessorBinding); + } + + [Theory, CombinatorialData] + public void InStruct_AssignStructToDefault(bool structTreeFirst) + { + var source1 = @" +struct S_WithAutoProperty +{ + public int P { get; set; } +} + +struct S_WithManualProperty +{ + public int P { get => 0; set => _ = value; } +} + +struct S_WithSemiAutoProperty +{ + public int P { get => field; set => field = value; } +} + +"; + + var source2 = @" +class C +{ + void M1() + { + S_WithAutoProperty s1 = default; + S_WithAutoProperty s2 = s1; + } + + void M2() + { + S_WithManualProperty s1 = default; + S_WithManualProperty s2 = s1; + } + + void M3() + { + S_WithSemiAutoProperty s1 = default; + S_WithSemiAutoProperty s2 = s1; + } +} +"; + var comp = CreateCompilation(new[] { source1, source2 }); + var accessorBindingData = new SourcePropertySymbolBase.AccessorBindingData(); + comp.TestOnlyCompilationData = accessorBindingData; + Assert.Equal("System.Int32 S_WithAutoProperty.

k__BackingField", comp.GetTypeByMetadataName("S_WithAutoProperty").GetMembers().OfType().Single().ToTestDisplayString()); + Assert.Empty(comp.GetTypeByMetadataName("S_WithManualProperty").GetMembers().OfType()); + Assert.Empty(comp.GetTypeByMetadataName("S_WithSemiAutoProperty").GetMembers().OfType()); + + if (structTreeFirst) + { + comp.GetDiagnosticsForSyntaxTree(CompilationStage.Compile, comp.SyntaxTrees[0], filterSpanWithinTree: null, includeEarlierStages: true).Verify(); + } + + comp.GetDiagnosticsForSyntaxTree(CompilationStage.Compile, comp.SyntaxTrees[1], filterSpanWithinTree: null, includeEarlierStages: true).Verify(); + + if (!structTreeFirst) + { + comp.GetDiagnosticsForSyntaxTree(CompilationStage.Compile, comp.SyntaxTrees[0], filterSpanWithinTree: null, includeEarlierStages: true).Verify(); + } + + comp.VerifyDiagnostics(); + + Assert.Equal(1, accessorBindingData.NumberOfPerformedAccessorBinding); + } + + [Theory, CombinatorialData] + public void InStruct_AssignStructViaConstructor(bool structTreeFirst) + { + var source1 = @" +struct S_WithAutoProperty +{ + public int P { get; set; } + + public S_WithAutoProperty() { } +} + +struct S_WithManualProperty +{ + public int P { get => 0; set => _ = value; } + + public S_WithManualProperty() { } +} + +struct S_WithSemiAutoProperty +{ + public int P { get => field; set => field = value; } + + public S_WithSemiAutoProperty() { } +} +"; + + var source2 = @" +class C +{ + void M1() + { + S_WithAutoProperty s1 = new S_WithAutoProperty(); + S_WithAutoProperty s2 = s1; + } + + void M2() + { + S_WithManualProperty s1 = new S_WithManualProperty(); + S_WithManualProperty s2 = s1; + } + + void M3() + { + S_WithSemiAutoProperty s1 = new S_WithSemiAutoProperty(); + S_WithSemiAutoProperty s2 = s1; + } +} +"; + var comp = CreateCompilation(new[] { source1, source2 }); + var accessorBindingData = new SourcePropertySymbolBase.AccessorBindingData(); + comp.TestOnlyCompilationData = accessorBindingData; + Assert.Equal("System.Int32 S_WithAutoProperty.

k__BackingField", comp.GetTypeByMetadataName("S_WithAutoProperty").GetMembers().OfType().Single().ToTestDisplayString()); + Assert.Empty(comp.GetTypeByMetadataName("S_WithManualProperty").GetMembers().OfType()); + Assert.Empty(comp.GetTypeByMetadataName("S_WithSemiAutoProperty").GetMembers().OfType()); + + if (structTreeFirst) + { + comp.GetDiagnosticsForSyntaxTree(CompilationStage.Compile, comp.SyntaxTrees[0], filterSpanWithinTree: null, includeEarlierStages: true).Verify(); + } + + comp.GetDiagnosticsForSyntaxTree(CompilationStage.Compile, comp.SyntaxTrees[1], filterSpanWithinTree: null, includeEarlierStages: true).Verify(); + + if (!structTreeFirst) + { + comp.GetDiagnosticsForSyntaxTree(CompilationStage.Compile, comp.SyntaxTrees[0], filterSpanWithinTree: null, includeEarlierStages: true).Verify(); + } + + comp.VerifyDiagnostics(); + + Assert.Equal(1, accessorBindingData.NumberOfPerformedAccessorBinding); } [Fact] @@ -3353,7 +3546,7 @@ public S(int arg) Assert.Empty(comp.GetTypeByMetadataName("S").GetMembers().OfType()); comp.VerifyDiagnostics(); - Assert.Equal(0, accessorBindingData.NumberOfPerformedAccessorBinding); + Assert.Equal(1, accessorBindingData.NumberOfPerformedAccessorBinding); } [Fact] @@ -3404,7 +3597,7 @@ public S1 P {{ {secondAccessor} }} // public S1 P { get; } Diagnostic(ErrorCode.ERR_StructLayoutCycle, "P").WithArguments("S2.P", "S1").WithLocation(9, 15) ); - Assert.Equal(0, accessorBindingData.NumberOfPerformedAccessorBinding); + Assert.Equal(firstIsSemi ? 2 : (secondIsSemi ? 1 : 0), accessorBindingData.NumberOfPerformedAccessorBinding); } [Fact] @@ -3423,7 +3616,7 @@ struct S // public S P { get => field; } Diagnostic(ErrorCode.ERR_StructLayoutCycle, "P").WithArguments("S.P", "S").WithLocation(4, 14) ); - Assert.Equal(0, accessorBindingData.NumberOfPerformedAccessorBinding); + Assert.Equal(1, accessorBindingData.NumberOfPerformedAccessorBinding); } [Theory] From 44fd4aa381557c5a59a2635e991a3b3963d52388 Mon Sep 17 00:00:00 2001 From: Youssef1313 Date: Sun, 24 Apr 2022 09:30:58 +0200 Subject: [PATCH 21/38] Update some tests and fix a NRE --- .../CSharp/Portable/Symbols/BaseTypeAnalysis.cs | 2 +- .../Semantics/PropertyFieldKeywordTests.cs | 15 +++++++++------ 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/src/Compilers/CSharp/Portable/Symbols/BaseTypeAnalysis.cs b/src/Compilers/CSharp/Portable/Symbols/BaseTypeAnalysis.cs index 9e05319f5fe86..d040fa29e527f 100644 --- a/src/Compilers/CSharp/Portable/Symbols/BaseTypeAnalysis.cs +++ b/src/Compilers/CSharp/Portable/Symbols/BaseTypeAnalysis.cs @@ -108,7 +108,7 @@ private static void StructDependsClosure(NamedTypeSymbol type, HashSet p Debug.Assert(!field.IsStatic); var fieldType = field.NonPointerType(); - if (fieldType.TypeKind != TypeKind.Struct) + if (fieldType is null || fieldType.TypeKind != TypeKind.Struct) { continue; } diff --git a/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs b/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs index 54a510fafd727..a3f9c811b2465 100644 --- a/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs +++ b/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs @@ -3579,23 +3579,25 @@ public void InStruct_Cycle(bool firstIsSemi, bool secondIsSemi) var comp = CreateCompilation($@" struct S1 {{ + public S1() {{ }} public S2 P {{ {firstAccessor} }} }} struct S2 {{ + public S2() {{ }} public S1 P {{ {secondAccessor} }} }} "); var accessorBindingData = new SourcePropertySymbolBase.AccessorBindingData(); comp.TestOnlyCompilationData = accessorBindingData; comp.VerifyDiagnostics( - // (4,15): error CS0523: Struct member 'S1.P' of type 'S2' causes a cycle in the struct layout + // (5,15): error CS0523: Struct member 'S1.P' of type 'S2' causes a cycle in the struct layout // public S2 P { get; } - Diagnostic(ErrorCode.ERR_StructLayoutCycle, "P").WithArguments("S1.P", "S2").WithLocation(4, 15), - // (9,15): error CS0523: Struct member 'S2.P' of type 'S1' causes a cycle in the struct layout + Diagnostic(ErrorCode.ERR_StructLayoutCycle, "P").WithArguments("S1.P", "S2").WithLocation(5, 15), + // (11,15): error CS0523: Struct member 'S2.P' of type 'S1' causes a cycle in the struct layout // public S1 P { get; } - Diagnostic(ErrorCode.ERR_StructLayoutCycle, "P").WithArguments("S2.P", "S1").WithLocation(9, 15) + Diagnostic(ErrorCode.ERR_StructLayoutCycle, "P").WithArguments("S2.P", "S1").WithLocation(11, 15) ); Assert.Equal(firstIsSemi ? 2 : (secondIsSemi ? 1 : 0), accessorBindingData.NumberOfPerformedAccessorBinding); } @@ -3606,15 +3608,16 @@ public void InStruct_SelfCycle() var comp = CreateCompilation(@" struct S { + public S() { } public S P { get => field; } } "); var accessorBindingData = new SourcePropertySymbolBase.AccessorBindingData(); comp.TestOnlyCompilationData = accessorBindingData; comp.VerifyDiagnostics( - // (4,14): error CS0523: Struct member 'S.P' of type 'S' causes a cycle in the struct layout + // (5,14): error CS0523: Struct member 'S.P' of type 'S' causes a cycle in the struct layout // public S P { get => field; } - Diagnostic(ErrorCode.ERR_StructLayoutCycle, "P").WithArguments("S.P", "S").WithLocation(4, 14) + Diagnostic(ErrorCode.ERR_StructLayoutCycle, "P").WithArguments("S.P", "S").WithLocation(5, 14) ); Assert.Equal(1, accessorBindingData.NumberOfPerformedAccessorBinding); } From e441ad37213f3289535ddae6d74a02aaca130181 Mon Sep 17 00:00:00 2001 From: Youssef1313 Date: Sun, 24 Apr 2022 14:45:41 +0200 Subject: [PATCH 22/38] Move struct circularity to CompilationStage.Compile --- .../Portable/Compilation/CSharpCompilation.cs | 30 ++++++++++++++++++ .../Source/SourceMemberContainerSymbol.cs | 4 +-- .../Semantics/PropertyFieldKeywordTests.cs | 31 ++++++++++--------- 3 files changed, 47 insertions(+), 18 deletions(-) diff --git a/src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs b/src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs index 86f01459b7973..6f50ea5d049ea 100644 --- a/src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs +++ b/src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs @@ -2581,7 +2581,19 @@ internal DiagnosticBag AdditionalCodegenWarnings } } + ///

+ /// A bag in which circular struct diagnostics should be reported. + /// + internal DiagnosticBag CircularStructDiagnostics + { + get + { + return _circularStructDiagnostics; + } + } + private readonly DiagnosticBag _additionalCodegenWarnings = new DiagnosticBag(); + private readonly DiagnosticBag _circularStructDiagnostics = new DiagnosticBag(); internal DeclarationTable Declarations { @@ -2735,11 +2747,29 @@ private void GetDiagnosticsWithoutFiltering(CompilationStage stage, bool include builder.DependenciesBag is object ? new ConcurrentSet() : null); RoslynDebug.Assert(methodBodyDiagnostics.DiagnosticBag is object); GetDiagnosticsForAllMethodBodies(methodBodyDiagnostics, doLowering: false, cancellationToken); + AddCircularStructDiagnostics(GlobalNamespace); builder.AddRange(methodBodyDiagnostics); + builder.AddRange(CircularStructDiagnostics); methodBodyDiagnostics.DiagnosticBag.Free(); } } + private void AddCircularStructDiagnostics(NamespaceOrTypeSymbol symbol) + { + if (symbol is SourceMemberContainerTypeSymbol sourceMemberContainerTypeSymbol) + { + _ = sourceMemberContainerTypeSymbol.KnownCircularStruct; + } + + foreach (var member in symbol.GetMembersUnordered()) + { + if (member is NamespaceOrTypeSymbol namespaceOrTypeSymbol) + { + AddCircularStructDiagnostics(namespaceOrTypeSymbol); + } + } + } + private static void AppendLoadDirectiveDiagnostics(DiagnosticBag builder, SyntaxAndDeclarationManager syntaxAndDeclarations, SyntaxTree syntaxTree, Func, IEnumerable>? locationFilterOpt = null) { ImmutableArray loadDirectives; diff --git a/src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol.cs b/src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol.cs index c5fdf5ee37f29..f3a3902bf6bbd 100644 --- a/src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol.cs +++ b/src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol.cs @@ -1617,8 +1617,6 @@ protected void AfterMembersChecks(BindingDiagnosticBag diagnostics) CheckTypeParameterNameConflicts(diagnostics); CheckAccessorNameConflicts(diagnostics); - bool unused = KnownCircularStruct; - CheckSequentialOnPartialType(diagnostics); CheckForProtectedInStaticClass(diagnostics); CheckForUnmatchedOperators(diagnostics); @@ -2137,7 +2135,7 @@ internal override bool KnownCircularStruct if (Interlocked.CompareExchange(ref _lazyKnownCircularStruct, value, (int)ThreeState.Unknown) == (int)ThreeState.Unknown) { - AddDeclarationDiagnostics(diagnostics); + DeclaringCompilation.CircularStructDiagnostics.AddRange(diagnostics.DiagnosticBag); } Debug.Assert(value == _lazyKnownCircularStruct); diff --git a/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs b/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs index a3f9c811b2465..31f3c9b51a781 100644 --- a/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs +++ b/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs @@ -265,7 +265,7 @@ .locals init (int V_0) Diagnostic(ErrorCode.WRN_UseDefViolationThisSupportedVersion, "P").WithArguments("this").WithLocation(13, 51) ); Assert.Empty(comp.GetTypeByMetadataName("C").GetMembers().OfType()); - Assert.Equal(1, accessorBindingData.NumberOfPerformedAccessorBinding); + Assert.Equal(0, accessorBindingData.NumberOfPerformedAccessorBinding); } [Fact] @@ -331,7 +331,7 @@ .locals init (int V_0) Diagnostic(ErrorCode.WRN_UseDefViolationThisSupportedVersion, "P").WithArguments("this").WithLocation(13, 69) ); Assert.Empty(comp.GetTypeByMetadataName("C").GetMembers().OfType()); - Assert.Equal(1, accessorBindingData.NumberOfPerformedAccessorBinding); + Assert.Equal(0, accessorBindingData.NumberOfPerformedAccessorBinding); } [Fact] // PROTOTYPE(semi-auto-props): Add test with semi-colon setter when mixed scenarios are supported. @@ -395,7 +395,7 @@ .locals init (int V_0) Diagnostic(ErrorCode.WRN_UseDefViolationThisSupportedVersion, "P").WithArguments("this").WithLocation(12, 69) ); Assert.Empty(comp.GetTypeByMetadataName("C").GetMembers().OfType()); - Assert.Equal(1, accessorBindingData.NumberOfPerformedAccessorBinding); + Assert.Equal(0, accessorBindingData.NumberOfPerformedAccessorBinding); } [Fact] @@ -442,7 +442,7 @@ .maxstack 2 Diagnostic(ErrorCode.WRN_UseDefViolationThisSupportedVersion, "P").WithArguments("this").WithLocation(16, 9) ); Assert.Empty(comp.GetTypeByMetadataName("C").GetMembers().OfType()); - Assert.Equal(1, accessorBindingData.NumberOfPerformedAccessorBinding); + Assert.Equal(0, accessorBindingData.NumberOfPerformedAccessorBinding); } [Fact] @@ -485,7 +485,7 @@ .maxstack 2 Diagnostic(ErrorCode.WRN_UseDefViolationThisSupportedVersion, "P").WithArguments("this").WithLocation(12, 9) ); Assert.Empty(comp.GetTypeByMetadataName("C").GetMembers().OfType()); - Assert.Equal(1, accessorBindingData.NumberOfPerformedAccessorBinding); + Assert.Equal(0, accessorBindingData.NumberOfPerformedAccessorBinding); } [Fact] @@ -1532,7 +1532,7 @@ .maxstack 3 IL_0009: ret } ").VerifyIL("C..ctor", ctorExpectedIL); - Assert.Equal(type == "struct" ? 1 : 0, accessorBindingData.NumberOfPerformedAccessorBinding); + Assert.Equal(0, accessorBindingData.NumberOfPerformedAccessorBinding); } [Theory] @@ -3096,7 +3096,7 @@ public ref struct S2 comp.VerifyDiagnostics( // PROTOTYPE(semi-auto-props): This should have ERR_FieldAutoPropCantBeByRefLike ); - Assert.Equal(2, accessorBindingData.NumberOfPerformedAccessorBinding); + Assert.Equal(0, accessorBindingData.NumberOfPerformedAccessorBinding); } [Fact] @@ -3116,7 +3116,7 @@ public struct S // public readonly string P { set => field = value; } Diagnostic(ErrorCode.ERR_AssgReadonlyLocal, "field").WithArguments("field").WithLocation(4, 39) ); - Assert.Equal(1, accessorBindingData.NumberOfPerformedAccessorBinding); + Assert.Equal(0, accessorBindingData.NumberOfPerformedAccessorBinding); } [Fact] @@ -3139,7 +3139,7 @@ public readonly struct S // public string P { set => field = value; } Diagnostic(ErrorCode.ERR_AssgReadonlyLocal, "field").WithArguments("field").WithLocation(4, 30) ); - Assert.Equal(1, accessorBindingData.NumberOfPerformedAccessorBinding); + Assert.Equal(0, accessorBindingData.NumberOfPerformedAccessorBinding); } [Fact] @@ -3385,7 +3385,7 @@ void M3() Diagnostic(ErrorCode.ERR_UseDefViolation, "s1").WithArguments("s1").WithLocation(19, 37) ); - Assert.Equal(1, accessorBindingData.NumberOfPerformedAccessorBinding); + Assert.Equal(structTreeFirst ? 0 : 1, accessorBindingData.NumberOfPerformedAccessorBinding); } [Theory, CombinatorialData] @@ -3452,7 +3452,7 @@ void M3() comp.VerifyDiagnostics(); - Assert.Equal(1, accessorBindingData.NumberOfPerformedAccessorBinding); + Assert.Equal(structTreeFirst ? 0 : 1, accessorBindingData.NumberOfPerformedAccessorBinding); } [Theory, CombinatorialData] @@ -3524,7 +3524,7 @@ void M3() comp.VerifyDiagnostics(); - Assert.Equal(1, accessorBindingData.NumberOfPerformedAccessorBinding); + Assert.Equal(structTreeFirst ? 0 : 1, accessorBindingData.NumberOfPerformedAccessorBinding); } [Fact] @@ -3546,7 +3546,7 @@ public S(int arg) Assert.Empty(comp.GetTypeByMetadataName("S").GetMembers().OfType()); comp.VerifyDiagnostics(); - Assert.Equal(1, accessorBindingData.NumberOfPerformedAccessorBinding); + Assert.Equal(0, accessorBindingData.NumberOfPerformedAccessorBinding); } [Fact] @@ -3591,6 +3591,7 @@ public S1 P {{ {secondAccessor} }} "); var accessorBindingData = new SourcePropertySymbolBase.AccessorBindingData(); comp.TestOnlyCompilationData = accessorBindingData; + // PROTOTYPE(semi-auto-props): Diagnostic is sometimes duplicated. comp.VerifyDiagnostics( // (5,15): error CS0523: Struct member 'S1.P' of type 'S2' causes a cycle in the struct layout // public S2 P { get; } @@ -3599,7 +3600,7 @@ public S1 P {{ {secondAccessor} }} // public S1 P { get; } Diagnostic(ErrorCode.ERR_StructLayoutCycle, "P").WithArguments("S2.P", "S1").WithLocation(11, 15) ); - Assert.Equal(firstIsSemi ? 2 : (secondIsSemi ? 1 : 0), accessorBindingData.NumberOfPerformedAccessorBinding); + Assert.Equal(0, accessorBindingData.NumberOfPerformedAccessorBinding); // PROTOTYPE(semi-auto-props): Not passing, and also flaky! } [Fact] @@ -3619,7 +3620,7 @@ public S() { } // public S P { get => field; } Diagnostic(ErrorCode.ERR_StructLayoutCycle, "P").WithArguments("S.P", "S").WithLocation(5, 14) ); - Assert.Equal(1, accessorBindingData.NumberOfPerformedAccessorBinding); + Assert.Equal(0, accessorBindingData.NumberOfPerformedAccessorBinding); } [Theory] From f530c53250d27d955e717ad0c4c240924851c2bd Mon Sep 17 00:00:00 2001 From: Youssef1313 Date: Sun, 24 Apr 2022 23:53:58 +0200 Subject: [PATCH 23/38] Assert non-null diagnostic bag --- .../Portable/Symbols/Source/SourceMemberContainerSymbol.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol.cs b/src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol.cs index f3a3902bf6bbd..579414ca938b4 100644 --- a/src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol.cs +++ b/src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol.cs @@ -2131,6 +2131,7 @@ internal override bool KnownCircularStruct else { var diagnostics = BindingDiagnosticBag.GetInstance(); + Debug.Assert(diagnostics.DiagnosticBag is not null); var value = (int)CheckStructCircularity(diagnostics).ToThreeState(); if (Interlocked.CompareExchange(ref _lazyKnownCircularStruct, value, (int)ThreeState.Unknown) == (int)ThreeState.Unknown) From a0e2749d9abf30eb36dc6046b49745d6ed00e417 Mon Sep 17 00:00:00 2001 From: Youssef1313 Date: Mon, 25 Apr 2022 11:17:32 +0200 Subject: [PATCH 24/38] Small fix --- .../Portable/Compilation/CSharpCompilation.cs | 18 ------------------ .../CSharp/Portable/Compiler/MethodCompiler.cs | 18 ++++++++++++++++++ .../Semantics/PropertyFieldKeywordTests.cs | 1 - 3 files changed, 18 insertions(+), 19 deletions(-) diff --git a/src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs b/src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs index 6f50ea5d049ea..f6e6494e5c953 100644 --- a/src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs +++ b/src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs @@ -2747,29 +2747,11 @@ private void GetDiagnosticsWithoutFiltering(CompilationStage stage, bool include builder.DependenciesBag is object ? new ConcurrentSet() : null); RoslynDebug.Assert(methodBodyDiagnostics.DiagnosticBag is object); GetDiagnosticsForAllMethodBodies(methodBodyDiagnostics, doLowering: false, cancellationToken); - AddCircularStructDiagnostics(GlobalNamespace); builder.AddRange(methodBodyDiagnostics); - builder.AddRange(CircularStructDiagnostics); methodBodyDiagnostics.DiagnosticBag.Free(); } } - private void AddCircularStructDiagnostics(NamespaceOrTypeSymbol symbol) - { - if (symbol is SourceMemberContainerTypeSymbol sourceMemberContainerTypeSymbol) - { - _ = sourceMemberContainerTypeSymbol.KnownCircularStruct; - } - - foreach (var member in symbol.GetMembersUnordered()) - { - if (member is NamespaceOrTypeSymbol namespaceOrTypeSymbol) - { - AddCircularStructDiagnostics(namespaceOrTypeSymbol); - } - } - } - private static void AppendLoadDirectiveDiagnostics(DiagnosticBag builder, SyntaxAndDeclarationManager syntaxAndDeclarations, SyntaxTree syntaxTree, Func, IEnumerable>? locationFilterOpt = null) { ImmutableArray loadDirectives; diff --git a/src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs b/src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs index c83be932c29fa..15c0916dcfb2f 100644 --- a/src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs +++ b/src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs @@ -202,6 +202,8 @@ public static void CompileMethodBodies( new LocalizableResourceString(messageResourceName, CodeAnalysisResources.ResourceManager, typeof(CodeAnalysisResources))); } + AddCircularStructDiagnostics(compilation.GlobalNamespace); + diagnostics.AddRange(compilation.CircularStructDiagnostics); diagnostics.AddRange(compilation.AdditionalCodegenWarnings); // we can get unused field warnings only if compiling whole compilation. @@ -216,6 +218,22 @@ public static void CompileMethodBodies( } } + private static void AddCircularStructDiagnostics(NamespaceOrTypeSymbol symbol) + { + if (symbol is SourceMemberContainerTypeSymbol sourceMemberContainerTypeSymbol) + { + _ = sourceMemberContainerTypeSymbol.KnownCircularStruct; + } + + foreach (var member in symbol.GetMembersUnordered()) + { + if (member is NamespaceOrTypeSymbol namespaceOrTypeSymbol) + { + AddCircularStructDiagnostics(namespaceOrTypeSymbol); + } + } + } + // Returns the MethodSymbol for the assembly entrypoint. If the user has a Task returning main, // this function returns the synthesized Main MethodSymbol. private static MethodSymbol GetEntryPoint(CSharpCompilation compilation, PEModuleBuilder moduleBeingBuilt, bool hasDeclarationErrors, bool emitMethodBodies, BindingDiagnosticBag diagnostics, CancellationToken cancellationToken) diff --git a/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs b/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs index 31f3c9b51a781..79121a57323f7 100644 --- a/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs +++ b/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs @@ -3591,7 +3591,6 @@ public S1 P {{ {secondAccessor} }} "); var accessorBindingData = new SourcePropertySymbolBase.AccessorBindingData(); comp.TestOnlyCompilationData = accessorBindingData; - // PROTOTYPE(semi-auto-props): Diagnostic is sometimes duplicated. comp.VerifyDiagnostics( // (5,15): error CS0523: Struct member 'S1.P' of type 'S2' causes a cycle in the struct layout // public S2 P { get; } From ea57d569393cfc9b3089cde256b863b43c0186ac Mon Sep 17 00:00:00 2001 From: Youssef1313 Date: Wed, 27 Apr 2022 08:38:31 +0200 Subject: [PATCH 25/38] Fix test --- .../Semantics/PropertyFieldKeywordTests.cs | 35 +++++++++++++++---- 1 file changed, 28 insertions(+), 7 deletions(-) diff --git a/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs b/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs index 79121a57323f7..e25f2b56ca8a0 100644 --- a/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs +++ b/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs @@ -3572,34 +3572,55 @@ public S(int arg) } [Theory, CombinatorialData] - public void InStruct_Cycle(bool firstIsSemi, bool secondIsSemi) + public void InStruct_Cycle(bool firstIsSemi, bool secondIsSemi, bool bindS1First) { var firstAccessor = firstIsSemi ? "get => field;" : "get;"; var secondAccessor = secondIsSemi ? "get => field;" : "get;"; - var comp = CreateCompilation($@" + var source1 = $@" struct S1 {{ public S1() {{ }} public S2 P {{ {firstAccessor} }} }} - +"; + var source2 = $@" struct S2 {{ public S2() {{ }} public S1 P {{ {secondAccessor} }} }} -"); +"; + var comp = CreateCompilation(new[] { source1, source2 }); var accessorBindingData = new SourcePropertySymbolBase.AccessorBindingData(); comp.TestOnlyCompilationData = accessorBindingData; + comp.GetDiagnosticsForSyntaxTree(CompilationStage.Compile, bindS1First ? comp.SyntaxTrees[0] : comp.SyntaxTrees[1], filterSpanWithinTree: null, includeEarlierStages: true).Verify( + // (5,15): error CS0523: Struct member 'S1.P' of type 'S2' causes a cycle in the struct layout + // public S2 P { get => field; } + Diagnostic(ErrorCode.ERR_StructLayoutCycle, "P").WithArguments(bindS1First ? "S1.P" : "S2.P", bindS1First ? "S2" : "S1").WithLocation(5, 15)); + + var expectedBinding = (firstIsSemi, secondIsSemi, bindS1First) switch + { + (true, true, _) => 1, + (true, false, false) => 1, + (false, true, true) => 1, + _ => 0, + }; + Assert.Equal(expectedBinding, accessorBindingData.NumberOfPerformedAccessorBinding); + + comp.GetDiagnosticsForSyntaxTree(CompilationStage.Compile, bindS1First ? comp.SyntaxTrees[1] : comp.SyntaxTrees[0], filterSpanWithinTree: null, includeEarlierStages: true).Verify( + // (5,15): error CS0523: Struct member 'S2.P' of type 'S1' causes a cycle in the struct layout + // public S1 P { get => field; } + Diagnostic(ErrorCode.ERR_StructLayoutCycle, "P").WithArguments(bindS1First ? "S2.P" : "S1.P", bindS1First ? "S1" : "S2").WithLocation(5, 15)); + comp.VerifyDiagnostics( // (5,15): error CS0523: Struct member 'S1.P' of type 'S2' causes a cycle in the struct layout // public S2 P { get; } Diagnostic(ErrorCode.ERR_StructLayoutCycle, "P").WithArguments("S1.P", "S2").WithLocation(5, 15), - // (11,15): error CS0523: Struct member 'S2.P' of type 'S1' causes a cycle in the struct layout + // (5,15): error CS0523: Struct member 'S2.P' of type 'S1' causes a cycle in the struct layout // public S1 P { get; } - Diagnostic(ErrorCode.ERR_StructLayoutCycle, "P").WithArguments("S2.P", "S1").WithLocation(11, 15) + Diagnostic(ErrorCode.ERR_StructLayoutCycle, "P").WithArguments("S2.P", "S1").WithLocation(5, 15) ); - Assert.Equal(0, accessorBindingData.NumberOfPerformedAccessorBinding); // PROTOTYPE(semi-auto-props): Not passing, and also flaky! + Assert.Equal(expectedBinding, accessorBindingData.NumberOfPerformedAccessorBinding); // PROTOTYPE(semi-auto-props): Not passing, and also flaky! } [Fact] From 45937beb040f8adeda31d0fd265d15c5cb46c93c Mon Sep 17 00:00:00 2001 From: Youssef1313 Date: Wed, 27 Apr 2022 09:12:11 +0200 Subject: [PATCH 26/38] Fix tests --- .../Semantic/Semantics/PropertyFieldKeywordTests.cs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs b/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs index e25f2b56ca8a0..915a624bf7aa8 100644 --- a/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs +++ b/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs @@ -262,7 +262,7 @@ .locals init (int V_0) ").VerifyDiagnostics( // (13,51): warning CS9020: The 'this' object is read before all of its fields have been assigned, causing preceding implicit assignments of 'default' to non-explicitly assigned fields. // System.Console.WriteLine("In C..ctor: " + P); - Diagnostic(ErrorCode.WRN_UseDefViolationThisSupportedVersion, "P").WithArguments("this").WithLocation(13, 51) + Diagnostic(ErrorCode.WRN_UseDefViolationThisSupportedVersion, "P").WithLocation(13, 51) ); Assert.Empty(comp.GetTypeByMetadataName("C").GetMembers().OfType()); Assert.Equal(0, accessorBindingData.NumberOfPerformedAccessorBinding); @@ -328,7 +328,7 @@ .locals init (int V_0) ").VerifyDiagnostics( // (13,69): warning CS9020: The 'this' object is read before all of its fields have been assigned, causing preceding implicit assignments of 'default' to non-explicitly assigned fields. // System.Console.WriteLine("In C..ctor before assignment: " + P); - Diagnostic(ErrorCode.WRN_UseDefViolationThisSupportedVersion, "P").WithArguments("this").WithLocation(13, 69) + Diagnostic(ErrorCode.WRN_UseDefViolationThisSupportedVersion, "P").WithLocation(13, 69) ); Assert.Empty(comp.GetTypeByMetadataName("C").GetMembers().OfType()); Assert.Equal(0, accessorBindingData.NumberOfPerformedAccessorBinding); @@ -392,7 +392,7 @@ .locals init (int V_0) ").VerifyDiagnostics( // (12,69): warning CS9020: The 'this' object is read before all of its fields have been assigned, causing preceding implicit assignments of 'default' to non-explicitly assigned fields. // System.Console.WriteLine("In C..ctor before assignment: " + P); - Diagnostic(ErrorCode.WRN_UseDefViolationThisSupportedVersion, "P").WithArguments("this").WithLocation(12, 69) + Diagnostic(ErrorCode.WRN_UseDefViolationThisSupportedVersion, "P").WithLocation(12, 69) ); Assert.Empty(comp.GetTypeByMetadataName("C").GetMembers().OfType()); Assert.Equal(0, accessorBindingData.NumberOfPerformedAccessorBinding); @@ -439,7 +439,7 @@ .maxstack 2 ").VerifyDiagnostics( // (16,9): warning CS9020: The 'this' object is read before all of its fields have been assigned, causing preceding implicit assignments of 'default' to non-explicitly assigned fields. // P = 5; - Diagnostic(ErrorCode.WRN_UseDefViolationThisSupportedVersion, "P").WithArguments("this").WithLocation(16, 9) + Diagnostic(ErrorCode.WRN_UseDefViolationThisSupportedVersion, "P").WithLocation(16, 9) ); Assert.Empty(comp.GetTypeByMetadataName("C").GetMembers().OfType()); Assert.Equal(0, accessorBindingData.NumberOfPerformedAccessorBinding); @@ -482,7 +482,7 @@ .maxstack 2 ").VerifyDiagnostics( // (12,9): warning CS9020: The 'this' object is read before all of its fields have been assigned, causing preceding implicit assignments of 'default' to non-explicitly assigned fields. // P = 5; - Diagnostic(ErrorCode.WRN_UseDefViolationThisSupportedVersion, "P").WithArguments("this").WithLocation(12, 9) + Diagnostic(ErrorCode.WRN_UseDefViolationThisSupportedVersion, "P").WithLocation(12, 9) ); Assert.Empty(comp.GetTypeByMetadataName("C").GetMembers().OfType()); Assert.Equal(0, accessorBindingData.NumberOfPerformedAccessorBinding); From e89e6b8afaa1624abde5b753c08e941606c16e85 Mon Sep 17 00:00:00 2001 From: Youssef1313 Date: Thu, 28 Apr 2022 09:56:24 +0200 Subject: [PATCH 27/38] Reduce binding, add test for static property --- .../Portable/Compiler/MethodCompiler.cs | 26 +++++++++++++++++ .../Semantics/PropertyFieldKeywordTests.cs | 28 +++++++++++++++++-- 2 files changed, 52 insertions(+), 2 deletions(-) diff --git a/src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs b/src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs index 15c0916dcfb2f..10aa3ae6ef83d 100644 --- a/src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs +++ b/src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs @@ -502,6 +502,11 @@ private void CompileNamedType(NamedTypeSymbol containingType) continue; } + if (member is not SourcePropertyAccessorSymbol { ContainsFieldKeyword: true }) + { + continue; + } + Debug.Assert(member.Kind == SymbolKind.Method); Binder.ProcessedFieldInitializers processedInitializers = default; CompileMethod(accessor, memberOrdinal, ref processedInitializers, synthesizedSubmissionFields, compilationState); @@ -524,6 +529,27 @@ private void CompileNamedType(NamedTypeSymbol containingType) } } + for (int memberOrdinal = 0; memberOrdinal < members.Length; memberOrdinal++) + { + var member = members[memberOrdinal]; + + //When a filter is supplied, limit the compilation of members passing the filter. + if (!PassesFilter(_filterOpt, member) || + member is not MethodSymbol { MethodKind: MethodKind.PropertyGet or MethodKind.PropertySet } accessor) + { + continue; + } + + if (member is SourcePropertyAccessorSymbol { ContainsFieldKeyword: true }) + { + continue; + } + + Debug.Assert(member.Kind == SymbolKind.Method); + Binder.ProcessedFieldInitializers processedInitializers = default; + CompileMethod(accessor, memberOrdinal, ref processedInitializers, synthesizedSubmissionFields, compilationState); + } + // Then we compile everything, excluding the accessors we already compiled in the loop above. for (int memberOrdinal = 0; memberOrdinal < members.Length; memberOrdinal++) { diff --git a/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs b/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs index 915a624bf7aa8..31e9a45b3807f 100644 --- a/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs +++ b/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs @@ -2140,7 +2140,7 @@ public struct S // public int P4 { get; readonly set => field = value; } // No ERR_AutoSetterCantBeReadOnly, but ERR_AssgReadonlyLocal Diagnostic(ErrorCode.ERR_ConcreteMissingBody, "get").WithArguments("S.P4.get").WithLocation(7, 21) ); - Assert.Equal(2, accessorBindingData.NumberOfPerformedAccessorBinding); + Assert.Equal(1, accessorBindingData.NumberOfPerformedAccessorBinding); } [Fact] @@ -3620,7 +3620,31 @@ public S1 P {{ {secondAccessor} }} // public S1 P { get; } Diagnostic(ErrorCode.ERR_StructLayoutCycle, "P").WithArguments("S2.P", "S1").WithLocation(5, 15) ); - Assert.Equal(expectedBinding, accessorBindingData.NumberOfPerformedAccessorBinding); // PROTOTYPE(semi-auto-props): Not passing, and also flaky! + Assert.Equal(expectedBinding, accessorBindingData.NumberOfPerformedAccessorBinding); + } + + [Theory, CombinatorialData] + public void InStruct_NoCycleBecauseStatic(bool firstIsSemi, bool secondIsSemi) + { + var firstAccessor = firstIsSemi ? "get => field;" : "get;"; + var secondAccessor = secondIsSemi ? "get => field;" : "get;"; + var comp = CreateCompilation($@" +struct S1 +{{ + public S1() {{ }} + public static S2 P {{ {firstAccessor} }} +}} + +struct S2 +{{ + public S2() {{ }} + public static S1 P {{ {secondAccessor} }} +}} +"); + var accessorBindingData = new SourcePropertySymbolBase.AccessorBindingData(); + comp.TestOnlyCompilationData = accessorBindingData; + comp.VerifyDiagnostics(); + Assert.Equal(0, accessorBindingData.NumberOfPerformedAccessorBinding); } [Fact] From bcf6974403132bab616eda41e5d19f97b680f8e3 Mon Sep 17 00:00:00 2001 From: Youssef1313 Date: Thu, 28 Apr 2022 10:25:46 +0200 Subject: [PATCH 28/38] Simplify --- .../Portable/Compiler/MethodCompiler.cs | 38 +++++-------------- 1 file changed, 9 insertions(+), 29 deletions(-) diff --git a/src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs b/src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs index 10aa3ae6ef83d..d375b937d3178 100644 --- a/src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs +++ b/src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs @@ -490,19 +490,14 @@ private void CompileNamedType(NamedTypeSymbol containingType) var members = containingType.GetMembers(); - // We first compile the accessors to avoid extra bindings for 'field' keyword. + // We first compile the accessors that contain 'field' keyword to avoid extra bindings for 'field' keyword when compiling other members. for (int memberOrdinal = 0; memberOrdinal < members.Length; memberOrdinal++) { var member = members[memberOrdinal]; //When a filter is supplied, limit the compilation of members passing the filter. if (!PassesFilter(_filterOpt, member) || - member is not MethodSymbol { MethodKind: MethodKind.PropertyGet or MethodKind.PropertySet } accessor) - { - continue; - } - - if (member is not SourcePropertyAccessorSymbol { ContainsFieldKeyword: true }) + member is not SourcePropertyAccessorSymbol { ContainsFieldKeyword: true } accessor) { continue; } @@ -512,6 +507,7 @@ private void CompileNamedType(NamedTypeSymbol containingType) CompileMethod(accessor, memberOrdinal, ref processedInitializers, synthesizedSubmissionFields, compilationState); } + // After compiling accessors containing 'field' keyword, we mark the backing field of the corresponding property as calculated. foreach (var member in members) { if (member is SourcePropertySymbolBase property) @@ -529,28 +525,7 @@ private void CompileNamedType(NamedTypeSymbol containingType) } } - for (int memberOrdinal = 0; memberOrdinal < members.Length; memberOrdinal++) - { - var member = members[memberOrdinal]; - - //When a filter is supplied, limit the compilation of members passing the filter. - if (!PassesFilter(_filterOpt, member) || - member is not MethodSymbol { MethodKind: MethodKind.PropertyGet or MethodKind.PropertySet } accessor) - { - continue; - } - - if (member is SourcePropertyAccessorSymbol { ContainsFieldKeyword: true }) - { - continue; - } - - Debug.Assert(member.Kind == SymbolKind.Method); - Binder.ProcessedFieldInitializers processedInitializers = default; - CompileMethod(accessor, memberOrdinal, ref processedInitializers, synthesizedSubmissionFields, compilationState); - } - - // Then we compile everything, excluding the accessors we already compiled in the loop above. + // Then we compile everything, excluding the accessors that contain field keyword we already compiled in the loop above. for (int memberOrdinal = 0; memberOrdinal < members.Length; memberOrdinal++) { var member = members[memberOrdinal]; @@ -578,6 +553,11 @@ private void CompileNamedType(NamedTypeSymbol containingType) Debug.Assert((object)method != scriptEntryPoint); Debug.Assert(!IsFieldLikeEventAccessor(method)); Debug.Assert(!method.IsPartialDefinition()); + } + + if (member is SourcePropertyAccessorSymbol { ContainsFieldKeyword: true }) + { + // We already compiled these accessors in the loop above. continue; } From b95f088312116712ff8b2039f9c8e449217fd2b9 Mon Sep 17 00:00:00 2001 From: Youssef1313 Date: Fri, 29 Apr 2022 21:47:31 +0200 Subject: [PATCH 29/38] Address part of review comments --- .../Portable/Compiler/MethodCompiler.cs | 47 ++++++++++++++----- .../Source/SourceMemberContainerSymbol.cs | 1 + .../Source/SourcePropertyAccessorSymbol.cs | 1 + .../Semantics/PropertyFieldKeywordTests.cs | 2 +- 4 files changed, 38 insertions(+), 13 deletions(-) diff --git a/src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs b/src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs index d375b937d3178..8352ce5a5811c 100644 --- a/src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs +++ b/src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs @@ -202,7 +202,7 @@ public static void CompileMethodBodies( new LocalizableResourceString(messageResourceName, CodeAnalysisResources.ResourceManager, typeof(CodeAnalysisResources))); } - AddCircularStructDiagnostics(compilation.GlobalNamespace); + addCircularStructDiagnostics(compilation.GlobalNamespace); diagnostics.AddRange(compilation.CircularStructDiagnostics); diagnostics.AddRange(compilation.AdditionalCodegenWarnings); @@ -216,21 +216,44 @@ public static void CompileMethodBodies( moduleBeingBuiltOpt.SetPEEntryPoint(entryPoint, diagnostics.DiagnosticBag); } } - } - private static void AddCircularStructDiagnostics(NamespaceOrTypeSymbol symbol) - { - if (symbol is SourceMemberContainerTypeSymbol sourceMemberContainerTypeSymbol) + void addCircularStructDiagnostics(NamespaceOrTypeSymbol symbol) { - _ = sourceMemberContainerTypeSymbol.KnownCircularStruct; + if (symbol is SourceMemberContainerTypeSymbol sourceMemberContainerTypeSymbol && PassesFilter(filterOpt, symbol)) + { + _ = sourceMemberContainerTypeSymbol.KnownCircularStruct; + } + + foreach (var member in symbol.GetMembersUnordered()) + { + if (member is NamespaceOrTypeSymbol namespaceOrTypeSymbol) + { + if (compilation.Options.ConcurrentBuild) + { + Task worker = addCircularStructDiagnosticsAsAsync(namespaceOrTypeSymbol); + methodCompiler._compilerTasks.Push(worker); + } + else + { + addCircularStructDiagnostics(namespaceOrTypeSymbol); + } + } + } } - foreach (var member in symbol.GetMembersUnordered()) + Task addCircularStructDiagnosticsAsAsync(NamespaceOrTypeSymbol symbol) { - if (member is NamespaceOrTypeSymbol namespaceOrTypeSymbol) + return Task.Run(UICultureUtilities.WithCurrentUICulture(() => { - AddCircularStructDiagnostics(namespaceOrTypeSymbol); - } + try + { + addCircularStructDiagnostics(symbol); + } + catch (Exception e) when (FatalError.ReportAndPropagateUnlessCanceled(e)) + { + throw ExceptionUtilities.Unreachable; + } + }), methodCompiler._cancellationToken); } } @@ -496,8 +519,8 @@ private void CompileNamedType(NamedTypeSymbol containingType) var member = members[memberOrdinal]; //When a filter is supplied, limit the compilation of members passing the filter. - if (!PassesFilter(_filterOpt, member) || - member is not SourcePropertyAccessorSymbol { ContainsFieldKeyword: true } accessor) + if (member is not SourcePropertyAccessorSymbol { ContainsFieldKeyword: true } accessor || + !PassesFilter(_filterOpt, member)) { continue; } diff --git a/src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol.cs b/src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol.cs index 579414ca938b4..66b688dd1f3e0 100644 --- a/src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol.cs +++ b/src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol.cs @@ -2136,6 +2136,7 @@ internal override bool KnownCircularStruct if (Interlocked.CompareExchange(ref _lazyKnownCircularStruct, value, (int)ThreeState.Unknown) == (int)ThreeState.Unknown) { + DeclaringCompilation.AddUsedAssemblies(diagnostics.DependenciesBag); DeclaringCompilation.CircularStructDiagnostics.AddRange(diagnostics.DiagnosticBag); } diff --git a/src/Compilers/CSharp/Portable/Symbols/Source/SourcePropertyAccessorSymbol.cs b/src/Compilers/CSharp/Portable/Symbols/Source/SourcePropertyAccessorSymbol.cs index 9ebf8fa0ec377..1fbb6f138471f 100644 --- a/src/Compilers/CSharp/Portable/Symbols/Source/SourcePropertyAccessorSymbol.cs +++ b/src/Compilers/CSharp/Portable/Symbols/Source/SourcePropertyAccessorSymbol.cs @@ -459,6 +459,7 @@ internal Accessibility LocalAccessibility /// This is only calculated from syntax, so we don't know if it /// will bind to something or will create a backing field. /// + // PROTOTYPE(semi-auto-props): Rename to ContainsFieldKeywordSyntactically or similar. internal bool ContainsFieldKeyword => _containsFieldKeyword; /// diff --git a/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs b/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs index 31e9a45b3807f..74d49de4c109c 100644 --- a/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs +++ b/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs @@ -1611,7 +1611,7 @@ public static void Main() CompileAndVerify(comp, expectedOutput: "0").VerifyDiagnostics().VerifyIL("C.P.get", @" ").VerifyIL("C..ctor", @" "); - Assert.Equal(1, accessorBindingData.NumberOfPerformedAccessorBinding); + Assert.Equal(0, accessorBindingData.NumberOfPerformedAccessorBinding); } [Theory] From e5abf646247113bd0b5406720e1bb88c09f86902 Mon Sep 17 00:00:00 2001 From: Youssef1313 Date: Fri, 29 Apr 2022 21:57:52 +0200 Subject: [PATCH 30/38] Update test --- .../Semantics/PropertyFieldKeywordTests.cs | 79 +++++++++++++------ 1 file changed, 57 insertions(+), 22 deletions(-) diff --git a/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs b/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs index 74d49de4c109c..edc1d7f091b9a 100644 --- a/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs +++ b/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs @@ -3572,23 +3572,21 @@ public S(int arg) } [Theory, CombinatorialData] - public void InStruct_Cycle(bool firstIsSemi, bool secondIsSemi, bool bindS1First) + public void InStruct_Cycle(bool bindS1First) { - var firstAccessor = firstIsSemi ? "get => field;" : "get;"; - var secondAccessor = secondIsSemi ? "get => field;" : "get;"; - var source1 = $@" + var source1 = @" struct S1 -{{ - public S1() {{ }} - public S2 P {{ {firstAccessor} }} -}} +{ + public S1() { } + public S2 P { get => field; } +} "; - var source2 = $@" + var source2 = @" struct S2 -{{ - public S2() {{ }} - public S1 P {{ {secondAccessor} }} -}} +{ + public S2() { } + public S1 P { get; } +} "; var comp = CreateCompilation(new[] { source1, source2 }); var accessorBindingData = new SourcePropertySymbolBase.AccessorBindingData(); @@ -3598,14 +3596,7 @@ public S1 P {{ {secondAccessor} }} // public S2 P { get => field; } Diagnostic(ErrorCode.ERR_StructLayoutCycle, "P").WithArguments(bindS1First ? "S1.P" : "S2.P", bindS1First ? "S2" : "S1").WithLocation(5, 15)); - var expectedBinding = (firstIsSemi, secondIsSemi, bindS1First) switch - { - (true, true, _) => 1, - (true, false, false) => 1, - (false, true, true) => 1, - _ => 0, - }; - Assert.Equal(expectedBinding, accessorBindingData.NumberOfPerformedAccessorBinding); + Assert.Equal(bindS1First ? 0 : 1, accessorBindingData.NumberOfPerformedAccessorBinding); comp.GetDiagnosticsForSyntaxTree(CompilationStage.Compile, bindS1First ? comp.SyntaxTrees[1] : comp.SyntaxTrees[0], filterSpanWithinTree: null, includeEarlierStages: true).Verify( // (5,15): error CS0523: Struct member 'S2.P' of type 'S1' causes a cycle in the struct layout @@ -3620,7 +3611,51 @@ public S1 P {{ {secondAccessor} }} // public S1 P { get; } Diagnostic(ErrorCode.ERR_StructLayoutCycle, "P").WithArguments("S2.P", "S1").WithLocation(5, 15) ); - Assert.Equal(expectedBinding, accessorBindingData.NumberOfPerformedAccessorBinding); + Assert.Equal(bindS1First ? 0 : 1, accessorBindingData.NumberOfPerformedAccessorBinding); + } + + [Fact] + public void InStruct_Cycle2() + { + var source1 = @" +struct S1 +{ + public S1() { } + public S2 P { get => field; } +} +"; + var source2 = @" +struct S2 +{ + public S2() { } + public S1 P { get => field; } +} +"; + var comp = CreateCompilation(new[] { source1, source2 }); + var accessorBindingData = new SourcePropertySymbolBase.AccessorBindingData(); + comp.TestOnlyCompilationData = accessorBindingData; + + comp.GetDiagnosticsForSyntaxTree(CompilationStage.Compile, comp.SyntaxTrees[0], filterSpanWithinTree: null, includeEarlierStages: true).Verify( + // (5,15): error CS0523: Struct member 'S1.P' of type 'S2' causes a cycle in the struct layout + // public S2 P { get; } + Diagnostic(ErrorCode.ERR_StructLayoutCycle, "P").WithArguments("S1.P", "S2").WithLocation(5, 15) + ); + + comp.GetDiagnosticsForSyntaxTree(CompilationStage.Compile, comp.SyntaxTrees[1], filterSpanWithinTree: null, includeEarlierStages: true).Verify( + // (5,15): error CS0523: Struct member 'S2.P' of type 'S1' causes a cycle in the struct layout + // public S1 P { get; } + Diagnostic(ErrorCode.ERR_StructLayoutCycle, "P").WithArguments("S2.P", "S1").WithLocation(5, 15) + ); + + comp.VerifyDiagnostics( + // (5,15): error CS0523: Struct member 'S1.P' of type 'S2' causes a cycle in the struct layout + // public S2 P { get; } + Diagnostic(ErrorCode.ERR_StructLayoutCycle, "P").WithArguments("S1.P", "S2").WithLocation(5, 15), + // (5,15): error CS0523: Struct member 'S2.P' of type 'S1' causes a cycle in the struct layout + // public S1 P { get; } + Diagnostic(ErrorCode.ERR_StructLayoutCycle, "P").WithArguments("S2.P", "S1").WithLocation(5, 15) + ); + Assert.Equal(1, accessorBindingData.NumberOfPerformedAccessorBinding); } [Theory, CombinatorialData] From c5e8afe848790d891c1786621f096d77ed4d54ea Mon Sep 17 00:00:00 2001 From: Youssef1313 Date: Sat, 30 Apr 2022 07:18:46 +0200 Subject: [PATCH 31/38] WaitForWorkers --- src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs b/src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs index 8352ce5a5811c..511ed56d671f2 100644 --- a/src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs +++ b/src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs @@ -203,6 +203,7 @@ public static void CompileMethodBodies( } addCircularStructDiagnostics(compilation.GlobalNamespace); + methodCompiler.WaitForWorkers(); diagnostics.AddRange(compilation.CircularStructDiagnostics); diagnostics.AddRange(compilation.AdditionalCodegenWarnings); From a93f24a5c927c42246c875f16a5152fec33f990c Mon Sep 17 00:00:00 2001 From: Youssef1313 Date: Sat, 30 Apr 2022 10:43:16 +0200 Subject: [PATCH 32/38] Fix definite assignment for field keyword --- .../Portable/FlowAnalysis/AbstractFlowPass.cs | 3 ++- .../FlowAnalysis/DefiniteAssignment.cs | 5 ++-- .../Semantics/PropertyFieldKeywordTests.cs | 25 +++++++------------ 3 files changed, 13 insertions(+), 20 deletions(-) diff --git a/src/Compilers/CSharp/Portable/FlowAnalysis/AbstractFlowPass.cs b/src/Compilers/CSharp/Portable/FlowAnalysis/AbstractFlowPass.cs index 59f0379650692..fd83fc9c97251 100644 --- a/src/Compilers/CSharp/Portable/FlowAnalysis/AbstractFlowPass.cs +++ b/src/Compilers/CSharp/Portable/FlowAnalysis/AbstractFlowPass.cs @@ -571,7 +571,8 @@ protected void VisitLvalue(BoundExpression node) if (Binder.IsPropertyAssignedThroughBackingField(access, _symbol)) { - var backingField = (access.PropertySymbol as SourcePropertySymbolBase)?.BackingField; + var property = (SourcePropertySymbolBase)access.PropertySymbol; + var backingField = property.BackingField ?? property.FieldKeywordBackingField; if (backingField != null) { VisitFieldAccessInternal(access.ReceiverOpt, backingField); diff --git a/src/Compilers/CSharp/Portable/FlowAnalysis/DefiniteAssignment.cs b/src/Compilers/CSharp/Portable/FlowAnalysis/DefiniteAssignment.cs index addf1fb8a4cb1..9d2b7db6e2d94 100644 --- a/src/Compilers/CSharp/Portable/FlowAnalysis/DefiniteAssignment.cs +++ b/src/Compilers/CSharp/Portable/FlowAnalysis/DefiniteAssignment.cs @@ -1038,14 +1038,13 @@ protected override bool TryGetReceiverAndMember(BoundExpression expr, out BoundE case BoundKind.PropertyAccess: { var propAccess = (BoundPropertyAccess)expr; - // PROTOTYPE(semi-auto-props): The call to IsPropertyAssignedThroughBackingField isn't sensible. // We get here for both property read and write. // This applies to ALL IsPropertyAssignedThroughBackingField calls in this file. if (Binder.IsPropertyAssignedThroughBackingField(propAccess, this.CurrentSymbol)) // PROTOTYPE(semi-auto-props): Revise this method call is the behavior we want and add unit tests.. { - var propSymbol = propAccess.PropertySymbol; - member = (propSymbol as SourcePropertySymbolBase)?.BackingField; + var propSymbol = propAccess.PropertySymbol as SourcePropertySymbolBase; + member = propSymbol?.BackingField ?? propSymbol?.FieldKeywordBackingField; if (member is null) { return false; diff --git a/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs b/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs index 934cae806320e..37b5208c9a842 100644 --- a/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs +++ b/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs @@ -662,22 +662,15 @@ public C() var accessorBindingData = new SourcePropertySymbolBase.AccessorBindingData(); comp.TestOnlyCompilationData = accessorBindingData; CompileAndVerify(comp, expectedOutput: "5").VerifyIL("C..ctor", @" -{ - // Code size 15 (0xf) - .maxstack 2 - IL_0000: ldarg.0 - IL_0001: ldc.i4.0 - IL_0002: stfld ""int C.

k__BackingField"" - IL_0007: ldarg.0 - IL_0008: ldc.i4.5 - IL_0009: stfld ""int C.

k__BackingField"" - IL_000e: ret -} -").VerifyDiagnostics( - // (12,9): warning CS9020: The 'this' object is read before all of its fields have been assigned, causing preceding implicit assignments of 'default' to non-explicitly assigned fields. - // P = 5; - Diagnostic(ErrorCode.WRN_UseDefViolationThisSupportedVersion, "P").WithLocation(12, 9) - ); + { + // Code size 8 (0x8) + .maxstack 2 + IL_0000: ldarg.0 + IL_0001: ldc.i4.5 + IL_0002: stfld ""int C.

k__BackingField"" + IL_0007: ret + } +").VerifyDiagnostics(); Assert.Empty(comp.GetTypeByMetadataName("C").GetMembers().OfType()); Assert.Equal(0, accessorBindingData.NumberOfPerformedAccessorBinding); } From 69a89bfc5196517548da166875fdebe3ca214569 Mon Sep 17 00:00:00 2001 From: Youssef Victor Date: Sat, 30 Apr 2022 14:07:12 +0200 Subject: [PATCH 33/38] Update AbstractFlowPass.cs --- .../CSharp/Portable/FlowAnalysis/AbstractFlowPass.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Compilers/CSharp/Portable/FlowAnalysis/AbstractFlowPass.cs b/src/Compilers/CSharp/Portable/FlowAnalysis/AbstractFlowPass.cs index fd83fc9c97251..6a3c5dd0faa48 100644 --- a/src/Compilers/CSharp/Portable/FlowAnalysis/AbstractFlowPass.cs +++ b/src/Compilers/CSharp/Portable/FlowAnalysis/AbstractFlowPass.cs @@ -571,8 +571,8 @@ protected void VisitLvalue(BoundExpression node) if (Binder.IsPropertyAssignedThroughBackingField(access, _symbol)) { - var property = (SourcePropertySymbolBase)access.PropertySymbol; - var backingField = property.BackingField ?? property.FieldKeywordBackingField; + var property = access.PropertySymbol as SourcePropertySymbolBase; + var backingField = property?.BackingField ?? property?.FieldKeywordBackingField; if (backingField != null) { VisitFieldAccessInternal(access.ReceiverOpt, backingField); From 6553698c0efbad8f71cb3571ff2f2429428b2390 Mon Sep 17 00:00:00 2001 From: Youssef Victor Date: Wed, 4 May 2022 23:44:05 +0200 Subject: [PATCH 34/38] Update BaseTypeAnalysis.cs --- .../CSharp/Portable/Symbols/BaseTypeAnalysis.cs | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/Compilers/CSharp/Portable/Symbols/BaseTypeAnalysis.cs b/src/Compilers/CSharp/Portable/Symbols/BaseTypeAnalysis.cs index d040fa29e527f..9e6ff467240af 100644 --- a/src/Compilers/CSharp/Portable/Symbols/BaseTypeAnalysis.cs +++ b/src/Compilers/CSharp/Portable/Symbols/BaseTypeAnalysis.cs @@ -92,12 +92,17 @@ private static void StructDependsClosure(NamedTypeSymbol type, HashSet p { foreach (var member in type.GetMembersUnordered()) { + if (member.IsStatic) + { + continue; + } + FieldSymbol field; - if (member is SourcePropertySymbolBase { IsStatic: false, FieldKeywordBackingField: { } fieldKeywordField }) + if (member is SourcePropertySymbolBase { FieldKeywordBackingField: { } fieldKeywordField }) { field = fieldKeywordField; } - else if (member.Kind == SymbolKind.Field && !member.IsStatic) + else if (member.Kind == SymbolKind.Field) { field = (FieldSymbol)member; } @@ -106,7 +111,6 @@ private static void StructDependsClosure(NamedTypeSymbol type, HashSet p continue; } - Debug.Assert(!field.IsStatic); var fieldType = field.NonPointerType(); if (fieldType is null || fieldType.TypeKind != TypeKind.Struct) { From 0b3dcee9d591e98374a8d341cba4a94c5f10dece Mon Sep 17 00:00:00 2001 From: Youssef1313 Date: Thu, 5 May 2022 22:22:20 +0200 Subject: [PATCH 35/38] Address review comments --- .../CSharp/Portable/Compilation/CSharpCompilation.cs | 4 ++-- src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs | 2 +- .../Portable/Symbols/Source/SourceMemberContainerSymbol.cs | 3 +-- 3 files changed, 4 insertions(+), 5 deletions(-) diff --git a/src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs b/src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs index f6e6494e5c953..cec53a267838e 100644 --- a/src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs +++ b/src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs @@ -2584,7 +2584,7 @@ internal DiagnosticBag AdditionalCodegenWarnings ///

/// A bag in which circular struct diagnostics should be reported. /// - internal DiagnosticBag CircularStructDiagnostics + internal BindingDiagnosticBag CircularStructDiagnostics { get { @@ -2593,7 +2593,7 @@ internal DiagnosticBag CircularStructDiagnostics } private readonly DiagnosticBag _additionalCodegenWarnings = new DiagnosticBag(); - private readonly DiagnosticBag _circularStructDiagnostics = new DiagnosticBag(); + private readonly BindingDiagnosticBag _circularStructDiagnostics = new BindingDiagnosticBag(); internal DeclarationTable Declarations { diff --git a/src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs b/src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs index ff6f1540ae298..99dc95f2c3c2e 100644 --- a/src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs +++ b/src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs @@ -202,7 +202,7 @@ public static void CompileMethodBodies( new LocalizableResourceString(messageResourceName, CodeAnalysisResources.ResourceManager, typeof(CodeAnalysisResources))); } - addCircularStructDiagnostics(compilation.GlobalNamespace); + addCircularStructDiagnostics(compilation.SourceModule.GlobalNamespace); methodCompiler.WaitForWorkers(); diagnostics.AddRange(compilation.CircularStructDiagnostics); diagnostics.AddRange(compilation.AdditionalCodegenWarnings); diff --git a/src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol.cs b/src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol.cs index 66b688dd1f3e0..81985dabfefe0 100644 --- a/src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol.cs +++ b/src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol.cs @@ -2136,8 +2136,7 @@ internal override bool KnownCircularStruct if (Interlocked.CompareExchange(ref _lazyKnownCircularStruct, value, (int)ThreeState.Unknown) == (int)ThreeState.Unknown) { - DeclaringCompilation.AddUsedAssemblies(diagnostics.DependenciesBag); - DeclaringCompilation.CircularStructDiagnostics.AddRange(diagnostics.DiagnosticBag); + DeclaringCompilation.CircularStructDiagnostics.AddRange(diagnostics); } Debug.Assert(value == _lazyKnownCircularStruct); From 3de51814b532142362569b2f2afa806e46872d24 Mon Sep 17 00:00:00 2001 From: Youssef1313 Date: Thu, 5 May 2022 23:17:00 +0200 Subject: [PATCH 36/38] Address review comment --- src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs b/src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs index cec53a267838e..b559e9837cfef 100644 --- a/src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs +++ b/src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs @@ -2593,7 +2593,7 @@ internal BindingDiagnosticBag CircularStructDiagnostics } private readonly DiagnosticBag _additionalCodegenWarnings = new DiagnosticBag(); - private readonly BindingDiagnosticBag _circularStructDiagnostics = new BindingDiagnosticBag(); + private readonly BindingDiagnosticBag _circularStructDiagnostics = new BindingDiagnosticBag(new DiagnosticBag(), new ConcurrentSet()); internal DeclarationTable Declarations { From fcb5e3f59d848c27b398e42c801c6883a30590a7 Mon Sep 17 00:00:00 2001 From: Youssef1313 Date: Thu, 5 May 2022 23:29:52 +0200 Subject: [PATCH 37/38] Fix assertion failure --- src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs b/src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs index 99dc95f2c3c2e..e455ccd918620 100644 --- a/src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs +++ b/src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs @@ -204,7 +204,7 @@ public static void CompileMethodBodies( addCircularStructDiagnostics(compilation.SourceModule.GlobalNamespace); methodCompiler.WaitForWorkers(); - diagnostics.AddRange(compilation.CircularStructDiagnostics); + diagnostics.AddRange(compilation.CircularStructDiagnostics, allowMismatchInDependencyAccumulation: true); diagnostics.AddRange(compilation.AdditionalCodegenWarnings); // we can get unused field warnings only if compiling whole compilation. From c42ed4a34066df2148d560301b1340f3f68d77ce Mon Sep 17 00:00:00 2001 From: Youssef1313 Date: Wed, 11 May 2022 20:40:28 +0200 Subject: [PATCH 38/38] Add test --- .../Semantics/PropertyFieldKeywordTests.cs | 43 +++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs b/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs index 9ad52a80b8bca..3a7c2877e996e 100644 --- a/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs +++ b/src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs @@ -3936,6 +3936,49 @@ public S2() { } Assert.Equal(1, accessorBindingData.NumberOfPerformedAccessorBinding); } + [Fact] + public void InStruct_NoCycle() + { + var source1 = @" +struct S1 +{ + public S1() { } + + public int P + { + get + { + _ = field; + var x = new S2(); + return field; + } + } +}"; + + var source2 = @" +struct S2 +{ + public S2() { } + + public int P + { + get + { + _ = field; + var x = new S1(); + return field; + } + } +}"; + var comp = CreateCompilation(new[] { source1, source2 }); + var accessorBindingData = new SourcePropertySymbolBase.AccessorBindingData(); + comp.TestOnlyCompilationData = accessorBindingData; + comp.GetDiagnosticsForSyntaxTree(CompilationStage.Compile, comp.SyntaxTrees[0], filterSpanWithinTree: null, includeEarlierStages: true).Verify(); + Assert.Equal(1, accessorBindingData.NumberOfPerformedAccessorBinding); + comp.GetDiagnosticsForSyntaxTree(CompilationStage.Compile, comp.SyntaxTrees[1], filterSpanWithinTree: null, includeEarlierStages: true).Verify(); + Assert.Equal(1, accessorBindingData.NumberOfPerformedAccessorBinding); + } + [Theory, CombinatorialData] public void InStruct_NoCycleBecauseStatic(bool firstIsSemi, bool secondIsSemi) {