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

ValueTuple to sequence value conversion #976

Merged
merged 6 commits into from Jun 3, 2017
Merged
Diff settings

Always

Just for now

@@ -74,7 +74,7 @@ partial class PropertyValueConverter : ILogEventPropertyFactory, ILogEventProper
new SimpleScalarConversionPolicy(BuiltInScalarTypes.Concat(additionalScalarTypes)),
new NullableScalarConversionPolicy(),
new EnumScalarConversionPolicy(),
new ByteArrayScalarConversionPolicy(),
new ByteArrayScalarConversionPolicy()
};

_destructuringPolicies = additionalDestructuringPolicies
@@ -162,6 +162,29 @@ LogEventPropertyValue CreatePropertyValue(object value, Destructuring destructur
}
}

if (TryConvertEnumerable(value, destructuring, valueType, limiter, out var enumerableResult))
return enumerableResult;

if (TryConvertValueTuple(value, destructuring, valueType, limiter, out var tupleResult))
return tupleResult;

if (destructuring == Destructuring.Destructure)
{
var type = value.GetType();
var typeTag = type.Name;
if (typeTag.Length <= 0 || IsCompilerGeneratedType(type))
{
typeTag = null;
}

return new StructureValue(GetProperties(value, limiter), typeTag);
}

return new ScalarValue(value.ToString());
}

bool TryConvertEnumerable(object value, Destructuring destructuring, Type valueType, DepthLimiter limiter, out LogEventPropertyValue result)
{
var enumerable = value as IEnumerable;
if (enumerable != null)
{
@@ -179,30 +202,59 @@ LogEventPropertyValue CreatePropertyValue(object value, Destructuring destructur
var keyProperty = typeInfo.GetDeclaredProperty("Key");
var valueProperty = typeInfo.GetDeclaredProperty("Value");

return new DictionaryValue(enumerable.Cast<object>().Take(_maximumCollectionCount)
result = new DictionaryValue(enumerable
.Cast<object>()
.Take(_maximumCollectionCount)
.Select(kvp => new KeyValuePair<ScalarValue, LogEventPropertyValue>(
(ScalarValue)limiter.CreatePropertyValue(keyProperty.GetValue(kvp), destructuring),
limiter.CreatePropertyValue(valueProperty.GetValue(kvp), destructuring)))
(ScalarValue) limiter.CreatePropertyValue(keyProperty.GetValue(kvp), destructuring),
limiter.CreatePropertyValue(valueProperty.GetValue(kvp), destructuring)))
.Where(kvp => kvp.Key.Value != null));
return true;
}

return new SequenceValue(
enumerable.Cast<object>().Take(_maximumCollectionCount).Select(o => limiter.CreatePropertyValue(o, destructuring)));
result = new SequenceValue(enumerable
.Cast<object>()
.Take(_maximumCollectionCount)
.Select(o => limiter.CreatePropertyValue(o, destructuring)));

return true;
}

if (destructuring == Destructuring.Destructure)
result = null;
return false;
}

static bool TryConvertValueTuple(object value, Destructuring destructuring, Type valueType, DepthLimiter limiter, out LogEventPropertyValue result)
{
if (!(value is IStructuralEquatable && valueType.IsConstructedGenericType))
{
var type = value.GetType();
var typeTag = type.Name;
if (typeTag.Length <= 0 || IsCompilerGeneratedType(type))
result = null;
return false;
}

var definition = valueType.GetGenericTypeDefinition();
if (definition == typeof(ValueTuple<>) || definition == typeof(ValueTuple<,>) ||
definition == typeof(ValueTuple<,,>) || definition == typeof(ValueTuple<,,,>) ||
definition == typeof(ValueTuple<,,,,>) || definition == typeof(ValueTuple<,,,,,>) ||
definition == typeof(ValueTuple<,,,,,,>)) // Ignore the 8+ value case for now.
{
var elements = new List<LogEventPropertyValue>();
foreach (var field in valueType.GetTypeInfo().DeclaredFields)
{
typeTag = null;
if (field.IsPublic && !field.IsStatic)
{
var fieldValue = field.GetValue(value);
var propertyValue = limiter.CreatePropertyValue(fieldValue, destructuring);
elements.Add(propertyValue);
}
}

return new StructureValue(GetProperties(value, limiter), typeTag);
result = new SequenceValue(elements);
return true;
}

return new ScalarValue(value.ToString());
result = null;
return false;
}

LogEventPropertyValue Stringify(object value)
Copy path View file
@@ -23,25 +23,44 @@
<ItemGroup Condition=" '$(TargetFramework)' == 'net45' ">
<Reference Include="System" />
<Reference Include="Microsoft.CSharp" />
<PackageReference Include="System.ValueTuple" Version="4.3.1" />

This comment has been minimized.

Copy link
@skomis-mm

skomis-mm May 31, 2017

It's a pity. Why you didn't considered the fourth option (along with the first)?

4.Support it everywhere, using reflection only (no direct dependency on ValueTuple itself)

Is that because of some performance concerns? I believe you could check type.Name to constants (ValueTuple'1, ValueTuple'2 etc) like you did with definition == comparisons

This comment has been minimized.

Copy link
@nblumhardt

nblumhardt Jun 1, 2017

Author Member

Thanks for the suggestion. I initially discounted this because it seemed a bit awkward, given tuples are a feature of C# that's here to stay. On second thought, it's not much worse to do the type name check than to do what we have here, so I'll switch it back over and #if VALUETUPLE conditionally-include the Type-based version 👍

</ItemGroup>

<ItemGroup Condition=" '$(TargetFramework)' == 'net46' ">
<Reference Include="System" />
<Reference Include="Microsoft.CSharp" />
<PackageReference Include="System.ValueTuple" Version="4.3.1" />
</ItemGroup>

<ItemGroup Condition=" '$(TargetFramework)' == 'netstandard1.0' ">
<PackageReference Include="Microsoft.CSharp" Version="4.0.1" />
<PackageReference Include="System.Collections" Version="4.0.11" />
<PackageReference Include="System.Dynamic.Runtime" Version="4.0.11" />
<PackageReference Include="System.Globalization" Version="4.0.11" />
<PackageReference Include="System.Linq" Version="4.1.0" />
<PackageReference Include="System.Reflection" Version="4.1.0" />
<PackageReference Include="System.Reflection.Extensions" Version="4.0.1" />
<PackageReference Include="System.Runtime" Version="4.1.0" />
<PackageReference Include="System.Runtime.Extensions" Version="4.1.0" />
<PackageReference Include="System.Text.RegularExpressions" Version="4.1.0" />
<PackageReference Include="System.Threading" Version="4.0.11" />
<PackageReference Include="Microsoft.CSharp" Version="4.3.0" />

This comment has been minimized.

Copy link
@nblumhardt

nblumhardt May 30, 2017

Author Member

These dependencies are updated; System.ValueTuple depends on s.Collections under .NET Standard, and requires at least 4.3.0, so we might as well be consistent.

This comment has been minimized.

Copy link
@khellang

khellang May 31, 2017

Member

The netstandard1.x targets should probably be updated to just depend on NETStandard.Library anyway, TBH.

This comment has been minimized.

Copy link
@nblumhardt

nblumhardt Jun 1, 2017

Author Member

Thanks @khellang - you're probably right here. I guess the dust has settled enough for us to embrace the new order of things :-). Perhaps we make the change when .NET Core 2.0 comes along? I am tempted to leave it alone in this PR, just to avoid any more chasing of corner cases....

<PackageReference Include="System.Collections" Version="4.3.0" />
<PackageReference Include="System.Dynamic.Runtime" Version="4.3.0" />
<PackageReference Include="System.Globalization" Version="4.3.0" />
<PackageReference Include="System.Linq" Version="4.3.0" />
<PackageReference Include="System.Reflection" Version="4.3.0" />
<PackageReference Include="System.Reflection.Extensions" Version="4.3.0" />
<PackageReference Include="System.Runtime" Version="4.3.0" />
<PackageReference Include="System.Runtime.Extensions" Version="4.3.0" />
<PackageReference Include="System.Text.RegularExpressions" Version="4.3.0" />
<PackageReference Include="System.Threading" Version="4.3.0" />
<PackageReference Include="System.ValueTuple" Version="4.3.1" />
</ItemGroup>

<ItemGroup Condition=" '$(TargetFramework)' == 'netstandard1.3' ">
<PackageReference Include="System.Collections.NonGeneric" Version="4.3.0" />
<PackageReference Include="Microsoft.CSharp" Version="4.3.0" />
<PackageReference Include="System.Collections" Version="4.3.0" />
<PackageReference Include="System.Dynamic.Runtime" Version="4.3.0" />
<PackageReference Include="System.Globalization" Version="4.3.0" />
<PackageReference Include="System.Linq" Version="4.3.0" />
<PackageReference Include="System.Reflection" Version="4.3.0" />
<PackageReference Include="System.Reflection.Extensions" Version="4.3.0" />
<PackageReference Include="System.Runtime" Version="4.3.0" />
<PackageReference Include="System.Runtime.Extensions" Version="4.3.0" />
<PackageReference Include="System.Text.RegularExpressions" Version="4.3.0" />
<PackageReference Include="System.Threading" Version="4.3.0" />
<PackageReference Include="System.ValueTuple" Version="4.3.1" />
</ItemGroup>

<PropertyGroup Condition=" '$(TargetFramework)' == 'net45' ">
@@ -56,19 +75,4 @@
<DefineConstants>$(DefineConstants);ASYNCLOCAL;HASHTABLE</DefineConstants>
</PropertyGroup>

<ItemGroup Condition=" '$(TargetFramework)' == 'netstandard1.3' ">
<PackageReference Include="System.Collections.NonGeneric" Version="4.0.1" />
<PackageReference Include="Microsoft.CSharp" Version="4.0.1" />
<PackageReference Include="System.Collections" Version="4.0.11" />
<PackageReference Include="System.Dynamic.Runtime" Version="4.0.11" />
<PackageReference Include="System.Globalization" Version="4.0.11" />
<PackageReference Include="System.Linq" Version="4.1.0" />
<PackageReference Include="System.Reflection" Version="4.1.0" />
<PackageReference Include="System.Reflection.Extensions" Version="4.0.1" />
<PackageReference Include="System.Runtime" Version="4.1.0" />
<PackageReference Include="System.Runtime.Extensions" Version="4.1.0" />
<PackageReference Include="System.Text.RegularExpressions" Version="4.1.0" />
<PackageReference Include="System.Threading" Version="4.0.11" />
</ItemGroup>

</Project>
@@ -8,6 +8,8 @@
using Serilog.Parsing;
using Serilog.Tests.Support;

// ReSharper disable UnusedAutoPropertyAccessor.Global, UnusedParameter.Local

namespace Serilog.Tests.Parameters
{
public class PropertyValueConverterTests
@@ -188,7 +190,7 @@ public class BaseWithProps

public class DerivedWithOverrides : BaseWithProps
{
new public string PropA { get; set; }
public new string PropA { get; set; }
public override string PropB { get; set; }
public string PropD { get; set; }
}
@@ -216,7 +218,7 @@ public void NewAndInheritedPropertiesAppearOnlyOnce()

class HasIndexer
{
public string this[int index] { get { return "Indexer"; } }
public string this[int index] => "Indexer";
}

[Fact]
@@ -231,7 +233,7 @@ public void IndexerPropertiesAreIgnoredWhenDestructuring()
// (reducing garbage).
class HasItem
{
public string Item { get { return "Item"; } }
public string Item => "Item";
}

[Fact]
@@ -253,6 +255,55 @@ public void CSharpAnonymousTypesAreRecognizedWhenDestructuring()
var structuredValue = (StructureValue)result;
Assert.Equal(null, structuredValue.TypeTag);
}

[Fact]
public void ValueTuplesAreRecognizedWhenDestructuring()
{
var o = (1, "A", new[] { "B" });
var result = _converter.CreatePropertyValue(o);

var sequenceValue = Assert.IsType<SequenceValue>(result);

Assert.Equal(3, sequenceValue.Elements.Count);
Assert.Equal(new ScalarValue(1), sequenceValue.Elements[0]);
Assert.Equal(new ScalarValue("A"), sequenceValue.Elements[1]);
var nested = Assert.IsType<SequenceValue>(sequenceValue.Elements[2]);
Assert.Equal(1, nested.Elements.Count);
Assert.Equal(new ScalarValue("B"), nested.Elements[0]);
}

[Fact]
public void AllTupleLengthsUpToSevenAreSupported()
{
var tuples = new object[]

This comment has been minimized.

Copy link
@merbla

merbla Jun 2, 2017

Contributor

Is an empty array test worthwhile?

This comment has been minimized.

Copy link
@merbla

merbla Jun 2, 2017

Contributor

Should we migrate to InlineData for use cases?

This comment has been minimized.

Copy link
@nblumhardt

nblumhardt Jun 3, 2017

Author Member

Tried, but ValueTuple isn't regarded by csc as a compile-time constant, so no joy :-(

{
ValueTuple.Create(1),
(1, 2),
(1, 2, 3),
(1, 2, 3, 4),
(1, 2, 3, 4, 5),
(1, 2, 3, 4, 5, 6),
(1, 2, 3, 4, 5, 6, 7)
};

foreach (var t in tuples)
Assert.IsType<SequenceValue>(_converter.CreatePropertyValue(t));
}

[Fact]
public void EightPlusValueTupleElementsAreIgnored()

This comment has been minimized.

Copy link
@merbla

merbla Jun 2, 2017

Contributor

Nit EightPlusValueTupleElementsAreIgnored -> EightPlusValueTupleElementsAreIgnoredWhenDestructuring

Why the arbitrary 8?

This comment has been minimized.

Copy link
@nblumhardt

nblumhardt Jun 3, 2017

Author Member

👍 probably should be WhenCapturing, but good point.

The limitation is in how ValueTuple works, up to seven elements can be represented directly in one tuple, but to go 8+ they need to be nested. No major issue with implementing it down the road.

{
var scalar = _converter.CreatePropertyValue((1, 2, 3, 4, 5, 6, 7, 8));
Assert.IsType<ScalarValue>(scalar);
}

[Fact]
public void DestructuringIsTransitivelyApplied()
{
var tuple = _converter.CreatePropertyValue(ValueTuple.Create(new {A = 1}), true);
var sequence = Assert.IsType<SequenceValue>(tuple);
Assert.IsType<StructureValue>(sequence.Elements[0]);
}
}
}

ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.