Module initializers deterministic ordering#43964
Module initializers deterministic ordering#43964RikkiGibson merged 9 commits intodotnet:features/module-initializersfrom
Conversation
|
@RikkiGibson It looks like there are legitimate test failures #Closed |
|
|
||
| // 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. |
There was a problem hiding this comment.
// 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
There was a problem hiding this comment.
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:
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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)
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 |
| 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); | ||
| }, |
There was a problem hiding this comment.
Should this be repeated? I'm not repeating it in any of the new tests in the diagnostics PR.
There was a problem hiding this comment.
(This includes the options: TestOptions.DebugExe.WithMetadataImportOptions(MetadataImportOptions.All), line too)
There was a problem hiding this comment.
It seems fine to extract the validator method and reuse it.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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 was a problem hiding this comment.
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)
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) |
| } | ||
|
|
||
| [Fact] | ||
| public void NonNullableField_01() |
There was a problem hiding this comment.
NonNullableField_01 [](start = 20, length = 19)
What is interesting about this scenario? #Closed
There was a problem hiding this comment.
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.
| "; | ||
|
|
||
| var comp = CreateCompilation(text, parseOptions: s_parseOptions); | ||
| comp.VerifyDiagnostics( |
There was a problem hiding this comment.
VerifyDiagnostics [](start = 17, length = 17)
VerifyEmitDiagnostics? Or better execute and observe if this scenario is really interesting. #Closed
|
|
||
| class C | ||
| { | ||
| internal static string s1 = null!; |
There was a problem hiding this comment.
null! [](start = 32, length = 5)
I think it would be interesting to observe what happens when initializer is not optimized away. #Closed
|
Done with review pass (iteration 5) #Closed |
|
|
||
| // PROTOTYPE(module-initializers): require deterministic order | ||
| foreach (var method in _moduleInitializerMethods) | ||
| foreach (MethodSymbol method in _moduleInitializerMethods.OrderBy<MethodSymbol>(LexicalOrderSymbolComparer.Instance)) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Yes, reading the source indicates this and ModuleInitializersTests.MultipleInitializers_MultipleFiles seems to demonstrate this. I will fix up the documentation.
| source, | ||
| parseOptions: s_parseOptions, | ||
| targetFramework: TargetFramework.NetStandardLatest, | ||
| options: TestOptions.DebugExe.WithMetadataImportOptions(MetadataImportOptions.All), |
There was a problem hiding this comment.
I don't believe these WithMetadataImportOptions lines serve a purpose unless a symbolValidator is provided.
| // 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) |
There was a problem hiding this comment.
_dictionary [](start = 32, length = 11)
Can we simply use foreach over this? #Closed
Related to #40500
/cc @jnm2 @AlekseyTs
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'.