Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[generator] Only skip generic "overloads" of bound types #178

Merged
merged 1 commit into from
Aug 22, 2017

Conversation

jonpryor
Copy link
Member

Fixes: https://bugzilla.xamarin.com/show_bug.cgi?id=57828

The #runtime team is attempting to update xamarin-android to use the
mono/2017-06 and mono/2017-08 (and...) branches, and when
doing so they are seeing a unit test fail in
Xamarin.Android.Build.Tests.BuildTest.GeneratorValidateMultiMethodEventName():
no BG850* warnings are expected but they are produced.

After much analysis, the cause of ght BG850* warnings is that
the mono/2017-06 branch introduces a non-generic "overload" of
System.Collections.Generic.KeyValuePair:

namespace System.Collections.Generic {
	// New with mono/2017-06
	partial class KeyValuePair {
		public static KeyValuePair<TKey, TValue> Create<TKey, TValue>(TKey key, TValue value);
	}

	// Existing since forever
	partial struct KeyValuePair<TKey, TValue> {
	}
}

Specifically, the above "KeyValuePair overload" type runs into a
FIXME within CodeGenerator.cs:

// FIXME: at some stage we want to import generic types.
// For now generator fails to load generic types that have conflicting type e.g.
// AdapterView`1 and AdapterView cannot co-exist.
// It is mostly because generator primarily targets jar (no real generics land).

The intent of the check that is that, given a type AdapterView<T>,
we only check to see if AdapterView exists as well. If it does exist,
then we ignore the AdapterView<T> type, and only use the
AdapterView type for code generation.

In the case of KeyValuePair, the same "check" is done, which cause
us to skip type registration for KeyValuePair<TKey, TValue>, which
in turn causes us to invalidate e.g. IDictionary<TKey, TValue> -- as
it implements ICollection<KeyValuePair<TKey, TValue>> -- and
everything promply falls apart from there:

warning BG8C00: For type System.Collections.Generic.IDictionary`2, base interface System.Collections.Generic.ICollection`1<System.Collections.Generic.KeyValuePair`2<TKey,TValue>> is invalid.
warning BG8C00: For type System.Collections.Generic.IDictionary`2, base interface System.Collections.Generic.IEnumerable`1<System.Collections.Generic.KeyValuePair`2<TKey,TValue>> is invalid.
warning BG8502: Invalidating System.Collections.Generic.IDictionary`2 and all nested types because some of its interfaces were invalid.
warning BG8801: Invalid parameter type System.Collections.Generic.IDictionary`2<System.String,System.Collections.Generic.IList`1<System.String>> in method MapEquivalents in managed type Java.Util.Locale.LanguageRange.
warning BG8801: Invalid parameter type System.Collections.Generic.IDictionary`2<System.String,System.Collections.Generic.IList`1<System.String>> in method Parse in managed type Java.Util.Locale.LanguageRange.
warning BG8801: Invalid parameter type System.Collections.Generic.IDictionary`2<System.String,System.Object> in method NewFileSystem in managed type Java.Nio.FileNio.Spi.FileSystemProvider.
warning BG8801: Invalid parameter type System.Collections.Generic.IDictionary`2<System.String,System.Object> in method NewFileSystem in managed type Java.Nio.FileNio.Spi.FileSystemProvider.
warning BG8801: Invalid parameter type System.Collections.Generic.IDictionary`2<System.String,System.String> in method .ctor in managed type Java.Security.Provider.Service.
warning BG8801: Invalid parameter type System.Collections.Generic.IDictionary`2<System.Object,System.Object> in method PutAll in managed type Java.Security.Provider.

I still don't fully understand the scenario that the check was
attempting to solve, so in lieu of actually fixing the FIXME, make the
check stricter so that we only ignore "generically overloaded" types
if they bind the same Java type. Since KeyValuePair<TKey, TValue>
binds no Java type, it will no longer be skipped, thus removing the
BG8502 and related warnings.

Additionally, a bit of code cleanup for consistency: some parts of the
code use HAVE_CECIL, and others use USE_CECIL. Standardize on
HAVE_CECIL for consistency and sanity.

Fixes: https://bugzilla.xamarin.com/show_bug.cgi?id=57828

The #runtime team is attempting to update xamarin-android to use the
[mono/2017-06][0] and [mono/2017-08][1] (and...) branches, and when
doing so they are seeing a unit test fail in
`Xamarin.Android.Build.Tests.BuildTest.GeneratorValidateMultiMethodEventName()`:
no `BG850*` warnings are expected but they are produced.

After much analysis, the cause of ght `BG850*` warnings is that
the mono/2017-06 branch introduces a non-generic "overload" of
`System.Collections.Generic.KeyValuePair`:

	namespace System.Collections.Generic {
		// New with mono/2017-06
		partial class KeyValuePair {
			public static KeyValuePair<TKey, TValue> Create<TKey, TValue>(TKey key, TValue value);
		}

		// Existing since forever
		partial struct KeyValuePair<TKey, TValue> {
		}
	}

Specifically, the above "`KeyValuePair` overload" type runs into a
FIXME within `CodeGenerator.cs`:

	// FIXME: at some stage we want to import generic types.
	// For now generator fails to load generic types that have conflicting type e.g.
	// AdapterView`1 and AdapterView cannot co-exist.
	// It is mostly because generator primarily targets jar (no real generics land).

The *intent* of the check that is that, given a type `AdapterView<T>`,
we only check to see if `AdapterView` exists as well. If it does exist,
then we *ignore* the `AdapterView<T>` type, and only use the
`AdapterView` type for code generation.

In the case of `KeyValuePair`, the same "check" is done, which cause
us to *skip* type registration for `KeyValuePair<TKey, TValue>`, which
in turn causes us to invalidate e.g. `IDictionary<TKey, TValue>` -- as
it implements `ICollection<KeyValuePair<TKey, TValue>>` -- and
everything promply falls apart from there:

	warning BG8C00: For type System.Collections.Generic.IDictionary`2, base interface System.Collections.Generic.ICollection`1<System.Collections.Generic.KeyValuePair`2<TKey,TValue>> is invalid.
	warning BG8C00: For type System.Collections.Generic.IDictionary`2, base interface System.Collections.Generic.IEnumerable`1<System.Collections.Generic.KeyValuePair`2<TKey,TValue>> is invalid.
	warning BG8502: Invalidating System.Collections.Generic.IDictionary`2 and all nested types because some of its interfaces were invalid.
	warning BG8801: Invalid parameter type System.Collections.Generic.IDictionary`2<System.String,System.Collections.Generic.IList`1<System.String>> in method MapEquivalents in managed type Java.Util.Locale.LanguageRange.
	warning BG8801: Invalid parameter type System.Collections.Generic.IDictionary`2<System.String,System.Collections.Generic.IList`1<System.String>> in method Parse in managed type Java.Util.Locale.LanguageRange.
	warning BG8801: Invalid parameter type System.Collections.Generic.IDictionary`2<System.String,System.Object> in method NewFileSystem in managed type Java.Nio.FileNio.Spi.FileSystemProvider.
	warning BG8801: Invalid parameter type System.Collections.Generic.IDictionary`2<System.String,System.Object> in method NewFileSystem in managed type Java.Nio.FileNio.Spi.FileSystemProvider.
	warning BG8801: Invalid parameter type System.Collections.Generic.IDictionary`2<System.String,System.String> in method .ctor in managed type Java.Security.Provider.Service.
	warning BG8801: Invalid parameter type System.Collections.Generic.IDictionary`2<System.Object,System.Object> in method PutAll in managed type Java.Security.Provider.

I still don't fully understand the scenario that the check was
attempting to solve, so in lieu of actually fixing the FIXME, make the
check *stricter* so that we only ignore "generically overloaded" types
if they bind the *same* Java type. Since `KeyValuePair<TKey, TValue>`
binds *no* Java type, it will no longer be skipped, thus removing the
BG8502 and related warnings.

Additionally, a bit of code cleanup for consistency: some parts of the
code use `HAVE_CECIL`, and others use `USE_CECIL`. Standardize on
`HAVE_CECIL` for consistency and sanity.

[0]: dotnet/android#631
[1]: dotnet/android#723
@atsushieno
Copy link
Contributor

Out of band - the Cecil condition is from 6+ years ago and insignificant. We can just remove them (but it does not have to be done in this PR).

@atsushieno atsushieno merged commit 3343634 into dotnet:master Aug 22, 2017
@github-actions github-actions bot locked and limited conversation to collaborators Apr 15, 2024
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants