From 4c804d2837f298462ec827214e4ab4d3ea1d258c Mon Sep 17 00:00:00 2001 From: Rikki Gibson Date: Mon, 4 May 2020 14:09:05 -0700 Subject: [PATCH 1/8] Support multiple in deterministic order --- .../Portable/Compilation/CSharpCompilation.cs | 3 +- .../Symbol/Symbols/ModuleInitializersTests.cs | 190 ++++++++++++++++++ 2 files changed, 191 insertions(+), 2 deletions(-) diff --git a/src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs b/src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs index a4af9691431ff..47cb779d491b3 100644 --- a/src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs +++ b/src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs @@ -2832,8 +2832,7 @@ private void GenerateModuleInitializer(PEModuleBuilder moduleBeingBuilt, Diagnos { var ilBuilder = new ILBuilder(moduleBeingBuilt, new LocalSlotManager(slotAllocator: null), OptimizationLevel.Release, areLocalsZeroed: false); - // PROTOTYPE(module-initializers): require deterministic order - foreach (var method in _moduleInitializerMethods) + foreach (var method in _moduleInitializerMethods.OrderBy(m => m, LexicalOrderSymbolComparer.Instance)) { ilBuilder.EmitOpCode(ILOpCode.Call, stackAdjustment: 0); diff --git a/src/Compilers/CSharp/Test/Symbol/Symbols/ModuleInitializersTests.cs b/src/Compilers/CSharp/Test/Symbol/Symbols/ModuleInitializersTests.cs index 558aab4d66028..ed96bb9864319 100644 --- a/src/Compilers/CSharp/Test/Symbol/Symbols/ModuleInitializersTests.cs +++ b/src/Compilers/CSharp/Test/Symbol/Symbols/ModuleInitializersTests.cs @@ -7,6 +7,7 @@ using Microsoft.CodeAnalysis.CSharp.Symbols.Metadata.PE; using Microsoft.CodeAnalysis.CSharp.Test.Utilities; using Microsoft.CodeAnalysis.Test.Utilities; +using Roslyn.Test.Utilities; using Xunit; namespace Microsoft.CodeAnalysis.CSharp.UnitTests.Symbols @@ -137,5 +138,194 @@ namespace System.Runtime.CompilerServices { class ModuleInitializerAttribute : S C.M Program.Main"); } + + [Fact] + public void MultipleInitializers_SingleFile() + { + string source = @" +using System; +using System.Runtime.CompilerServices; + +class C1 +{ + [ModuleInitializer] + internal static void M1() => Console.Write(1); + + internal class C2 + { + [ModuleInitializer] + internal static void M2() => Console.Write(2); + } + + [ModuleInitializer] + internal static void M3() => Console.Write(3); +} + +class Program +{ + static void Main() => Console.Write(4); +} + +namespace System.Runtime.CompilerServices { class ModuleInitializerAttribute : System.Attribute { } } +"; + + CompileAndVerify( + source, + parseOptions: s_parseOptions, + options: TestOptions.DebugExe.WithMetadataImportOptions(MetadataImportOptions.All), + symbolValidator: module => + { + var rootModuleType = (TypeSymbol)module.GlobalNamespace.GetMember(""); + var staticConstructor = (PEMethodSymbol)rootModuleType.GetMember(".cctor"); + + Assert.NotNull(staticConstructor); + Assert.Equal(MethodKind.StaticConstructor, staticConstructor.MethodKind); + + var expectedFlags = + MethodAttributes.Private + | MethodAttributes.Static + | MethodAttributes.SpecialName + | MethodAttributes.RTSpecialName + | MethodAttributes.HideBySig; + + Assert.Equal(expectedFlags, staticConstructor.Flags); + }, + expectedOutput: "1234"); + } + + [Fact] + public void MultipleInitializers_DifferentContainingTypeKinds() + { + string source = @" +using System; +using System.Runtime.CompilerServices; + +class C1 +{ + [ModuleInitializer] + internal static void M1() => Console.Write(1); +} + +struct S1 +{ + [ModuleInitializer] + internal static void M2() => Console.Write(2); +} + +interface I1 +{ + [ModuleInitializer] + internal static void M3() => Console.Write(3); +} + +class Program +{ + static void Main() => Console.Write(4); +} + +namespace System.Runtime.CompilerServices { class ModuleInitializerAttribute : System.Attribute { } } +"; + + CompileAndVerify( + source, + parseOptions: s_parseOptions, + targetFramework: TargetFramework.NetStandardLatest, + options: TestOptions.DebugExe.WithMetadataImportOptions(MetadataImportOptions.All), + symbolValidator: module => + { + var rootModuleType = (TypeSymbol)module.GlobalNamespace.GetMember(""); + var staticConstructor = (PEMethodSymbol)rootModuleType.GetMember(".cctor"); + + Assert.NotNull(staticConstructor); + Assert.Equal(MethodKind.StaticConstructor, staticConstructor.MethodKind); + + var expectedFlags = + MethodAttributes.Private + | MethodAttributes.Static + | MethodAttributes.SpecialName + | MethodAttributes.RTSpecialName + | MethodAttributes.HideBySig; + + Assert.Equal(expectedFlags, staticConstructor.Flags); + }, + expectedOutput: !ExecutionConditionUtil.IsMonoOrCoreClr ? null : "1234"); + } + + [Fact] + public void MultipleInitializers_MultipleFiles() + { + string source1 = @" +using System; +using System.Runtime.CompilerServices; + +class C1 +{ + [ModuleInitializer] + internal static void M1() => Console.Write(1); + [ModuleInitializer] + internal static void M2() => Console.Write(2); +} + +namespace System.Runtime.CompilerServices { class ModuleInitializerAttribute : System.Attribute { } } +"; + string source2 = @" +using System; +using System.Runtime.CompilerServices; + +class C2 +{ + internal class C3 + { + [ModuleInitializer] + internal static void M3() => Console.Write(3); + } + + [ModuleInitializer] + internal static void M4() => Console.Write(4); +} + +class Program +{ + static void Main() => Console.Write(6); +} +"; + + string source3 = @" +using System; +using System.Runtime.CompilerServices; + +class C4 +{ + // shouldn't be called + internal static void M() => Console.Write(0); + + [ModuleInitializer] + internal static void M5() => Console.Write(5); +} +"; + + CompileAndVerify( + new[] { source1, source2, source3 }, + parseOptions: s_parseOptions, + options: TestOptions.DebugExe.WithMetadataImportOptions(MetadataImportOptions.All), + symbolValidator: module => + { + var rootModuleType = (TypeSymbol)module.GlobalNamespace.GetMember(""); + var staticConstructor = (PEMethodSymbol)rootModuleType.GetMember(".cctor"); + + Assert.NotNull(staticConstructor); + Assert.Equal(MethodKind.StaticConstructor, staticConstructor.MethodKind); + + var expectedFlags = + MethodAttributes.Private + | MethodAttributes.Static + | MethodAttributes.SpecialName + | MethodAttributes.RTSpecialName + | MethodAttributes.HideBySig; + + Assert.Equal(expectedFlags, staticConstructor.Flags); + }, + expectedOutput: "123456"); + } } } From d78f2762b77130066ba236c9cb2b504809745211 Mon Sep 17 00:00:00 2001 From: Rikki Gibson Date: Mon, 4 May 2020 15:11:17 -0700 Subject: [PATCH 2/8] Don't crash in ConcurrentSet.CopyTo --- src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs b/src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs index 47cb779d491b3..fb919b9029697 100644 --- a/src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs +++ b/src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs @@ -2832,7 +2832,8 @@ private void GenerateModuleInitializer(PEModuleBuilder moduleBeingBuilt, Diagnos { var ilBuilder = new ILBuilder(moduleBeingBuilt, new LocalSlotManager(slotAllocator: null), OptimizationLevel.Release, areLocalsZeroed: false); - foreach (var method in _moduleInitializerMethods.OrderBy(m => m, LexicalOrderSymbolComparer.Instance)) + // note that we are casting the Symbol returned by the OrderBy enumerable to MethodSymbol, expecting it will succeed. + foreach (MethodSymbol method in _moduleInitializerMethods.OrderBy(LexicalOrderSymbolComparer.Instance)) { ilBuilder.EmitOpCode(ILOpCode.Call, stackAdjustment: 0); From 488e63d34fd33457a8682d1cc1c0e5a18343c110 Mon Sep 17 00:00:00 2001 From: Rikki Gibson Date: Tue, 5 May 2020 10:58:28 -0700 Subject: [PATCH 3/8] Fix test --- .../CSharp/Test/Symbol/Symbols/ModuleInitializersTests.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/Compilers/CSharp/Test/Symbol/Symbols/ModuleInitializersTests.cs b/src/Compilers/CSharp/Test/Symbol/Symbols/ModuleInitializersTests.cs index ed96bb9864319..5533827679b8e 100644 --- a/src/Compilers/CSharp/Test/Symbol/Symbols/ModuleInitializersTests.cs +++ b/src/Compilers/CSharp/Test/Symbol/Symbols/ModuleInitializersTests.cs @@ -248,7 +248,8 @@ namespace System.Runtime.CompilerServices { class ModuleInitializerAttribute : S Assert.Equal(expectedFlags, staticConstructor.Flags); }, - expectedOutput: !ExecutionConditionUtil.IsMonoOrCoreClr ? null : "1234"); + expectedOutput: !ExecutionConditionUtil.IsMonoOrCoreClr ? null : "1234", + verify: !ExecutionConditionUtil.IsMonoOrCoreClr ? Verification.Skipped : Verification.Passes); } [Fact] From 2c82cf8a611318f2f24593773c40dcb716b9f079 Mon Sep 17 00:00:00 2001 From: Rikki Gibson Date: Tue, 5 May 2020 10:58:56 -0700 Subject: [PATCH 4/8] Add various tests --- .../Symbol/Symbols/ModuleInitializersTests.cs | 178 ++++++++++++++++++ 1 file changed, 178 insertions(+) diff --git a/src/Compilers/CSharp/Test/Symbol/Symbols/ModuleInitializersTests.cs b/src/Compilers/CSharp/Test/Symbol/Symbols/ModuleInitializersTests.cs index 5533827679b8e..85fec924f9191 100644 --- a/src/Compilers/CSharp/Test/Symbol/Symbols/ModuleInitializersTests.cs +++ b/src/Compilers/CSharp/Test/Symbol/Symbols/ModuleInitializersTests.cs @@ -328,5 +328,183 @@ class C4 }, expectedOutput: "123456"); } + + [Fact] + public void NonNullableField_01() + { + const string text = @" +#nullable enable + +using System.Runtime.CompilerServices; + +class C +{ + static string s1; + + [ModuleInitializer] + static void Init() + { + s1 = """"; + } +} + +namespace System.Runtime.CompilerServices { class ModuleInitializerAttribute : System.Attribute { } } +"; + + var comp = CreateCompilation(text, parseOptions: s_parseOptions); + comp.VerifyDiagnostics( + // (8,19): warning CS8618: Non-nullable field 's1' is uninitialized. Consider declaring the field as nullable. + // static string s1; + Diagnostic(ErrorCode.WRN_UninitializedNonNullableField, "s1").WithArguments("field", "s1").WithLocation(8, 19), + // (8,19): warning CS0414: The field 'C.s1' is assigned but its value is never used + // static string s1; + Diagnostic(ErrorCode.WRN_UnreferencedFieldAssg, "s1").WithArguments("C.s1").WithLocation(8, 19)); + } + + [Fact] + public void StaticConstructor_Ordering() + { + const string text = @" +using System; +using System.Runtime.CompilerServices; + +class C1 +{ + [ModuleInitializer] + internal static void Init() => Console.Write(1); +} + +class C2 +{ + static C2() => Console.Write(2); + + static void Main() + { + Console.Write(3); + } +} + +namespace System.Runtime.CompilerServices { class ModuleInitializerAttribute : System.Attribute { } } +"; + var verifier = CompileAndVerify(text, parseOptions: s_parseOptions, expectedOutput: "123"); + verifier.VerifyDiagnostics(); + } + + [Fact] + public void StaticConstructor_Ordering_SameType() + { + const string text = @" +using System; +using System.Runtime.CompilerServices; + +class C +{ + static C() => Console.Write(1); + + [ModuleInitializer] + internal static void Init() => Console.Write(2); + + static void Main() + { + Console.Write(3); + } +} + +namespace System.Runtime.CompilerServices { class ModuleInitializerAttribute : System.Attribute { } } +"; + var verifier = CompileAndVerify(text, parseOptions: s_parseOptions, expectedOutput: "123"); + verifier.VerifyDiagnostics(); + } + + [Fact] + public void StaticConstructor_DefaultInitializer_SameType() + { + const string text = @" +using System; +using System.Runtime.CompilerServices; + +class C +{ + internal static string s1 = null!; + + [ModuleInitializer] + internal static void Init() + { + s1 = ""hello""; + } + + static void Main() + { + Console.Write(s1); + } +} + +namespace System.Runtime.CompilerServices { class ModuleInitializerAttribute : System.Attribute { } } +"; + var verifier = CompileAndVerify( + text, + parseOptions: s_parseOptions, + options: TestOptions.DebugExe.WithMetadataImportOptions(MetadataImportOptions.All), + expectedOutput: "hello", + symbolValidator: validator); + verifier.VerifyDiagnostics(); + + void validator(ModuleSymbol module) + { + var cType = module.ContainingAssembly.GetTypeByMetadataName("C"); + // static constructor should be optimized out + Assert.Null(cType.GetMember(".cctor")); + + var moduleType = module.ContainingAssembly.GetTypeByMetadataName(""); + Assert.NotNull(moduleType.GetMember(".cctor")); + } + } + + [Fact] + public void StaticConstructor_DefaultInitializer_OtherType() + { + const string text = @" +using System; +using System.Runtime.CompilerServices; + +class C1 +{ + [ModuleInitializer] + internal static void Init() + { + C2.s1 = ""hello""; + } +} + +class C2 +{ + internal static string s1 = null!; + + static void Main() + { + Console.Write(s1); + } +} + +namespace System.Runtime.CompilerServices { class ModuleInitializerAttribute : System.Attribute { } } +"; + var verifier = CompileAndVerify( + text, + parseOptions: s_parseOptions, + options: TestOptions.DebugExe.WithMetadataImportOptions(MetadataImportOptions.All), + expectedOutput: "hello", + symbolValidator: validator); + verifier.VerifyDiagnostics(); + + void validator(ModuleSymbol module) + { + var c2Type = module.ContainingAssembly.GetTypeByMetadataName("C2"); + // static constructor should be optimized out + Assert.Null(c2Type.GetMember(".cctor")); + + var moduleType = module.ContainingAssembly.GetTypeByMetadataName(""); + Assert.NotNull(moduleType.GetMember(".cctor")); + } + } } } From 3892e4b8f0769fa7afc8ff347ea044f200c77ec6 Mon Sep 17 00:00:00 2001 From: Rikki Gibson Date: Tue, 5 May 2020 14:06:08 -0700 Subject: [PATCH 5/8] Implement CopyTo. Update tests. --- .../Portable/Compilation/CSharpCompilation.cs | 3 +- .../Symbol/Symbols/ModuleInitializersTests.cs | 188 ++++++++++-------- .../InternalUtilities/ConcurrentSet.cs | 8 +- 3 files changed, 111 insertions(+), 88 deletions(-) diff --git a/src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs b/src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs index fb919b9029697..9b61c902c06ba 100644 --- a/src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs +++ b/src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs @@ -2832,8 +2832,7 @@ private void GenerateModuleInitializer(PEModuleBuilder moduleBeingBuilt, Diagnos { var ilBuilder = new ILBuilder(moduleBeingBuilt, new LocalSlotManager(slotAllocator: null), OptimizationLevel.Release, areLocalsZeroed: false); - // note that we are casting the Symbol returned by the OrderBy enumerable to MethodSymbol, expecting it will succeed. - foreach (MethodSymbol method in _moduleInitializerMethods.OrderBy(LexicalOrderSymbolComparer.Instance)) + foreach (MethodSymbol method in _moduleInitializerMethods.OrderBy(LexicalOrderSymbolComparer.Instance)) { ilBuilder.EmitOpCode(ILOpCode.Call, stackAdjustment: 0); diff --git a/src/Compilers/CSharp/Test/Symbol/Symbols/ModuleInitializersTests.cs b/src/Compilers/CSharp/Test/Symbol/Symbols/ModuleInitializersTests.cs index 85fec924f9191..9363240ba843b 100644 --- a/src/Compilers/CSharp/Test/Symbol/Symbols/ModuleInitializersTests.cs +++ b/src/Compilers/CSharp/Test/Symbol/Symbols/ModuleInitializersTests.cs @@ -173,23 +173,6 @@ namespace System.Runtime.CompilerServices { class ModuleInitializerAttribute : S source, parseOptions: s_parseOptions, options: TestOptions.DebugExe.WithMetadataImportOptions(MetadataImportOptions.All), - symbolValidator: module => - { - var rootModuleType = (TypeSymbol)module.GlobalNamespace.GetMember(""); - var staticConstructor = (PEMethodSymbol)rootModuleType.GetMember(".cctor"); - - Assert.NotNull(staticConstructor); - Assert.Equal(MethodKind.StaticConstructor, staticConstructor.MethodKind); - - var expectedFlags = - MethodAttributes.Private - | MethodAttributes.Static - | MethodAttributes.SpecialName - | MethodAttributes.RTSpecialName - | MethodAttributes.HideBySig; - - Assert.Equal(expectedFlags, staticConstructor.Flags); - }, expectedOutput: "1234"); } @@ -231,23 +214,6 @@ namespace System.Runtime.CompilerServices { class ModuleInitializerAttribute : S parseOptions: s_parseOptions, targetFramework: TargetFramework.NetStandardLatest, options: TestOptions.DebugExe.WithMetadataImportOptions(MetadataImportOptions.All), - symbolValidator: module => - { - var rootModuleType = (TypeSymbol)module.GlobalNamespace.GetMember(""); - var staticConstructor = (PEMethodSymbol)rootModuleType.GetMember(".cctor"); - - Assert.NotNull(staticConstructor); - Assert.Equal(MethodKind.StaticConstructor, staticConstructor.MethodKind); - - var expectedFlags = - MethodAttributes.Private - | MethodAttributes.Static - | MethodAttributes.SpecialName - | MethodAttributes.RTSpecialName - | MethodAttributes.HideBySig; - - Assert.Equal(expectedFlags, staticConstructor.Flags); - }, expectedOutput: !ExecutionConditionUtil.IsMonoOrCoreClr ? null : "1234", verify: !ExecutionConditionUtil.IsMonoOrCoreClr ? Verification.Skipped : Verification.Passes); } @@ -309,58 +275,9 @@ class C4 new[] { source1, source2, source3 }, parseOptions: s_parseOptions, options: TestOptions.DebugExe.WithMetadataImportOptions(MetadataImportOptions.All), - symbolValidator: module => - { - var rootModuleType = (TypeSymbol)module.GlobalNamespace.GetMember(""); - var staticConstructor = (PEMethodSymbol)rootModuleType.GetMember(".cctor"); - - Assert.NotNull(staticConstructor); - Assert.Equal(MethodKind.StaticConstructor, staticConstructor.MethodKind); - - var expectedFlags = - MethodAttributes.Private - | MethodAttributes.Static - | MethodAttributes.SpecialName - | MethodAttributes.RTSpecialName - | MethodAttributes.HideBySig; - - Assert.Equal(expectedFlags, staticConstructor.Flags); - }, expectedOutput: "123456"); } - [Fact] - public void NonNullableField_01() - { - const string text = @" -#nullable enable - -using System.Runtime.CompilerServices; - -class C -{ - static string s1; - - [ModuleInitializer] - static void Init() - { - s1 = """"; - } -} - -namespace System.Runtime.CompilerServices { class ModuleInitializerAttribute : System.Attribute { } } -"; - - var comp = CreateCompilation(text, parseOptions: s_parseOptions); - comp.VerifyDiagnostics( - // (8,19): warning CS8618: Non-nullable field 's1' is uninitialized. Consider declaring the field as nullable. - // static string s1; - Diagnostic(ErrorCode.WRN_UninitializedNonNullableField, "s1").WithArguments("field", "s1").WithLocation(8, 19), - // (8,19): warning CS0414: The field 'C.s1' is assigned but its value is never used - // static string s1; - Diagnostic(ErrorCode.WRN_UnreferencedFieldAssg, "s1").WithArguments("C.s1").WithLocation(8, 19)); - } - [Fact] public void StaticConstructor_Ordering() { @@ -425,7 +342,7 @@ public void StaticConstructor_DefaultInitializer_SameType() class C { - internal static string s1 = null!; + internal static string s1 = null; [ModuleInitializer] internal static void Init() @@ -460,6 +377,55 @@ void validator(ModuleSymbol module) } } + [Fact] + public void StaticConstructor_EffectingInitializer_SameType() + { + const string text = @" +using System; +using System.Runtime.CompilerServices; + +class C +{ + internal static int i = InitField(); + + internal static int InitField() + { + Console.Write(1); + return -1; + } + + [ModuleInitializer] + internal static void Init() + { + i = 2; + } + + static void Main() + { + Console.Write(i); + } +} + +namespace System.Runtime.CompilerServices { class ModuleInitializerAttribute : System.Attribute { } } +"; + var verifier = CompileAndVerify( + text, + parseOptions: s_parseOptions, + options: TestOptions.DebugExe.WithMetadataImportOptions(MetadataImportOptions.All), + expectedOutput: "12", + symbolValidator: validator); + verifier.VerifyDiagnostics(); + + void validator(ModuleSymbol module) + { + var cType = module.ContainingAssembly.GetTypeByMetadataName("C"); + Assert.NotNull(cType.GetMember(".cctor")); + + var moduleType = module.ContainingAssembly.GetTypeByMetadataName(""); + Assert.NotNull(moduleType.GetMember(".cctor")); + } + } + [Fact] public void StaticConstructor_DefaultInitializer_OtherType() { @@ -478,7 +444,7 @@ internal static void Init() class C2 { - internal static string s1 = null!; + internal static string s1 = null; static void Main() { @@ -506,5 +472,57 @@ void validator(ModuleSymbol module) Assert.NotNull(moduleType.GetMember(".cctor")); } } + + [Fact] + public void StaticConstructor_EffectingInitializer_OtherType() + { + const string text = @" +using System; +using System.Runtime.CompilerServices; + +class C1 +{ + [ModuleInitializer] + internal static void Init() + { + C2.i = 2; + } +} + +class C2 +{ + internal static int i = InitField(); + + static int InitField() + { + Console.Write(1); + return -1; + } + + static void Main() + { + Console.Write(i); + } +} + +namespace System.Runtime.CompilerServices { class ModuleInitializerAttribute : System.Attribute { } } +"; + var verifier = CompileAndVerify( + text, + parseOptions: s_parseOptions, + options: TestOptions.DebugExe.WithMetadataImportOptions(MetadataImportOptions.All), + expectedOutput: "12", + symbolValidator: validator); + verifier.VerifyDiagnostics(); + + void validator(ModuleSymbol module) + { + var c2Type = module.ContainingAssembly.GetTypeByMetadataName("C2"); + Assert.NotNull(c2Type.GetMember(".cctor")); + + var moduleType = module.ContainingAssembly.GetTypeByMetadataName(""); + Assert.NotNull(moduleType.GetMember(".cctor")); + } + } } } diff --git a/src/Compilers/Core/Portable/InternalUtilities/ConcurrentSet.cs b/src/Compilers/Core/Portable/InternalUtilities/ConcurrentSet.cs index bf23405bf7252..d36f8541f7a97 100644 --- a/src/Compilers/Core/Portable/InternalUtilities/ConcurrentSet.cs +++ b/src/Compilers/Core/Portable/InternalUtilities/ConcurrentSet.cs @@ -178,7 +178,13 @@ void ICollection.Add(T item) public void CopyTo(T[] array, int arrayIndex) { - throw new NotImplementedException(); + // PERF: Do not use dictionary.Keys here because that creates a snapshot + // of the collection resulting in a List allocation. + // Instead, enumerate the underlying dictionary and copy over the keys. + foreach (var kvp in _dictionary) + { + array[arrayIndex++] = kvp.Key; + } } } } From 0078cf20805af870777b52be6bab49ee8c650607 Mon Sep 17 00:00:00 2001 From: Rikki Gibson Date: Tue, 5 May 2020 14:29:48 -0700 Subject: [PATCH 6/8] Address feedback from previous PR --- .../CSharp/Portable/Compilation/CSharpCompilation.cs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs b/src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs index 9b61c902c06ba..a5fbf6b653c21 100644 --- a/src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs +++ b/src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs @@ -2811,7 +2811,10 @@ internal override bool CompileMethods( filterOpt: filterOpt, cancellationToken: cancellationToken); - GenerateModuleInitializer(moduleBeingBuilt, methodBodyDiagnosticBag); + if (!hasDeclarationErrors && !CommonCompiler.HasUnsuppressableErrors(methodBodyDiagnosticBag)) + { + GenerateModuleInitializer(moduleBeingBuilt, methodBodyDiagnosticBag); + } bool hasMethodBodyError = !FilterAndAppendAndFreeDiagnostics(diagnostics, ref methodBodyDiagnosticBag); From 6d8582a68fd6745aab363ffb349c31711e5bd42b Mon Sep 17 00:00:00 2001 From: Rikki Gibson Date: Wed, 6 May 2020 10:39:29 -0700 Subject: [PATCH 7/8] Address feedback --- .../Portable/Compilation/LexicalOrderSymbolComparer.cs | 10 ++++++---- .../Test/Symbol/Symbols/ModuleInitializersTests.cs | 3 --- 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/src/Compilers/CSharp/Portable/Compilation/LexicalOrderSymbolComparer.cs b/src/Compilers/CSharp/Portable/Compilation/LexicalOrderSymbolComparer.cs index 72e22335d964d..4039b9ad4c519 100644 --- a/src/Compilers/CSharp/Portable/Compilation/LexicalOrderSymbolComparer.cs +++ b/src/Compilers/CSharp/Portable/Compilation/LexicalOrderSymbolComparer.cs @@ -8,10 +8,11 @@ namespace Microsoft.CodeAnalysis.CSharp { - /// This is an implementation of a special symbol comparer, which is supposed to be used for - /// sorting original definition symbols (explicitly or explicitly declared in source within the same - /// container) in lexical order of their declarations. It will not work on anything that uses non-source locations. - /// + /// + /// This is an implementation of a special symbol comparer, which is supposed to be used for sorting + /// original definition symbols (explicitly or implicitly declared in source within the same compilation) + /// in lexical order of their declarations. It will not work on anything that uses non-source locations. + /// internal class LexicalOrderSymbolComparer : IComparer { public static readonly LexicalOrderSymbolComparer Instance = new LexicalOrderSymbolComparer(); @@ -30,6 +31,7 @@ public int Compare(Symbol x, Symbol y) var xSortKey = x.GetLexicalSortKey(); var ySortKey = y.GetLexicalSortKey(); + Debug.Assert((object)x.DeclaringCompilation == y.DeclaringCompilation); comparison = LexicalSortKey.Compare(xSortKey, ySortKey); if (comparison != 0) diff --git a/src/Compilers/CSharp/Test/Symbol/Symbols/ModuleInitializersTests.cs b/src/Compilers/CSharp/Test/Symbol/Symbols/ModuleInitializersTests.cs index 9363240ba843b..07354d26d4399 100644 --- a/src/Compilers/CSharp/Test/Symbol/Symbols/ModuleInitializersTests.cs +++ b/src/Compilers/CSharp/Test/Symbol/Symbols/ModuleInitializersTests.cs @@ -172,7 +172,6 @@ namespace System.Runtime.CompilerServices { class ModuleInitializerAttribute : S CompileAndVerify( source, parseOptions: s_parseOptions, - options: TestOptions.DebugExe.WithMetadataImportOptions(MetadataImportOptions.All), expectedOutput: "1234"); } @@ -213,7 +212,6 @@ namespace System.Runtime.CompilerServices { class ModuleInitializerAttribute : S source, parseOptions: s_parseOptions, targetFramework: TargetFramework.NetStandardLatest, - options: TestOptions.DebugExe.WithMetadataImportOptions(MetadataImportOptions.All), expectedOutput: !ExecutionConditionUtil.IsMonoOrCoreClr ? null : "1234", verify: !ExecutionConditionUtil.IsMonoOrCoreClr ? Verification.Skipped : Verification.Passes); } @@ -274,7 +272,6 @@ class C4 CompileAndVerify( new[] { source1, source2, source3 }, parseOptions: s_parseOptions, - options: TestOptions.DebugExe.WithMetadataImportOptions(MetadataImportOptions.All), expectedOutput: "123456"); } From 4dd65de0b2c60a2bbf4a277212da78b841aacc5c Mon Sep 17 00:00:00 2001 From: Rikki Gibson Date: Wed, 6 May 2020 11:47:19 -0700 Subject: [PATCH 8/8] Address feedback --- .../Core/Portable/InternalUtilities/ConcurrentSet.cs | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/Compilers/Core/Portable/InternalUtilities/ConcurrentSet.cs b/src/Compilers/Core/Portable/InternalUtilities/ConcurrentSet.cs index d36f8541f7a97..b8a194ba97a43 100644 --- a/src/Compilers/Core/Portable/InternalUtilities/ConcurrentSet.cs +++ b/src/Compilers/Core/Portable/InternalUtilities/ConcurrentSet.cs @@ -4,7 +4,6 @@ #nullable enable -using System; using System.Collections; using System.Collections.Concurrent; using System.Collections.Generic; @@ -180,10 +179,10 @@ public void CopyTo(T[] array, int arrayIndex) { // PERF: Do not use dictionary.Keys here because that creates a snapshot // of the collection resulting in a List allocation. - // Instead, enumerate the underlying dictionary and copy over the keys. - foreach (var kvp in _dictionary) + // Instead, enumerate the set and copy over the elements. + foreach (var element in this) { - array[arrayIndex++] = kvp.Key; + array[arrayIndex++] = element; } } }