[generator] Replace ApiXmlAdjuster with Java.Interop.Tools.JavaTypeSystem #849
Conversation
jpobst
changed the title
jpobst
changed the title
|
Question/clarification: the PR message contains an "Example java-resolution-report.log", which says:
What is a "cycle" in this context, particularly in a user-facing capacity? I think we'll want "clearer" names than "Cycle 1"/"Cycle 2"/etc. |
| if (pkg.PropertyBag.TryGetValue ("merge.SourceFile", out var source)) | ||
| writer.WriteAttributeString ("merge.SourceFile", source); | ||
|
|
||
| if (!string.IsNullOrEmpty (pkg.JniName)) |
There was a problem hiding this comment.
We can "normalize" things here, and if pkg.JniName is null/empty -- for whatever reason -- then we can create one based on the pkg.Name value:
writer.WriteAttributeString ("jni-name",
!string.IsNullOrEmpty (pkg.JniName)
? pkg.JniName
: pkg.Name.Replace (".", "/"));There was a problem hiding this comment.
I think there are 2 cases when pkg.JniName is empty:
- Built-in types, which will never be output here
- Types that are not in a package, which will not have a
pkg.Nameeither
It "cannot" be null, as we have it set to not-null.
There was a problem hiding this comment.
If it "cannot" be null, then why have a check which assumes that it could be null (or empty)?
Or is it instead that for "globally scoped" types, we want:
<package name="">
<class name="TypeInGlobalPackage" … />
</package>and not:
<package name="" jni-name="">
<class name="TypeInGlobalPackage" … />
</package>
| } | ||
|
|
||
| /// <summary> | ||
| /// Adds a reference type to the collection. |
There was a problem hiding this comment.
This nomenclature is somewhat confusing, in that there is already a concept of a "reference type".
Perhaps this should be named AddReferencedType() (note addition of d).
There was a problem hiding this comment.
oh dear, I suggested "referenced type", and now no longer remember what it means!
| /// <summary> | ||
| /// Returns string containing package name, parent name, and type's name. (ex: 'java.util.ArrayList.Keys') | ||
| /// </summary> | ||
| public string FullName { |
There was a problem hiding this comment.
Possibly a "crazy" idea, but could we avoid "FullName" as a construct? It's not immediately obvious at callsites if this is a Java full name or a C# full name.
Even the <summary/> docs are inconsistent with the code: docs suggest a Java name ("ex java.util.ArrayList.Keys"), while code suggests a C# name (replaces boolean with bool).
Furthermore, in the context of C#, it's possible for the same Java type to be bound by multiple different assemblies, which would necessitate "AssemblyQualifiedNames"…which can't be used in generated C# source code.
There was a problem hiding this comment.
I agree that FullName can get problematic in generator.
I tried to keep this context a little clearer as this library (JavaTypeSystem) only deals in Java types, and the class is called JavaTypeModel. Originally every property was prefixed with "Java" like JavaFullName/JavaVisibility/JavaStatic/etc, but it seemed redundant in a "Java"TypeModel class.
The bool thing appears to be leftover code from some earlier stuff. We still output boolean, so I have removed this unintended C#-ism to keep this library pure "Java".
There was a problem hiding this comment.
I tried to keep this context a little clearer as this library (
JavaTypeSystem) only deals in Java types
which may address my earlier questions/suggestions around naming, e.g. to prefer System.Reflection-style names for similar purposes, such as DeclaringType, FieldType, etc.
The benefit to System.Reflection-style names is that, I assume, more C# developers will be familiar with those nouns, and thus the resulting code will be easier for other people to understand.
The alternative to using System.Reflection-style names is to use "Java-style" names, of which there are two: Java language as per JLS, or the JVM spec, assuming that those use different names. (One hopes that they're consistent with each other, but in the off chance they're not…)
So what nouns do the JLS & JVM use? JLS uses "declared" and synonyms: https://docs.oracle.com/javase/specs/jls/se16/html/jls-8.html#jls-8.3
The notion of overriding includes methods that override another from some subclass of their declaring class.
"Parent" doesn't appear anywhere in jls-8.
JVM is similar, e.g. https://docs.oracle.com/javase/specs/jvms/se7/html/jvms-4.html#jvms-4.6: no mention of "parent", many mentions of "declared", "declaring", "declares", etc.
There was a problem hiding this comment.
Switched to Declared*.
I updated the PR description with some clarification on what a cycle is. I'm up for suggestions on better names for this. My only other thoughts are |
| static void SaveMethod (JavaMethodModel method, XmlWriter writer) | ||
| { | ||
| bool check (JavaMethodModel _) => _.BaseMethod?.ParentType?.Visibility == "public" && | ||
| !method.IsStatic && |
There was a problem hiding this comment.
This should be consistently indented with tabs.
| //// - none of the arguments are type parameters. | ||
| //// - finally, it is the synthetic method already checked above. | ||
| if (method.BaseMethod != null && | ||
| !method.BaseMethod.IsAbstract && |
There was a problem hiding this comment.
Please indent with two tab stops within the if (…:
if (method.BaseMethod != null &&
[tab][tab]!method.BaseMethod.IsAbstract &&
[tab][tab]method.BaseMethod.Visibility == method.Visibility &&
… &&
[tab][tab]check (method))
[tab]return;
| XmlConvert.ToString (method.IsNative), | ||
| GetVisibleReturnTypeString (method), | ||
| XmlConvert.ToString (method.IsSynchronized), | ||
| null, |
There was a problem hiding this comment.
At this point, could you please use named parameters, possibly for all of them? This is a lot of parameters…
| return parameter.GenericType; | ||
| } | ||
|
|
||
| static JavaTypeReference? GetVisibleNonSpecialType (JavaMethodModel method, JavaTypeReference? r) |
There was a problem hiding this comment.
Naming: I don't understand what a "non-special" type is. It apparently relates to JavaTypeReference.SpecialName, but I don't currently understand what that means either.
There was a problem hiding this comment.
| } | ||
|
|
||
| static void SaveMember (JavaMemberModel m, XmlWriter writer, string elementName, | ||
| string? abs, string? native, string? ret, string? sync, |
There was a problem hiding this comment.
Only two tab stops are needed here, not three.
…wow, that's a lot of parameters...
| var root = doc.Root; | ||
|
|
||
| if (root is null) | ||
| throw new Exception ("Invalid XML file"); |
There was a problem hiding this comment.
Should be InvalidOperationException, or ArgumentException.
| case "package": | ||
| packages.Add (ParsePackage (elem, collection)); | ||
| break; | ||
| } |
There was a problem hiding this comment.
Should we emit a warning or something when elem.Name.LocalName != "package"?
The only other element I know of which could plausibly show up here is <enum/>, e.g. in src/Mono.Android/obj/Release/monoandroid10/android-30/mcw/api.xml.fixed:
<enum name="Android.Hardware.Fingerprints.FingerprintAuthenticationFlags" />
</api>Anything else is likely to be a mistake, e.g. if there's an /api/class element.
There was a problem hiding this comment.
I don't know that adding warnings for things like this is useful. On one hand, they should never happen because we're consuming our own output from class-parse. On the other hand, if it did happen no one would notice because we generate so many other warnings in the binding process.
| } | ||
|
|
||
| // First add all non-nested types | ||
| foreach (var type in packages.SelectMany (p => p.Types).Where (t => !t.NestedName.Contains ('.'))) |
There was a problem hiding this comment.
This "all non-nested types followed by all nested types" feels "weird", in that it'll potentially separate nested types from their containing type.
For example, there is one android.accessibilityservice.AccessibilityButtonController nested type, AccessibilityButtonController.AccessibilityButtonCallback.
If we "split out" nested types in this fashion, we'd wind up with:
<class name="AccessibilityButtonController" …/>
<class name="AccessibilityGestureEvent" … />
<class name="AccessibilityService" … />
<!-- at least three other types… -->
<class name="AccessibilityButtonController.AccessibilityButtonCallback" … />This differs from current output -- which is probably fine? -- but means that there's no "proximity" between nested types and their declaring types in document order.
This shouldn't break any tooling, but feels like something that could be "weird" or "annoying" for human readers.
Alternatively, could we sort packages in the order that we want?
There was a problem hiding this comment.
The output is done the way you suggest. We output a type and then recurse into its nested types, so the nested types will always be directly after their declaring type.
| collection.AddType (type); | ||
|
|
||
| // Remove nested types from Package | ||
| type.Package.Types.Remove (type); |
There was a problem hiding this comment.
This and the associated comment confuse me: why aren't nested types in the package?
There was a problem hiding this comment.
Package.Types isn't "flattened". It's a "tree", where it contains top-level types, which then contain their nested types.
| foreach (var elem in package.Elements ()) { | ||
| switch (elem.Name.LocalName) { | ||
| case "class": | ||
| if (package.XGetAttributeAsBool ("obfuscated")) |
There was a problem hiding this comment.
Shouldn't this be elem.XGetAttributeASBool ("obfuscated")? We care about the class being obfuscated, not it's containing package.
| pkg.Types.Add (ParseClass (pkg, elem)); | ||
| break; | ||
| case "interface": | ||
| if (package.XGetAttributeAsBool ("obfuscated")) |
There was a problem hiding this comment.
Shouldn't this be elem.XGetAttributeASBool ("obfuscated")? We care about the interface being obfuscated, not it's containing package.
| case "method": | ||
| model.Methods.Add (ParseMethod (model, child)); | ||
| break; | ||
| } |
There was a problem hiding this comment.
Should we warn on any other unexpected element values?
There was a problem hiding this comment.
Same reply about generating too many warnings.
| foreach (var child in element.Elements ()) { | ||
| switch (child.Name.LocalName) { | ||
| case "constructor": | ||
| model.Constructors.Add (ParseConstructor (model, child)); |
There was a problem hiding this comment.
It feels odd that model is present twice on this (and other) statements. Would it be better if this were instead:
AddParsedConstructor (model, child);
// or AddParsedField(), AddParsedImplements(), etc.There was a problem hiding this comment.
Doesn't feel very important either way.
| model.Implements.Add (ParseImplements (child)); | ||
| break; | ||
| case "method": | ||
| if (child.XGetAttribute ("synthetic") != "true") |
There was a problem hiding this comment.
Does this need to be "consistent" with: https://github.com/xamarin/java.interop/pull/849/files#diff-41afcd0fb56d03aabb52fc26b23822c138457f30e40940ae9d8b43e018e02690R199
(The "Here we skip most of the overriding methods of a virtual method, unless" comment block, above.)
There was a problem hiding this comment.
I think at this point we haven't figured out the rest of those conditions. IE: we can't find the appropriate base method because it may not have been parsed yet.
| ParseTypeParameters (method.TypeParameters, tp); | ||
|
|
||
| foreach (var child in element.Elements ("parameter")) | ||
| method.Parameters.Add (ParseParameter (method, child)); |
There was a problem hiding this comment.
Ditto previous comment about method being listed twice in this statement. Perhaps an AddParsedParameter(method, child)) method would be better?
| isBridge: element.XGetAttributeAsBool ("bridge") | ||
| ); | ||
|
|
||
| if (element.Element ("typeParameters") is XElement tp) |
There was a problem hiding this comment.
Constructors can't be generic; this should be "dead code".
| static bool ShouldImport (TypeDefinition td) | ||
| { | ||
| // We want to exclude "IBlahInvoker" and "IBlahImplementor" and "BlahConsts" types | ||
| if (td.Name.EndsWith ("Invoker")) { |
There was a problem hiding this comment.
string.EndsWith(string) is locale-aware. This should instead use string.EndsWith(string, StringComparison.Ordinal).
Ditto other uses of .EndsWith().
There was a problem hiding this comment.
I rebased on main, which now raises errors for these issues, and fixed them.
jpobst
force-pushed
the
java-resolver
branch
2 times, most recently
from
6d13592 to
505a96c
Compare
| // - Cycle 2 removed 'com.example.MyDerivedType' because 'com.example.MyType' is now missing | ||
| // This distinction can be interesting, because Cycle 1 removals are often due to missing | ||
| // dependencies, whereas the remaining cycles are just the internal fallout from Cycle 1. | ||
| public class CollectionResolutionResults : List<CollectionResolutionResult> |
There was a problem hiding this comment.
List<T> should be avoided as a base class. Prefer System.Collections.ObjectModel.Collection<T>..
|
|
||
| namespace Java.Interop.Tools.JavaTypeSystem.Models | ||
| { | ||
| public class JavaTypeParameters : List<JavaTypeParameter> |
There was a problem hiding this comment.
List<T> should be avoided as a base class. Prefer System.Collections.ObjectModel.Collection<T>..
|
|
||
| public class CollectionResolutionResult | ||
| { | ||
| public List<JavaUnresolvableModel> Unresolvables { get; } |
There was a problem hiding this comment.
This should likely also use Collection<JavaUnresolvableModel>.
Fixes: https://github.com/xamarin/java.interop/issues/739
Fixes: https://github.com/xamarin/java.interop/issues/852
Fixes: https://github.com/xamarin/java.interop/issues/853
Context: https://github.com/xamarin/java.interop/issues/789
Context: https://github.com/xamarin/java.interop/issues/814
Context: https://github.com/xamarin/java.interop/issues/815
Today, we use a process called `ApiXmlAdjuster` to convert our "new"
`class-parse` format to the old `jar2xml` format. This process
attempts to resolve every Java type referenced by the library we
intend to bind. However, performing this as a separate step has
some disadvantages:
- It duplicates work by reading in `api.xml.class-parse`
and Cecil'ing all reference assemblies, then writing it back out
as `api.xml`.
`generator` then reads in `api.xml` and Cecils all the reference
assemblies again.
- This work is performed as a separate step before `generator` and
thus cannot be influenced by `metadata`.
- This work is cached, which is "good", but it creates issues for
users because they only see warnings from it for full Rebuilds.
"Replace" `ApiXmlAdjuster` with `Java.Interop.Tools.JavaTypeSystem`,
which is a much more robust Java model to perform this work, and
contains enough information to be reused by `generator`.
This prevents us from needing to have a second Java and Cecil import
step, and we can run `metadata` directly on `api.xml.class-parse`
before the import.
NOTE: This commit sets the groundwork for the above benefits, but
does not achieve them yet. Initially we are only focused on
achieving parity with `ApiXmlAdjuster` to make correctness
comparisons possible.
The new `JavaTypeSystem` infrastructure is used by default; the
previous `ApiXmlAdjuster` system can be used instead by using:
generator --use-legacy-java-resolver=true …
We will provide an MSBuild property to control this when we integrate
with xamarin-android in the future.
~~ Warning Messages ~~
In order to better surface potentially real errors to users, we have
tweaked the warnings displayed. Errors we encounter during the
initial phase of Java type resolution are emitted as warnings.
These are missing external Java types and generally indicate the user
is missing a reference `.jar` or binding NuGet:
warning BG8605: The Java type 'androidx.appcompat.widget.AppCompatTextView' could not be found (are you missing a Java reference jar/aar or a Java binding library NuGet?)
The remaining resolution errors are generally the fallout from those
missing external types. For example, if the above missing type
caused us to remove `com.example.MyTextView`, then that may result in
us removing the method `GetMyTextView()` due to a missing return type.
Today these messages are only displayed in a Diagnostic log and are
often missed. Instead, we are going to write them to a log file
called `java-resolution-report.log`, and then display a warning
notifying the user that some types could not be bound, which can be
double clicked to view the log file.
warning BG8606: Some types or members could not be bound because referenced Java types could not be found. See the 'java-resolution-report.log' file for details.
Example `java-resolution-report.log`:
==== Cycle 1 ====
'[Class] com.example.MyTextView' was removed because the Java type 'androidx.appcompat.widget.AppCompatTextView' could not be found.
==== Cycle 2 ====
'[Method] com.example.MyActivity.getMyTextView' was removed because the Java type 'com.example.MyTextView' could not be found.
"Cycles" represent the passes through the type collection when
resolving the collection. For example:
- Cycle 1 removed type `com.example.MyType` because its external
base type `android.util.List` was missing
- Cycle 2 removed type `com.example.MyDerivedType` because its base
type `com.example.MyType` is now missing
- Cycle 3 removed method `com.example.Foo.getBar()` because it
returns `com.example.MyDerivedType`, which is now missing
This distinction can be interesting, because Cycle 1 removals are
generally due to missing `.jar` dependencies, whereas the remaining
cycles are just the internal fallout from previous cycles, which will
largely be fixed by fixing the first cycle issues.
~~ Additional Fixes ~~
There are several issues with the `ApiXmlAdjuster` process that *can*
be fixed by this change. For the initial purpose of being able to
verify that we produce the same output, these have been disabled in
the new `JavaTypeSystem` code.
* [ApiXmlAdjuster removes required methods from interfaces][0] -
Currently disabled via flag
`TypeResolutionOptions.RemoveInterfacesWithUnresolvableMembers`.
* [ApiXmlAdjuster fails to resolve parent type parameters for nested types][1] -
Currently commented out in `JavaTypeModel.GetApplicableTypeParameters()`.
* [ApiXmlAdjuster doesn't remove nested types][2] -
Currently enabled, compares have been done with a custom
`ApiXmlAdjuster` where this has been fixed.
* [ApiXmlAdjuster prefers JavaList over ArrayList][3] -
Currently enabled, compares have been done with a custom
`ApiXmlAdjuster` where this has been fixed.
~~ Performance ~~
Although performance isn't really a focus, there are wins here as well,
largely due to only resolving reference types that are actually
referenced. Example: `ApiXmlAdjuster` fully resolves all 5K+ types in
`Mono.Android.dll`, while `JavaTypeSystem` only resolves the ones
needed by the library being bound.
Time to resolve `androidx.appcompat.appcompat`:
* ApiXmlAdjuster: 2849ms
* JavaTypeSystem: 1514ms
~~ Testing ~~
- Unit tests have been ported from `ApiXmlAdjuster-Tests`
- Tested for identical output for 136 current AndroidX packages
- Tested for identical output for 140 current GPS/FB packages
Note `Mono.Android.dll` does not use `ApiXmlAdjuster`, so no testing
was performed with it.
[0]: https://github.com/xamarin/java.interop/issues/814
[1]: https://github.com/xamarin/java.interop/issues/815
[2]: https://github.com/xamarin/java.interop/issues/852
[3]: https://github.com/xamarin/java.interop/issues/853 |
Context: #789
Today, we use a process called
ApiXmlAdjusterto convert our "new"class-parseformat to the oldjar2xmlformat. This process attempts to resolve every Java type referenced by the library we intend to bind. However, performing this as a separate step has some disadvantages.api.xml.class-parseand Cecil'ing all reference assemblies, then writing it back out asapi.xml. Thengeneratorreads inapi.xmland Cecils all the reference assemblies again.generatorand thus cannot be influenced bymetadata.This commit creates a much more robust Java model to perform this work that contains enough information to be reused by
generator. This prevents us from needing to have a second Java and Cecil import, and we can runmetadatadirectly onapi.xml.class-parsebefore the import.NOTE: This commit sets the groundwork for the above benefits, but does not achieve them yet. Initially we are only focused on achieving parity with
ApiXmlAdjusterto make correctness comparisons possible.Legacy Flag
There is definitely some concern that a big change like this may cause regressions. In order to give users an escape hatch should they hit issues, we have added the temporary flag
--use-legacy-java-resolver=truethat usesApiXmlAdjuster. We will add a temporarily MSBuild flag for users to trigger this.Warnings
Fixes: #739
In order to better surface potentially real errors to users we have tweaked the warnings displayed. Errors we encounter during the initial phase of Java type resolution are emitted as warnings. These are missing external Java types and generally indicate the user is missing a reference
.jaror binding NuGet.The remaining resolution errors are generally the fallout from those missing external types. For example, if the above missing type caused us to remove
com.example.MyTextView, then that may result in us removing the methodGetMyTextView ()due to a missing return type.Today these messages are only displayed in a Diagnostic log and are often missed. Instead, we are going to write them to a log file called
java-resolution-report.log, and then display a warning notifying the user that some types could not be bound, which can be double clicked to view the log file.Example
java-resolution-report.log:"Cycles" represent the passes through the type collection when resolving the collection.
Example:
com.example.MyTypebecause its external base typeandroid.util.Listwas missingcom.example.MyDerivedTypebecause its base typecom.example.MyTypeis now missingcom.example.Foo.getBar()because it returnscom.example.MyDerivedTypewhich is now missingThis distinction can be interesting, because Cycle 1 removals are generally due to missing
.jardependencies, whereas the remaining cycles are just the internal fallout from previous cycles, which will largely be fixed by fixing the first cycle issues.Additional Fixes
There are several issues with the
ApiXmlAdjusterprocess that can be fixed by this change. For the initial purpose of being able to verify that we produce the same output, these have been disabled in the newJavaTypeSystemcode.TypeResolutionOptions.RemoveInterfacesWithUnresolvableMembers.JavaTypeModel.GetApplicableTypeParameters ().ApiXmlAdjusterwhere this has been fixed.ApiXmlAdjusterwhere this has been fixed.Performance
Although performance isn't really a focus, there are wins here as well, largely due to only resolving reference types that are actually referenced. Example:
ApiXmlAdjusterfully resolves all 5K+ types inMono.Android.dll, whileJavaTypeSystemonly resolves the ones needed by the library being bound.Time to resolve
androidx.appcompat.appcompatTesting
ApiXmlAdjuster-TestsNote
Mono.Android.dlldoes not useApiXmlAdjusterso no testing was performed with it.