Skip to content

Commit

Permalink
[generator] Only skip generic "overloads" of bound types
Browse files Browse the repository at this point in the history
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 problem is that the check that is performed is too strict: 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.

This check causes 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
  • Loading branch information
jonpryor committed Aug 21, 2017
1 parent 1cd0361 commit 86338a8
Show file tree
Hide file tree
Showing 11 changed files with 38 additions and 25 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,8 @@
#if HAVE_CECIL
using Mono.Cecil;
using Java.Interop.Tools.Cecil;
#if !GENERATOR
using Android.Runtime;
#if !GENERATOR
using Java.Interop.Tools.JavaCallableWrappers;
#endif // !GENERATOR
#endif // HAVE_CECIL
Expand Down Expand Up @@ -252,7 +252,7 @@ static string GetSpecialExportJniType (string typeName, ExportParameterKind expo
return null;
}

#if !GEN_JAVA_STUBS && !GENERATOR && !JAVADOC_TO_MDOC
#if !GEN_JAVA_STUBS && !JAVADOC_TO_MDOC
// Keep in sync with ToJniNameFromAttributes(TypeDefinition)
public static string ToJniNameFromAttributes (Type type)
{
Expand Down Expand Up @@ -384,7 +384,7 @@ static string ToJniNameWhichShouldReplaceExistingToJniName (Type type, ExportPar
}
#endif

#if HAVE_CECIL && !GENERATOR
#if HAVE_CECIL

internal static ExportParameterKind GetExportKind (Mono.Cecil.ICustomAttributeProvider p)
{
Expand Down
4 changes: 2 additions & 2 deletions tools/generator/ClassGen.cs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
using System.Xml.Linq;

namespace MonoDroid.Generation {
#if USE_CECIL
#if HAVE_CECIL
static class ManagedExtensions
{
public static string FullNameCorrected (this TypeReference t)
Expand Down Expand Up @@ -69,7 +69,7 @@ public override bool IsFinal {
get { return t.IsSealed; }
}
}
#endif
#endif // HAVE_CECIL

public class XmlClassGen : ClassGen {
bool is_abstract;
Expand Down
20 changes: 16 additions & 4 deletions tools/generator/CodeGenerator.cs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
using Xamarin.Android.Tools.ApiXmlAdjuster;

using Java.Interop.Tools.Cecil;
using Java.Interop.Tools.TypeNameMappings;

namespace Xamarin.Android.Binder {

Expand Down Expand Up @@ -262,8 +263,10 @@ static void Run (CodeGeneratorOptions options, DirectoryAssemblyResolver resolve
// 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).
if (td.HasGenericParameters &&
md.GetType (td.FullName.Substring (0, td.FullName.IndexOf ('`'))) != null)
var nonGenericOverload = td.HasGenericParameters
? md.GetType (td.FullName.Substring (0, td.FullName.IndexOf ('`')))
: null;
if (BindSameType (td, nonGenericOverload))
continue;
ProcessReferencedType (td, opt);
}
Expand Down Expand Up @@ -366,6 +369,15 @@ static void AddTypeToTable (GenBase gb)
AddTypeToTable (nt);
}

static bool BindSameType (TypeDefinition a, TypeDefinition b)
{
if (a == null || b == null)
return false;
if (!a.ImplementsInterface ("Android.Runtime.IJavaObject") || !b.ImplementsInterface ("Android.Runtime.IJavaObject"))
return false;
return JniType.ToJniName (a) == JniType.ToJniName (b);
}

static IEnumerable<GenBase> FlattenNestedTypes (IEnumerable<GenBase> gens)
{
foreach (var g in gens) {
Expand Down Expand Up @@ -404,7 +416,7 @@ static void Validate (List<GenBase> gens, CodeGenerationOptions opt)
} while (removed.Count > 0);
}

#if USE_CECIL
#if HAVE_CECIL
static void ProcessReferencedType (TypeDefinition td, CodeGenerationOptions opt)
{
if (!td.IsPublic && !td.IsNested)
Expand Down Expand Up @@ -434,7 +446,7 @@ static void ProcessReferencedType (TypeDefinition td, CodeGenerationOptions opt)
foreach (var nt in td.NestedTypes)
ProcessReferencedType (nt, opt);
}
#endif
#endif // HAVE_CECIL

static void GenerateAnnotationAttributes (List<GenBase> gens, IEnumerable<string> zips)
{
Expand Down
4 changes: 2 additions & 2 deletions tools/generator/Ctor.cs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
using Xamarin.Android.Tools;

namespace MonoDroid.Generation {
#if USE_CECIL
#if HAVE_CECIL
public class ManagedCtor : Ctor {
MethodDefinition m;
string name;
Expand Down Expand Up @@ -53,7 +53,7 @@ public override string CustomAttributes {
get { return null; }
}
}
#endif
#endif // HAVE_CECIL

public class XmlCtor : Ctor {
string name;
Expand Down
4 changes: 2 additions & 2 deletions tools/generator/Field.cs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
using System.Xml.Linq;

namespace MonoDroid.Generation {
#if USE_CECIL
#if HAVE_CECIL
public class ManagedField : Field {
FieldDefinition f;
string java_name;
Expand Down Expand Up @@ -83,7 +83,7 @@ protected override Parameter SetterParameter {
}
}
}
#endif
#endif // HAVE_CECIL

public class XmlField : Field {

Expand Down
4 changes: 2 additions & 2 deletions tools/generator/GenBaseSupport.cs
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ public static bool IsPrefixableName (string name)
}
}

#if USE_CECIL
#if HAVE_CECIL
public class ManagedGenBaseSupport : GenBaseSupport
{
TypeDefinition t;
Expand Down Expand Up @@ -152,7 +152,7 @@ public override string Visibility {
get { return t.IsPublic || t.IsNestedPublic ? "public" : "protected internal"; }
}
}
#endif
#endif // HAVE_CECIL

public class XmlGenBaseSupport : GenBaseSupport
{
Expand Down
4 changes: 2 additions & 2 deletions tools/generator/InterfaceGen.cs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
using Xamarin.Android.Tools;

namespace MonoDroid.Generation {
#if USE_CECIL
#if HAVE_CECIL
public class ManagedInterfaceGen : InterfaceGen {
public ManagedInterfaceGen (TypeDefinition t)
: base (new ManagedGenBaseSupport (t))
Expand All @@ -34,7 +34,7 @@ public override bool MayHaveManagedGenericArguments {
get { return !this.IsAcw; }
}
}
#endif
#endif // HAVE_CECIL

public class XmlInterfaceGen : InterfaceGen {

Expand Down
4 changes: 2 additions & 2 deletions tools/generator/Method.cs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
using System.Xml.Linq;

namespace MonoDroid.Generation {
#if USE_CECIL
#if HAVE_CECIL
public class ManagedMethod : Method {
MethodDefinition m;
string java_name;
Expand Down Expand Up @@ -121,7 +121,7 @@ public override string CustomAttributes {
get { return null; }
}
}
#endif
#endif // HAVE_CECIL

public class XmlMethod : Method {

Expand Down
4 changes: 2 additions & 2 deletions tools/generator/MethodBase.cs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ public interface IMethodBaseSupport {
string Visibility { get; }
}

#if USE_CECIL
#if HAVE_CECIL
public class ManagedMethodBaseSupport : IMethodBaseSupport {
MethodDefinition m;
public ManagedMethodBaseSupport (MethodDefinition m)
Expand Down Expand Up @@ -79,7 +79,7 @@ public IEnumerable<Parameter> GetParameters (CustomAttribute regatt)
}
}
}
#endif
#endif // HAVE_CECIL

public class XmlMethodBaseSupport : IMethodBaseSupport {

Expand Down
4 changes: 2 additions & 2 deletions tools/generator/Parameter.cs
Original file line number Diff line number Diff line change
Expand Up @@ -284,7 +284,7 @@ public static Parameter FromClassElement (XElement elem)
return new Parameter (name, java_package + "." + java_type, null, false);
}

#if USE_CECIL
#if HAVE_CECIL
public static Parameter FromManagedParameter (ParameterDefinition p, string jnitype, string rawtype)
{
// FIXME: safe to use CLR type name? assuming yes as we often use it in metadatamap.
Expand All @@ -297,6 +297,6 @@ public static Parameter FromManagedType (TypeDefinition t, string javaType)
{
return new Parameter ("__self", javaType ?? t.FullName, t.FullName, false);
}
#endif
#endif // HAVE_CECIL
}
}
5 changes: 3 additions & 2 deletions tools/generator/generator.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,11 @@
<DebugType>full</DebugType>
<Optimize>False</Optimize>
<OutputPath>$(UtilityOutputFullPath)</OutputPath>
<DefineConstants>DEBUG;GENERATOR;USE_CECIL;JCW_ONLY_TYPE_NAMES</DefineConstants>
<DefineConstants>DEBUG;GENERATOR;HAVE_CECIL;JCW_ONLY_TYPE_NAMES</DefineConstants>
<ErrorReport>prompt</ErrorReport>
<WarningLevel>4</WarningLevel>
<ConsolePause>false</ConsolePause>
<PlatformTarget>anycpu</PlatformTarget>
</PropertyGroup>
<PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Release|AnyCPU' ">
<DebugType>full</DebugType>
Expand All @@ -35,7 +36,7 @@
<ErrorReport>prompt</ErrorReport>
<WarningLevel>4</WarningLevel>
<Externalconsole>True</Externalconsole>
<DefineConstants>GENERATOR;USE_CECIL;JCW_ONLY_TYPE_NAMES</DefineConstants>
<DefineConstants>GENERATOR;HAVE_CECIL;JCW_ONLY_TYPE_NAMES</DefineConstants>
</PropertyGroup>
<ItemGroup>
<Reference Include="System" />
Expand Down

0 comments on commit 86338a8

Please sign in to comment.