[jnigen][ffigen] Standardize filtering policy

[liamappelbe]

In ffigen, when an API element is included by the config, all its transitive deps are also included. Eg, if the user asks for a class to be generated (and doesn't filter the methods), we generate bindings for all the methods and all the classes consumed or returned by those methods.

In jnigen, transitive deps are not included by default. Instead, if a method refers to a class that hasn't been explicitly requested by the user, it will be replaced by a JObject (or maybe a stub in future).

We should settle on one approach. I'm about to refactor ffigen's filtering, so now is a good time to think about it.

• Ffigen's approach leads to a huge amount of bindings being generated even when the user only asks for one class.
• Jnigen's approach means more iterative rounds of adding things to the config and regenerating the bindings.

@stuartmorgan Any opinions about which has been easier?

[stuartmorgan]

I've been giving this some thought over the past couple of weeks; I don't think either is the ideal model, but my ideal would be much closer to jnigen's

ffigen's transitive behavior creates truly enormous files, which caused me no end of problems:

• I routinely was blocked or semi-blocked by generation bugs in edge cases that had absolutely nothing to do with the code I was trying to use, but couldn't not generate. Unless we believe we will achieve a completely bug-free state in the generator (I wouldn't want to bet on that), making orders of magnitude more code will make people orders of magnitude more likely to hit bugs.
• Giant files are awful. They are nearly impossible to navigate, they bog down IDEs, they bring the analyzer to its knees (e.g., very substantial lag in any warnings or errors showing up in my IDE after updating the generation), they make the formatter take forever (I know this one in particular is something we believe is fixed for the future), they make code search slower, etc. Basically, any tool—whether we own it or not—has a reasonable chance of struggling with code files of the sizes we are talking about, and we cannot expect that to change in the foreseeable future even if we fix some of the individual instances we control.

jnigen's approach made files that felt reasonable to read/navigate as a human, and didn't cause any tooling problems. There are a couple of issues (some of which I've filed issues for):

• I think it's currently too conservative about some core transitive dependencies. Most notably, I think including class A should fully bring in every class that A inherits from and interface that A implements (transitively), because it's extremely confusing to have methods missing from A unless I know the class structure. E.g., I've maintained code in video_player for quite a while, and never needed to know that most of the methods I call on ExoPlayer are actually methods from the Player superclass—until I had to troubleshoot why the generated Dart ExoPlayer was missing most of its methods.
• I think JObject is a dangerous replacement for missing classes. I accidentally wrote code that I didn't realize was wrong until it crashed because it was all just JObjects; it's pretty common to have code where you transfer objects around without really interacting with them, and thus not think to generate them, and then end up with code like foo = Foo(bar.baz()), where I never made Baz, and so I could replace bar.baz() with literally any Java object and it would compile. I don't expect to be able to trivially write code that has essentially no type safety—neither normal Dart code nor normal Java code would have allowed that.

What I think I'd like to see for both is something that has ffigen-style upward transitive inclusion, but jnigen-style pruning for everything else, but (and maybe this is what you mean by "a stub in future") instead of replacing non-included classes with JObject, replace them with a minimal but fully-typed class. I think it would be nice if we could indicate that in the name—e.g., a non-included Foo becomes Foo_Stub or something—to reduce potential confusion if someone then tries to use it, although of course there are potential collision problems if we alter only some class names. That class would be essentially empty, but I would want it to have all of the correct inheritance (creating more stubs up the hierarchy as necessary) so that it could always be passed around in the same ways the real object could be.

[stuartmorgan]

Jnigen's approach means more iterative rounds of adding things to the config and regenerating the bindings.

Something we could consider if this ends up being a usability problem is an optional mode to do current-ffigen-style exhaustive generation (although we still have the unnecessary-broken-generation problem to contend with there). I could imagine a workflow where you start with exhaustive generation once, write your Dart code, and then look at everything you used to generate a config for what you ended up needing and that being what you check in (and iterate on if/when you change the code). Incremental small additions would likely be easier than the initial process of finding all the things you need to start.

If we wanted to get really fancy, in theory it seems like we could build a tool that would analyze the code a client has written to see what it uses, and then automatically generate a tight-bound config file from it. That would be very cool, but not necessarily worth the eng effort depending on how complex it would be in practice.

(I will say that in practice, I had to do a lot of incremental generation with ffigen too, because I wasn't generating the entire SDK, so I still had to keep adding new root items as I found them. It was just a more painful iteration because of the file size. Maybe that's my mistake for using exclude-all-by-default: true, but I had enough issues with transitive includes without it that I didn't feel like I had much choice when dealing with a whole system SDK. I'd be interested to see what that experience is like once we've ensured that we can actually generate the entire iOS system API surface without errors.)

[liamappelbe]

What I think I'd like to see for both is something that has ffigen-style upward transitive inclusion, but jnigen-style pruning for everything else

SGTM!

@HosseinYousefi is this what you're planning to do in jnigen? We should try and make the behavior as similar as possible.

I think it would be nice if we could indicate that in the name—e.g., a non-included Foo becomes Foo_Stub or something—to reduce potential confusion if someone then tries to use it

Foo_Stub might make migration annoying, if the user later decides they want Foo. What if we just document it clearly in the bindings instead?

// <Foo's API documentation, if any>
class Foo extends objc.NSObject {
  // NOTE: This class is a stub. To generate its methods, add Foo to your config's includes.
}

Something we could consider if this ends up being a usability problem is an optional mode to do current-ffigen-style exhaustive generation (although we still have the unnecessary-broken-generation problem to contend with there).

Yeah, I'll definitely make this behavior a config option (for backwards compatibility, and because the current behavior works well for C bindings).

But one thing I want to do is make some helper functions for constructing configs for ffigen's Dart API. We're at a stage now with the config yaml where the defaults are tuned for C bindings, and for ObjC we have a whole bunch of recommended options. So it'd be good to have helper functions that generate configs for specific use cases. In that case I'd enable the new behavior by default in the ObjC helper.

[HosseinYousefi]

• Most notably, I think including class A should fully bring in every class that A inherits from and interface that A implements (transitively), because it's extremely confusing to have methods missing from A unless I know the class structure.

That makes sense. I will address this.

• I think JObject is a dangerous replacement for missing classes.

I'm with you, that's why I had opened #642.

If we wanted to get really fancy, in theory it seems like we could build a tool that would analyze the code a client has written to see what it uses, and then automatically generate a tight-bound config file from it. That would be very cool, but not necessarily worth the eng effort depending on how complex it would be in practice.

That's a fantastic idea and actually we need to do something similar to this to prune the native code, in case of jnigen, in form of generating proguard-rules: #681.

@HosseinYousefi is this what you're planning to do in jnigen? We should try and make the behavior as similar as possible.

Yep, sgtm!

[stuartmorgan]

Foo_Stub might make migration annoying, if the user later decides they want Foo.

If they have ever actually explicitly made something of type Foo_Stub it could be annoying, but I would argue (and we could mention in docs) that if you have enough explicit references to Foo_Stub that it would be annoying to change then you should be generating Foo. I would expect that the usual number of references to be zero; in the cases I hit, I did not realize when writing the code that I had a JObject, because I never had a reference to any of these objects. It was all inline calls. If I'd made a variable, I would have realized I needed Foo and added it to my config.

I'm not sure the use case of "I want to store a Foo just to pass it to other things later but I really don't want to generate Foo" is one we need to design for; I'd be curious what the use cases would be where generating Foo is actually undesirable.

What if we just document it clearly in the bindings instead?

// <Foo's API documentation, if any>
class Foo extends objc.NSObject {
  // NOTE: This class is a stub. To generate its methods, add Foo to your config's includes.
}

My question/concern there is whether people find that comment easily enough. In particular I'm thinking about someone trying to figure out why some methods are in the autocomplete list on an instance of Foo (because they are from a generated superclass), but others aren't.

One of my biggest usability concerns of the *gen tools as compared to Pigeon is that with Pigeon, I explicitly write a simple interface definition, and the output is entirely predictable, so I'm not surprised when I go to interact with the generated code, whereas with *gen there's a ton of complex output that can't be trivially predicted from the config I write, so I keep hitting surprises, and then trying to figure out if it's because:

• I just forgot something,
• I have a mistake in my config (e.g., a typo)
• there's a bug in the generator
• there's an intentional-but-not-yet-known-to-me behavior of the generator (e.g., method renaming)

Because of that, my default position is to err on the side of making stubs really obvious. Maybe I'm overly concerned about this and it would be fine with that comment though; we'd probably need to do some user studies to actually find out.

[HosseinYousefi]

Because of that, my default position is to err on the side of making stubs really obvious.

+1. Foo_Stub might be too disruptive. I would at least put it as a WARNING at the beginning of its doc comments:

/// WARNING: This class is a stub, to actually generate it run 
/// `dart run jnigen:add_class 'baz.bar.Foo' --config <the path to your jnigen.yaml>`.
///
/// ... the rest of the doc comment ...
// Maybe a `@stub` annotation that shows a lint as well?
class Foo extends JObject { /* ... */ }

And also add a add_class to both jnigen and ffigen which adds the class and reruns the generation to improve the user experience.

[liamappelbe]

The ObjC side of this is done.

@HosseinYousefi can we close this issue? Are you using this to track the feature, or do you have separate bugs for your side?

[HosseinYousefi]

The ObjC side of this is done.

@HosseinYousefi can we close this issue? Are you using this to track the feature, or do you have separate bugs for your side?

I'm using this to track it. (Since you had tagged both, I didn't create another issue)