Create ParameterProxy to wrap logic surrounding parameters and use one ParameterIndex enum to index
#3059
Conversation
src/ILLink.RoslynAnalyzer/DynamicallyAccessedMembersAnalyzer.cs
Outdated
Show resolved
Hide resolved
Use struct enumerator to avoid allocating when calling GetParameters Ternary formatting Leave property reference and indexer reference separate
| && !calledMethod.IsStatic () | ||
| && calledMethod.HasImplicitThis () | ||
| && calledMethod.HasMetadataParametersCount (1) | ||
| && calledMethod.HasParameterOfType ((ParameterIndex) 1, "System.Reflection.BindingFlags") |
There was a problem hiding this comment.
It's a little funny to see a check that the count is 1, then retrieving index 1. Maybe instead of casting an int to ParameterIndex we could add ParameterIndex.FromMetadataIndex (0) or similar? And similar with the other casts - I would prefer to be explicit about what the int means.
There was a problem hiding this comment.
I think as we get to the point where each IntrinsicId corresponds to exactly one method, we'll be able to use only HasImplicitThis and HasParameterCount. I only made HasMetadataParameterCount to make it simpler to transition everything in the first PR, and then we can remove all uses of it in a separate PR with all the intrinsics specificity improvements.
For being more explicit on what the int means in the casts to ParameterIndex, I'm not sure if calling a method on an int makes it any clearer than a cast, either way you need to check the xml docs to make sure you understand what the int represents when you cast it or pass it as an argument.
There was a problem hiding this comment.
either way you need to check the xml docs to make sure you understand what the int represents
My thinking was that it's clearer if the method name indicates what the int means, whereas the cast relies on knowledge of the ParameterIndex internal representation. I also think in this particular callsite it would be clearer if we used index 0 (with FromMetadataIndex or similar).
The plan is to get rid of HasMetadataParametersCount would mostly address my initial concern though - I'll leave it up to you.
[main] Update dependencies from dotnet/arcade
[main] Update dependencies from dotnet/runtime
…RC2 SDK. Took the versions from dotnet/runtime.
Ban HasParameters Use Pragma instead of attribute for banned symbol warning suppression
| { | ||
| if (IsImplicitThis) | ||
| return Method.Method; | ||
| #pragma warning disable RS0030 // MethodReference is banned -- this class provides wrappers to use |
There was a problem hiding this comment.
Personally I would put this around the entire class - it's a proxy so it is meant to access the underlying thing directly.
There was a problem hiding this comment.
I wanted to disable the warning on as little code as possible since there are other banned symbols, and a different banned symbol could sneak in if we disable RS0030 on the whole class. It's not pretty, but I think it might be better to limit the scope of disabled warnings.
- Rename ParameterCollection - Fix comment typos - Use "this" as ParameterProxy.GetDisplayName for implicit this parameter - Remove empty file
…one `ParameterIndex` struct to index (dotnet/linker#3059) This reverts commit dotnet/linker@45e2e59. This commit creates a 'ParameterProxy' type and 'ParameterIndex' to represent parameters in a consistent way. This is motivated by using 'int' to represent different ways of counting parameters (sometimes offset by 1 for the 'this' parameter, sometimes not offset). ParameterIndex should now be the only type used to index into a method's parameters. ParameterIndex represents the index of the parameter as it is passed to the method in IL (i.e. 'this' is the 0 index in instance methods). ParameterProxy wraps the common convention of a MethodProxy and int to represent a parameter, and now holds a Method and a ParameterIndex to encapsulate a Parameter. No behavioral changes are expected from this change. Co-authored-by: Vitek Karas <[email protected]> Co-authored-by: dotnet-maestro[bot] <42748379+dotnet-maestro[bot]@users.noreply.github.com> Commit migrated from dotnet/linker@ca39998
…one `ParameterIndex` struct to index (dotnet/linker#3059) This reverts commit dotnet/linker@45e2e59. This commit creates a 'ParameterProxy' type and 'ParameterIndex' to represent parameters in a consistent way. This is motivated by using 'int' to represent different ways of counting parameters (sometimes offset by 1 for the 'this' parameter, sometimes not offset). ParameterIndex should now be the only type used to index into a method's parameters. ParameterIndex represents the index of the parameter as it is passed to the method in IL (i.e. 'this' is the 0 index in instance methods). ParameterProxy wraps the common convention of a MethodProxy and int to represent a parameter, and now holds a Method and a ParameterIndex to encapsulate a Parameter. No behavioral changes are expected from this change. Co-authored-by: Vitek Karas <[email protected]> Co-authored-by: dotnet-maestro[bot] <42748379+dotnet-maestro[bot]@users.noreply.github.com> Commit migrated from dotnet/linker@ca39998
3 participants
Creates
ParameterProxyto represent parameters as a pair of method + parameter index.IParameterSymbolandParameterDefinitiondon't usually include the implicit 'this' parameter, so this lets us treat the implicit 'this' parameter as the same as any other parameter. This also should help put all theHasImplicitThis() ? 1 : 0in a limited number of places (ParameterProxyand the extensions methods that produce them).ParameterIndexrepresents 'this' as the 0 index. See the code docs for more info. I found it was best to represent the 'this' as 0 and provide a property on theParameterProxyfor indexing int theParameterscollection (int MetadataIndex)."Metadata Parameter" is what I used to name the index in the
.parametersmetadata section, which corresponds to the SourceParamterIndex from before.In the process, I found an issue in the analyzer where some properties were indexers but were not being passed the index value in
HandleCallAction. Since we previously only iterated over the arguments, no issues arose, but after changing to iterate over the parameters in the called method signature, the issue showed up. LocalDataflowVisitor has been updated to visit the indexer argument and pass the value toHandleCallAction.