[generator] Use 'var' when possible in generated code to reduce nullability annotations.

[jpobst]

Our generated code may predate var, or at least predate var becoming widely accepted. As such, we generate code like this:

java.code.MyClass __this = global::Java.Lang.Object.GetObject<java.code.MyClass> (jnienv, native__this, JniHandleOwnership.DoNotTransfer);
string key = JNIEnv.GetString (native_key, JniHandleOwnership.DoNotTransfer);

In an NRT world, each one of these declarations will have to be audited and optionally annotated with the nullable operator ?. Or..... we can just mark them as var and not worry about it.

var __this = global::Java.Lang.Object.GetObject<java.code.MyClass> (jnienv, native__this, JniHandleOwnership.DoNotTransfer);
var key = JNIEnv.GetString (native_key, JniHandleOwnership.DoNotTransfer);

Pulling this change into a separate commit in order to lower the noise in the eventual NRT PR.

[jonpryor]

I feel like, as-is, this only moves the warning, it doesn't "fix" the warning.

Consider ISpannableInvoker.n_GetSpanFlags_Ljava_lang_Object_(): https://github.com/xamarin/java.interop/blob/80b4667f7085027d66592a0c36490d9488c7ce60/tests/generator-Tests/Tests-Core/expected.ji/Android.Text.ISpannable.cs#L74-L80

Updating line 76 to use var avoids a possible CS8600, but that still leaves line 78, which would instead produce a CS8602 warning:

int __ret = (int) __this.GetSpanFlags (tag);
// warning CS8602: Dereference of a possibly null reference.

Replacing CS8600 with CS8602 doesn't feel like a "win".

We could fix the CS8602 by using __this?.GetSpanFlags(tag), but that replaces the CS8602 warning with a CS0266 error:

error CS0266: Cannot implicitly convert type 'int?' to 'int'.

Which is to say, this feels like a losing game. It's simple-yet-massive change which won't really fix what it attempts to fix.

I fear that the "real" fix will be to just stick #nullable disable and #nullable enable around each method body:

staticintn_GetSpanFlags_Ljava_lang_Object_(IntPtrjnienv,IntPtrnative__this,IntPtrnative_tag){
#nullable disable
Android.Text.ISpannable__this=global::Java.Lang.Object.GetObject<Android.Text.ISpannable>(jnienv,native__this,JniHandleOwnership.DoNotTransfer);Java.Lang.Objecttag=global::Java.Lang.Object.GetObject<Java.Lang.Object>(native_tag,JniHandleOwnership.DoNotTransfer);int__ret=(int)__this.GetSpanFlags(tag);return__ret;
#nullable enable
}

which would be an even larger change, diff wise, except we should only emit this if e.g. --lang-features=nullable-reference-types is used so that older compilers can still compile our code. (If that's even a requirement? This might be debatable.)

[jpobst]

Correct, this PR does not attempt to introduce nullability annotations, or fix the right-hand side of the equals sign, it simply fixes the left-hand side.

The NRT PR will need to generate this when NRT is turned off:

var __ret = (int) __this.GetSpanFlags (tag);

and this when NRT is turned on:

var __ret = (int?) __this.GetSpanFlags (tag);

This PR makes our eventual NRT logic simpler by not needing to conditionally annotate the left-hand side.

When NRT is turned off (<nullable>disable</nullable>) the compiler will not check the annotations and the first example will not generate a warning.

These tests are the current tests and will continue testing the !NRT case, so they will not change in the NRT PR. New tests will be added for the NRT case. Thus we can move the "noise" of updating the non-NRT tests to this PR.

[jonpryor]

and this when NRT is turned on:

var __ret = (int?) __this.GetSpanFlags (tag);

I assume this should use __this?.GetSpanFlags(tag), as __this would be of type ISpannable??.

However, that won't work, because the return type of the method will still be int -- and can't change! -- resulting in the same CS0266 error.

Additionally, in this context this doesn't seem quite right? The only time that Object.GetObject<T>() should return null is if the handle parameter is IntPtr.Zero. (Or if someone passes in a JNI Weak Reference to an instance which has been collected, but that feels like a very rare corner case.) Most of the time this method will never return null.

Given how rare the return value will actually be null, should Object.GetObject<T>() return T? in the first place?

(Unfortunately we can't use NotNullIfNotNullAttribute as that doesn't "work" meaningfully with IntPtr, only things for which null can be implicitly converted.)

After discussing on #onedotnet, it feels like Object.GetObject<T>() should return T? even if it's highly unlikely, as @spouliot believes that the "iOS equivalent" in ObjCRuntime.Runtime.GetNSObject(IntPtr) will also be updated to return NSObject?.

[jpobst]

I think in this specific case, the NRT PR uses the null-forgiving operator (!) to suppress the warning, as we are reasonably sure GetObject<T> will not return null due to the way our bindings are structured:

var __this = global::Java.Lang.Object.GetObject<Android.Text.ISpannable> (jnienv, native__this, JniHandleOwnership.DoNotTransfer)!;

As stated in the NRT non-goals, we are not attempting to rewrite our generated code to be more provably non-null at this time. We are working under the assumption that our current generated code works and thus we are currently preventing an NRE here via other means.

However, for a user calling GetObject<T> directly, I have less faith that they have done enough due diligence to ensure they are always passing in a valid handle.

[jonpryor]

Context: https://github.com/xamarin/xamarin-android/pull/4227
Our generated code *may* predate `var`, or at least predate `var`
becoming widely accepted. As such, we generate code like this:
java.code.MyClass __this = global::Java.Lang.Object.GetObject<java.code.MyClass> (jnienv, native__this, JniHandleOwnership.DoNotTransfer);
string key = JNIEnv.GetString (native_key, JniHandleOwnership.DoNotTransfer);
Migrate this code to `var` as it simplifies `generator` and reasoning
about behavior when "cross-compiling" code between C#7 and C#8-with-
[Nullable Reference Type][0] toolchains:
var __this = global::Java.Lang.Object.GetObject<java.code.MyClass> (jnienv, native__this, JniHandleOwnership.DoNotTransfer);
var key = JNIEnv.GetString (native_key, JniHandleOwnership.DoNotTransfer);
[0]: https://docs.microsoft.com/en-us/dotnet/csharp/nullable-references