Emit forwarded types in a deterministic order #16303

OmarTawfik · 2017-01-06T21:29:47Z

Fixes #11990

Customer and Scenario Info
Who is impacted by this bug: customers trying to use the deterministic output feature of compilers
What is the customer scenario and impact of the bug: two identical builds might not produce the same bits.
What is the workaround: none.
How was the bug found: Manual code inspection.
If this fix is for a regression - what had regressed, when was the regression introduced, and why was the regression originally missed: No.
What is a Customer Ready description of change should we need to document this: builds are now one step closer to be 100% deterministic (see #372).

Bug Fix Details
Overview of the fix: Enumerating a hashset in an ordered way when emitting forwarded types in an assembly.
Which VS products, SKUs, or components need the fix: all SKUs shipping Roslyn compilers.
What branch does the fix go into: lab/ml.
What feature areas are impacted by the fix: Roslyn compilers.
What is the risk of the code change: none.
Does this change have impact on anything else (e.g. setup, perf, stress, ux, visual freeze, go live, breaking change, samples, etc): no.
How is this fix rolling forward in the next major release / sprintly train (e.g. Dev15): RTM.

Validation
What testing/validation was done on the fix, and what were the results: manual testing an unit tests were included in the PR.
Which engineering partner teams have signed off on the fix: @dotnet/roslyn-compiler
Who code reviewed the fix: @AlekseyTs @cston

Additional information during Divisional Escrow Mode
Which Engineering DIRECTOR is accountable for the code change and quality: @MattGertz
What are you doing to ensure that this bug does not happen again: not a regression.
Which part of the D15 Escrow Bar does this fall under?: RTM ASK MODE

OmarTawfik · 2017-01-06T22:02:48Z

@dotnet/roslyn-compiler

cston · 2017-01-06T22:07:18Z

Please add a test if possible.

cston · 2017-01-06T22:08:42Z

src/Compilers/CSharp/Portable/Emitter/Model/PEModuleBuilder.cs

@@ -557,7 +557,10 @@ private void ReportExportedTypeNameCollisions(ImmutableArray<Cci.ExportedType> e
                // (type, index of the parent exported type in builder, or -1 if the type is a top-level type)
                var stack = ArrayBuilder<(NamedTypeSymbol type, int parentIndex)>.GetInstance();

-                foreach (NamedTypeSymbol forwardedType in wellKnownAttributeData.ForwardedTypes)
+                // Hashset enumeration is not guarenteed to be deterministic. Emitting in the order of fully qualified names.


Typo: guaranteed

AlekseyTs · 2017-01-06T22:32:16Z

src/Compilers/CSharp/Portable/Emitter/Model/PEModuleBuilder.cs

@@ -557,7 +557,10 @@ private void ReportExportedTypeNameCollisions(ImmutableArray<Cci.ExportedType> e
                // (type, index of the parent exported type in builder, or -1 if the type is a top-level type)
                var stack = ArrayBuilder<(NamedTypeSymbol type, int parentIndex)>.GetInstance();

-                foreach (NamedTypeSymbol forwardedType in wellKnownAttributeData.ForwardedTypes)
+                // Hashset enumeration is not guarenteed to be deterministic. Emitting in the order of fully qualified names.
+                var orderedForwardedTypes = wellKnownAttributeData.ForwardedTypes.OrderBy(t => t.ToDisplayString(SymbolDisplayFormat.FullyQualifiedFormat));


t.ToDisplayString(SymbolDisplayFormat.FullyQualifiedFormat) [](start = 95, length = 59)

t.ToDisplayString(SymbolDisplayFormat.FullyQualifiedFormat) [](start = 95, length = 59)

We probably should get display string from OriginalDefinition and use metadata format, for example "...Dictionary`2" vs. "...Dictionary<K, V>". #Closed

@AlekseyTs you mean SymbolDisplayFormat.QualifiedNameArityFormat?

you mean SymbolDisplayFormat.QualifiedNameArityFormat?

Yes. #Closed

Isn't this unnecessarily building display strings? Don't we have any other symbol comparer?

AlekseyTs · 2017-01-06T22:33:44Z

Similar issue should be addressed for VB. There should be test that verifies the order. #Closed

Emit forwarded types in a deterministic order

OmarTawfik · 2017-01-10T17:51:52Z

@cston @AlekseyTs comments addressed.

cston · 2017-01-10T21:11:07Z

src/Compilers/CSharp/Test/Emit/Emit/DeterministicTests.cs

@@ -184,6 +185,54 @@ public void TestWriteOnlyStream()
            compilation.Emit(output);
        }

+        [Fact, WorkItem(11990, "https://github.com/dotnet/roslyn/issues/11990")]
+        public void ForwardedTypesAreEmmittedInADeterministicOrder()


Typo: Emitted

cston · 2017-01-10T21:14:05Z

src/Compilers/CSharp/Test/Emit/Emit/DeterministicTests.cs

+                for (var i = 1; i <= generatedTypes; i++)
+                {
+                    forwardingCode.AppendLine($"[assembly: TypeForwardedTo(typeof(ForwardedToNamespace.T{i}))]");
+                }


forwardingCode can be created once, outside of local function.

AlekseyTs · 2017-01-10T21:16:23Z

src/Compilers/CSharp/Test/Emit/Emit/DeterministicTests.cs

+            Assert.Equal(generatedTypes, baseline.Length);
+
+            var reference = compileAndGetExportedTypes().ToArray();
+            Assert.Equal(baseline, reference);


Assert.Equal(baseline, reference); [](start = 12, length = 34)

It feels like the test would be better if TypeForwardedTo attributes would not be in sorted order in source and we simply verify that ExportedTypes are sorted in the way we expect. #Closed

AlekseyTs · 2017-01-10T21:16:53Z

src/Compilers/CSharp/Test/Emit/Emit/DeterministicTests.cs

+            forwardedToCode.AppendLine("namespace ForwardedToNamespace {");
+            for (var i = 1; i <= generatedTypes; i++)
+            {
+                forwardedToCode.AppendLine($"public class T{i} {{}}");


T{i} [](start = 58, length = 4)

Consider incorporating generic types into the test. #Closed

AlekseyTs · 2017-01-10T21:18:14Z

src/Compilers/CSharp/Test/Emit/Emit/DeterministicTests.cs

+            const int generatedTypes = 100;
+
+            var forwardedToCode = new StringBuilder();
+            forwardedToCode.AppendLine("namespace ForwardedToNamespace {");


ForwardedToNamespace [](start = 50, length = 20)

It would be good to add variations in namespace name into the mix. #Closed

AlekseyTs · 2017-01-10T21:21:24Z

src/Compilers/VisualBasic/Test/Emit/Emit/DeterministicTests.vb

+                    assemblyName:="ForwardingAssembly",
+                    source:=String.Empty,
+                    options:=New VisualBasicCompilationOptions(OutputKind.DynamicallyLinkedLibrary),
+                    references:={forwardingNetModule.EmitToImageReference(), forwardedToCompilation.EmitToImageReference()})


forwardedToCompilation.EmitToImageReference() [](start = 77, length = 45)

Doing this twice, consider caching result and reusing.

It looks like this issue is not addressed.

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

AlekseyTs · 2017-01-10T21:21:48Z

src/Compilers/VisualBasic/Test/Emit/Emit/DeterministicTests.vb

+            Assert.Equal(generatedTypes, baseline.Length)
+
+            Dim reference = compileAndGetExportedTypes().ToArray()
+            Assert.Equal(baseline, reference)


Assert.Equal(baseline, reference) [](start = 12, length = 33)

Same comments as for C# test. #Closed

cston · 2017-01-10T21:26:18Z

src/Compilers/VisualBasic/Portable/Emit/PEModuleBuilder.vb

@@ -517,7 +517,10 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.Emit
                ' (type, index of the parent exported type in builder, or -1 if the type is a top-level type)
                Dim stack = ArrayBuilder(Of (type As NamedTypeSymbol, parentIndex As Integer)).GetInstance()

-                For Each forwardedType As NamedTypeSymbol In wellKnownAttributeData.ForwardedTypes
+                ' Hashset enumeration Is Not guaranteed to be deterministic. Emitting in the order of fully qualified names.
+                Dim orderedForwardedTypes = wellKnownAttributeData.ForwardedTypes.OrderBy(Function(t) t.OriginalDefinition.ToDisplayString(SymbolDisplayFormat.QualifiedNameArityFormat))


Lowercase for Is Not.

OmarTawfik · 2017-01-12T01:30:34Z

@AlekseyTs @cston tests updated to include multiple namespaces and generics. Typos fixed.

cston · 2017-01-12T15:43:10Z

src/Compilers/CSharp/Test/Emit/Emit/DeterministicTests.cs

@@ -12,6 +12,7 @@
 using Roslyn.Test.Utilities;
 using Microsoft.CodeAnalysis.Emit;
 using System.Diagnostics;
+using System.Text;


Is this namespace used? Same question for VB.

cston · 2017-01-12T15:52:44Z

src/Compilers/CSharp/Test/Emit/Emit/DeterministicTests.cs

+                        var namespaceName = block.MetadataReader.GetString(type.Namespace);
+
+                        metadataFullNames.Add($"{namespaceName}.{typeName}");
+                    }


Minor point: Consider extracting a helper method shared with VB. See MetadataValidation.GetFullTypeNames() for example.

@cston done.

cston · 2017-01-12T15:53:20Z

LGTM

AlekseyTs · 2017-01-12T20:52:41Z

src/Compilers/CSharp/Test/Emit/Emit/DeterministicTests.cs

+    public class Type1 {}
+}
+namespace Namespace3 {
+    public class GenericType2<T, U> {}


public class GenericType2<T, U> {} [](start = 4, length = 34)

public class GenericType2<T, U> {} [](start = 4, length = 34)

It would be good to have types that differ only by generic arity. #Closed

OmarTawfik · 2017-01-12T22:16:37Z

@AlekseyTs do I've a sign off?

AlekseyTs · 2017-01-12T22:48:51Z

LGTM

OmarTawfik · 2017-01-13T00:15:36Z

@MattGertz @natidea can I get approval for RTM?

MattGertz · 2017-01-13T03:27:29Z

@OmarTawfik Template is missing.

OmarTawfik · 2017-01-13T19:52:40Z

@MattGertz done.

MattGertz · 2017-01-13T19:59:35Z

RTW.

OmarTawfik · 2017-01-13T20:05:17Z

@jaredpar I see the milestone for this bug changed. Should I rebase to post-dev15?

jaredpar · 2017-01-13T20:17:26Z

@OmarTawfik no this needs to target master / RTW

OmarTawfik added the Area-Compilers label Jan 6, 2017

dnfclas added the cla-already-signed label Jan 6, 2017

cston reviewed Jan 6, 2017

View reviewed changes

AlekseyTs reviewed Jan 6, 2017

View reviewed changes

Omar Tawfik added 2 commits January 6, 2017 15:55

Emit forwarded types in a deterministic order

db133b4

Emit forwarded types in a deterministic order

Added tests

94a1519

cston reviewed Jan 10, 2017

View reviewed changes

AlekseyTs reviewed Jan 10, 2017

View reviewed changes

cston reviewed Jan 10, 2017

View reviewed changes

cston reviewed Jan 12, 2017

View reviewed changes

AlekseyTs reviewed Jan 12, 2017

View reviewed changes

Addressed CR comments

4bc408f

MattGertz added this to the 2.0 (RTM) milestone Jan 13, 2017

MattGertz added Needs Shiproom Approval Proactive merge allowed labels Jan 13, 2017

OmarTawfik merged commit d77c3e2 into dotnet:master Jan 13, 2017

OmarTawfik deleted the fix-11990-deterministic-forwarded-types-emit branch January 13, 2017 20:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Emit forwarded types in a deterministic order #16303

Emit forwarded types in a deterministic order #16303

OmarTawfik commented Jan 6, 2017 •

edited

Loading

OmarTawfik commented Jan 6, 2017

cston commented Jan 6, 2017

cston Jan 6, 2017

AlekseyTs Jan 6, 2017 •

edited

Loading

OmarTawfik Jan 6, 2017

AlekseyTs Jan 6, 2017 •

edited

Loading

tmat Jan 7, 2017 •

edited

Loading

AlekseyTs commented Jan 6, 2017 •

edited

Loading

OmarTawfik commented Jan 10, 2017

cston Jan 10, 2017

cston Jan 10, 2017

AlekseyTs Jan 10, 2017 •

edited

Loading

AlekseyTs Jan 10, 2017 •

edited

Loading

AlekseyTs Jan 10, 2017 •

edited

Loading

AlekseyTs Jan 10, 2017

AlekseyTs Jan 12, 2017

AlekseyTs Jan 10, 2017 •

edited

Loading

cston Jan 10, 2017

OmarTawfik commented Jan 12, 2017

cston Jan 12, 2017

cston Jan 12, 2017

OmarTawfik Jan 12, 2017

cston commented Jan 12, 2017

AlekseyTs Jan 12, 2017 •

edited

Loading

OmarTawfik commented Jan 12, 2017

AlekseyTs commented Jan 12, 2017

OmarTawfik commented Jan 13, 2017

MattGertz commented Jan 13, 2017

OmarTawfik commented Jan 13, 2017

MattGertz commented Jan 13, 2017

OmarTawfik commented Jan 13, 2017

jaredpar commented Jan 13, 2017

Emit forwarded types in a deterministic order #16303

Emit forwarded types in a deterministic order #16303

Conversation

OmarTawfik commented Jan 6, 2017 • edited Loading

OmarTawfik commented Jan 6, 2017

cston commented Jan 6, 2017

Choose a reason for hiding this comment

AlekseyTs Jan 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Jan 6, 2017 • edited Loading

Choose a reason for hiding this comment

tmat Jan 7, 2017 • edited Loading

Choose a reason for hiding this comment

AlekseyTs commented Jan 6, 2017 • edited Loading

OmarTawfik commented Jan 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Jan 10, 2017 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Jan 10, 2017 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Jan 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Jan 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OmarTawfik commented Jan 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston commented Jan 12, 2017

AlekseyTs Jan 12, 2017 • edited Loading

Choose a reason for hiding this comment

OmarTawfik commented Jan 12, 2017

AlekseyTs commented Jan 12, 2017

OmarTawfik commented Jan 13, 2017

MattGertz commented Jan 13, 2017

OmarTawfik commented Jan 13, 2017

MattGertz commented Jan 13, 2017

OmarTawfik commented Jan 13, 2017

jaredpar commented Jan 13, 2017

OmarTawfik commented Jan 6, 2017 •

edited

Loading

AlekseyTs Jan 6, 2017 •

edited

Loading

AlekseyTs Jan 6, 2017 •

edited

Loading

tmat Jan 7, 2017 •

edited

Loading

AlekseyTs commented Jan 6, 2017 •

edited

Loading

AlekseyTs Jan 10, 2017 •

edited

Loading

AlekseyTs Jan 10, 2017 •

edited

Loading

AlekseyTs Jan 10, 2017 •

edited

Loading

AlekseyTs Jan 10, 2017 •

edited

Loading

AlekseyTs Jan 12, 2017 •

edited

Loading