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

[vm/ffi] Change Pointer to use sound variance? #38646

Open
dcharkes opened this issue Sep 30, 2019 · 5 comments
Open

[vm/ffi] Change Pointer to use sound variance? #38646

dcharkes opened this issue Sep 30, 2019 · 5 comments
Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. library-ffi

Comments

@dcharkes
Copy link
Contributor

dcharkes commented Sep 30, 2019

The normal Dart variance is unsound, which will be addressed in a future Dart version: dart-lang/language#524.

In dart:ffi we use this unsound covariance when loading from and storing into Pointer:

Pointer<Pointer<Int8>> p = Pointer.allocate();
Pointer<Pointer<NativeType>> p2 = p;
Pointer<Double> p3 = Pointer.allocate();
p2.store(p3); // Runtime exception.

We could consider changing the type parameter of Pointer to inout to prevent that:

class Pointer<inout T extends NativeType> {
  // ...
}

Pointer<Pointer<Int8>> p = Pointer.allocate();
Pointer<Pointer<NativeType>> p2 = p; // Compile-time error.

However, that would force dart:ffi users who do want to write generic code to use .cast(), which makes them lose runtime type information:

Pointer<Pointer<Int8>> p = Pointer.allocate();
Pointer<Pointer<NativeType>> p2 = p.cast(); // Explicit FFI casting.
Pointer<Double> p3 = Pointer.allocate();
Pointer<Pointer<Double>> p4 = p2.cast(); // Explicit FFI casting.
p4.store(p3); // May be not what developer intended.

Unsound covariance (current Dart semantics):

  • pro: flexible for writing generic code.
  • con: some errors are only caught at runtime.

Using inout to declare invariant:

  • pro: all errors in non generic code are caught at compile time.
  • con: generic code which stores and loads must opt out of static and dynamic checking by using cast.

Thoughts @sjindel-google @lrhn @eernstg @mkustermann @mraleph ?

If we want to go for invariant, we probably want to lock out generic use before we unmark dart:ffi as experimental.

@dcharkes dcharkes added area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. library-ffi labels Sep 30, 2019
@eernstg
Copy link
Member

eernstg commented Sep 30, 2019

The Pointer class isn't immediately able to make use of a variance annotation like inout on its type argument: The only methods where that type argument is used in the member signature are elementAt and offsetBy, and they just produce another Pointer<T> which would be similarly unable to depend directly on T.

So the magic must be in the @DartRepresentationOf("T") annotations on the parameters of load and store (and at least one more), but the actual type parameter of load and store defined by the method and completely independent of T. Moreover, "T" in the metadata is a constant string so it must be backed up by the ability to look up the actual value of T and, say, expect that R is int when T is Uint32, and R is double when T is Float, etc. (including the inductive cases like Pointer and Function which could presumably have any size).

Anyway, given that a Pointer<T> could be said to represent a mutable variable of type T (based on the fact that we have a load and a store method), it makes sense to make it invariant in T, no matter how much magic it takes to thread that T down to the actual reads and writes. ;-)

With that, Pointer<Pointer<T>> would automatically be invariant down to T, which is again a meaningful choice because that's in some sense a mutable variable of type Pointer<T>.

However, we could still use use-site variance to specify that we wish to allow a certain kind of variance:

Pointer<out Pointer<out T>> p;

Use-site variance is an extension of the model that I've described in dart-lang/language#557: It extends use-site invariance (where only exactly is available) to cover all three kinds of variance, and then it uses the keywords out/inout/in to denote them (because it would be crazy to have 6 different keywords for variance, and that overrides the desire to make use-site variance visibly different from declaration site variance).

This would allow for maintaining the information that we are working on a Pointer<S> where S is some subtype of Pointer<out T>. For instance, p could have a value whose dynamic type implements Pointer<Pointer<Int8>>.

I'm not sure whether that will precisely preserve the level of static safety that you have currently, but it seems likely to be at least very similar.

@dcharkes
Copy link
Contributor Author

dcharkes commented Sep 30, 2019

@eernstg with the move to extension methods we get rid of most of the @DartRepresentationOf("T") (only asFunction and fromFunction will keep it, not load and store).

For example:

extension PointerPointer<T extends NativeType> on Pointer<Pointer<T>> {
  /// Load a Dart value from this location.
  ///
  /// The value is automatically unmarshalled from its native representation.
  ///
  /// Note that [address] needs to be aligned the size of [Pointer].
  Pointer<T> get val => _loadPointer(this);

  /// Store a Dart value into this location.
  ///
  /// The [value] is automatically marshalled into its native representation.
  ///
  /// Note that [address] needs to be aligned the size of [Pointer].
  set val(Pointer<T> val) => _storePointer(this, val);

  /// Load a Dart value from this location offset by [index].
  ///
  /// The value is automatically unmarshalled from its native representation.
  ///
  /// Note that [address] needs to be aligned the size of [Pointer].
  Pointer<T> operator [](int index) => this.elementAt(index).val;

  /// Store a Dart value into this location offset by [index].
  ///
  /// The [value] is automatically marshalled into its native representation.
  ///
  /// Note that [address] needs to be aligned the size of [Pointer].
  operator []=(int index, Pointer<T> value) =>
      this.elementAt(index).val = value;
}

Source: https://dart-review.googlesource.com/c/sdk/+/118992/1/sdk/lib/ffi/ffi.dart#550

Does declaring the type parameter of Pointer as inout restrict these extension methods? Or would we have to put the variance on the extension (instead of the class)?

For use site variance, does use site variance always mean you do not specify definition variance (e.g. we keep Pointer unsound covaraint)? That would mean users of the Pointer API opt-in to more static checks on a per use site basis.

@eernstg
Copy link
Member

eernstg commented Sep 30, 2019

Does declaring the type parameter of Pointer as inout restrict these extension methods?

Yes: When Pointer has an invariant type argument and an expression has static type Pointer<T> then soundness implies that the value of that expression will always be an object whose type argument at Pointer is T (for instance, it's not a subtype of T).

So we know that the static and the dynamic type argument at the call site of the extension method are identical, so the T that the extension methods can access is guaranteed to be the T of the actual receiver.

The application of use-site variance that I mentioned was based on the assumption that Pointer would use the declaration-site variance that you mentioned. (If it doesn't do that then there's no need to widen the type, because it's already that wide when the type argument has no variance modifier).

So it actually means that the users of the Pointer API would be able to opt out of the more strict type checking at each use-site (well, at each location where they write something that contributes to the specification of the type of a receiver of the relevant calls).

Having opted out, actual instance methods would be filtered out when they use the type argument in a non-covariant manner (for a type like Pointer<out T>).

An example of this kind of filtering which is based on a well-known class would be that you can't invoke e.add(...) when the static type of e is List<out T> for any T. The method add is filtered out because its signature is unsound when the receiver has a type where T is covariant (this is exactly the same approach as that of Kotlin type projections and Java wildcards).

We haven't specified the details at this point, but extension methods could (and presumably should) be filtered in the same way. We have this notion of 'erasure' in the variance feature specification, and there would be a similar concept for the extended version (using out/inout/in for use-site variance, rather than exactly), and the result would be that get would return a Pointer<out T> when the receiver has type Pointer<out Pointer<out T>>; so you would be able to get val, but you couldn't set it.

@dcharkes
Copy link
Contributor Author

So it actually means that the users of the Pointer API would be able to opt out of the more strict type checking at each use-site (well, at each location where they write something that contributes to the specification of the type of a receiver of the relevant calls).

Having opted out, actual instance methods would be filtered out when they use the type argument in a non-covariant manner (for a type like Pointer<out T>).

Ah that clarifies things!

and the result would be that get would return a Pointer<out T> when the receiver has type Pointer<out Pointer<out T>>; so you would be able to get val, but you couldn't set it.

So if we change it to inout, then users can still specify out and read generically, and they can still specify in and store generically. That is pretty cool @eernstg!

That makes the pros/cons tradeoffs as follows.

Unsound covariance (current Dart semantics):

  • pro: flexible for writing generic code.
  • con: some errors are only caught at runtime.

Using inout to declare invariant:

  • pro: all errors in non generic code are caught at compile time.
  • con: generic code which stores and loads must opt out of static and dynamic checking by using cast.

@eernstg
Copy link
Member

eernstg commented Sep 30, 2019

Exactly!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. library-ffi
Projects
None yet
Development

No branches or pull requests

2 participants