[jnigen] Class bindings implement all super interfaces#3201
[jnigen] Class bindings implement all super interfaces#3201liamappelbe wants to merge 20 commits intomainfrom
Conversation
PR HealthBreaking changes ✔️
This check can be disabled by tagging the PR with
API leaks
|
| Package | Leaked API symbol | Leaking sources |
|---|---|---|
| jni | $JIterator | core_bindings.dart::JIterator::implementIn::$impl core_bindings.dart::JIterator::implement::$impl |
| jni | $JCollection | core_bindings.dart::JCollection::implementIn::$impl core_bindings.dart::JCollection::implement::$impl |
| jni | $JList | core_bindings.dart::JList::implementIn::$impl core_bindings.dart::JList::implement::$impl |
| jni | $JMap$JEntry | core_bindings.dart::JMap$JEntry::implementIn::$impl core_bindings.dart::JMap$JEntry::implement::$impl |
| jni | $JMap | core_bindings.dart::JMap::implementIn::$impl core_bindings.dart::JMap::implement::$impl |
| jni | $JSet | core_bindings.dart::JSet::implementIn::$impl core_bindings.dart::JSet::implement::$impl |
This check can be disabled by tagging the PR with skip-leaking-check.
… into jnigen_super_interfaces
github-actions
bot
added
the
type-infra
github-actions
bot
removed
the
type-infra
liamappelbe
changed the title
dcharkes
left a comment
dcharkes
left a comment
There was a problem hiding this comment.
We should test something like diamond inheritance
And also something with methods satisfying the two methods from different interfaces at the same time. (Where for example the params are more general in the implementation than in either of the interfaces.)
|
|
||
| // ignore_for_file: prefer_relative_imports'''; | ||
|
|
||
| final packageRoot = Platform.script.resolve('..'); |
There was a problem hiding this comment.
sidenote if you want this script to be includable from other places you could
adopt findPackageRoot that we have in other packages in this repo.
| @@ -84,6 +84,8 @@ extension type Example$Nested$NestedTwice._(jni$_.JObject _$this) | |||
| } | |||
| } | |||
|
|
|||
| extension Example$Nested$NestedTwice$$Methods on Example$Nested$NestedTwice {} | |||
There was a problem hiding this comment.
We're also generating the extension if its empty. That's probably fine, but a
bit weird.
| r'com/github/dart_lang/jnigen/inheritance/DerivedInterface'); | ||
| /// from: `com.github.dart_lang.jnigen.inheritance.Child` | ||
| extension type Child._(jni$_.JObject _$this) | ||
| implements BaseClass<jni$_.JString?>, BaseInterface { |
|
|
||
| package com.github.dart_lang.jnigen.inheritance; | ||
|
|
||
| public class Child extends BaseClass<String> implements BaseInterface { |
There was a problem hiding this comment.
Add a more crazy test with a bunch of weird diamond patterns
- interface Animal
- interface Mammal implements Animal
- interface FourLegger implements Animal
- interface Dog implements Mammal, Fourlegger (and thus implements Animal in two
different ways) - interface Furry
- class ShibaInu implements Dog, Furry
And make sure to add some non-safe-nullability overrides for the methods, just
so that we cover that.
dcharkes
left a comment
dcharkes
left a comment
There was a problem hiding this comment.
LGTM once test cases have been added.
2 participants
This is a minor QoL improvement, allowing child types to be passed to super types without a cast. Also improves self-documentation.
After this change I started getting extension_type_inherited_member_conflict errors. The fix for this in FFIgen was to move all instance methods to extension methods, so I've done that here too. Static methods remain part of the extension type.
Unrelated change: updated some of the JNIgen scripts to explicitly set their
AndroidSdkConfig.androidExampledirectory, so that they don't depend on the working directory.TODO: Still trying to figure out why the pdfbox example is failing. Can't repro the issue locally.
Fixes #2012