Skip to content
This repository has been archived by the owner on Oct 26, 2018. It is now read-only.

Add to Visited if in Cache #69

Merged
merged 7 commits into from
Jun 6, 2017
Merged
Show file tree
Hide file tree
Changes from 5 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 @@ -66,10 +66,10 @@ public static Type GetProxyType(ProxyTypeCache cache, Type targetType, Type sour
// We only want to publish the results after all of the proxies are totally generated.
foreach (var verificationResult in context.Visited)
{
cache[verificationResult.Key] = ProxyTypeCacheResult.FromType(
cache.TryAdd(verificationResult.Key, ProxyTypeCacheResult.FromType(
verificationResult.Key,
verificationResult.Value.Type,
verificationResult.Value.Constructor);
verificationResult.Value.Constructor));
}

return context.Visited[context.Key].Type;
Expand All @@ -80,7 +80,7 @@ public static Type GetProxyType(ProxyTypeCache cache, Type targetType, Type sour
}
else if (result.Type == null)
{
// This is an identity convertion
// This is an identity conversion
return null;
}
else
Expand All @@ -89,7 +89,7 @@ public static Type GetProxyType(ProxyTypeCache cache, Type targetType, Type sour
}
}

private static bool VerifyProxySupport(ProxyBuilderContext context, Tuple<Type, Type> key)
internal static bool VerifyProxySupport(ProxyBuilderContext context, Tuple<Type, Type> key)
{
var sourceType = key.Item1;
var targetType = key.Item2;
Expand All @@ -101,8 +101,12 @@ private static bool VerifyProxySupport(ProxyBuilderContext context, Tuple<Type,
}

ProxyTypeCacheResult cacheResult;
var verificationResult = new VerificationResult();
if (context.Cache.TryGetValue(key, out cacheResult))
{
verificationResult.Constructor = cacheResult.Constructor;
Copy link
Member

Choose a reason for hiding this comment

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

Add a comment here, this should explain how/why this can happen.

verificationResult.Type = cacheResult.Type;
context.Visited.Add(key, verificationResult);
Copy link
Member

Choose a reason for hiding this comment

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

I think we also need to copy the related properties from catchResult -> verificationResult

Copy link
Member

Choose a reason for hiding this comment

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

Line 69 also need to change to a 'TryAdd`.

Basically we need to deal with the fact that we could find out from this method that our proxy type as already been created.

Copy link
Member

Choose a reason for hiding this comment

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

Add a blank line after this, it's bad Feng Shui to put a comment directly between two lines of code.

// If we get here we've got a published conversion or error, so we can stop searching.
return !cacheResult.IsError;
}
Expand All @@ -123,7 +127,6 @@ private static bool VerifyProxySupport(ProxyBuilderContext context, Tuple<Type,

// This is a combination we haven't seen before, and it *might* support proxy generation, so let's
// start trying.
var verificationResult = new VerificationResult();
context.Visited.Add(key, verificationResult);

// We support conversions from IList<T> -> IReadOnlyList<U> and IReadOnlyList<T> -> IReadOnlyList<U>
Expand Down Expand Up @@ -453,7 +456,7 @@ private static Type GetGenericImplementation(Type type, Type openGenericInterfac
return null;
}

private class ProxyBuilderContext
internal class ProxyBuilderContext
{
public ProxyBuilderContext(ProxyTypeCache cache, Type targetType, Type sourceType)
{
Expand All @@ -474,7 +477,7 @@ public ProxyBuilderContext(ProxyTypeCache cache, Type targetType, Type sourceTyp
public Dictionary<Tuple<Type, Type>, VerificationResult> Visited { get; }
}

private class VerificationResult
internal class VerificationResult
{
public ConstructorInfo Constructor { get; set; }

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System.Runtime.CompilerServices;

[assembly: InternalsVisibleTo("Microsoft.Extensions.DiagnosticAdapter.Test, PublicKey=0024000004800000940000000602000000240000525341310004000001000100f33a29044fa9d740c9b3213a93e57c84b472c84e0b8a0e1ae48e67a9f8f6de9d5f7f3d52ac23e48ac51801f1dc950abe901da34d2a9e3baadb141a17c77ef3c565dd5ee5054b91cf63bb3c6ab83f72ab3aafe93d0fc3c2348b764fafb0b1c0733de51459aeab46580384bf9d74c4e28164b7cde247f891ba07891c9d872ad2bb")]
[assembly: InternalsVisibleTo("DynamicProxyGenAssembly2, PublicKey=0024000004800000940000000602000000240000525341310004000001000100c547cac37abd99c8db225ef2f6c8a3602f3b3606cc9891605d02baa56104f4cfc0734aa39b93bf7852f7d9266654753cc297e7d2edfe0bac1cdcf9f717241550e0a7b191195b7667bb4f64bcb8e2121380fd1d9d46ad2d92d2d15605093924cceaf74c4861eff62abf69b9291ed0a340e113be11e6a7d3113e92484cf7045cc7")]
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
using System;
using System.Collections.Generic;
using System.Linq;
using Microsoft.AspNetCore.Testing.xunit;
using Xunit;

namespace Microsoft.Extensions.DiagnosticAdapter.Internal
Expand Down Expand Up @@ -66,6 +65,48 @@ public void GetProxyType_Assignable(Type sourceType, Type targetType)
Assert.Null(result);
}

[Fact]
public void GetProxyType_IfAlreadyInCache_AlsoAddedToVisited_FromType()
{
// Arrange
var sourceType = typeof(ProxyPerson);
Copy link
Member

Choose a reason for hiding this comment

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

Can you pick an example that should at least by possible?

var targetType = typeof(Person);
var key = new Tuple<Type, Type>(sourceType, targetType);
var cache = new ProxyTypeCache();
cache[key] = ProxyTypeCacheResult.FromType(key, sourceType, sourceType.GetConstructor(Array.Empty<Type>()));
var context = new ProxyTypeEmitter.ProxyBuilderContext(cache, targetType, sourceType);
Copy link
Member

Choose a reason for hiding this comment

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

Let it breathe.


// Act
var result = ProxyTypeEmitter.VerifyProxySupport(context, key);
var result2 = ProxyTypeEmitter.VerifyProxySupport(context, key);

// Assert
Assert.True(result);
Assert.True(result2);
Assert.Single(context.Visited);
Assert.Equal(key, context.Visited.Single().Key);
}

[Fact]
public void GetProxyType_IfAlreadyInCache_AlsoAddedToVisited_FromError()
{
// Arrange
var sourceType = typeof(ProxyPerson);
var targetType = typeof(Person);
var key = new Tuple<Type, Type>(sourceType, targetType);
var cache = new ProxyTypeCache();
cache[key] = ProxyTypeCacheResult.FromError(key, "Test Error");
var context = new ProxyTypeEmitter.ProxyBuilderContext(cache, targetType, sourceType);

// Act
var result = ProxyTypeEmitter.VerifyProxySupport(context, key);

// Assert
Assert.False(result);
Assert.Single(context.Visited);
Assert.Equal(key, context.Visited.Single().Key);
}

[Fact]
public void GetProxyType_Fails_DestinationIsNotInterface()
{
Expand Down Expand Up @@ -728,6 +769,14 @@ public class DerivedPerson : Person
public double CoolnessFactor { get; set; }
}

public class ProxyPerson
{
public double CoolnessFactor { get; set; }
public string FirstName { get; set; }
public string LastName { get; set; }
public Address Address { get; set; }
}

public class Address
{
public string City { get; set; }
Expand Down