Share intrinsic handling of GetMember and similar APIs #2639
Conversation
| private partial IEnumerable<SystemReflectionMethodBaseValue> GetMethodsOnTypeHierarchy (TypeProxy type, string name, BindingFlags? bindingFlags) | ||
| { | ||
| foreach (var method in type.Type.GetMethodsOnTypeHierarchy (m => m.Name == name, bindingFlags)) | ||
| yield return new SystemReflectionMethodBaseValue (new MethodProxy (method)); |
There was a problem hiding this comment.
I think this is going to cause a quadratic number of calls in the depth of the tree. Since iterators are lazy, we'll walk all the way down the tree to fetch the first element, then walk n-1 levels to fetch the second, and so on.
There was a problem hiding this comment.
Sorry I don't understand. The GetMethodsOnTypeHierarchy helper we're calling here already uses iterator, so this is basically just a pass through...
I guess I could rewrite both to use ImmutableArray...
I must admit I don't understand what the problem is.
| @@ -88,6 +104,35 @@ public override MultiValue VisitLiteral (ILiteralOperation literalOperation, Sta | |||
| return literalOperation.ConstantValue.Value == null ? NullValue.Instance : TopValue; | |||
| } | |||
|
|
|||
| public override MultiValue VisitBinaryOperator (IBinaryOperation operation, StateValue argument) | |||
There was a problem hiding this comment.
I tried this locally and it looks like operation has a constant value for many of the tests. Is there a specific case I'm missing where it doesn't and we need to evaluate manually? And is it worth checking the ConstantValue anyway for the easy cases?
There was a problem hiding this comment.
You're right - I was looking at the ArgumentOperation which is the parent of this and that one doesn't have a constant value. I'll improve this to use the Roslyns const evaluator when available.
There was a problem hiding this comment.
You could maybe put it in the top-level Visit(IOperation) helper, unless there's some reason we would want to skip that.
There was a problem hiding this comment.
I'm kind of split on that - it's semi-dangerous. It would mean that the actual specific operation visitor method would not have final say in what the return value is. Maybe not a big deal... I'll probably do it.
There was a problem hiding this comment.
I went with that approach - Visit(IOperation) now handles constants (if the operation specific visit doesn't provide a value). It solves lot of easy cases and works generally for things like null and strings as well.
I kept the BinaryOperation visitor as there are cases where it still works (if one or both of the operands are local variables for example - that requires data flow to figure out values, so constant detection itself doesn't do it) and also as a sample what we would need to add for other operators if necessary.
vitek-karas
force-pushed
the
IntrinsicMembers
branch
from
6697124 to
f7e7672
Compare
Other than sharing more code and adapting it so that it works on both linker and analyzer, this change brings simple analysis of constant integer values in the analyzer. This is necessary to make most reflection API calls recognize binding flags. For example `GetMethods(BindingFlags.Public | BindingFlags.Static)`. So this change adds analysis of constant values (as recognized by the Roslyn's operation tree) and the OR binary operator for integers and enums. Added some new tests for the binding flags handling. Reenabled some disabled tests for analyzer. Moved the main affected tests from the generated source files to the hardcoded one and force them to exact match of warnings for both linker and analyzer.
Detect constant expressions generally for all operations and return the constant value representation.
vitek-karas
force-pushed
the
IntrinsicMembers
branch
from
f7e7672 to
1e989bf
Compare
…#2639) Other than sharing more code and adapting it so that it works on both linker and analyzer, this change brings simple analysis of constant integer values in the analyzer. This is necessary to make most reflection API calls recognize binding flags. For example `GetMethods(BindingFlags.Public | BindingFlags.Static)`. So this change adds analysis of constant values (as recognized by the Roslyn's operation tree) and the OR binary operator for integers and enums. Added some new tests for the binding flags handling. Reenabled some disabled tests for analyzer. Moved the main affected tests from the generated source files to the hardcoded one and force them to exact match of warnings for both linker and analyzer. Commit migrated from dotnet/linker@f0bd2ae
Other than sharing more code and adapting it so that it works on both linker and analyzer, this change brings simple analysis of constant integer values in the analyzer. This is necessary to make most reflection API calls recognize binding flags. For example
GetMethods(BindingFlags.Public | BindingFlags.Static). So this change adds analysis of constant values (as recognized by the Roslyn's operation tree) and the OR binary operator for integers and enums.Added some new tests for the binding flags handling.
Reenabled some disabled tests for analyzer.
Moved the main affected tests from the generated source files to the hardcoded one and force them to exact match of warnings for both linker and analyzer.