Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

[Xamarin.Android.Tools.Bytecode] Support @JvmOverloads #651

Merged

Conversation

jonpryor
Copy link
Member

@moljac ran into an interesting warning from class-parse when
binding OfficeUIFrabric:

warning : class-parse: warning: method com/microsoft/officeuifabric/widget/BottomNavigationView.<init>(Landroid/content/Context;)V: Local variables array has 0 entries ('LocalVariableTableAttribute(LocalVariableTableEntry(Name='context', Descriptor='Landroid/content/Context;'))'); descriptor has 1 entries!

For starters, this shouldn't be a warning; it's not actionable.
There is nothing a user can do to fix this warning; it's only
meaningful to the Java.Interop team.

Update Xamarin.Android.Tools.Bytecode.MethodInfo.GetParameters() so
that these messages are debug messages, not warnings.

class-parse knows how many parameters are present based on the JNI
method descriptor, in this case (Landroid/content/Context;)V, which
specifies one parameter type. The JNI descriptor doesn't contain
parameter name information; where do parameter names come from?

If the .class file was built via javac -parameters, then the
MethodParametersAttribute blob can be used; see 4273e5c.

However, before checking for MethodParametersAttribute,
class-parse will attempt to use the LocalVariableTableAttribute
and LocalVariableTableEntry values to infer parameter names.
As part of this inference, method parameters are assumed to be
variables with LocalVariableTableEntry.StartPC is 0; from
class-parse --dump BottomNavigationView.class:

LocalVariableTableAttribute(
  LocalVariableTableEntry(Name='context', Descriptor='Landroid/content/Context;', StartPC=0, Index=1))

Next, class-parse will "skip" the first variable for instance
methods. This results in "skipping" the context variable, resulting
in the message:

Local variables array has 0 entries …; descriptor has 1 entries!

To fix this, class-parse needs a stricter "skip the this
parameter" check: the first parameter should only be skipped when:

  1. The method is an instance method, not a static method, and
  2. The JNI descriptor of the first parameter is the same as the
    descriptor of the declaring type.

Additionally, update the code style to use enumValue.HasFlag(V)
instead of (enumValue & V) == V for readability.

Finally, why was this failing in the first place? In some
circumstances, when the Kotlin [@JvmOverloads][2] annotation is used
on a constructor, Kotlin will emit all possible overloads for the
constructor, and in many of those overloads the this parameter
won't be present in the LocalVariableTableAttribute data, as seen
above with BottomNavigationView.

@moljac ran into an interesting warning from `class-parse` when
binding [OfficeUIFrabric][0]:

	warning : class-parse: warning: method com/microsoft/officeuifabric/widget/BottomNavigationView.<init>(Landroid/content/Context;)V: Local variables array has 0 entries ('LocalVariableTableAttribute(LocalVariableTableEntry(Name='context', Descriptor='Landroid/content/Context;'))'); descriptor has 1 entries!

For starters, this shouldn't be a warning; it's not actionable.
There is nothing a user can do to fix this warning; it's only
meaningful to the Java.Interop team.

Update `Xamarin.Android.Tools.Bytecode.MethodInfo.GetParameters()` so
that these messages are *debug* messages, not warnings.

`class-parse` knows how many parameters are present based on the JNI
method descriptor, in this case `(Landroid/content/Context;)V`, which
specifies one parameter type.  The JNI descriptor *doesn't* contain
parameter name information; where do parameter names come from?

If the `.class` file was built via `javac -parameters`, then the
`MethodParametersAttribute` blob can be used; see 4273e5c.

However, before checking for `MethodParametersAttribute`,
`class-parse` will attempt to use the `LocalVariableTableAttribute`
and `LocalVariableTableEntry` values to infer parameter names.
As part of this inference, method parameters are assumed to be
variables with `LocalVariableTableEntry.StartPC` is 0; from
`class-parse --dump BottomNavigationView.class`:

	LocalVariableTableAttribute(
	  LocalVariableTableEntry(Name='context', Descriptor='Landroid/content/Context;', StartPC=0, Index=1))

Next, `class-parse` will "skip" the first variable for instance
methods.  This results in "skipping" the `context` variable, resulting
in the message:

	Local variables array has 0 entries …; descriptor has 1 entries!

To fix this, `class-parse` needs a stricter "skip the `this`
parameter" check: the first parameter should *only* be skipped when:

 1. The method is an instance method, not a `static` method, *and*
 2. The JNI descriptor of the first parameter is the same as the
    descriptor of the declaring type.

Additionally, update the code style to use `enumValue.HasFlag(V)`
instead of `(enumValue & V) == V` for readability.

Finally, why was this failing in the first place?  In some
circumstances, when the Kotlin [`@JvmOverloads`][2] annotation is used
on a `constructor`, Kotlin will emit all possible overloads for the
constructor, and in many of those overloads the `this` parameter
*won't* be present in the `LocalVariableTableAttribute` data, as seen
above with `BottomNavigationView`.

[0]: https://jcenter.bintray.com/com/microsoft/uifabric/OfficeUIFabric/
[1]: https://kotlinlang.org/api/latest/jvm/stdlib/kotlin.jvm/-jvm-overloads/
@jonpryor jonpryor requested a review from jpobst May 20, 2020 20:12
@jpobst jpobst merged commit 4b266fa into dotnet:master May 20, 2020
@jpobst jpobst added this to the 10.5 (16.8 / 8.8) milestone Jun 5, 2020
jonpryor added a commit that referenced this pull request Jun 11, 2020
@moljac ran into an interesting warning from `class-parse` when
binding [OfficeUIFrabric][0]:

	warning : class-parse: warning: method com/microsoft/officeuifabric/widget/BottomNavigationView.<init>(Landroid/content/Context;)V: Local variables array has 0 entries ('LocalVariableTableAttribute(LocalVariableTableEntry(Name='context', Descriptor='Landroid/content/Context;'))'); descriptor has 1 entries!

For starters, this shouldn't be a warning; it's not actionable.
There is nothing a user can do to fix this warning; it's only
meaningful to the Java.Interop team.

Update `Xamarin.Android.Tools.Bytecode.MethodInfo.GetParameters()` so
that these messages are *debug* messages, not warnings.

`class-parse` knows how many parameters are present based on the JNI
method descriptor, in this case `(Landroid/content/Context;)V`, which
specifies one parameter type.  The JNI descriptor *doesn't* contain
parameter name information; where do parameter names come from?

If the `.class` file was built via `javac -parameters`, then the
`MethodParametersAttribute` blob can be used; see 4273e5c.

However, before checking for `MethodParametersAttribute`,
`class-parse` will attempt to use the `LocalVariableTableAttribute`
and `LocalVariableTableEntry` values to infer parameter names.
As part of this inference, method parameters are assumed to be
variables with `LocalVariableTableEntry.StartPC` is 0; from
`class-parse --dump BottomNavigationView.class`:

	LocalVariableTableAttribute(
	  LocalVariableTableEntry(Name='context', Descriptor='Landroid/content/Context;', StartPC=0, Index=1))

Next, `class-parse` will "skip" the first variable for instance
methods.  This results in "skipping" the `context` variable, resulting
in the message:

	Local variables array has 0 entries …; descriptor has 1 entries!

To fix this, `class-parse` needs a stricter "skip the `this`
parameter" check: the first parameter should *only* be skipped when:

 1. The method is an instance method, not a `static` method, *and*
 2. The JNI descriptor of the first parameter is the same as the
    descriptor of the declaring type.

Additionally, update the code style to use `enumValue.HasFlag(V)`
instead of `(enumValue & V) == V` for readability.

Finally, why was this failing in the first place?  In some
circumstances, when the Kotlin [`@JvmOverloads`][2] annotation is used
on a `constructor`, Kotlin will emit all possible overloads for the
constructor, and in many of those overloads the `this` parameter
*won't* be present in the `LocalVariableTableAttribute` data, as seen
above with `BottomNavigationView`.

[0]: https://jcenter.bintray.com/com/microsoft/uifabric/OfficeUIFabric/
[1]: https://kotlinlang.org/api/latest/jvm/stdlib/kotlin.jvm/-jvm-overloads/
@github-actions github-actions bot locked and limited conversation to collaborators Apr 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants