Custom attributes, additional interfaces, and constructor parameters unused when we create a Fake delegate

[blairconrad]

Mentioned by @thomaslevesque in #1352 (comment)

We should

1. document this, as Creating Fakes does not mention it, and
2. make sure we throw a nice exception if attributes are supplied during creation

[thomaslevesque]

We should also investigate if it's possible at all to add attributes to fake delegates. Would that even make sense? I'm not sure, my brain is a bit slow today.

[thomaslevesque]

Would that even make sense?

It wouldn't make sense to apply attributes to the delegate type (because we're not creating a new type, it already exists) or to the delegate instance (because instances are not reflection objects, they don't have attributes). It might make sense to apply attributes to the method that the delegate instance encapsulates. I can't think of many real-world use cases, though.

We should also investigate if it's possible at all to add attributes to fake delegates.

I don't think it's possible, at least with our current approach to generate fake delegates. Using CompileToMethod instead of Compile might make it possible to apply attributes to the method, but CompileToMethod isn't available in .NET Core.

[blairconrad]

@thomaslevesque, after your last comment, I think we're left with "just document the behaviour and throw when attributes are supplied on delegate creation", no?

[thomaslevesque]

@thomaslevesque, after your last comment, I think we're left with "just document the behaviour and throw when attributes are supplied on delegate creation", no?

Yes, I think so.

[blairconrad]

Throwing when the user tries to add attributes to a delegate is easy, if we want to do it while trying constructors (of which I'd expect only one). We can elicit this error:

  Failed to create fake of type System.Func`1[System.Int32]:
   Below is a list of reasons for failure per attempted constructor:
    Constructor with signature (System.Object, System.IntPtr) failed:
      Cannot add custom attributes to faked delegates.

If we like that, I can send a PR real soon now.
I did start chasing a pre-constructor based rejection, but it starts to spread, not entirely unlike the recent "why can't we Dummy?" issue.
This is mostly due to the current practice of saying "if a delegate proxy generator isn't appropriate, then we can use the dynamic proxy generator".

Still, if there's perceived value in rejecting the construction without trying constructors, I'll carry on!

[thomaslevesque]

if we want to do it while trying constructors

I'm not sure which constructors you're talking about... We don't "try constructors" when faking delegates.

Couldn't we check for this in IFakeOptions.WithAttributes?

[blairconrad]

I'm not sure which constructors you're talking about... We don't "try constructors" when faking delegates

Excellent point. Was approaching it from the FakeObectCreator side, where we resolve constructor arguments for the delegate (and object and and int pointer, IIRC) and pass along to IProxyGenerator.GenerateProxy where, as you say, they are unused! The things I'm learning! Sadly, about the amount of waste in the system…

Couldn't we check for this in IFakeOptions.WithAttributes?

We could, if we wanted to spread knowledge of the various proxy types and what they can handle around the system. Right now it's a little more concentrated in the creation code.

I'm not too bothered by modifying WithAttributes, though. Would you like me to go for it? (Or if you'd like to go for it, that's fine too. I'm not trying to steal your idea and keep the fun of implementing.)

[thomaslevesque]

We could, if we wanted to spread knowledge of the various proxy types and what they can handle around the system. Right now it's a little more concentrated in the creation code.

Good point. So the proper place to check this would probably be DelegateProxyGenerator.GenerateProxy, no?

BTW, I just realized that it's not just the attribute list: we also ignore additionalInterfacesToImplement and argumentsForConstructor.

[blairconrad]

BTW, I just realized that it's not just the attribute list: we also ignore additionalInterfacesToImplement and argumentsForConstructor.

Yup. I noticed additionalInterfacesToImplement last night and argumentsForConstructor this morning as a result of your comment.

Good point. So the proper place to check this would probably be DelegateProxyGenerator.GenerateProxy, no?

I think so. So proper. Somewhat annoying. Okay. I'll keep plugging!

[blairconrad]

Good point. So the proper place to check this would probably be DelegateProxyGenerator.GenerateProxy, no?

Oh. That actually might work out easier than I was thinking. I'd thunk of DelegateProxyGenerator.CanGenerateProxy. But delaying until GenerateProxy might significantly simplify things! Thanks!

[blairconrad]

Ha. I'm foolish. I'd originally put the check in GenerateProxy, which is what gives us the error I pasted way upstairs. The one that yammers on about "constructors".

[thomaslevesque]

Ah. So maybe we need to special case delegates to not mention constructors...

[blairconrad]

I'm sensing a "ResolveStrategy" for Fakes to match the one we have for Dummies. :)

That might actually not be a bad approach. MethodCanBeInterceptedOnInstance might cause a hiccough, though. Or maybe not…

[thomaslevesque]

Maybe not a bad approach, but probably overkill...
For now, maybe it's easier to just check in IFakeOptions.WithAttributes, although it's not ideal. No sense in upsetting everything to fix an issue that probably isn't bothering anyone.

[blairconrad]

Oh, but I'm having fun. Let me go a bit longer and then I can send a PR and you can reject it and I'll do the IFakeOptions.WithAttributes thing!

[thomaslevesque]

Lol! OK, have a go at it then ;)

[blairconrad]

Relabelling as a bug, since we're no longer planning on supporting the attributes (interfaces, constructor arguments…)

[blairconrad]

This change has been released as part of FakeItEasy 4.8.0.