[vm/ffi] FfiNative return types treatment inconsistent with asFunction

[dcharkes]

@FfiNative<Handle Function()>('doesntmatter')
external String? doesntmatter();

https://dart-review.googlesource.com/c/sdk/+/252601/6/pkg/vm/testcases/transformations/ffi/ffinative.dart

asFunction and lookupFunction do not allow the return type to be sticter than Object when the native type is Handle.

The old natives do allow the return type to be stricter than Object.

It would be good to update FfiNatives to be consistent with asFunction, but that would make them inconsistent with old natives.

[mkustermann]

@dcharkes The code you are quoting is not executed but rather there for generating expectation files of what the transformer produces. So there's no issue with this code. It doesn't really matter whether its String? or Object?.

[dcharkes]

If we add static linking, this will be transformed to an immediate FFI call that does not do a type check on the result value, allowing non String?s to flow there. (I haven't checked if the current transform does add a type check, maybe it also doesn't.)

I've disallowed that behavior in the linked CL, so I had to update the transformer test to make it pass again.

[mkustermann]

Whether one uses static or dynamic linking shouldn't change any behavior.

Whether we trust the types written by the developer is another question entirely: If user writes Int8 we expect native function takes int8_t. If user writes String? we expect native code to return null | String. We could check explicitly via as-check or not - that's a choice. Irrespective of what choice we make, it will be true for dynamic (e.g. lookupFunction) or static binding (@FfiNative)

Coming back to the test itself: It just checks how the transform handles returning Handle and uses String? but could use any other type.

Maybe I misunderstand the concern here?

[dcharkes]

Whether one uses static or dynamic linking shouldn't change any behavior.

For asFunction and lookupFunction we opted to only allow Object as return value. For FfiNative we apparently opted to let the user specify a more narrow type. This is inconsistent. We should fix this inconsistency. Using asFunction or dynamic FfiNative linking shouldn't change any behavior.

Whether we trust the types written by the developer is another question entirely: If user writes Int8 we expect native function takes int8_t. If user writes String? we expect native code to return null | String. We could check explicitly via as-check or not - that's a choice. Irrespective of what choice we make, it will be true for dynamic (e.g. lookupFunction) or static binding (@FfiNative)

Agreed, it is a choice we make. I assume we made that choice consciously for asFunction and lookupFunction when adding Handles. For FfiNatives, we have a function dedicated specifically to this behavior, indicating deviating from asFunction intentionally, but I cannot find any reasoning for this on the CL. (And I didn't catch the differing behavior when reviewing it.)

I propose we stick to asFunction behavior. We can always allow developers to write stricter types later if need be. The other way around would be a breaking change later.

[mkustermann]

For asFunction and lookupFunction we opted to only allow Object as return value. For FfiNative we apparently opted to
let the user specify a more narrow type. This is inconsistent.

Thanks for the explanation - it's the inconsistency you're concerned about (though that's not related to the test that is quoted in this issue).

I'd like to avoid a situation where we do more checking for @FfiNative as we do for normal natives. So if the ladder doesn't check, I don't think we should check by-default (maybe in --enable-assertions mode).

[dcharkes]

it's the inconsistency you're concerned about

Yes, sorry for the confusion. Thanks for helping me clarify. 😄

though that's not related to the test that is quoted in this issue

By removing the inconsistency (by only allowing Object) the test started failing.

I'd like to avoid a situation where we do more checking for @FfiNative as we do for normal natives. So if the ladder doesn't check, I don't think we should check by-default (maybe in --enable-assertions mode).

By only allowing Object, we need no checking.
Of course a subsequent as check by users will introduce a check.

If we want to avoid those user-written as checks, we should change asFunction and lookupFunction to allow stricter return types instead.

[dcharkes]

This resurfaced in https://dart-review.googlesource.com/c/sdk/+/426121, because we tried to change the type of Handle in Dart to Object? and are trying to take nullability into account in the checking. (@chloestefantsova)

With the types being allowed to be in the opposite direction in @Natives, so we were checking the type in the opposite direction. We'll need to come up with a proper semantics. (I'm thinking that type checking the opposite direction is also not always the right thing to do.) Or just bite the bullet, make Handle type-safe and do a breaking change that forces the return types to be Object? for @Natives.

It would be a big breaking change as we're using it various places (for example dart:ui). However, we would make the world type-safe again.

[mkustermann]

Connecting on #49518 (comment),

I'd like to avoid a situation where we do more checking for @FfiNative as we do for normal natives. So if the ladder doesn't check, I don't think we should check by-default (maybe in --enable-assertions mode).

I don't know what our old natives used to allow, whether they ensured soundness or not. My original thought was that we don't want to regress old natives when switching to @Native.

We recently had a very similar discussion for JS interop declarations, which similar to FFI declarations, we kind of trust the types written. In FFI we trust so much that the program may crash if they are wrong. But there we settled for still maintaining sound dart typing.

So I think I'd opt for making us perform type checks on return values from C if the native return type is Handle.

[mkustermann]

With Chloe's upcoming CL, the rules will be relaxed and we allow any type for Handle. To maintain language semantics we should emit corresponding type checks (if there's evidence this is slowing things down - currently I doubt it - we could consider doing it only in assertions mode).

The places where we need to insert checks are

For Dart->C calls we have to check return values we get from C

• @Native() annotations
• DynamicLibrary.lookupFunction or the equivalent on Pointer

For C -> Dart calls we have to check argument values we get from C

• Pointer.fromFunction
• NativeCallable.listener
• NativeCallable.isolateLocal
• NativeCallable.*

I've tried to make a prototype, but run out of my time budget. @dcharkes Could you look into this?

[dcharkes]

the rules will be relaxed and we allow any type for Handle

We already did allow this the whole time without any type checks. That's why this issue was filed. Before @chloestefantsova's CL, all types were allowed due to Object? and Object being considered equal. Any subtype of those type was allowed. Landing her CL without the fix would start disallowing Object? or Object. So the CL is actually semantics preserving while removing the unsound null-safety type checks.

Could you look into this?

👍 I'll put it on my list.

[mkustermann]

@dcharkes The CL will allow this in more places where we used to issue a compile-time errors

[dcharkes]

Ah, the arguments in callbacks. 👍