Create analyzer for non-serializable data in Theory

[andrewlock]

We just discovered that someone had used non-serializable data (a dictionary) in a Theory test case, because we happened to look at the CI logs.

It would be nice if there was an analyzer that could warn you about this ahead of time. I had a look around but couldn't find any mention of one.

Thanks!

[jared-chevalier]

Hi Andrew.

Problem

Could you post a code snippet to demonstrate the issue? Did your test method have a dictionary-type parameter declaration? Or did it have an object-type or interface-type parameter where certain test cases happened to provide a dictionary object as a test case value?

For context, could you clarify why this problem was a source of pain and how a solution would be valuable?

I'm guessing it has to do with the fact that, if a theory test method has any non-serializable test case value, then none of its test cases will be enumerated in the test runner, collapsing them into a single item for the test method as a whole. As a result, you will need to run all of the method's test cases together every time, and you won't be able to run a single test case or subset of test cases in isolation, which might be especially problematic for longer-running tests or test methods with many test cases.

Solution

Could you clarify what you had in mind for a diagnostic produced by such an analyzer? Here are a few alternatives I imagine you might have in mind.

1. Diagnostic on a theory method parameter, if the parameter declaration's static type is not serializable, such as Dictionary<int, string> (or if it is not guaranteed to be serializable, such as object, object[], IEnumerable, etc.)
2. Diagnostic on an InlineData, MemberData, or ClassData attribute, if it has any corresponding test case argument expression whose static type is not serializable (or not guaranteed to be serializable)
3. Diagnostic on a test case argument expression, if the expression's static type is not serializable (or not guaranteed to be serializable)

Here are my thoughts on each of these. Am I missing anything? If anyone has any additional thoughts or suggestions, please let me know.

[jared-chevalier]

1. Diagnostic on a theory method parameter

Relatively straightforward to implement, but sometimes would produce less useful diagnostics.

• This is because the analysis and diagnostics would be applied at the parameter level, regardless of the actual values present in test cases and the expressions used to define them
• Diagnostics might be less useful when the parameter type is a base type or interface, and the concrete runtime types of values in different test cases might or might not be serializable, producing false positives when actual test case values are in fact serializable

Expected diagnostics

[Theory]
[InlineData(...)]
[MemberData(...)]
[ClassData(...)]
public void IntegerTestMethod(int number) // No diagnostic
{ ... }

[Theory]
[MemberData(...)]
[ClassData(...)]
public void DictionaryTestMethod(Dictionary<int, string> dictionary) // DIAGNOSTIC
{ ... }

[Theory]
[InlineData(...)] // Test cases might actually be serializable
[MemberData(...)] // Test cases might actually be serializable
[ClassData(...)]  // Test cases might actually be serializable
public void ObjectTestMethod(object? parameter) // DIAGNOSTIC, with possible false positives
{ ... }

[Theory]
[InlineData(...)] // Test cases might actually be serializable
[MemberData(...)] // Test cases might actually be serializable
[ClassData(...)]  // Test cases might actually be serializable
public void EnumerableTestMethod(IEnumerable<int> numbers) // DIAGNOSTIC, with possible false positives
{ ... }

[jared-chevalier]

2. Diagnostic on a theory data attribute

Very ambitious and complex to implement, but would generally produce more useful diagnostics.

• The diagnostics would not apply to attributes whose test case values are clearly serializable, and would apply only to attributes whose test case values are clearly non-serializable (or cannot be verified to be serializable), producing fewer false positives
• But consider the vast number of syntactic and semantic forms that the definition of an IEnumerable<object[]> might take, including all combinations of:
  • Ways a test case element might be defined
  • Ways a test case array might be defined
  • Ways an enumerable of test case arrays might be defined
• Arbitrary levels of indirection may be involved, including deeply nested or chained invocations, conversions, and data copying
• Arbitrary amounts of control flow logic may be involved
• For example, here we would have to dig deep from the TestCases property to find that the two test case values are in fact defined using an int expression (serializable) and a Dictionary<int, string> expression (non-serializable):

    public static IEnumerable<object[]> TestCases => GetTestCases();
    private static IEnumerable<object[]> GetTestCases() => new object[][] { GetTestCase1(), GetTestCase2() };
    private static object[] GetTestCase1() => new object[] { GetValue1() };
    private static object[] GetTestCase2() { var array = new object[1]; array[0] = GetValue2(); return array; }
    private static object GetValue1() => 1;
    private static object GetValue2() => GetCollection(array: false);
    private static object GetCollection(bool array) => array ? new int[0] : new Dictionary<int, string>();

• Invocations might be virtual when base type references or interface references are involved, making it sometimes impossible to determine with static analysis which implementation will be executed and therefore what expression types will be involved

• If I understand correctly, analyzers need to work at the level of static analysis, and cannot feasibly enumerate test cases to check concrete runtime types with GetType(). See @bradwilson's comment from xUnit1010 is triggered for strings which are convertible to decimal / int #2354:

  You can't realistically new up instances of things in an analyzer and run the code and see what happens; even if it weren't punishingly slow to do at code analysis time, it's not guaranteed to even be possible.

• It's unrealistic to expect such an analyzer to be able to correctly handle every possible case

Alternative suggestion

Maybe such an analyzer could have more modest ambitions?

• It could attempt to analyze only a narrow range of the simplest possible syntactic/semantic forms, where theory data is defined directly with little or no indirection
• For simple cases, the analyzer would verify whether the test cases are serializable, and produce a diagnostic for the attribute if they are not
• For complex cases, the analyzer would not attempt to further analyze the test case definitions, and either:
  • Always produce a diagnostic (with possible false positives)
  • Never produce a diagnostic (with possible false negatives)
• In practice, the analyzer should still provide useful and accurate diagnostics most of the time, since simple, direct test case definitions are probably much more common in practice than complex, indirect ones

Expected diagnostics

public enum NonSerializableEnum { Zero } // Enumeration from a non-local assembly

public class SerializableClass : IEnumerable<object[]>
{
    public IEnumerator<object[]> GetEnumerator()
    {
        yield return new object[] { 1 };
        yield return new object[] { false };
        yield return new object[] { "Text" };
    }

    IEnumerator IEnumerable.GetEnumerator() => GetEnumerator();
}

public class NonSerializableClass : IEnumerable<object[]>
{
    public IEnumerator<object[]> GetEnumerator()
    {
        yield return new object[] { 1 };
        yield return new object[] { false };
        yield return new object[] { "Text" };
        yield return new object[] { NonSerializableEnum.Zero };
        yield return new object[] { new Dictionary<int, string> { { 1, "1" }, { 2, "2" } } };
    }

    IEnumerator IEnumerable.GetEnumerator() => GetEnumerator();
}

public class TestClass
{
    public static IEnumerable<object[]> SerializableMember =>
    [
        [1],
        [false],
        ["Text"]
    ];

    public static IEnumerable<object[]> NonSerializableMember =>
    [
        [1],
        [false],
        ["Text"],
        [NonSerializableEnum.Zero],
        [new Dictionary<int, string> { { 1, "1" }, { 2, "2" } }]
    ];

    [Theory]
    [InlineData(1)]
    [InlineData(false)]
    [InlineData("Text")]
    [MemberData(nameof(SerializableMember))]
    [ClassData(typeof(SerializableClass))]
    public void SerializableTestMethod(object? parameter)
    { ... }

    [Theory]
    [InlineData(NonSerializableEnum.Zero)]                               // DIAGNOSTIC
    [InlineData(new NonSerializableEnum[] { NonSerializableEnum.Zero })] // DIAGNOSTIC
    [MemberData(nameof(NonSerializableMember))]                          // DIAGNOSTIC
    [ClassData(typeof(NonSerializableClass))]                            // DIAGNOSTIC
    public void NonSerializableTestMethod(object? parameter)
    { ... }
}

[jared-chevalier]

3. Diagnostic on a test case argument expression

Similar assessment as for section 2.

• The diagnostics would be even more useful than those in section 2
  • A diagnostic would directly highlight an individual test case argument expression that is clearly non-serializable
  • In section 2, a diagnostic would merely highlight an attribute with one or more non-serializable test case values (or none in the case of a false positive), requiring the developer to then find the non-serializable test case argument expression(s) to be fixed
• The same complications for analysis apply here as well

Alternative suggestion

Maybe such an analyzer could have more modest ambitions?

• It could attempt to analyze only a narrow range of the simplest possible syntactic/semantic forms, where theory data is defined directly with little or no indirection
• For simple cases, the analyzer would verify whether the test case argument expresions are serializable, and produce a diagnostic for each non-serializable test case argument expression
• For complex cases, the analyzer would not attempt to further analyze the test case definitions, and either:
  • Always produce a diagnostic for the attribute itself (with possible false positives)
  • Never produce a diagnostic (with possible false negatives)
• In practice, the analyzer should still provide useful and accurate diagnostics most of the time, since simple, direct test case definitions are probably much more common in practice than complex, indirect ones

Expected diagnostics

public enum NonSerializableEnum { Zero } // Enumeration from a non-local assembly

public class SerializableClass : IEnumerable<object[]>
{
    public IEnumerator<object[]> GetEnumerator()
    {
        yield return new object[] { 1 };
        yield return new object[] { false };
        yield return new object[] { "Text" };
    }

    IEnumerator IEnumerable.GetEnumerator() => GetEnumerator();
}

public class NonSerializableClass : IEnumerable<object[]>
{
    public IEnumerator<object[]> GetEnumerator()
    {
        yield return new object[] { 1 };
        yield return new object[] { false };
        yield return new object[] { "Text" };
        yield return new object[]
        {
            NonSerializableEnum.Zero                               // DIAGNOSTIC
        };
        yield return new object[]
        {
            new Dictionary<int, string> { { 1, "1" }, { 2, "2" } } // DIAGNOSTIC
        };
    }

    IEnumerator IEnumerable.GetEnumerator() => GetEnumerator();
}

public class TestClass
{
    public static IEnumerable<object[]> SerializableMember =>
    [
        [1],
        [false],
        ["Text"]
    ];

    public static IEnumerable<object[]> NonSerializableMember =>
    [
        [1],
        [false],
        ["Text"],
        [
            NonSerializableEnum.Zero                               // DIAGNOSTIC
        ],
        [
            new Dictionary<int, string> { { 1, "1" }, { 2, "2" } } // DIAGNOSTIC
        ]
    ];

    [Theory]
    [InlineData(1)]
    [InlineData(false)]
    [InlineData("Text")]
    [MemberData(nameof(SerializableMember))]
    [ClassData(typeof(SerializableClass))]
    public void SerializableTestMethod(object? parameter)
    { ... }

    [Theory]
    [InlineData(
        NonSerializableEnum.Zero                                   // DIAGNOSTIC
    )]
    [InlineData(
        new NonSerializableEnum[] { NonSerializableEnum.Zero }     // DIAGNOSTIC
    )]
    [MemberData(nameof(NonSerializableMember))]
    [ClassData(typeof(NonSerializableClass))]
    public void NonSerializableTestMethod(object? parameter)
    { ... }
}

[bradwilson]

@andrewlock wrote:

It would be nice if there was an analyzer that could warn you about this ahead of time.

It's possible, but the difficulty depends heavily on the code in question. Here are just two scenarios:

You use yield return with IEnumerable<object[]>

The simplest version is when you use yield return and you create the data inline, like:

public static IEnumerable<object[]> MyDataSource()
{
    yield return new object[] { new object() };
}

We can see the type from new object() and tell you that it's not serializable.

Now add a layer of indirection:

public static IEnumerable<object[]> MyDataSource()
{
    var data = new object();
    yield return new object[] { data };
}

I think Roslyn would be able to tell us the type of data without us having to track it backward.

I think this is doable here. Next up...

You use return <SomeCollection> with IEnumerable<object[]>

Let's convert the second sample:

public static IEnumerable<object[]> MyDataSource()
{
    var result = new List<object[]>();
    var data = new object();
    result.Add(new[] { data });
    return result;
}

Here's where I'm not sure how complex everything gets. We start by looking at return result, at which point we realize we need to go figure out where all the data came from. In the sample, that's fairly "easy" (air quotes, because it's easy for us as humans, but starts to get a little complicate in code terms). So now rather than being able to rely on Roslyn to tell us the types of the values in yield return, we're chasing down all the code that created and/or added to the container, which doesn't necessarily have to live here in this function, or here in this assembly, or even something that we can chase down as source (imagine the data source comes from somewhere that's a binary dependency, not source code you wrote yourself).

---

You can see how this becomes complicated very quickly. We can consider adding some of the simpler scenarios, but it would be helpful to have real world examples of situations you're running into, and then it would be easier to understand whether that's a scenario that's easily covered by an analyzer or not. It's not easy to solve this hypothetically without knowing what the actual code is that you're using, hoping we can catch.

The lowest hanging fruit (easiest for us to identify) would be if you are using TheoryData<>, it should be easy to determine whether every type in the return data is known to be safe to return. That's not perfect, because of object (which is not supported as a concrete type, but could at runtime point to literally anything, including the supported types). It's an interesting part of the discussion, if we did this, to determine whether "this might be a problem" is something we want about, as opposed to "this will definitely be a problem".

Interested in hearing thoughts.

[jared-chevalier]

The lowest hanging fruit (easiest for us to identify) would be if you are using TheoryData<>

I think this is a good idea. Doing this, if nothing else, would strike a good balance between value provided and effort required to implement.

Here are some of my thoughts. Let me know what you think, and please point out anything I might have gotten wrong.

Possibly serializable types vs. definitely non-serializable types

That's not perfect, because of object (which is not supported as a concrete type, but could at runtime point to literally anything, including the supported types).

Yes, you're right. But it doesn't stop there. Any interfaces, unsealed classes, and unsealed records that are not themselves serializable might be used as TheoryData<> type arguments, polymorphically referencing concrete instances of various derived types at runtime. For a particular set of test case values, all / some / none of their concrete types may happen to be serializable. If all of them are in fact serializable, then the test method will be eligible for discovery enumeration.

Here are a few examples of possibly serializable types:

• object
• object[]
• Array
• Array[]
• Enum (could reference a boxed value whose type is either from a local assembly or from the Global Assembly Cache)
• Enum[]
• IEnumerable
• IEnumerable[]
• List<int>
• List<int>[]

On the other hand, any structs, record structs, sealed classes, and sealed records that are non-serializable are definitely non-serializable. Plus various other type kinds are never serializable, such as delegates and pointers (if I'm not mistaken), etc.

Definitely a problem vs. possibly a problem

It's an interesting part of the discussion, if we did this, to determine whether "this might be a problem" is something we want [a diagnostic] about, as opposed to "this will definitely be a problem".

For a large number of possible TheoryData<> type arguments (many of which are likely to be used in practice, such as unsealed classes and interfaces), all we can determine using static analysis is that they may or may not be serializable.

It seems to me that completely ignoring these cases, and providing a diagnostic only for definitely non-serializable types, would significantly take away from the value that this analyzer would provide.

On the other hand, I'm reluctant to annoy developers with excessive warnings that may include many false positives. Some developers will know what they are doing, and will intentionally use only serializable values in test cases when using polymorphic TheoryData<> type arguments that are not necessarily serializable. Other developers probably won't care about whether their theory test cases are enumerated during discovery or collapsed in the test runner.

But of course, if the diagnostics are obtrusive, developers can manually suppress or configure them according to their preferences.

I'd suggest providing a diagnostic for both non-serializable and possibly serializable TheoryData<> type arguments, but differentiating between them by using a strong or a weak variant of the message. Something along the lines of:

• The type argument {0} is not serializable. Consider using a type that is known to be serializable.
• The type argument {0} might not be serializable. Consider using a type that is known to be serializable.

Diagnostic severity

Do you think DiagnosticSeverity.Info or DiagnosticSeverity.Warning would be more appropriate?

No diagnostics when discovery enumeration is disabled

One key point to consider is that, if a developer has explicitly set DisableDiscoverEnumeration = true in a MemberData attribute (or Theory attribute in version 3), then serialization is irrelevant and the analyzer should probably skip those cases.

Statically analyzing serializability of enumerations

Can static analysis determine whether an enum type is from a local assembly or from the GAC?

ReflectionExtensions.IsFromLocalAssembly is implemented by trying to load the type's assembly by name, using reflection.

But analyzers are banned from using reflection, so another approach would be needed. I did not find any way of determining this through static analysis. Is there some way of doing this?

If not, then since we can never know whether they're local or from the GAC, would it be best to ignore enumeration types and not provide diagnostics for them?

[bradwilson]

Sorry for the delay in responding to this. I will look at the PR today.

I'd suggest providing a diagnostic for both non-serializable and possibly serializable TheoryData<> type arguments

This sounds reasonable. These should reported as two different diagnostics so that developers could easily disable the rule about "possibly not serializable value" without having to give up the "definitely not serializable value" rule.

The rule text should also include some language about why, so maybe the wording could be something like:

• The type argument {0} is not serializable, which will cause Test Explorer to not enumerate individual data rows. Consider using a type that is known to be serializable.
• The type argument {0} might not be serializable, which may cause Test Explorer to not enumerate individual data rows. Consider using a type that is known to be serializable.

Do you think DiagnosticSeverity.Info or DiagnosticSeverity.Warning would be more appropriate?

Since the issue is one of usability (in Test Explorer) and not one of correctness (since the tests run correctly), I think Info is the appropriate choice.

One key point to consider is that, if a developer has explicitly set DisableDiscoverEnumeration = true in a MemberData attribute (or Theory attribute in version 3), then serialization is irrelevant and the analyzer should probably skip those cases.

Definitely agree with this (and a true anywhere means you ignore all the data, not just the one with the flag). I'm just now realizing that we never added DisableDiscoveryEnumeration to ClassData...I suspect nobody's really flagged it much because its usage is probably much lower than MemberData. 😂

Can static analysis determine whether an enum type is from a local assembly or from the GAC?

A quick and obvious optimization: there is only a GAC with .NET Framework, so this test is unnecessary unless the test project targets .NET Framework.

As for a definitive determination? I'm not aware of one. While checking an enum, we could try to see if Roslyn has source-level access to the enum's definition (assuming that's doable and cheap), in which case we'd know it was safe since it came from the test project itself or one of the projects it references. Otherwise, everything else would fall into the "not sure" category.

[jared-chevalier]

Sorry for the delay in responding to this.

No problem!

This sounds reasonable. These should reported as two different diagnostics so that developers could easily disable the rule about "possibly not serializable value" without having to give up the "definitely not serializable value" rule.
The rule text should also include some language about why, so maybe the wording could be something like:
[...]
Since the issue is one of usability (in Test Explorer) and not one of correctness (since the tests run correctly), I think Info is the appropriate choice.

Sounds good.

One key point to consider is that, if a developer has explicitly set DisableDiscoverEnumeration = true in a MemberData attribute (or Theory attribute in version 3), then serialization is irrelevant and the analyzer should probably skip those cases.

Definitely agree with this (and a true anywhere means you ignore all the data, not just the one with the flag). I'm just now realizing that we never added DisableDiscoveryEnumeration to ClassData...I suspect nobody's really flagged it much because its usage is probably much lower than MemberData. 😂

Yes, I noticed that and wondered why ClassData didn't include that property as well!

Alright, I'll work on adding logic to ignore methods with one or more Theory/MemberData attributes with DisableDiscoveryEnumeration set to true.

Though what about preEnumerateTheories in a test project configuration file? If this is set to false, then discovery pre-enumeration will never happen for tests in that assembly, right? In which case, the analyzer should ignore all tests in that assembly? And if this is correct, what would be the best way of checking for that in the analyzer?

A quick and obvious optimization: there is only a GAC with .NET Framework, so this test is unnecessary unless the test project targets .NET Framework.
As for a definitive determination? I'm not aware of one. While checking an enum, we could try to see if Roslyn has source-level access to the enum's definition (assuming that's doable and cheap), in which case we'd know it was safe since it came from the test project itself or one of the projects it references. Otherwise, everything else would fall into the "not sure" category.

Alright, I'll look into this some more.

[bradwilson]

Though what about preEnumerateTheories in a test project configuration file? If this is set to false, then discovery pre-enumeration will never happen for tests in that assembly, right?

I agree, though file I/O is supposedly prohibited in analyzers, so I'm not sure how you figure this out (not to mention the logic for configuration files, while not super complex, is also not exactly trivial).

[bradwilson]

Fixed in xunit/xunit.analyzers#183