AssignmentPatterns can break with two out parameters from the same method

[vitek-karas]

Currently we track each assignment pattern based on its Origin - which really means method + IL offset. So if the system reports two assignment patterns for the same IL offset we'll treat them as the same assignment operation (and merge the values as per the recent multi-method scanning). This causes problems if there are two out parameters for the same callsite:

			[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)]
			static Type _publicMethodsField;

			[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicProperties)]
			static Type _publicPropertiesField;

			static void TwoOutRefs(
				[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)]
				out Type publicMethods,
				[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicProperties)]
				out Type publicProperties)
			{
				publicMethods = null;
				publicProperties = null;
			}

			public static void Test()
			{
                                // IL2069 since we think the value from publicMethods parameter is assigned to _publicPropertiesField
				TwoOutRefs (out _publicMethodsField, out _publicPropertiesField);
			}

Out (and ref) parameters get assignments after each method call (source value if the parameter value of the called method). In this both assignments will have the same Origin (the same callsite) and thus will be merged. If the annotations between the two parameters don't match this will produce warnings.

[vitek-karas]

The problem is using the Origin as the key for assignments, we will need to come up with something better. We've already ran into this problem with return values and solved it by adding a boolean to the key, but that's not enough for this case.
In theory we should use the target value, but given our analysis that can be a multi-value and this gets fuzzy. In this specific case we could use the source value (the method parameter), but that only works in this case.

I think we may need to add a "custom" key part - a differentiator. Each place where we create an assignment pattern will need to specify this (could be empty). We already have two such use cases (return values and out parameters), but there may be more.

Note that we can't get rid of the imprecise Origin either - if there are two callsites to the same method in the same method body we need the IL offset to differentiate between them. So it has to be a composite key with Origin being one part of it.

[vitek-karas]

@jtschuster @sbomer - I welcome any ideas how to solve this.

[vitek-karas]

dotnet/linker#2875 adds a test which hits this.

[jtschuster]

@jtschuster Jackson Schuster FTE @sbomer Sven Boemer FTE - I welcome any ideas how to solve this.

My first thought is that we could copy the same logic as return values if we have a RefParameterValue. Then if the source value in TrimAnalysisValue.Add() is a RefParameterValue, we don't merge the values. I don't fully understand the TrimAnalysisPattern though, so that might not be a full solution.

Or maybe we could use the targeted (Field/LocalVariable/Parameter)Definition with the origin as the key

[vitek-karas]

I think this is very close to what I proposed above. The main difference is that I'm not sure we should handle this in the TrimAnalysisPatternStore.Add - maybe we should push this onto the callers as they have more context.

Honestly the current code is relatively weird already, since it will handle return values only if they're the only target - which I think is correct, but it makes assumptions which I would like to keep out of the general store.

[sbomer]

If we can handle it inside of the pattern store, I would do that. I see the pattern store not as a general-purpose thing, but as modeling the specific kinds of source/target mismatches we can encounter in the dataflow. Letting callers introduce their own context would make it harder to reason about the kinds of patterns track.

The return value logic is just a workaround for this bug: dotnet/linker#2778.

What information do we need to disambiguate the ref param case? I think it would be enough to just store the index of the parameter. For out params, the source will just be the annotated out parameter. For ref params since the parameter acts as both a source and a target, we'd end up merging the patterns and storing {annotated ref parameter, passed-in annotated value} as both the source and target. This would have some redundant cases since it will check the parameter against itself and the annotated value against itself, but I think it captures the behavior we want.

[ghost]

Tagging subscribers to this area: @dotnet/area-system-reflection
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Currently we track each assignment pattern based on its Origin - which really means method + IL offset. So if the system reports two assignment patterns for the same IL offset we'll treat them as the same assignment operation (and merge the values as per the recent multi-method scanning). This causes problems if there are two out parameters for the same callsite:

			[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)]
			static Type _publicMethodsField;

			[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicProperties)]
			static Type _publicPropertiesField;

			static void TwoOutRefs(
				[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)]
				out Type publicMethods,
				[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicProperties)]
				out Type publicProperties)
			{
				publicMethods = null;
				publicProperties = null;
			}

			public static void Test()
			{
                                // IL2069 since we think the value from publicMethods parameter is assigned to _publicPropertiesField
				TwoOutRefs (out _publicMethodsField, out _publicPropertiesField);
			}

Out (and ref) parameters get assignments after each method call (source value if the parameter value of the called method). In this both assignments will have the same Origin (the same callsite) and thus will be merged. If the annotations between the two parameters don't match this will produce warnings.

Author:	vitek-karas
Assignees:	-
Labels:	area-System.Reflection
Milestone:	-

[vitek-karas]

This also applies to NativeAOT.