From ca9bfe72fe6d6302585949ce1d02c2a6f0c3bbff Mon Sep 17 00:00:00 2001 From: Atsushi Eno Date: Thu, 22 Sep 2016 02:38:03 +0900 Subject: [PATCH] [generator][api-xml-adjuster] Fix managed type resolution in API adjuster. Until now, ApiXmlAdjuster was trying to resolve types using ClassGen.BaseGen property which is in fact assigned only after Validate() step in generator. API XML analyzer work is done way before that step, and therefore it failed to resolve types from managed code (DLL references). Missing type hierarchy in Java type model from managed code means that any bindings that references Mono.Android (which wouldn't?) had to rely on insufficient hierarchy. Especially, it failed to resolve method overrides when the target Java library types overrode methods in android.jar indirectly. SherlockExpandableListActivity.addContentView() was such a method. And it had appeared as a regression from our MSBuild tests[*1]. Fixes: - removed dependency on BaseGen in ApiXmlAdjuster. Use BaseType instead, which doesn't depend on validation step. - Though this change uncovered an issue that Ctor and Parameter needed to hold Java type information. So, made some changes to them. - Now that ApiXmlAdjuster is getting valid managed type hierarchy, it had uncovered another kind of issues: managed objects need to exist to not cause some null crashes. Hence there is ManagedType class now. - Type resolution now premises this ManagedType in type resolver code. - Fixed incorrect Load() invocation for ClassGen and InterfaceGen (that had caused missing BaseType information). - Added (almost irrelevant) ApiXmlAdjuster test to make sure override resolution is done for overrides from indirect inheritance. [*1] that needs to be OSS-ed... --- .../JavaApi.XmlModel.cs | 31 +++++++++++++++++++ .../JavaApiOverrideMarkerExtensions.cs | 2 +- .../JavaApiTypeResolverExtensions.cs | 5 ++- .../Tests/OverrideMarkerTest.cs | 28 +++++++++++++++++ tools/generator/Ctor.cs | 3 +- tools/generator/Field.cs | 2 +- tools/generator/JavaApiDllLoaderExtensions.cs | 19 +++++++----- tools/generator/Parameter.cs | 4 +-- 8 files changed, 79 insertions(+), 15 deletions(-) diff --git a/src/Xamarin.Android.Tools.ApiXmlAdjuster/JavaApi.XmlModel.cs b/src/Xamarin.Android.Tools.ApiXmlAdjuster/JavaApi.XmlModel.cs index a8c21f3bc..6e63e2296 100644 --- a/src/Xamarin.Android.Tools.ApiXmlAdjuster/JavaApi.XmlModel.cs +++ b/src/Xamarin.Android.Tools.ApiXmlAdjuster/JavaApi.XmlModel.cs @@ -108,6 +108,37 @@ public override string ToString () } } + + class ManagedType : JavaType + { + static JavaPackage dummy_system_package, dummy_system_io_package; + static JavaType system_object, system_exception, system_io_stream; + + static ManagedType () + { + dummy_system_package = new JavaPackage (null) { Name = "System" }; + system_object = new ManagedType (dummy_system_package) { Name = "Object" }; + system_exception = new ManagedType (dummy_system_package) { Name = "Exception" }; + dummy_system_package.Types.Add (system_object); + dummy_system_package.Types.Add (system_exception); + dummy_system_io_package = new JavaPackage (null) { Name = "System.IO" }; + system_io_stream = new ManagedType (dummy_system_package) { Name = "Stream" }; + dummy_system_io_package.Types.Add (system_io_stream); + } + + public static IEnumerable DummyManagedPackages { + get { + yield return dummy_system_package; + yield return dummy_system_io_package; + } + } + + public ManagedType (JavaPackage package) : base (package) + { + } + } + + public partial class JavaImplements { public string Name { get; set; } diff --git a/src/Xamarin.Android.Tools.ApiXmlAdjuster/JavaApiOverrideMarkerExtensions.cs b/src/Xamarin.Android.Tools.ApiXmlAdjuster/JavaApiOverrideMarkerExtensions.cs index 1d15e551d..b5582f9a3 100644 --- a/src/Xamarin.Android.Tools.ApiXmlAdjuster/JavaApiOverrideMarkerExtensions.cs +++ b/src/Xamarin.Android.Tools.ApiXmlAdjuster/JavaApiOverrideMarkerExtensions.cs @@ -42,7 +42,7 @@ static void MarkBaseMethod (this JavaClass cls, JavaMethod method) { JavaClass k = cls; while (true) { - k = k.ResolvedExtends != null ? (JavaClass) k.ResolvedExtends.ReferencedType : null; + k = k.ResolvedExtends != null ? k.ResolvedExtends.ReferencedType as JavaClass : null; if (k == null) break; diff --git a/src/Xamarin.Android.Tools.ApiXmlAdjuster/JavaApiTypeResolverExtensions.cs b/src/Xamarin.Android.Tools.ApiXmlAdjuster/JavaApiTypeResolverExtensions.cs index e525762e2..8847b526e 100644 --- a/src/Xamarin.Android.Tools.ApiXmlAdjuster/JavaApiTypeResolverExtensions.cs +++ b/src/Xamarin.Android.Tools.ApiXmlAdjuster/JavaApiTypeResolverExtensions.cs @@ -38,9 +38,12 @@ public static JavaType FindNonGenericType (this JavaApi api, string name) var ret = api.Packages .SelectMany (p => p.Types) .FirstOrDefault (t => name.StartsWith (t.Parent.Name, StringComparison.Ordinal) && name == t.Parent.Name + '.' + t.Name); + if (ret == null) + ret = ManagedType.DummyManagedPackages + .SelectMany (p => p.Types) + .FirstOrDefault (t => t.FullName == name); if (ret == null) throw new JavaTypeResolutionException (string.Format ("Type '{0}' was not found.", name)); - return ret; } diff --git a/src/Xamarin.Android.Tools.ApiXmlAdjuster/Tests/OverrideMarkerTest.cs b/src/Xamarin.Android.Tools.ApiXmlAdjuster/Tests/OverrideMarkerTest.cs index 06e82545c..b4939b3d1 100644 --- a/src/Xamarin.Android.Tools.ApiXmlAdjuster/Tests/OverrideMarkerTest.cs +++ b/src/Xamarin.Android.Tools.ApiXmlAdjuster/Tests/OverrideMarkerTest.cs @@ -1,5 +1,7 @@ using System; +using System.IO; using System.Linq; +using System.Xml; using NUnit.Framework; namespace Xamarin.Android.Tools.ApiXmlAdjuster.Tests @@ -29,6 +31,32 @@ public void InstantiatedGenericArgumentName () Assert.AreEqual (method.Parameters.First (), method.Parameters.Last (), "There should be only one parameter."); Assert.AreEqual ("T", para.InstantiatedGenericArgumentName, "InstantiatedGenericArgumentName mismatch"); } + + [Test] + public void AncestralOverrides () + { + string xml = @" + + + + + + + + + + +"; + var xapi = JavaApiTestHelper.GetLoadedApi (); + using (var xr = XmlReader.Create (new StringReader (xml))) + xapi.Load (xr, false); + xapi.Resolve (); + xapi.CreateGenericInheritanceMapping (); + xapi.MarkOverrides (); + var t = xapi.Packages.First (_ => _.Name == "XXX").Types.First (_ => _.Name == "SherlockExpandableListActivity"); + var m = t.Members.OfType ().First (_ => _.Name == "addContentView"); + Assert.IsNotNull (m.BaseMethod, "base method not found"); + } } } diff --git a/tools/generator/Ctor.cs b/tools/generator/Ctor.cs index d0e6e8865..8b5534499 100644 --- a/tools/generator/Ctor.cs +++ b/tools/generator/Ctor.cs @@ -27,8 +27,7 @@ public ManagedCtor (GenBase declaringType, MethodDefinition m) // the type of the containing class must be inserted as the first // argument if (IsNonStaticNestedType) - Parameters.AddFirst (Parameter.FromManagedType (m.DeclaringType.DeclaringType)); - + Parameters.AddFirst (Parameter.FromManagedType (m.DeclaringType.DeclaringType, DeclaringType.JavaName)); var regatt = m.CustomAttributes.FirstOrDefault (a => a.AttributeType.FullName == "Android.Runtime.RegisterAttribute"); is_acw = regatt != null; foreach (var p in support.GetParameters (regatt)) diff --git a/tools/generator/Field.cs b/tools/generator/Field.cs index cb8ecc500..e1a7b644c 100644 --- a/tools/generator/Field.cs +++ b/tools/generator/Field.cs @@ -76,7 +76,7 @@ public override string Visibility { protected override Parameter SetterParameter { get { - var p = Parameter.FromManagedType (f.FieldType.Resolve ()); + var p = Parameter.FromManagedType (f.FieldType.Resolve (), null); p.Name = "value"; return p; } diff --git a/tools/generator/JavaApiDllLoaderExtensions.cs b/tools/generator/JavaApiDllLoaderExtensions.cs index 7809e6428..661b7754e 100644 --- a/tools/generator/JavaApiDllLoaderExtensions.cs +++ b/tools/generator/JavaApiDllLoaderExtensions.cs @@ -19,11 +19,11 @@ public static void LoadReferences (this JavaApi api, GenBase [] gens) if (gen is InterfaceGen) { var iface = new JavaInterface (pkg); pkg.Types.Add (iface); - iface.Load (gen); + iface.Load ((InterfaceGen) gen); } else if (gen is ClassGen) { var kls = new JavaClass (pkg); pkg.Types.Add (kls); - kls.Load (gen); + kls.Load ((ClassGen) gen); } else throw new InvalidOperationException (); @@ -75,11 +75,11 @@ static void Load (this JavaInterface iface, InterfaceGen gen) ((JavaType) iface).Load (gen); } - static string ExpandTypeParameters (GenericParameterDefinitionList tps) + static string ExpandTypeParameters (ISymbol [] tps) { if (tps == null) return null; - return '<' + string.Join (", ", tps.Select (_ => _.Name)) + '>'; + return '<' + string.Join (", ", tps.Select (_ => _.JavaName)) + '>'; } static void Load (this JavaClass kls, ClassGen gen) @@ -88,10 +88,13 @@ static void Load (this JavaClass kls, ClassGen gen) kls.Abstract = gen.IsAbstract; kls.Final = gen.IsFinal; - if (gen.BaseGen != null) { - kls.Extends = gen.BaseGen.JavaSimpleName; - kls.ExtendsGeneric = gen.BaseGen.JavaSimpleName + ExpandTypeParameters (gen.BaseGen.TypeParameters); - kls.ExtendedJniExtends = gen.BaseGen.JniName; + var baseGen = gen.BaseType != null ? SymbolTable.Lookup (gen.BaseType) : null; + + if (baseGen != null) { + kls.Extends = baseGen.JavaName; + var gs = baseGen as GenericSymbol; + kls.ExtendsGeneric = gs != null ? gs.JavaName + ExpandTypeParameters (gs.TypeParams) : baseGen.JavaName; + kls.ExtendedJniExtends = baseGen.JniName; } foreach (var c in gen.Ctors) { var ctor = new JavaConstructor (kls); diff --git a/tools/generator/Parameter.cs b/tools/generator/Parameter.cs index 098460399..1dbbc0ddc 100644 --- a/tools/generator/Parameter.cs +++ b/tools/generator/Parameter.cs @@ -291,9 +291,9 @@ public static Parameter FromManagedParameter (ParameterDefinition p, string jnit return new Parameter (SymbolTable.MangleName (p.Name), jnitype ?? p.ParameterType.FullNameCorrected (), null, isEnumType, rawtype); } - public static Parameter FromManagedType (TypeDefinition t) + public static Parameter FromManagedType (TypeDefinition t, string javaType) { - return new Parameter ("__self", t.FullName, null, false); + return new Parameter ("__self", javaType ?? t.FullName, t.FullName, false); } #endif }