Skip to content
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.

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

The use of Action<...> instead of Expression<Action<...>> in the public API causes a variety of problems #414

Closed
4 tasks done
stakx opened this issue Jul 11, 2017 · 2 comments
Labels

Comments

@stakx
Copy link
Contributor

stakx commented Jul 11, 2017

Action<TMock, ...>s are used instead of Expression<Action<TMock, ...>> because the compiler doesn't support e.g. assignments in expression tree lambdas. Moq solves this by then executing the setup action "expression" in some kind of dry-run mode and observing the side effects caused. The internal component used for this trickery is called FluentMockContext, and because it's basically an incomplete workaround around a missing language feature, it causes a variety of problems.

Many of them are related to recursive mocks setup: [...] At least in some cases, it seems probable that these issues could be caused by the (internal) use of FluentMockContext to execute the setup "expression" in order to install the inner mocks relationships and set up all properties on the inner mocks. (These are actually unrelated to this and are now being tracked in #643.)

In order for the setup expression to be executable, its expression tree must be compiled (which btw. in #188 is given as the main reason for Moq's bad performance on 64-bit .NET). Compiling an expression tree to a regular Func<> or Action<> also means that matchers cannot properly work. (Don't know what I was thinking, this is a non sequitur.)

The use of FluentMockContext also causes the following problem(s) which aren't about recursive mocks:

I'm closing the above issues in favor of this one.

It might be a good idea to replace the use of FluentMockContext to "dry-run" setup expressions and instead do proper analysis of the expression trees, wherever possible. The potential advantages are:

  • Make matchers (It.Is…) work.
  • Avoid setting up more properties than necessary.
  • Avoid overriding existing setups.
  • Improve Moq performance by avoiding expensive expression tree compilation.
@stakx stakx changed the title Meta-issue: Using FluentMockContext to set up recursive mocks causes a variety of problems Meta-issue: FluentMockContext (used e.g. to set up recursive mocks) causes a variety of problems Jul 28, 2017
@stakx
Copy link
Contributor Author

stakx commented Oct 4, 2017

It might be a good idea to replace the use of FluentMockContext to "dry-run" setup expressions and instead do proper analysis of the expression trees, wherever possible. The potential advantages are:

  • Make matchers (It.Is…) work.
  • [...]
  • Improve Moq performance by avoiding expensive expression tree compilation.

The best solution to reach those goals would be for C# to support assignments in expression tree lambdas. See dotnet/csharplang#158 (comment).

@stakx stakx changed the title Meta-issue: FluentMockContext (used e.g. to set up recursive mocks) causes a variety of problems The use of Action<...> instead of Expression<Action<...>> in the public API causes a variety of problems Oct 4, 2017
@stakx
Copy link
Contributor Author

stakx commented Jul 13, 2018

As much as I would love to see all of these bugs fixed, this isn't actually possible unless either...

  • the Roslyn (C# and VB.NET) compilers start supporting assignments in LINQ expression trees; or
  • Moq switches from method compilation to method decompilation, which would mean that a very robust method decompiler must be implemented, which is unlikely to be practical so shortly before Moq 5.

I will keep tracking dotnet/csharplang#158 (comment), but in the meantime I am closing this issue since unfortunately there's nothing much that we can do about these bugs. 😢

@stakx stakx closed this as completed Jul 13, 2018
@stakx stakx removed the unresolved label Mar 10, 2019
@devlooped devlooped locked and limited conversation to collaborators Sep 7, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

1 participant