native icon indicating copy to clipboard operation
native copied to clipboard

Obviously incorrect configurations should error or warn

Open stuartmorgan-g opened this issue 3 weeks ago • 3 comments

If I run the following generation script:

  FfiGenerator(
    [...]
    objectiveC: ObjectiveC(
      interfaces: Interfaces(
        includeMember: (Declaration declaration, String member) {
          final String interfaceName = declaration.originalName;
          final String signature = member;
          return switch (interfaceName) {
            'NSFileManager' => <String>{
              'containerURLForSecurityApplicationGroupIdentifier:',
              'defaultManager',
            }.contains(signature),
            'NSURL' => <String>{
              'fileURLWithPath:',
              'URLByAppendingPathComponent:',
            }.contains(signature),
            _ => false,
          };
        },
      ),
    ),
  ).generate();

it succeeds without any warning messages or errors. It also doesn't contain any NSFileManager or NSURL methods, because there's no include directive.

I can see by adding logging that it's calling my includeMember filter for every interface; if it's going to call a method that checks whether to include (interface, method), and also call a method (in this case a default that always returns false) to check whether to include (interface), and the behavior is that both have to return true for a given interface, then it should be either a very prominent warning or an error if include(interface) returns false but includeMember(interface, member) returns true.

(Alternately, the behavior could change such that includeMember overrides include; it's not clear to me what the use case for the current behavior actually is.)

stuartmorgan-g avatar Nov 10 '25 21:11 stuartmorgan-g

Alternately, the behavior could change such that includeMember overrides include; it's not clear to me what the use case for the current behavior actually is.

Historical reasons. Member filtering was bolted onto the yaml config after the fact, and that approach was carried over when we moved to dart configs. We should redesign this API as part of the wider config API cleanup planned for ffigen 21.

It's useful to support both styles of filtering, since include is a good first step on a new project, and includeMember is only needed later, if at all. Applying both approaches to different classes in the same project makes sense too. But maybe there's a way of unifying these filters, while still making the include entire class use case easy to do. I suppose if we added a rule that excluding all the methods of a class excludes the class, that would work. Then include could be emulated by includeMember.

liamappelbe avatar Nov 10 '25 23:11 liamappelbe

It's useful to support both styles of filtering, since include is a good first step on a new project, and includeMember is only needed later, if at all.

Agreed; to be clear, when I said "overrides" I meant "causes include not to be used" (e.g., with an assert they aren't both passed), not "replaces in the API surface".

Applying both approaches to different classes in the same project makes sense too.

This is the part I question; it seems like by the time you've written an includeMember function, having it return true for some set of classes without looking at the member is pretty trivial, and requiring that would allow for making the behavior much simpler because there's no potential interaction between the two members.

stuartmorgan-g avatar Nov 11 '25 00:11 stuartmorgan-g

Yeah. The same argument probably also applies to rename. Whether or not you choose to rename a declaration is currently independent of whether you choose to include it, but that feels redundant. If you rename a declaration, presumably you want to include it. There's also renameMember.

I wonder if there's an API that makes sense that would allow us to merge all 4 of these methods. jnigen solves this by just giving the user the ability to write visitors, which can rename or filter or even add methods. But Hossein found that to be a bit of a can-of-worms, and it took ages to implement, so I'm also open to other ideas.

Maybe we replace these with a single filter function, like bool filter(Declaration declaration, List<Member> members). The function can return true/false to include/exclude the entire declaration, and it can remove entries from the members list to filter the members. We'd rename Declaration.originalName to Declaration.name, and make it mutable, for renaming. And Member would just have a mutable String name field for renaming the member.

liamappelbe avatar Nov 11 '25 02:11 liamappelbe