Handling of byref return values and locals in data flow analysis

[vitek-karas]

This issue is now tracking all the various cases of ref locals and return values for both the linker an analyzer.
It's referred to from some tests, so just search the codebase for the issue number to find some of the known cases where it's problematic (definitely not all).

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

			static ref Type GetAnnotatedRefType () { return ref _annotatedField; } // No warning currently

			static void ByRefReturnValue ()
			{
				ref Type typeShouldHaveAllMethods = GetAnnotatedRefType();
				typeShouldHaveAllMethods = typeof(TestType); // Doesn't apply annotations
				_annotatedField.GetMethods (); // Doesn't warn, but now contains typeof(TestType)
			}

We will need to think this through, how the system should work around byref values.
In a way handling of byref values needs to effectively work "in reverse". Meaning that we need to enforce when assigning to a byref value that the byref value has at least the same annotations as the source value. But then assigning a known type needs to apply annotations from the byref to the type... so it's like writing to an annotated field.

There's also a reflection based test which also needs to correctly warn:

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

			[return: DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)]
			static ref Type GetAnnotatedRefType () { return ref _annotatedField; }

			delegate ref Type DelegateOverGetRefType ();
			static void ByRefReturnValue ()
			{
				var d = (DelegateOverGetRefType)typeof (AnnotatedMethodReturnValue)
					.GetMethod (nameof (GetAnnotatedRefType))
					.CreateDelegate (typeof (DelegateOverGetRefType));

				ref Type typeShouldHaveAllMethods = ref d ();
				typeShouldHaveAllMethods = typeof (TestType); // Doesn't apply annotations
				_annotatedField.GetMethods (); // Doesn't warn, but now contains typeof(TestType)
			}

[sbomer]

We will also need to ensure that this works correctly in the analyzer, as mentioned in #2390 (comment). Let's use this as a tracking issue for both the linker and the analyzer work.

[MichalStrehovsky]

Basically it reverses data flow direction to a degree....

Does it really affect dataflow direction? There's just some C# language level restrictions/relaxations for each, but I think the data flows the same way (e.g. for out methods one won't ever see a read from the variable before there was an assignment because the language won't allow it, so all reads will see a known value that we saw being assigned, but that just falls out from how the analysis works).

For the formatting as ref/out, it might be worth pointing out that illink as an IL tool is not tied to C#. The language might choose all sorts of keywords (there's also in introduced recently), but they don't have to map to how VB or F# do it. The & matches how the reflection stack represents this.

[vitek-karas]

I must admit I haven't spent too much time thinking about this (recently, I did when creating this issue :-)). I looked into it a bit more and I think you're right. There's probably no backward data flow necessary, but there are holes in the analysis.
I'm adding some more tests for these cases here: #2407

[vitek-karas]

Regarding the formatting - so far most of the formatting has been copying C# syntax. The tool is IL and so the reflection syntax would be natural fit, on the other hand, I suspect that lot of developers will be confused by it. That said, for the by ref it's probably not as important (should be very rare).