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

Figure out and document (a) pattern(s) for handling #if #1589

Open
stuartmorgan opened this issue Sep 23, 2024 · 9 comments
Open

Figure out and document (a) pattern(s) for handling #if #1589

stuartmorgan opened this issue Sep 23, 2024 · 9 comments

Comments

@stuartmorgan
Copy link

stuartmorgan commented Sep 23, 2024

This is a high-level issue for general tracking, since it's a topic I'm sure we'll need at least some solution to, even if it's docs. Maybe we will identify specific use cases that we can split out into sub-issues to address in specific ways, but we will need some way of answering a user with the question "What do I do about this #if/#ifdef when I'm trying to use FFI?"

There are lots of potential use cases; here are a couple of concrete ones to provide a starting point:

  1. I have a plugin that is 95+% shared iOS/macOS code, and I'm trying to convert it from Obj-C to FFI. The code that's not shared is behind a platform #if check. What's the right way to handle that in Dart? For now, I changed it to a Platform runtime check, but that's objectively worse: in the native version, if I accidentally try to use an iOS API in the macOS codepath, the code won't compile. In the Dart version, my IDE will happily auto-complete APIs that I can't actually use, without even a warning, and the code will crash at runtime, and only (I think?) once I hit that code. Can we do better?

  2. WKWebView has the following definition:

    #if TARGET_OS_IPHONE
    WK_EXTERN API_AVAILABLE(macos(10.10), ios(8.0))
    @interface WKWebView : UIView
    #else
    WK_EXTERN API_AVAILABLE(macos(10.10), ios(8.0))
    @interface WKWebView : NSView
    #endif 

    What should ffigen even create here? Is there any good option? The general case of this can get arbitrarily complicated; e.g., if the answer to the first question is that it inherits from some franken-class that is a mashup of NSView and UIView so that I can get any method I need, what happens if there's a collision between methods (e.g., each has a method with the same name but a different return type—a plausible scenario since methods called view that return the platform view type are common, for example). Or do we recommend putting everything behind a more FFI-friendly native facade in a case like this?

(#1469 touched on this slightly, but was specifically about the generation of code that did not compile, rather than the overall use case.)

@dcharkes
Copy link
Collaborator

👍 To the above.

Somewhat related: If the variability per OS is mappable to one type, we could generate ABI specific types. We already support that for integers, but not yet for structs (dart-lang/sdk#42816). There will definitely be cases which don't map to types such as different arguments to a function or different functions altogether.

What should ffigen even create here? Is there any good option? The general case of this can get arbitrarily complicated.

In general, the variation can be arbitrary, behind arbitrary if-defs. In practice, I've mostly seen variation based on target OS. Maybe we should generate bindings per ifdef config. I believe @liamappelbe had some ideas about this? But it would be good if everything that's not behind an ifdef is shared. But then every type refering to such type cannot be shared anymore either.

I think it would be good to start accumulating examples so that we can try to make an informed decision. @stuartmorgan would you mind linking the various examples that you find in this issue?

if the answer to the first question is that it inherits from some franken-class that is a mashup of NSView and UIView so that I can get any method I need

Yeah that probably doesn't scale. 😄

Or do we recommend putting everything behind a more FFI-friendly native facade in a case like this?

Do you mean a wrapper in native code and then running FFIgen on that wrapper? Or did you mean something else?

@stuartmorgan
Copy link
Author

I think it would be good to start accumulating examples so that we can try to make an informed decision. @stuartmorgan would you mind linking the various examples that you find in this issue?

I am only going to personally come across a vanishingly small fraction of cases in my own development. Since the goal is to provide access to the entire OS SDK surface, the way to make a reasonably fully informed decision (for Obj-C/Swift) seems like it would be to use tooling to audit all use of #if in the iOS and macOS SDKs.

Or do we recommend putting everything behind a more FFI-friendly native facade in a case like this?

Do you mean a wrapper in native code and then running FFIgen on that wrapper? Or did you mean something else?

Yes, a native wrapper that does not itself expose any structural platform #ifs like the WKWebView inheritance case. But of course, that means in this example that would have to wrap all of WKWebView, as well as any *View superclass methods I needed, in manual native code. I mention that option not because I think it's good, but to set a lower bound of "if we can't figure out anything better, we at least need to document the workaround".

@liamappelbe
Copy link
Contributor

liamappelbe commented Sep 24, 2024

2. WKWebView has the following definition:

If the platform differences are that great, my suggestion would be to generate 2 different sets of ffigen bindings, and conditionally import them. If you only use a shared subset of the methods, that would just work. If you need to use methods that only exist on one platform then you'd need to also put that code behind a conditional import. But at least that would let you write the compatibility layer in Dart.

It'd be nice if Dart had something like C++'s if constexpr(cond) { body }, where body isn't even compiled if cond is false at compile time. That way you wouldn't have to write multiple conditionally imported files just to use a few differing methods.

@stuartmorgan
Copy link
Author

stuartmorgan commented Sep 24, 2024

and conditionally import them

Based on what condition? Last I checked conditional import by platform was impossible. Was there a language change?

(Edit: The public docs still seem to indicate that this is impossible.)

@liamappelbe
Copy link
Contributor

and conditionally import them

Based on what condition? Last I checked conditional import by platform was impossible. Was there a language change?

(Edit: The public docs still seem to indicate that this is impossible.)

Yeah, you're right. I had a vague memory that this was possible, but I was wrong. I think @sstrickl was working on it, but I haven't heard any updates in a while: dart-lang/sdk#50473. If this sort of conditional import is important for iOS/macOS interop, we should reprioritize that bug. It was P1 for a while.

@stuartmorgan
Copy link
Author

stuartmorgan commented Sep 25, 2024

If this sort of conditional import is important for iOS/macOS interop, we should reprioritize that bug. It was P1 for a while.

We should probably figure out if there's a better option before reprioritizing anything, because this:

If you only use a shared subset of the methods, that would just work. If you need to use methods that only exist on one platform then you'd need to also put that code behind a conditional import. But at least that would let you write the compatibility layer in Dart.

is still significantly worse than the native development experience.

@sstrickl
Copy link

Currently packages that are only used within guarded code blocks like, for Dart:

import 'dart:io';
import 'pkg:for_windows' as for_windows;

...
  if (Platform.isWindows) {
    ... direct or indirect use of for_windows package...
  }
...

or for Flutter,

import 'package:flutter/foundation.dart';
import 'pkg:for_windows' as for_windows;

...
  if (defaultTargetPlatform == TargetPlatform.windows) {
    ... direct or indirect use of for_windows package...
  }
...

should end up with that package tree-shaken out on non-Windows platforms, since Platform.isWindows and defaultTargetPlatform now evaluate to compile-time constants when the target OS is known (i.e., through the use of @pragma('vm:platform-const')/@pragma('vm:platform-const-if', ...)). The target OS should be implicitly provided by dart compile or flutter build when they are used to AOT compile a Dart program or a Flutter application, so there's no need to manually pass it for this to work.

For cases where there's an interface and separate implementation classes for each platform, something like this example from dart-lang/language#2133 (comment) should work nowadays with only the platform-specific implementation being kept in the final program:

import "dart:io";

class Client { }
class WindowsClient extends Client { }
class MacClient extends Client { } 
class AndroidClient extends Client { }
class IPhoneClient extends Client { }

Client getClient() => 
  Platform.isWindows ? WindowsClient() : 
  Platform.isMacOS ? MacClient() : 
  Platform.isAndroid ? AndroidClient() : 
  Platform.isIOS ? IPhoneClient() : 
  throw "Unrecognized platform";

So perhaps one of these approaches would be useful for what ffigen generates re: platform-specific code?

@stuartmorgan
Copy link
Author

I'm not seeing how either of those approaches would address case 2 at all; could you give an example?

@sstrickl
Copy link

Sadly, thinking about it, I don't think they do. Initial thoughts below.

It'd be nice if you could just, say, Platform.isWindows with conditional imports a la the conditional export example at dart-lang/language#2133 (comment), but that would only work with AOT-compiled code, as those "constants" wouldn't be constant in JIT mode.

In addition, the constant evaluation of vm:platform-const annotated fields and getters is done in during the global transformations in pkg/vm, which happens after the initial kernel loading and type checking in the current implementation. Even if you put the platform specific code into a separate file to be imported conditionally, say something like:

if (Platform.iOS) import 'wkwebview_ios.dart' as wkwebview;
if (Platform.macOS) import 'wkwebview_macos.dart' as wkwebview;    

and the front end allowed multiple conditional imports that bind the same name (wkwebview) above as long as there is one and only one import that is not tree-shaken away by the end of compilation, the two files being imported above would still need to export the same module-level interface for the front end to be able to typecheck uses of wkwebview.<member> appropriately, but the WkWebView case above would break that (since it extends a different class in each case, and those two classes don't have the same interface).


I guess the TL;DR is if you're not able to make a common interface that supports what's on both platforms, then I don't see a way of supporting that in our current implementation, since code that's tree-shaken is still type-checked, and so simply using platform-based guards in your code isn't enough if there are unrelated types involved in those guarded bits of code, even if they're consistent if you only consider a specific condition for the guards (e.g., compiling for iOS).

We'd need to sink the idea of the target platform much deeper into the CFE so it could determine which import is "active", so to speak, for a given compilation run, and then we'd also start running into the question of "how does/should this work with modular compliation", where now intermediate kernel files may be platform-dependent, not just the final tree-shaken kernel file used to create the snapshot. (Perhaps that is what we'd need to do to support this use case, or perhaps there's another way around it I haven't considered yet. I'll continue to think on this.)

@liamappelbe liamappelbe added this to the ffigen 18.0.0 milestone Oct 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Todo
Development

No branches or pull requests

4 participants