-
Notifications
You must be signed in to change notification settings - Fork 26
Add to Visited if in Cache #69
Changes from 5 commits
7356d5c
c18e7ae
ba54a9c
ad7dd29
65af0d1
2711d36
55345cd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
@@ -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 | ||
|
@@ -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; | ||
|
@@ -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; | ||
verificationResult.Type = cacheResult.Type; | ||
context.Visited.Add(key, verificationResult); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we also need to copy the related properties from There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
} | ||
|
@@ -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> | ||
|
@@ -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) | ||
{ | ||
|
@@ -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; } | ||
|
||
|
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 |
---|---|---|
|
@@ -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 | ||
|
@@ -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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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() | ||
{ | ||
|
@@ -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; } | ||
|
There was a problem hiding this comment.
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.