Merge pull request #236 from msgpack/fix/#233

Fix constructor based deserialization code gen #233

Author: yfakariya

CHANGES.txt

@@ -640,3 +640,8 @@ Release 0.9.0 beta2 2017/2/12
   * Fix null items of complex type in List<T> or Dictionary<TKey, TValue> will not be deserialized as null. Issue #211. (from 0.8.1)
   * Fix types which implement IPackable and IUnpackable but do not have any members cannot be serialized. Issue #202
   * Fix Windows Native build error. Issue #206.
+
+Release 0.9.0 RC1 T.B.D.
+
+  BUG FIXES
+  * Fix constructor deserialization fails if the constructor parameters order is not lexical. Issue #233

build/Version.txt

@@ -1 +1 @@
-0.9.0-beta2
+0.9.0-rc1

samples/Samples/Sample07_ConstructorBasedDeserialization.cs

@@ -40,7 +40,7 @@ public void DoConstructorBasedDeserialization()
 		{
 			// As of 0.8, constructor based deserialization is relaxed.
 			// 1. If the type have a constructor with MessagePackDeserializationConstructorAttribute, then it will be used for deserialization.
-			// 2. Else, ff the type have a default public constructor then it will be used for deserialization.
+			// 2. Else, if the type have a default public constructor then it will be used for deserialization.
 			// 3. Otherwise, most parameterful constructor will be used.
 
 			var serializerForSimpleRecord = MessagePackSerializer.Get<MySimpleRecordClass>();

src/MsgPack/Serialization/AbstractSerializers/DynamicUnpackingContext.cs

@@ -845,12 +845,6 @@ private TConstruct EmitObjectUnpackFromCore( TContext context, SerializationTarg
 #endif // FEATURE_TAP
 				;
 
-			int constructorParameterIndex = 0;
-			var fieldNames =
-				targetInfo.IsConstructorDeserialization
-					? targetInfo.DeserializationConstructor.GetParameters().Select( p => p.Name ).ToArray()
-					: targetInfo.Members.Where( m => m.MemberName != null ).Select( m => m.MemberName ).ToArray();
-
 			for ( int i = 0; i < targetInfo.Members.Count; i++ )
 			{
 				var count = i;
@@ -881,33 +875,16 @@ private TConstruct EmitObjectUnpackFromCore( TContext context, SerializationTarg
 				}
 				else
 				{
+					var name = targetInfo.Members[ count ].MemberName;
+					Contract.Assert( !String.IsNullOrEmpty( name ), targetInfo.Members[ count ] + "@" + i + " does not have member name.");
 					var unpackedItem =
 						context.DefineUnpackedItemParameterInSetValueMethods( targetInfo.Members[ count ].Member.GetMemberValueType() );
 					Func<TConstruct> storeValueStatementEmitter;
-					if ( unpackingContext.VariableType.TryGetRuntimeType() == typeof( DynamicUnpackingContext ) )
-					{
-						storeValueStatementEmitter =
-							() =>
-								this.EmitInvokeVoidMethod(
-									context,
-									context.UnpackingContextInSetValueMethods,
-									Metadata._DynamicUnpackingContext.Set,
-									this.MakeStringLiteral( context, fieldNames[ count ] ),
-									targetInfo.Members[ count ].Member.GetMemberValueType().GetIsValueType()
-									? this.EmitBoxExpression(
-										context,
-										unpackedItem.ContextType,
-										unpackedItem
-									) : unpackedItem
-								);
-					}
-					else if ( targetInfo.IsConstructorDeserialization || this.TargetType.GetIsValueType() )
+					if ( targetInfo.IsConstructorDeserialization || this.TargetType.GetIsValueType() )
 					{
-						var name = fieldNames[ constructorParameterIndex ];
 						storeValueStatementEmitter =
 							() =>
 								this.EmitSetField( context, context.UnpackingContextInSetValueMethods, unpackingContext.Type, name, unpackedItem );
-						constructorParameterIndex++;
 					}
 					else
 					{
@@ -1013,7 +990,7 @@ private UnpackingContextInfo EmitObjectUnpackingContextInitialization( TContext
 			{
 				var constructorParameters = targetInfo.DeserializationConstructor.GetParameters();
 				var contextFields =
-					constructorParameters.Select( p => new KeyValuePair<string, TypeDefinition>( p.Name, p.ParameterType ) ).ToArray();
+					constructorParameters.Select( ( p, i ) => new KeyValuePair<string, TypeDefinition>( targetInfo.GetCorrespondingMemberName( i ) ?? ( "__OrphanParameter" + i.ToString( CultureInfo.InvariantCulture ) ), p.ParameterType ) ).ToArray();
 				var constructorArguments = new List<TConstruct>( constructorParameters.Length );
 				var mappableConstructorArguments = new HashSet<string>();
 				var initializationStatements =
@@ -1148,39 +1125,7 @@ IEnumerable<TConstruct> argumentInitializers
 			unpackingContext.Statements.AddRange( argumentInitializers );
 
 			unpackingContext.Statements.Add(
-				unpackingContext.VariableType.TryGetRuntimeType() == typeof( DynamicUnpackingContext )
-				? this.EmitSequentialStatements(
-					context,
-					TypeDefinition.VoidType,
-					new[]
-					{
-						this.EmitStoreVariableStatement(
-							context,
-							unpackingContext.Variable,
-							this.EmitCreateNewObjectExpression(
-								context,
-								unpackingContext.Variable,
-								unpackingContext.Constructor,
-								this.MakeInt32Literal( context, constructorArguments.Count )
-							)
-						)
-					}.Concat(
-						constructorArguments.Select( ( a, i ) =>
-							this.EmitInvokeVoidMethod(
-								context,
-								unpackingContext.Variable,
-								Metadata._DynamicUnpackingContext.Set,
-								this.MakeStringLiteral( context, contextFields[ i ].Key ),
-								a.ContextType.ResolveRuntimeType().GetIsValueType()
-									? this.EmitBoxExpression(
-										context,
-										a.ContextType,
-										a
-									) : a
-							)
-						)
-					)
-				) : this.EmitStoreVariableStatement(
+				this.EmitStoreVariableStatement(
 					context,
 					unpackingContext.Variable,
 					this.EmitCreateNewObjectExpression(
@@ -1224,26 +1169,15 @@ private IEnumerable<TConstruct> EmitCreateObjectFromContextCore(
 						context,
 						result,
 						member,
-						unpackingContext.VariableType.TryGetRuntimeType() == typeof( DynamicUnpackingContext )
-							? this.EmitUnboxAnyExpression(
-								context,
-								field.Value,
-								this.EmitInvokeMethodExpression(
-									context,
-									context.UnpackingContextInCreateObjectFromContext,
-									Metadata._DynamicUnpackingContext.Get,
-									this.MakeStringLiteral( context, field.Key )
-									)
-								)
-							: this.EmitGetFieldExpression(
-								context,
-								context.UnpackingContextInCreateObjectFromContext,
-								new FieldDefinition(
-									unpackingContext.Type,
-									field.Key,
-									field.Value
-								)
+						this.EmitGetFieldExpression(
+							context,
+							context.UnpackingContextInCreateObjectFromContext,
+							new FieldDefinition(
+								unpackingContext.Type,
+								field.Key,
+								field.Value
 							)
+						)
 					);
 			}
 
@@ -1523,18 +1457,7 @@ private IEnumerable<TConstruct> EmitInvokeDeserializationConstructorStatementsCo
 						constructor,
 						fields.Select(
 							f =>
-								unpackingContext.ContextType.TryGetRuntimeType() == typeof( DynamicUnpackingContext )
-								? this.EmitUnboxAnyExpression(
-									context,
-									f.Value,
-									this.EmitInvokeMethodExpression(
-										context,
-										unpackingContext,
-										Metadata._DynamicUnpackingContext.Get,
-										this.MakeStringLiteral( context, f.Key )
-									)
-								)
-								: this.EmitGetFieldExpression( context, unpackingContext, new FieldDefinition( unpackingContext.ContextType, f.Key, f.Value ) )
+								this.EmitGetFieldExpression( context, unpackingContext, new FieldDefinition( unpackingContext.ContextType, f.Key, f.Value ) )
 						).ToArray()
 					)
 				);

src/MsgPack/Serialization/AbstractSerializers/SerializerBuilder`2.Object.cs

@@ -350,18 +350,8 @@ private IEnumerable<TConstruct> BuildTupleUnpackFromCore( TContext context, ILis
 								setUnpackValueOfMethodName,
 								false, // isStatic
 								TypeDefinition.VoidType,
-								() => unpackingContext.VariableType.TryGetRuntimeType() == typeof( DynamicUnpackingContext )
-									? this.EmitInvokeVoidMethod(
-										context,
-										context.UnpackingContextInSetValueMethods,
-										Metadata._DynamicUnpackingContext.Set,
-										this.MakeStringLiteral( context, propertyName ),
-										this.EmitBoxExpression( 
-											context,
-											itemTypes[ index ],
-											unpackedItem
-										)
-									) : this.EmitSetField(
+								() =>
+									this.EmitSetField(
 										context,
 										context.UnpackingContextInSetValueMethods,
 										unpackingContext.VariableType,
@@ -383,25 +373,15 @@ private IEnumerable<TConstruct> BuildTupleUnpackFromCore( TContext context, ILis
 				var gets =
 					Enumerable.Range( nest * 7, Math.Min( itemTypes.Count - nest * 7, 7 ) )
 						.Select( i =>
-							unpackingContext.VariableType.TryGetRuntimeType() == typeof( DynamicUnpackingContext )
-								? this.EmitUnboxAnyExpression(
-									context,
-									itemTypes[ i ],
-									this.EmitInvokeMethodExpression(
-										context,
-										context.UnpackingContextInCreateObjectFromContext,
-										Metadata._DynamicUnpackingContext.Get,
-										this.MakeStringLiteral( context, SerializationTarget.GetTupleItemNameFromIndex( i ) )
-									)
-								) : this.EmitGetFieldExpression(
-									context,
-									context.UnpackingContextInCreateObjectFromContext,
-									new FieldDefinition( 
-										unpackingContext.VariableType,
-										SerializationTarget.GetTupleItemNameFromIndex( i ),
-										itemTypes[ i ]
-									)
+							this.EmitGetFieldExpression(
+								context,
+								context.UnpackingContextInCreateObjectFromContext,
+								new FieldDefinition( 
+									unpackingContext.VariableType,
+									SerializationTarget.GetTupleItemNameFromIndex( i ),
+									itemTypes[ i ]
 								)
+							)
 						);
 				if ( currentTuple != null )
 				{
@@ -499,9 +479,7 @@ out constructor
 						context,
 						unpackingContext.Variable,
 						unpackingContext.Constructor,
-						unpackingContext.VariableType.TryGetRuntimeType() == typeof( DynamicUnpackingContext )
-							? new[] { this.MakeInt32Literal( context, itemTypes.Count ) }
-							: itemTypes.Select( t => this.MakeDefaultLiteral( context, t ) ).ToArray()
+						itemTypes.Select( t => this.MakeDefaultLiteral( context, t ) ).ToArray()
 					)
 				)
 			);

src/MsgPack/Serialization/AbstractSerializers/SerializerBuilder`2.Tuple.cs

@@ -3855,6 +3855,31 @@ public void TestOptionalConstructorString_Success()
 			}
 		}
 
+		// Issue233
+		[Test]
+		public void TestConstructorDeserializationWithParametersNotInLexicalOrder()
+		{
+			var endpoints =
+				new EndpointList(
+					"Test String One",
+					new Dictionary<string, string[]>
+					{
+						{ "ConfigService", new [] { "ur1", "ur2" } },
+						{ "TestService", new [] { "ur1", "ur2" } }
+					},
+					"Test String Two"
+				);
+
+			var context = new SerializationContext();
+			var ser = context.GetSerializer<EndpointList>();
+			var bytes = ser.PackSingleObject( endpoints );
+			var endpointsDeser = ser.UnpackSingleObject( bytes );
+
+			Assert.That( endpointsDeser.StringOne, Is.EqualTo( endpoints.StringOne ) );
+			Assert.That( endpointsDeser.StringTwo, Is.EqualTo( endpoints.StringTwo ) );
+			Assert.That( endpointsDeser.Endpoints, Is.EqualTo( endpoints.Endpoints ) );
+		}
+
 		[Test]
 		public void TestCollection_Success()
 		{