Skip to content

Cache reflection lookups in DynamicDataOperations #7535

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.

Sign up for GitHub

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

Open
Evangelink wants to merge 2 commits into
base: main
Choose a base branch
from
+7 −60
Open

Cache reflection lookups in DynamicDataOperations #7535

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
28dac23
Simplify DynamicDataOperations: use FlattenHierarchy instead of manua…
Evangelink Mar 12, 2026
d340830
Fix
Evangelink Mar 12, 2026
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
53 changes: 4 additions & 49 deletions src/TestFramework/TestFramework/Attributes/DataSource/DynamicDataOperations.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ namespace Microsoft.VisualStudio.TestTools.UnitTesting;

internal static class DynamicDataOperations
{
private const BindingFlags DeclaredOnlyLookup = BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Instance | BindingFlags.Static | BindingFlags.DeclaredOnly;
private const BindingFlags MemberLookup = BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Instance | BindingFlags.Static | BindingFlags.FlattenHierarchy;
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

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

This change switches from a most-derived-first, per-type lookup (via DeclaredOnly + manual BaseType walk) to a single reflection lookup using FlattenHierarchy. That is not behavior-equivalent: (1) BindingFlags.FlattenHierarchy will not return private static members declared on base types, whereas the previous implementation would find them; and (2) the single-call lookup can surface multiple matches across the inheritance chain and throw AmbiguousMatchException in cases where the previous implementation would have returned the derived member. If the intent is purely perf, consider keeping the previous resolution semantics (derived-first, include non-public base members) and introducing caching as described in the PR, rather than changing the BindingFlags behavior.

Copilot uses AI. Check for mistakes.
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

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

The PR title/description mention adding ConcurrentDictionary caches (and a TypeMemberKey struct for net462), but this diff instead removes the custom hierarchy-walk helpers and performs direct reflection lookups with FlattenHierarchy—no caching is introduced. Please update the PR description/title to reflect the actual approach, or include the intended caching changes.

Copilot uses AI. Check for mistakes.
Copy link
Member

@Youssef1313 Youssef1313 Mar 13, 2026

Choose a reason for hiding this comment

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

I'm fine with the behavior change. Let's ensure that the analyzer that validates dynamic data handles it correctly though.

Copy link
Member

@Youssef1313 Youssef1313 Mar 13, 2026

Choose a reason for hiding this comment

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

In DynamicDataShouldBeValidAnalyzer, we have TryGetMemberCore local function. We can possibly add bool disallowPrivate there, and we pass false in the first iteration, and true on subsequent iterations.


public static IEnumerable<object[]> GetData(Type? dynamicDataDeclaringType, DynamicDataSourceType dynamicDataSourceType, string dynamicDataSourceName, object?[] dynamicDataSourceArguments, MethodInfo methodInfo)
{
Expand Down Expand Up @@ -167,56 +167,11 @@ private static bool TryGetData(object dataSource, [NotNullWhen(true)] out IEnume
}

Copy link
Member

@Youssef1313 Youssef1313 Mar 16, 2026

Choose a reason for hiding this comment

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

Suggested change

private static FieldInfo? GetFieldConsideringInheritance(Type type, string fieldName)
{
// NOTE: Don't use GetRuntimeField. It considers inheritance only for instance fields.
Type? currentType = type;
while (currentType is not null)
{
FieldInfo? field = currentType.GetField(fieldName, DeclaredOnlyLookup);
if (field is not null)
{
return field;
}

currentType = currentType.BaseType;
}

return null;
}
=> type.GetField(fieldName, MemberLookup);

private static PropertyInfo? GetPropertyConsideringInheritance(Type type, string propertyName)
{
// NOTE: Don't use GetRuntimeProperty. It considers inheritance only for instance properties.
Type? currentType = type;
while (currentType is not null)
{
PropertyInfo? property = currentType.GetProperty(propertyName, DeclaredOnlyLookup);
if (property is not null)
{
return property;
}

currentType = currentType.BaseType;
}

return null;
}
=> type.GetProperty(propertyName, MemberLookup);

private static MethodInfo? GetMethodConsideringInheritance(Type type, string methodName)
{
// NOTE: Don't use GetRuntimeMethod. It considers inheritance only for instance methods.
Type? currentType = type;
while (currentType is not null)
{
MethodInfo? method = currentType.GetMethod(methodName, DeclaredOnlyLookup);
if (method is not null)
{
return method;
}

currentType = currentType.BaseType;
}

return null;
}
=> type.GetMethod(methodName, MemberLookup);
}