Optimize creation of string and value-type Dummies

[blairconrad]

As discussed in #1348 (comment)

(@blairconrad is the first speaker, @thomaslevesque the second)

But what if somebody has made a Dummy factory for Char[]? Then all of a sudden we've changed the string Dummies out from under them. 😄

I don't remember the priorities when creating a dummy... Don't we use user-provided dummy factories over built-in resolution strategies?

What if something else uses a ReadOnlySpan?

Maybe we could have a special case for this too, e.g. create an empty ReadOnlySpan. We'd have to do it via reflection, though, since we don't want to take a dependency on System.Memory

Ties to #1345.

[blairconrad]

I don't remember the priorities when creating a dummy... Don't we use user-provided dummy factories over built-in resolution strategies?

For sure, and this means that people could return whatever string they want. However, without any special consideration for string, we're currently finding one of the constructors and using it. On .NET Framework, I think it's String(Char[], Int32, Int32). So if someone had introduced a Dummy factory for Char[] or Int32, then our special-handling of String could potentially return something different than they expected. I am probably worrying way too much.

Maybe we could have a special case for this too, e.g. create an empty ReadOnlySpan. We'd have to do it via reflection, though, since we don't want to take a dependency on System.Memory

I'm not against it, but I think we'd have to do research. #1345 arose because our naive attempts to create a ReadOnlySpan<T> failed.

[thomaslevesque]

For sure, and this means that people could whatever string they want. However, without any special consideration for string, we're currently finding one of the constructors and using it. On .NET Framework, I think it's String(Char[], Int32, Int32). So if someone had introduced a Dummy factory for Char[] or Int32, then our special-handling of String could potentially return something different than they expected. I am probably worrying way too much.

I see your point, but I think it's unlikely that anyone would rely on a char[] dummy factory to control how dummy strings are created 😉

I'm not against it, but I think we'd have to do research. #1345 arose because our naive attempts to create a ReadOnlySpan<T> failed.

In fact, forget it; as mentioned by @ryanwischkaemper in #1345, Span/ReadOnlySpan are stack-only types, so we can't box them to return them from TryCreateDummyValue. The only thing we can do with those types is skip the constructors that accept them.

[blairconrad]

That's what I feared.

[blairconrad]

@thomaslevesque, if we're still interested in this, I'm happy to take the issue. So far it goes well.

In my sandbox I have special cases for

• string
• int
• long
• DateTime
• Guid

Are you interested in others? I can easily expand to all the integral types and floating-point types, but after that my imagination peters out.

[thomaslevesque]

To be honest, I don't remember exactly what problem we were trying to solve...
I know that for string the current approach is not ideal, because we're using ResolveByInstantiatingClassUsingDummyValuesAsConstructorArgumentsStrategy, which is not very efficient and relies on creating a dummy char[]. But what's the problem for value types? Is there any benefit in special-casing int or long? Currently they're processed by ResolveByActivatingValueTypeStrategy, which is probably fine.
I guess it makes sense to special-case DateTime (return Now?) or Guid (Guid.NewGuid()), though.

[blairconrad]

Your analysis is generally correct. For most cases, this is an optimization, at best. For string, it'd keep us from running through the constructors until we found the one that worked (the first time, after which we know which one works, and we just make its arguments and invoke it).

For the value types, there's not much benefit, except that we currently try to fake them before we call ResolveByActivatingValueTypeStrategy. We might want to just move that ahead of ResolveByActivatingValueTypeStrategy instead and call it a day. Or do nothing.

I'm not inclined to special case DateTime and Guid at this time - they've been returning essentially DateTime.MinDate and Guid.Empty forever, and I don't think anyone has minded. We could, but I'd separate the issue and save it for a major release, if you really want to do it.

Truth be told, after I created this issue, I did have second thoughts and was keeping it around mostly because I thought you were a fan.

I'd be content to close this, or severely limit the scope (to string?).

[blairconrad]

I've been thinking more about special-casing DateTime or Guid, and I think we shouldn't. The story behind Dummies has always been "this is what you get when you don't care about the result". We've made some tweaks, in that Tasks are completed and Lazys have values, but I don't see much benefit in further deviating, especially when it would introduce variability into clients' tests.

[thomaslevesque]

I think we should do it for string. Currently strings are created from an empty char array and the int values 0 and 0; this is just dumb...

You're probably right about DateTime and Guid, though.

And 👍 to move value type strategy ahead of fake strategy, since we know we can't fake value types.

[blairconrad]

Thanks for the merge, @thomaslevesque!

[blairconrad]

This change has been released as part of FakeItEasy 4.9.0.