Module initializers deterministic ordering #43964

RikkiGibson · 2020-05-04T21:19:08Z

Related to #40500

I looked around a bit at how the compiler merges partial declarations and it didn't feel very applicable to this situation. LexicalOrderSymbolComparer seems to be a good answer. This approach assumes we only generate the module initializer once, since it would repeat the work of sorting each time we call 'GenerateModuleInitializer'.

AlekseyTs · 2020-05-05T17:48:05Z

@RikkiGibson It looks like there are legitimate test failures #Closed

…into mi-multiple

AlekseyTs · 2020-05-05T17:58:28Z

src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs

@@ -2832,8 +2832,8 @@ 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)
+                // note that we are casting the Symbol returned by the OrderBy enumerable to MethodSymbol, expecting it will succeed.


// note that we are casting the Symbol returned by the OrderBy enumerable to MethodSymbol, expecting it will succeed. [](start = 16, length = 117)

I am not sure why did we end up with Symbol after OrderBy. It looks like the start with a collection of MethodSymbols, LexicalOrderSymbolComparer implements IComparer<Symbol>, which is variant and for our case is equivalent `IComparer<MethodSymbol>. I am not suggesting to replace explicit type with var, in fact I prefer explicit types. I am just want to make sure that the right API is used. What OrderBy are we calling here? #Closed

I found it a little surprising as well but didn't look closely. Now I see this overload actually comes from Roslyn. We call this method:

roslyn/src/Compilers/Core/Portable/InternalUtilities/EnumerableExtensions.cs

Line 345 in f8009ac

public static IOrderedEnumerable<T> OrderBy<T>(this IEnumerable<T> source, IComparer<T>? comparer)

Maybe we should change this to two type parameters which are constrained to have an inheritance relationship so we support an IComparer<in T> as expected.

I am seeing some surprising behavior when I change this code to either:

add a simple keySelector: .OrderBy(m => m, LexicalOrderSymbolComparer.Instance)

add an explicit type argument: .OrderBy<MethodSymbol>(LexicalOrderSymbolComparer.Instance)

Both cases cause several tests to crash:

Error Message: System.NotImplementedException : The method or operation is not implemented. Stack Trace: at Roslyn.Utilities.ConcurrentSet`1.CopyTo(T[] array, Int32 arrayIndex) in C:\Users\rikki\src\roslyn\src\Compilers\Core\Portable\InternalUtilities\ConcurrentSet.cs:line 181 at System.Linq.Buffer`1..ctor(IEnumerable`1 source) at System.Linq.OrderedEnumerable`1.<GetEnumerator>d__1.MoveNext() at Microsoft.CodeAnalysis.CSharp.CSharpCompilation.GenerateModuleInitializer(PEModuleBuilder moduleBeingBuilt, DiagnosticBag methodBodyDiagnosticBag) in C:\Users\rikki\src\roslyn\src\Compilers\CSharp\Portable\Compilation\CSharpCompilation.cs:line 2836 at Microsoft.CodeAnalysis.CSharp.CSharpCompilation.CompileMethods(CommonPEModuleBuilder moduleBuilder, Boolean emittingPdb, Boolean emitMetadataOnly, Boolean emitTestCoverageData, DiagnosticBag diagnostics, Predicate`1 filterOpt, CancellationToken cancellationToken) in C:\Users\rikki\src\roslyn\src\Compilers\CSharp\Portable\Compilation\CSharpCompilation.cs:line 2814 at Microsoft.CodeAnalysis.Compilation.Emit(Stream peStream, Stream metadataPEStream, Stream pdbStream, Stream xmlDocumentationStream, Stream win32Resources, IEnumerable`1 manifestResources, EmitOptions options, IMethodSymbol debugEntryPoint, Stream sourceLinkStream, IEnumerable`1 embeddedTexts, CompilationTestData testData, CancellationToken cancellationToken) in C:\Users\rikki\src\roslyn\src\Compilers\Core\Portable\Compilation\Compilation.cs:line 2502 at Roslyn.Test.Utilities.RuntimeEnvironmentUtilities.EmitCompilationCore(Compilation compilation, IEnumerable`1 manifestResources, DiagnosticBag diagnostics, CompilationTestData testData, EmitOptions emitOptions) in C:\Users\rikki\src\roslyn\src\Test\Utilities\Portable\Compilation\IRuntimeEnvironment.cs:line 252 at Roslyn.Test.Utilities.RuntimeEnvironmentUtilities.EmitCompilation(Compilation compilation, IEnumerable`1 manifestResources, List`1 dependencies, DiagnosticBag diagnostics, CompilationTestData testData, EmitOptions emitOptions) in C:\Users\rikki\src\roslyn\src\Test\Utilities\Portable\Compilation\IRuntimeEnvironment.cs:line 225 at Roslyn.Test.Utilities.Desktop.DesktopRuntimeEnvironment.Emit(Compilation mainCompilation, IEnumerable`1 manifestResources, EmitOptions emitOptions, Boolean usePdbForDebugging) in C:\Users\rikki\src\roslyn\src\Test\Utilities\Portable\Platform\Desktop\DesktopRuntimeEnvironment.cs:line 202 at Microsoft.CodeAnalysis.Test.Utilities.CompilationVerifier.Emit(IRuntimeEnvironment testEnvironment, IEnumerable`1 manifestResources, EmitOptions emitOptions) in C:\Users\rikki\src\roslyn\src\Test\Utilities\Portable\CompilationVerifier.cs:line 235 at Microsoft.CodeAnalysis.Test.Utilities.CompilationVerifier.Emit(String expectedOutput, Nullable`1 expectedReturnCode, String[] args, IEnumerable`1 manifestResources, EmitOptions emitOptions, Verification peVerify, SignatureDescription[] expectedSignatures) in C:\Users\rikki\src\roslyn\src\Test\Utilities\Portable\CompilationVerifier.cs:line 201 at Microsoft.CodeAnalysis.Test.Utilities.CommonTestBase.Emit(Compilation compilation, IEnumerable`1 dependencies, IEnumerable`1 manifestResources, SignatureDescription[] expectedSignatures, String expectedOutput, Nullable`1 expectedReturnCode, String[] args, Action`1 assemblyValidator, Action`1 symbolValidator, EmitOptions emitOptions, Verification verify) in C:\Users\rikki\src\roslyn\src\Test\Utilities\Portable\CommonTestBase.cs:line 156 at Microsoft.CodeAnalysis.Test.Utilities.CommonTestBase.CompileAndVerifyCommon(Compilation compilation, IEnumerable`1 manifestResources, IEnumerable`1 dependencies, Action`1 sourceSymbolValidator, Action`1 assemblyValidator, Action`1 symbolValidator, SignatureDescription[] expectedSignatures, String expectedOutput, Nullable`1 expectedReturnCode, String[] args, EmitOptions emitOptions, Verification verify) in C:\Users\rikki\src\roslyn\src\Test\Utilities\Portable\CommonTestBase.cs:line 70 at Microsoft.CodeAnalysis.CSharp.Test.Utilities.CSharpTestBase.CompileAndVerify(Compilation compilation, IEnumerable`1 manifestResources, IEnumerable`1 dependencies, Action`1 sourceSymbolValidator, Action`1 validator, Action`1 symbolValidator, SignatureDescription[] expectedSignatures, String expectedOutput, Nullable`1 expectedReturnCode, String[] args, EmitOptions emitOptions, Verification verify) in C:\Users\rikki\src\roslyn\src\Compilers\Test\Utilities\CSharp\CSharpTestBase.cs:line 816 at Microsoft.CodeAnalysis.CSharp.Test.Utilities.CSharpTestBase.CompileAndVerify(CSharpTestSource source, IEnumerable`1 references, IEnumerable`1 manifestResources, IEnumerable`1 dependencies, Action`1 sourceSymbolValidator, Action`1 assemblyValidator, Action`1 symbolValidator, SignatureDescription[] expectedSignatures, String expectedOutput, Nullable`1 expectedReturnCode, String[] args, CSharpCompilationOptions options, CSharpParseOptions parseOptions, EmitOptions emitOptions, TargetFramework targetFramework, Verification verify) in C:\Users\rikki\src\roslyn\src\Compilers\Test\Utilities\CSharp\CSharpTestBase.cs:line 775 at Microsoft.CodeAnalysis.CSharp.UnitTests.Symbols.ModuleInitializersTests.MultipleInitializers_SingleFile() in C:\Users\rikki\src\roslyn\src\Compilers\CSharp\Test\Symbol\Symbols\ModuleInitializersTests.cs:line 172

Is there something about the type arguments or conversions in use that is making linq decide to perform the sort in a different way?

Is there something about the type arguments or conversions in use that is making linq decide to perform the sort in a different way?

It looks like we need to properly implement the API that throws, it is our code that throws. Linq might attempt to apply some optimizations and the types involved might affect that and affect what code path is taken.

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

Maybe we should change this to two type parameters which are constrained to have an inheritance relationship so we support an IComparer as expected.

There is only one type parameter and the inference result is probably expected, we infer from two interfaces with different variance. It isn't quite obvious what direction should be taken.

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

AlekseyTs · 2020-05-05T18:01:13Z

This approach assumes we only generate the module initializer once, since it would repeat the work of sorting each time we call 'GenerateModuleInitializer'.

I think it is fine to not add any additional caching around the sort operation. I doubt that having more than 10 module initializer will be a common scenario. #Closed

jnm2 · 2020-05-05T18:05:46Z

src/Compilers/CSharp/Test/Symbol/Symbols/ModuleInitializersTests.cs

+                symbolValidator: module =>
+                {
+                    var rootModuleType = (TypeSymbol)module.GlobalNamespace.GetMember("<Module>");
+                    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);
+                },


Should this be repeated? I'm not repeating it in any of the new tests in the diagnostics PR.

(This includes the options: TestOptions.DebugExe.WithMetadataImportOptions(MetadataImportOptions.All), line too)

It seems fine to extract the validator method and reuse it.

Should this be repeated? I'm not repeating it in any of the new tests in the diagnostics PR.

Are you referring to assert for flags? I think it is fine to test that once, but it is your call

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

Not just the flags but the symbol validation itself, since verification is being run. Though I see now that it only runs when ExecutionConditionUtil.IsMonoOrCoreClr.

Not just the flags but the symbol validation itself, since verification is being run. Though I see now that it only runs when ExecutionConditionUtil.IsMonoOrCoreClr.

There is no conditional logic in how we emit metadata for the initializer. I would say testing symbol properties once is sufficient, but, if you'd like, feel free to test in more scenarios.

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

AlekseyTs · 2020-05-05T18:10:48Z

            GenerateModuleInitializer(moduleBeingBuilt, methodBodyDiagnosticBag);

Since you are making changes in this file, could you please implement suggestion from this thread https://github.com/dotnet/roslyn/pull/43281/files#r415088304? #Closed

Refers to: src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs:2814 in 2c82cf8. [](commit_id = 2c82cf8, deletion_comment = False)

AlekseyTs · 2020-05-05T20:22:56Z

src/Compilers/CSharp/Test/Symbol/Symbols/ModuleInitializersTests.cs

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


NonNullableField_01 [](start = 20, length = 19)

What is interesting about this scenario? #Closed

In the test plan there is a suggestion to avoid issuing WRN_UninitializedNonNullableField if a module initializer writes to the field. I added this test while considering that suggestion. See also this discussion. #40500 (comment)

At this point I will probably just delete this particular test as not being related to this PR.

AlekseyTs · 2020-05-05T20:24:48Z

src/Compilers/CSharp/Test/Symbol/Symbols/ModuleInitializersTests.cs

+";
+
+            var comp = CreateCompilation(text, parseOptions: s_parseOptions);
+            comp.VerifyDiagnostics(


VerifyDiagnostics [](start = 17, length = 17)

VerifyEmitDiagnostics? Or better execute and observe if this scenario is really interesting. #Closed

AlekseyTs · 2020-05-05T20:29:42Z

src/Compilers/CSharp/Test/Symbol/Symbols/ModuleInitializersTests.cs

+
+class C
+{
+    internal static string s1 = null!;


null! [](start = 32, length = 5)

I think it would be interesting to observe what happens when initializer is not optimized away. #Closed

AlekseyTs · 2020-05-05T20:31:05Z

Done with review pass (iteration 5) #Closed

jaredpar · 2020-05-06T16:46:09Z

src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs

@@ -2832,8 +2835,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 (MethodSymbol method in _moduleInitializerMethods.OrderBy<MethodSymbol>(LexicalOrderSymbolComparer.Instance))


The documentation for LexicalOrderSymbolComparer indicates it's only supported for Symbol instances that are declared in the same container:

(explicitly or explicitly declared in source within the same container)

Module initializers can be declared across containers though. Have you verified that LexicalOrderSymbolComparer is supported for this scenario? If so can you clean up the documentation to reflect the correctly supported behaviors?

Yes, reading the source indicates this and ModuleInitializersTests.MultipleInitializers_MultipleFiles seems to demonstrate this. I will fix up the documentation.

jnm2 · 2020-05-06T16:52:51Z

src/Compilers/CSharp/Test/Symbol/Symbols/ModuleInitializersTests.cs

+                source,
+                parseOptions: s_parseOptions,
+                targetFramework: TargetFramework.NetStandardLatest,
+                options: TestOptions.DebugExe.WithMetadataImportOptions(MetadataImportOptions.All),


I don't believe these WithMetadataImportOptions lines serve a purpose unless a symbolValidator is provided.

AlekseyTs · 2020-05-06T18:04:15Z

src/Compilers/Core/Portable/InternalUtilities/ConcurrentSet.cs

+            // PERF: Do not use dictionary.Keys here because that creates a snapshot
+            // of the collection resulting in a List<T> allocation.
+            // Instead, enumerate the underlying dictionary and copy over the keys.
+            foreach (var kvp in _dictionary)


_dictionary [](start = 32, length = 11)

Can we simply use foreach over this? #Closed

AlekseyTs

LGTM (iteration 9)

Support multiple in deterministic order

4c804d2

RikkiGibson added the Area-Compilers label May 4, 2020

RikkiGibson requested a review from a team as a code owner May 4, 2020 21:19

Don't crash in ConcurrentSet.CopyTo

d78f276

Merge remote-tracking branch 'upstream/features/module-initializers' …

0d9bdd9

…into mi-multiple

AlekseyTs reviewed May 5, 2020

View reviewed changes

RikkiGibson added 2 commits May 5, 2020 10:58

Fix test

488e63d

Add various tests

2c82cf8

jnm2 reviewed May 5, 2020

View reviewed changes

AlekseyTs reviewed May 5, 2020

View reviewed changes

RikkiGibson added 2 commits May 5, 2020 14:06

Implement CopyTo. Update tests.

3892e4b

Address feedback from previous PR

0078cf2

jnm2 mentioned this pull request May 6, 2020

Codegen for module initializers feature #43281

Merged

jaredpar reviewed May 6, 2020

View reviewed changes

jnm2 reviewed May 6, 2020

View reviewed changes

Address feedback

6d8582a

AlekseyTs reviewed May 6, 2020

View reviewed changes

Address feedback

4dd65de

AlekseyTs approved these changes May 6, 2020

View reviewed changes

jaredpar approved these changes May 6, 2020

View reviewed changes

RikkiGibson merged commit 00f92ac into dotnet:features/module-initializers May 7, 2020

RikkiGibson deleted the mi-multiple branch May 7, 2020 00:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Module initializers deterministic ordering #43964

Module initializers deterministic ordering #43964

RikkiGibson commented May 4, 2020

AlekseyTs commented May 5, 2020 •

edited

Loading

AlekseyTs May 5, 2020 •

edited

Loading

RikkiGibson May 5, 2020

RikkiGibson May 5, 2020

AlekseyTs May 5, 2020

AlekseyTs May 5, 2020

AlekseyTs commented May 5, 2020 •

edited

Loading

jnm2 May 5, 2020

jnm2 May 5, 2020

RikkiGibson May 5, 2020

AlekseyTs May 5, 2020

jnm2 May 5, 2020

AlekseyTs May 5, 2020

AlekseyTs commented May 5, 2020 •

edited

Loading

AlekseyTs May 5, 2020 •

edited

Loading

RikkiGibson May 5, 2020

AlekseyTs May 5, 2020 •

edited

Loading

AlekseyTs May 5, 2020 •

edited

Loading

AlekseyTs commented May 5, 2020 •

edited

Loading

jaredpar May 6, 2020

RikkiGibson May 6, 2020

jnm2 May 6, 2020 •

edited

Loading

AlekseyTs May 6, 2020 •

edited

Loading

AlekseyTs left a comment

Module initializers deterministic ordering #43964

Module initializers deterministic ordering #43964

Conversation

RikkiGibson commented May 4, 2020

AlekseyTs commented May 5, 2020 • edited Loading

AlekseyTs May 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs commented May 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs commented May 5, 2020 • edited Loading

AlekseyTs May 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs May 5, 2020 • edited Loading

Choose a reason for hiding this comment

AlekseyTs May 5, 2020 • edited Loading

Choose a reason for hiding this comment

AlekseyTs commented May 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jnm2 May 6, 2020 • edited Loading

Choose a reason for hiding this comment

AlekseyTs May 6, 2020 • edited Loading

Choose a reason for hiding this comment

AlekseyTs left a comment

Choose a reason for hiding this comment

AlekseyTs commented May 5, 2020 •

edited

Loading

AlekseyTs May 5, 2020 •

edited

Loading

AlekseyTs commented May 5, 2020 •

edited

Loading

AlekseyTs commented May 5, 2020 •

edited

Loading

AlekseyTs May 5, 2020 •

edited

Loading

AlekseyTs May 5, 2020 •

edited

Loading

AlekseyTs May 5, 2020 •

edited

Loading

AlekseyTs commented May 5, 2020 •

edited

Loading

jnm2 May 6, 2020 •

edited

Loading

AlekseyTs May 6, 2020 •

edited

Loading