[ffigen] skip SWIFT_UNAVAILABLE

[Hassnaa9]

Fixes: #2665
ffigen was generating a no arg constructor, for Swift classes that explicitly mark init/new asSWIFT_UNAVAILABLE,
causing runtime crashes. Fixed by detecting the annotation via Clang's platform availability AST and blocking the inherited new from being copied onto the interface during code generation.

[Hassnaa9]

Hi @liamappelbe,

Following your suggestion, I added print statements to dump the full platform availability information for every method. This revealed that SWIFT_UNAVAILABLE expands to a platform availability attribute with name=swift, unavailable=1 in Clang's AST.

After correctly filtering init and new from Animal's parsed methods, the no argument Animal() constructor was still being generated. It turned out that the root cause i think is CopyMethodsFromSuperTypesVisitation, which copies class methods from NSObject onto every subclass. As a result, new was being re-added to Animal after the parser filter had already run.

I initially tried guarding against this in the visitor, but due to the execution order the guard triggered too early.

The final fix stores the unavailable selectors in swiftUnavailableSelectors on ObjCInterface, and checks them during code generation in objc_interface.dart.

Thanks!

[github-actions]

PR Health

Breaking changes ✔️

Package	Change	Current Version	New Version	Needed Version	Looking good?
objective_c	Breaking	9.3.0	9.4.0-wip	9.4.0-wip	✔️

This check can be disabled by tagging the PR with skip-breaking-check.

API leaks ✔️

The following packages contain symbols visible in the public API, but not exported by the library. Export these symbols or remove them from your publicly visible API.

Package	Leaked API symbol	Leaking sources

This check can be disabled by tagging the PR with skip-leaking-check.

[liamappelbe]

Good job getting it working despite those issues. I wonder if there's a better approach to the filtering issues though.

Your current approach, where these methods aren't filtered until codegen, has a minor issue where they'll still participate in the logic to pull in transitive deps. This could result in unnecessary bindings being generated. So you'd have to also update the other visitors to check if the method is swiftUnavailable.

A better approach would be to remove ApiAvailability.swiftUnavailable, and set ApiAvailability.alwaysUnavailable instead. You'll run into the same bug as before, where filtering removes the method only for copying from supertypes to add it back in.

But I think this is actually an existing bug you've discovered, which also applies to any other method with an availability annotation. We filter out methods (and other APIs) based on their availability annotation during parsing, which is a bit of a hack. If this results in a method being filtered, there's nothing stopping a supertype from re-adding that method later.

So I think it would be better to fix that bug for all cases, rather than special casing swiftUnavailable to work around it. Rather than deleting filtered methods, we should keep them around so that the copy supertype methods visitor can avoid overwriting the unavailable method.

1. Remove ApiAvailability.swiftUnavailable and set ApiAvailability.alwaysUnavailable instead when you see that swift availability annotation.
2. Remove this hacky filter. And this one too.
3. Add an unavailable util method to ObjCMethod that checks the availability annotation, same as the one on ObjCInterface.
4. Update ObjCMethods._shouldReplaceMethod to return false if either oldMethod.unavailable or newMethod.unavailable. This fixes the copy supertype methods bug.
5. Update ApplyConfigFiltersVisitation, adding a check for m.unavailable to all 3 node.filterMethods methods.

[Hassnaa9]

@liamappelbe Thanks for the detailed feedback, I think i am lucky at discovering bugs through issues i try to resolve :)

I tried implementing your suggested approach but hit a conflict in Step 1.

Setting alwaysUnavailable=true for swift unavailable methods caused issues because __attribute__((unavailable)) also sets alwaysUnavailable=true via Clang's clang_getCursorPlatformAvailability directly. The existing deprecated_test expects methods marked with __attribute__((unavailable)) to still appear in bindings, so filtering on !m.unavailable in Step 5 was incorrectly removing them too.

Since the two annotations come from different sources in the AST __attribute__((unavailable)) sets the alwaysUnavailable output parameter directly, while SWIFT_UNAVAILABLE comes through the platform loop as swift with unavailable=1, I tried to merge two solutions by keping swiftUnavailable as a separate field but addressed your transitive deps concern by filtering it in ApplyConfigFiltersVisitation (Step 5) rather than at codegen time. So ended with addressing points 3,4,5 only

[liamappelbe]

The existing deprecated_test expects methods marked with __attribute__((unavailable)) to still appear in bindings, so filtering on !m.unavailable in Step 5 was incorrectly removing them too.

That's bizarre. Sounds like the test is incorrect. But it was passing, so I guess that's also a bug? Can you investigate that and figure out why they're not being omitted? Check if those method's availability data is being correctly parsed here.

Were there any other issues with the approach I suggested, or was it just that failing test?

[Hassnaa9]

. Sounds like the test is incorrect. But it was passing,

@liamappelbe You were right the deprecated_test expectations were incorrect.
I found a bug In _getAvailability, when no version info is configured (iosVer == null && macosVer == null), it returns Availability.all immediately, before checking alwaysUnavailable. This means methods marked with __attribute__((unavailable)) incorrectly get Availability.all instead of Availability.none when no platform versions are configured. The test was written around this buggy behavior.
So i try to fix it by moving the alwaysUnavailable check before the early return so it always returns Availability.none regardless of version config. Updated the 3 incorrect deprecated_test expectations and 1 api_availability_test expectation accordingly.

and all tests pass now based on the approach you suggested, i will push the changes now

[Hassnaa9]

@liamappelbe I think this failure due to that our solution can't distinguish between developer explicitly marking init as SWIFT_UNAVAILABLE and Swift compiler auto-generating?
So can you help me to know what's different in the ObjC header between them

[liamappelbe]

No, these failures just mean that you need to regenerate those bindings.

[Hassnaa9]

No, these failures just mean that you need to regenerate those bindings.

@liamappelbe Oh ok, can you please do it as I do not have a mac and test using GitHub actions?

Or let me first try using AI to compare between expected and actual and update them.

[liamappelbe]

Done

[liamappelbe]

That's weird. Why did the signature of NSOutputStream.write change?

[liamappelbe]

I get the same diff at HEAD, so this isn't a bug in your PR. I'll investigate and push a fix asap (probably tomorrow).

[Hassnaa9]

I get the same diff at HEAD, so this isn't a bug in your PR. I'll investigate and push a fix asap (probably tomorrow).

Ok, Let me know if there's anything I can help with.

[liamappelbe]

Figured it out. #3135 made it so that FFIgen respects which version of xcode you have selected, and on my machine I have a few versions of xcode. The version I had selected was a really old version that apparently declares these APIs differently in the headers. That's concerning, but I can easily work around it by selecting a different xcode version.

[Hassnaa9]

FFIgen respects which version of xcode you have selected

FFIgen dealing with many layers,So i think it's more complex, unlike swift2objc, which has to go in one direction, which encourages me to put more effort into it the next days
anyway thanks alot for you help.

[liamappelbe]

Looks pretty good. Needs a changelog entry though, in ffigen and objective_c.

The objective_c changelog should say: "Removed NSOrderedCollectionChange.init(). This is not a breaking change because this method never would have worked, as it doesn't exist in Objective-C.

[liamappelbe]

Is this still necessary? I thought your fixes to the deprecated test meant you didn't need this extra treatSwiftUnavailableAsUnavailable logic anymore?