Extend the set of expressions supported by CustomAttributeInfo.FromExpression

[thomaslevesque]

Prompted by https://stackoverflow.com/q/54044725/98713

Currently the expression analyzer only supports constants and arrays as the constructor arguments. This is enough in most scenarios, but not if the argument values need to be computed at runtime. Supporting any arbitrary expression would be difficult (or would involve compiling and executing expressions, which is slow), but it should at least be possible to use local variables.

[thomaslevesque]

Since I wrote the original FromExpression method, I volunteer for doing this.

[stakx]

While I have some reservations about that API (see below), I have no objections to expanding CustomAttributeInfo.FromExpression, as long as this doesn't blow up out of proportion (also see below).

it should at least be possible to use local variables

It might make sense to also cover method arguments, since from a user's usage point of view, they are often near indistinguishable from local variables. And you might also have to add support for fields, too, since the .NET compilers can turn captured locals and arguments into "display class" fields.

This is what makes me afraid that this could blow up to a fairly large amount of code. As soon as LINQ expression trees get involved, one quickly ends up with the need for a full-blown interpreter that accepts almost all possibilities (and you correctly said yourself that this is probably undesirable here).

Supporting any arbitrary expression would be difficult (or would involve compiling and executing expressions, which is slow)

Perhaps compilation & execution could be used as a final fallback method of evaluating arguments, so you'd have the most common use cases covered by specialized handling and if someone does something unusual, things still work albeit with a performance penalty. Having such a "slow but general-purpose" fallback might mean we won't need to expand this method yet again in the future...?

@castleproject/committers, in a more ideal future world, I think, DynamicProxy should stay away from LINQ expression trees and let the library layers above it handle these. It appears that we already have a bunch of lower-level factory methods in AttributeUtil—unfortunately they were placed in a ".Internal" namespace, suggesting that these factory methods aren't part of the public API.

Perhaps the public API should be rectified by bringing those low-level static factory methods over to CustomAttributeInfo (where the FromExpression method in question is located), then deprecate FromExpression in favor of the lower-level methods and let higher-up libs implement FromExpression in term of the lower-level methods).

[thomaslevesque]

It might make sense to also cover method arguments, since from a user's usage point of view, they are often near indistinguishable from local variables.

Yes, that was my intention. In fact, it makes no difference in terms of implementation, since arguments are captured in the same way as local variables.

And you might also have to add support for fields, too, since the .NET compilers can turn captured locals and arguments into "display class" fields.

I already started implementing it, and I'm accessing locals as fields of the display class created by the capture. Currently I'm doing this only for the display class, i.e. I don't support accessing fields that are not actually locals. I think it makes the rules easier to understand, but I could remove this limitation if you prefer.

Perhaps compilation & execution could be used as a final fallback method of evaluating arguments, so you'd have the most common use cases covered by specialized handling and if someone does something unusual, things still work albeit with a performance penalty. Having such a "slow but general-purpose" fallback might mean we won't need to expand this method yet again in the future...?

Honestly, I think it would be overkill. It's probably not a very widely used method, and being able to use locals should enable most reasonable scenarios.

[thomaslevesque]

DynamicProxy should stay away from LINQ expression trees

For more details on how they were introduced in DynamicProxy, you can have a look at #219. It seemed a good idea at the time; but yes, I guess it could have been done in a higher-level library.

[stakx]

@thomaslevesque - Thanks for the pointer to that old PR. It seems you've got a clear idea how to draw the line to keep this from exploding, so I'm content to let you work out the details. I'll be happy to review your PR once it's ready.

[thomaslevesque]

I'll be happy to review your PR once it's ready.

As far as I'm concerned, it's ready; I only marked it WIP because I know there are details that are up to discussion.

[blairconrad]

being able to use locals should enable most reasonable scenarios

Perhaps. I worry about someone making a method call from within the expression.

[thomaslevesque]

Perhaps. I worry about someone making a method call from within the expression.

Yes, but it's not a major issue as long as locals are supported, because they can call the method outside the expression and store the result in a local. As long as the error message is clear enough, at least it doesn't prevent legitimate scenarios.