[Xamarin.Android.Tools.Bytecode] MethodParameters Attribute parsing #107
Conversation
Fixes: dotnet#106 Java 8 adds support for a `javac -parameters` option, which adds a `MethodParameters` attribute to the `.class` file. The `MethodParameters` attribute blob contains *reliable* parameter names, including for abstract methods and interface methods, unlike existing `LocalVariableTable`, which only exists for `javac -g` builds, and only for methods with *bodies*, i.e. *not* abstract methods and interface methods. Add support for parsing the `MethodParameters` attribute blob.
| <TestJar Include="java\**\*.java" /> | ||
| <TestJar | ||
| Include="java\**\*.java" | ||
| Exclude="java\java\util\Collection.java" |
There was a problem hiding this comment.
Or better question: is it missing parameters?
There was a problem hiding this comment.
It's excluded/missing parameters so that the Xamarin.Android.Tools.BytecodeTests.ParameterFixupTests.XmlDeclaration_FixedUpFromApiXmlDocumentation test continues to work.
That test works by "fixing up" parameter names based on imported XML, but the import only works if the parameter names are "generated" (e.g. p1...). If Collection.java is compiled with javac -parameters, it will have correct parameter names, not generated names, which "breaks" the ParameterFixupTests.XmlDeclaration_FixedUpFromApiXmlDocumentation() tests.
This breakage might be acceptable -- i.e. we fixup the expected XML, and the test passes -- but by doing so, we're no longer testing/ensuring that parameter fixup continues to work.
There was a problem hiding this comment.
Excluding the source means we are not testing that type either, right? Leaving that extra sources would just confuse future test coding and I don't see reason to not change the expected XML.
There was a problem hiding this comment.
The source isn't fully excluded. Collection.java is added to the @(TestJarNoParameters) item group. @(TestJar) is built with javac -parameters, while @(TestJarNoParameters) is built without javac -parameters.
The Collection.java source is still built.
Additionally, there are no other tests on Collection.class that check the names of parameters, so keeping it around as a file built without javac -parameters makes sense.
There was a problem hiding this comment.
But then the resulting state is that future test writers are likely lost when they need to add new test files because the build involves two different javac compilation steps? What if they are mutually dependent on the ones in @(TestJar) and @(TestJarNoParameters) ? I still don't think this is the way to go.
There was a problem hiding this comment.
I'm open to suggestions for alternate strategies.
What if they are mutually dependent on the ones in @(TestJar) and @(TestJarNoParameters) ?
Try to avoid that? Or we could introduce a third item group which is compiled after those other item groups... It's solvable. Not necessarily "pretty" or "sane," but solvable.
Changes: dotnet/android-tools@479931c...554d45a * dotnet/android-tools@554d45a: [Xamarin.Android.Tools.AndroidSdk] Fix CS8600 in AndroidSdkBase (#107) * dotnet/android-tools@19454f9: Bump to xamarin/LibZipSharp/main@86f8ae57 [1.0.24] (#111) * dotnet/android-tools@3582b39: [macOS] fix DirectoryNotFoundException on clean systems (#110) * dotnet/android-tools@ca820e5: Bump to xamarin/LibZipSharp/main@521b54ec [1.0.23] (#109)
Fixes: #106
Java 8 adds support for a
javac -parametersoption, which adds aMethodParametersattribute to the.classfile. TheMethodParametersattribute blob contains reliable parameter names,including for abstract methods and interface methods, unlike existing
LocalVariableTable, which only exists forjavac -gbuilds, andonly for methods with bodies, i.e. not abstract methods and
interface methods.
Add support for parsing the
MethodParametersattribute blob.