Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Module initializers deterministic ordering #43964

Merged
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Contributor

@AlekseyTs AlekseyTs May 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// 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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. add a simple keySelector: .OrderBy(m => m, LexicalOrderSymbolComparer.Instance)
  2. 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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

foreach (MethodSymbol method in _moduleInitializerMethods.OrderBy(LexicalOrderSymbolComparer.Instance))
{
ilBuilder.EmitOpCode(ILOpCode.Call, stackAdjustment: 0);

Expand Down
190 changes: 190 additions & 0 deletions src/Compilers/CSharp/Test/Symbol/Symbols/ModuleInitializersTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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("<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);
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

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),
Copy link
Contributor

@jnm2 jnm2 May 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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);
},
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("<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);
},
expectedOutput: "123456");
}
}
}