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

NNBD List constructor is confusing. Consider removing it entirely. #746

Closed
lrhn opened this issue Dec 16, 2019 · 14 comments
Closed

NNBD List constructor is confusing. Consider removing it entirely. #746

lrhn opened this issue Dec 16, 2019 · 14 comments
Labels
nnbd NNBD related issues request Requests to resolve a particular developer problem

Comments

@lrhn
Copy link
Member

lrhn commented Dec 16, 2019

tl;dr: Proposal

I propose that we:

  • Change the declared signature of the unnamed List constructor to List([int? count, T? value = null]), but statically treat it as List(int count, T value) in NNBD code, and as List([int count]) in non-NNBD code (like now).
  • Migrate all existing uses of List<T>() to <T>[].
  • Migrate all existing uses of List<T>(n) to List<T?>(n, null). (If the code is List<T>(x.length)..setAll(0, x) or similar, perhaps recommend to the user to rewrite using a literal with a spread).

Background

The NNBD behavior of the unnamed List constructor is that:

  1. List<T>() creates an empty growable list, and is redundant with the [] literal.
  2. List<T>(n) is invalid if T is potentially non-nullable (even if n is zero, because the compiler doesn't check the value, just whether there is syntactically an argument).
  3. List<T>(null) is still possible and a run-time error. It differs from List<T>().

See also #509.

The second item means that you can never create a List<T>(n) where T is a type variable.

This has lead to some amount of confusion during migration of existing code. Feedback is that it is confusing, and that, e.g., they expect List<T>(0) to work.

I recommend that we reconsider this design (which was never great) and either:

  • remove the constructor entirely, which will force everybody to move to literals and List.filled/List.empty.
  • Disallow the zero-argument version in NNBD and either:
    • add a second required parameter and otherwise make it work like List.filled. Then List<int>(5, 0) works. It's redundant with List.filled, but shorter and if we make the second argument required, it will prevent the errors from List.filled when the fill value is null. Or
    • just statically disallow its use with any type parameter which is potentially non-nullable. There's no good way to do so.

We'll have to include this in the migration tool, but I think it might be worth it. The List() is useless, the List(n) is dangerous and doesn't cover the uses that users actually care about.

The current behavior is inconsistent and confusing. There has always been special-casing around the List constructor, and we are already planning to special behavior for it in NNBD code. I think this change is a cleaner approach than just sticking to the current behavior.

@lrhn lrhn added request Requests to resolve a particular developer problem nnbd NNBD related issues labels Dec 16, 2019
@lrhn
Copy link
Member Author

lrhn commented Dec 17, 2019

With this proposal, in NNBD code, var x = List<int>(10, null); would be invalid. The type of the List<int> constructor in NNBD code would be treated as if it is List(int count, int value), and null is not assignable to int value.

The signature List([int count, T? value = null]) is indeed confusing, that's why no code sees it. We special case the static type in both NNBD and non-NNBD code, and the signature above is just the actual implementation which accepts both behaviors.

Only allowing nullable type parameters is, as you say, not supported by the language, and it has to work with type variables too. I'm also not just worrying about migration, but also about the API that we end up with. The List(count, fillValue) to create a fixed-length list is actually a good API.
A API where we accept a nullable fill value and then throw at run-time if it's not there, is worse.

@munificent
Copy link
Member

I think removing the constructor entirely is likely to be annoyingly painful for relatively little benefit. How about we just mark it deprecated and have the error message point people to list literals, List.empty(), and List.filled()?

(For what it's worth, "Effective Dart" has long pushed users towards list literals and a significant fraction of calls to List() that I've seen in code were, ahem, written by you personally @lrhn. :) I don't think it's that common in the wild outside of people that are new to Dart and bringing existing C#/Java idioms with them.)

@leafpetersen
Copy link
Member

3. List<T>(null) is still possible and a run-time error. It differs from List<T>().

Remind me why we made this choice? Why are we not migrating this constructor to List([int length = 0])?

@leafpetersen
Copy link
Member

  • just statically disallow its use with any type parameter which is potentially non-nullable. There's no good way to do so.

This is what is currently planned right? What do you mean by "There's no good way to do so"?

@leafpetersen
Copy link
Member

  • Migrate all existing uses of List<T>(n) to List<T?>(n, null).

This is probably a bad idea. It sounds like a recipe for runtime casts failures and/or static errors to me - most of the time you probably want to end up with a List<T>.

@leafpetersen
Copy link
Member

  • but statically treat it as List(int count, T value) in NNBD code

This feel odd to me. The existing functionality is unique, and useful (albeit only in relatively rare situations). This change make the functionality redundant with List.filled.

@ds84182
Copy link

ds84182 commented Dec 18, 2019

I think a secondary issue is what developers have used the unnamed List constructor for. In the past, I've used the unnamed constructor to preallocate the size of a growable array: List<String>(128)..length = 0;

This is because Dart's List lacks what other language's lists/vectors have: capacity. Capacity is an implementation detail. Capacity exists because there are times where we know better than the VM, we know our final list size, and we don't want to pay the cost of growing the list in the application's common case, just in the extreme case.

Post-NNBD, Dart has no way of maintaining this behavior without making the entire list nullable.

The lack of expressive List types have made juggling the behaviors of various types of lists hard. We have lists that act like lists, we have lists that act like arrays, we have lists that are immutable, etc. It may be too late for a Iterable -> ReadOnlyArray -> Array -> List split, but having to remove parts of dart:core or add static rules to warn against using parts of it shows that a split in list types would be valuable, and establishes developer intent.

@ds84182
Copy link

ds84182 commented Dec 18, 2019

In addition to that @tatumizer, typed arrays (which are Lists!) are initialized to 0 and do not allow null. They do not allow for growing, but can technically shrink with typed array views. These are 1:1 with Java's primitive arrays. You cannot resize them, you can (unlike Java) make sub-views.

Question for @lrhn: If you have a constructor like List(int count, [T value]), does value become required when the parameter is non-null? If not, it seems like it would be useful to have const default values now.

Implementation might be a bit weird, but ultimately having the ability to get the default zero value for a type parameter would be useful. It wouldn't even need to be exposed directly, just indirectly when you omit the default value for a parameterized type (so devs could write T defaultOf<T>([T value]) => value; as a tiny escape hatch to turn a type into some default instance of it, just like where Type typeOf<T>() => T is used). Then the list constructor could be defined as List(int count, [T value]), which would gracefully zero initialize an int list and would still null initialize an int? list.

When the given type is statically known to not have a default initializer, the parameter (and transitively all preceding parameters) becomes required. If the type is not statically known to have a default initializer, a runtime failure should occur (MissingDefaultValueError?).

To generalize default initialization, you could say something like "the default value for a union type is the first type with a default value, ignoring any preceding types without default values". Then you could declare that int? == Null | int, FutureOr<int> = int | Future<int>, and default initialization would work in both cases to provide null for int? and 0 for FutureOr<int>.

@lrhn
Copy link
Member Author

lrhn commented Dec 18, 2019

tl;dr: I think the currently specified NNBD behavior for the List constructor is painful in its inconsistency, and I'd prefer a version which does not need special-casing in the type system in the long run (when we remove non-NNBD mode).

My underlying motivation here is to end up with a good API in the long run, even if it costs a little more in the migration period.

And yes, we statically disallow creating a list with a potentially nullable type. When I say that there is "no good way to do that", I mean that the currently specified behavior isn't that good. It's not founded in the type system, and it's an exception only for this one constructor, and it's an exception we'll have to keep around forever.

The current special-casing of the List constructor is a minimal change, but it's not an a API I would have come up with if I started from scratch today.
The restriction that you cannot write List<T>(n) if T is potentially non-nullable is fairly intrusive and not supported by the type system. It means that you must always write a ? on the type when you use that constructor. If constructors had return types, we would have made it List<E?>. So it's a special case. It's not alone; length= has a similar issue, I have no solution for that, though.

What I would create from scratch is something like List(int count, E fillValue) . The fill value is required because in many cases it will be necessary. The value null is no longer a valid default value, and treating it as such for backwards compatibility will likely just extend the pain indefinitely instead of fixing it now.

It's is redundant with List.filled (except that that one can create growable lists). I'd likely be fine with removing the List constructor entirely, but if there is a need for a short way to create a fixed-length list, then List(len, 0) seems like it solves that. Another option is to introduce a literal syntax (probably not [: 6 * null :], no matter how neat it looks :)).

Question for @lrhn: If you have a constructor like List(int count, [T value]), does value become required when the parameter is non-null? If not, it seems like it would be useful to have const default values now.

No, it does not. The declaration is invalid because the optional parameter has a type which is potentially non-nullable and it has no default value.
It would be nice if that was possible, but ... it's not possible in practice. We tried it, and there was too many ways it just didn't work.

We have made some migrations where an optional parameter gets type T? instead of T, and it's a run-time error to pass null when T is non-nullable (fx List.fillRange). I wish I had a better solution for those, but the "treat the static type in one way in NNBD and another in non-NNBD mode" hack is a hack, and it only works for static functions, not interface signatures that someone else need to implement.

@lrhn
Copy link
Member Author

lrhn commented Dec 18, 2019

  1. List(null) is still possible and a run-time error. It differs from List().

Remind me why we made this choice? Why are we not migrating this constructor to List([int length = 0])?

Maybe we can. We still need to make List() act differently from List(0), and the current implementation relies on there being no default value.

@leafpetersen
Copy link
Member

Maybe we can. We still need to make List() act differently from List(0), and the current implementation relies on there being no default value.

I think the current implementations already use magic here, since List() works, but List(null) throws an error. So clearly they must be detecting whether a value was passed or not, which is not otherwise possible with an argument of type int.

@leafpetersen
Copy link
Member

It's is redundant with List.filled (except that that one can create growable lists). I'd likely be fine with removing the List constructor entirely,

I think I might be more supportive of this approach. We say:

  • It is an error to use the List() constructor in NNBD code
    • This has to be supported until Dart 3, at which point we can just
      • remove the specified special case error
      • remove the constructor entirely
      • and/or add it back in with whatever parameters we want.

This doesn't seem substantially more breaking to me (the migration tool should just rewrite the constructor to List.filled) and it avoids having to build in support for rewriting the API of core libraries in the front ends.

@lrhn
Copy link
Member Author

lrhn commented Dec 20, 2019

Deprecation plus disallow in NNBD mode should make the constructor eventually go away.
I'm fine with this. All use-cases can be covered by List.empty and List.filled, the only thing we lose is a short syntax for creating fixed-length lists. They are relatively rare. As Bob pointed out, I use them, mainly because I know they are cheaper for the VM, but I don't see much other use.

@leafpetersen
Copy link
Member

I have specified that it is an error to use this in Null safe code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
nnbd NNBD related issues request Requests to resolve a particular developer problem
Projects
None yet
Development

No branches or pull requests

4 participants