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

Need to update package:dart_internal to support SDK version 2.5 #37324

Closed
grouma opened this issue Jun 20, 2019 · 10 comments
Closed

Need to update package:dart_internal to support SDK version 2.5 #37324

grouma opened this issue Jun 20, 2019 · 10 comments
Assignees
Labels
area-pkg Used for miscellaneous pkg/ packages not associated with specific area- teams. P1 A high priority bug; for example, a single project is unusable or has many test failures

Comments

@grouma
Copy link
Member

grouma commented Jun 20, 2019

The current Dart SDK version is 2.5.0-dev.0.0.

Ran into this error:
Because angular_components >=0.13.0 depends on observable ^0.22.2 which depends on dart_internal ^0.1.1, angular_components >=0.13.0 requires dart_internal ^0.1.1.
So, because dart_internal >=0.1.1 requires SDK version >=2.0.0-dev.12.0 <2.4.0 and webdev_example_app depends on angular_components ^0.13.0, version solving failed.

cc @nshahan

@grouma grouma changed the title Need to update package:dart_internal to support SDK version 2.4 Need to update package:dart_internal to support SDK version 2.5 Jun 20, 2019
@grouma grouma added P1 A high priority bug; for example, a single project is unusable or has many test failures area-pkg Used for miscellaneous pkg/ packages not associated with specific area- teams. labels Jun 20, 2019
@nshahan
Copy link
Contributor

nshahan commented Jun 20, 2019

@athomas @whesse

What can we do to ensure this is part of the release process before new 2.x releases (stable or dev) of the SDK get published on public channels?

This causes pub version solving failures for anyone with a transitive dependency on pkg:dart_internal every time we publish a new version.

@munificent
Copy link
Member

@aadilmaan, can we add this to the release checklist? We just need to make sure we publish a new version of dart_internal with an expanded SDK constraint each time we ship a new minor version of the SDK. I'm happy to be the owner of the task if you want.

dart-bot pushed a commit that referenced this issue Jun 21, 2019
Issue: #37324
Change-Id: Ibea63bb775c83484523fb8d6caeda918df25e849
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/106949
Reviewed-by: Bob Nystrom <rnystrom@google.com>
Commit-Queue: Nicholas Shahan <nshahan@google.com>
@aadilmaan
Copy link
Contributor

Checklist has been updated and you are tagged as owner =).

@nshahan
Copy link
Contributor

nshahan commented Jun 21, 2019

Thanks for updating the checklist! I published dart_internal: 0.1.5 with an expanded SDK constraint.

@nshahan nshahan closed this as completed Jun 21, 2019
@sortie
Copy link
Contributor

sortie commented Jun 24, 2019

Can we do better than simply adding another item to the release checklist? Complicating the release process with manual steps involving multiple people has a cos, increases the risk of making a mistake, and takes more time.

The documentation dart_internal suggests the package is temporary. What is the status on deprecating it?

If it doesn't seem that the package is going away anytime soon, we might want to do something to avoid breakage like this for subsequent stable releases:

  • Change the upper SDK constraint to <3.0.0. Although that's good for discouraging use, it is pulled in by package observable, which is indirectly used by a lot (through angular_components), so that means a lot of stuff is falsely not compatible with new stable releases. Is there any compelling reason for not supporting the latest SDK?
  • We could make our release script automatically publish an upgrade to the package? That would keep the release procedure simple.

What do you think?

@munificent
Copy link
Member

The documentation dart_internal suggests the package is temporary. What is the status on deprecating it?

There isn't currently any other alternative, and I don't think we'll get one until after NNBD and extension methods. (There was a chance that the design of extension methods would have covered this case and allowed us to remove dart_internal, but we went with a simpler, more static approach.)

Is there any compelling reason for not supporting the latest SDK?

This package talks to a special "dart:" library. We'd like to be able to deprecate and remove that library as soon as we have a better solution. If we set the constraint to "<3.0.0", then it means the SDK must continue to support that library through every single 2.x version. Otherwise, users would be broken if they used dart_internal with a version of the SDK that had removed that library.

Keeping the constraint tight lets us remove the "dart:" library in a 2.x release without breaking users.

@matanlurey
Copy link
Contributor

Looking internally, this is used basically twice, once in Streamy and once in Observable, right?:

CustomList toCustomList(List list) {
  return extractIterableTypeArguments(list, <T>() {
    return CustomList<T>(list);
  });
}

Basically, type information has been lost by upcasting, but they want to retain the original (runtime) type so they can treat CustomList<dynamic> as CustomList<FooEntity> where List<FooEntity> is expected?

How will this be expected to work if/when implicit downcasts are no longer a thing? I would consider starting a process where we find ways to move these two libraries (and their users) off this requirement.

@munificent
Copy link
Member

Looking internally, this is used basically twice, once in Streamy and once in Observable

I believe so.

Basically, type information has been lost by upcasting

I think it's actually been lost by serialization. We are deserializing a list ex nihilo and then need to be able to pass it to code that expects it to have the correct reified type. We added dart_internal specifically because I don't think there is any other way to plumb the expected type through the static type system. It's simply not there statically.

How will this be expected to work if/when implicit downcasts are no longer a thing?

I don't think that will affect it. There's still explicit downcasts.

I would consider starting a process where we find ways to move these two libraries (and their users) off this requirement.

We did spend a lot of time trying to find other solutions before settling on this. (That's why so few libraries use this hack. All of the others, we found other solutions. These were the sticking points.)

I don't think the capability this exposes is very common, but I do think it's a useful one. Dart is a reified language where type arguments hang around at runtime, so it makes sense for the language to give you a mechanism to access them and reuse them.

The specific way we expose it here is gross, but I hope we can add a better feature directly in the language to do it. If we're lucky, pattern matching will cover this. @eernstg has some ideas around there, I think.

@matanlurey
Copy link
Contributor

matanlurey commented Jun 25, 2019

I think it's actually been lost by serialization.

I am not so sure. Looking at the examples, we want to take a List<T> and make an ObservableList<T>, though we do not really know what the T is - somebody or some part of the system needs to know - right? I know its a huge hack, but couldn't Streamy just store this information in an Expando instead of needing dart_internal to do it?

library streamy.hack;

typedef ExtractUnaryTypeArgument = Object Function(Object Function<T> callWithType);
final _unaryTypeInformation = Expando<ExtractUnaryTypeArgument>();

// ... elsewhere in streamy ...
void _deserializeIntoList<T>(List<Object> data) {
  final List<T> list = _createList(data);
  _unaryTypeInformation[list] ??= (callWithType) => callWithType<T>();
  return list;
}

... and then basically have Streamy expose a extractXTypeArgs instead of this package/library. There are a lot of other RPC/serialization systems that have never run into this problem - I think this is more of a "library that was written with Dart 1.x in mind" than an issue specifically with Dart.

Dart is a reified language where type arguments hang around at runtime, so it makes sense for the language to give you a mechanism to access them and reuse them.

I agree in principle, but I think the fact that type information is being lost via upcasts shows IMO the bigger problem, they are trying to get a hold of the runtime type because they have no way to hold on to the static type.

@eernstg
Copy link
Member

eernstg commented Jun 27, 2019

@munificent wrote:

@eernstg has some ideas [about accessing actual type arguments]

Right. This issue is closed, but somehow ongoing, so here we go:

I proposed an overall framework for getting access to actual type arguments by pattern matching in dart-lang/language#170, aka 'type patterns'. That framework can be used to match types with other types in a compile-time setting, as well as extracting actual type arguments from an object at run time, it's just the core mechanism for extracting the type arguments from any given type.

It can be used in an 'existential open' as follows (based on the example that @matunlurey gave here):

CustomList toCustomList(List list) {
  list as List<var X>; // Open `list`.
  return CustomList<X>(list);
}

This snippet assumes that we can promote list to T using a plain list as T expression statement. In this case we use a type pattern var X for the type argument. That pattern is irrefutable (given that we already know list is a List the pattern match will succeed), so the as statement just works as a device that extracts the type argument of list at List and binds it to X, which we can then use as any other type.

This won't help us revealing the resulting type of CustomList in the return type, but given that any piece of code where there is a need to work with that type argument could also do as List<var X> it is in general easier to restore information about the actual type arguments of objects, and then use them in a statically typed manner (e.g., we can then do if (x is X) list.add(x) where the add is statically safe).

@matanlurey mentioned some tricks that we could try to play with an Expando, but
if we can touch the target class then we can actually do it in a pretty general way today:

class A<X> {
  callWithATypeArguments(Function<X>(A<X> self) f) =>
      f<X>(this);
}

class B<X, Y> extends A<X> {
  callWithBTypeArguments(Function<X, Y>(B<X, Y> self) f) =>
      f<X, Y>(this);
}

main() {
  A<num> a = B<int, String>();
  print(a.callWithATypeArguments(<X>(self) => List<X>()).runtimeType);
  if (a is B<num, Object>) {
    print(a.callWithBTypeArguments(<X, Y>(self) => List<Y>()).runtimeType);  
  }
}

The main difference between this code and the existential open (apart from the verbosity and the need to add a method to the target class) is that the existential open allows the static analysis to rely on the exactness of the type argument: In the first example we'd know that if (x is X) list.add(x) is statically safe (so we could call an entry point for add that does not check the type of its actual argument), but with the emulation using methods like callWithListTypeArguments we would only know that the actual type argument is some subtype of the actual value of X, so even with if (x is X) list.add(x) we would still have to have a dynamic type check on x when it's passed to add.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-pkg Used for miscellaneous pkg/ packages not associated with specific area- teams. P1 A high priority bug; for example, a single project is unusable or has many test failures
Projects
None yet
Development

No branches or pull requests

7 participants