Support "const" constructors for Build collections

[RobertSzymacha]

Having const constructors would make it possible to use it with named, optional parameters (with default values)

[davidmorgan]

Thanks.

With the current model of const, this is hard--there's no way to do any processing, so we can't calculate and store the hash code, for example. (Usually the hashcode is lazily calculated). We'd have to have an alternative implementation just for const.

Hopefully relaxing the rules around const (or around defaults) comes up as a language change at some point.

[elliottmb]

@davidmorgan Given the expansion of const in the last two releases of dart, is this potentially back on the table?

[davidmorgan]

I'm afraid not; the same problems remain, we'd have to provide multiple implementations.

[elliottmb]

What about at least the addition of an empty constructor that is const? I have a number of classes that currently null coalesce an optional field. They can't be const due to it being an empty BuiltList. I don't want to pass that burden onto the user by making it a required one they must pass an empty instance to.

[davidmorgan]

I think we could have a separate named const constructor, e.g. BuiltList.empty. But that's pretty verbose.

[elliottmb]

While verbose, it is not unheard of in the dart community. Stream.empty() for streams, Observable.empty() in rxdart, and a number of others.

Just tossing it out there, as I selfishly would benefit from this smaller case. Ideally having const constructors for all cases would be nice, but given the non-final _hashCode I see the dilema.

[davidmorgan]

Yes, I see that it's an awkward corner.

Personally I prefer not to use optional named parameters precisely because defaults don't work well. built_value is designed to fill that corner of the language--builders can correctly handle whether something is passed or not.

I'm still hopefully that the language will improve things here.

[kentcb]

Arrived here looking for exactly the same thing: const BuiltList<whatever>.empty(), or perhaps BuiltList<whatever>.empty as a static const field.

[knaeckeKami]

I currently have a file with

final emptyBuiltList = BuiltList.of(const <Null>[]);

final emptyBuiltSet = BuiltSet.of(const <Null>[]);

I need empty lists/sets in a couple of places (e.g. initialization, resetting the state).

I was annoyed by having to specify by constructing a new empty collection each then, and by specifying the type parameter for the empty list.

By using the Null type (which is a bottom type) , I can avoid creating unnecessary new objects, and specifying the type parameters for empty collections and probably gain a tiny bit of performance, too.

BuiltList<int> emptyInts = emptyBuiltList; compiles and runs fine.

Maybe it would be nice if this was included in built_collection, but I don't mind having a file with a couple of one-liners either.

[davidmorgan]

Thanks!

I think you'll run into trouble if you call toBuilder on those, though? Which might actually make them dangerous if you pass them to another method that does that.

[knaeckeKami]

Wow, you're right. Didn't run into this yet.
Don't use this code.

[davidmorgan]

I realized only recently that toBuilder should be toBuilder<T> where you specify which type you want to be able to add :/

[cbenhagen]

Added a separate issue for the special case of an empty const. I think there are many use cases where this might be helpful.

[davidmorgan]

I looked today at performance of Expando; it doesn't do too well:

• Significantly slower to access (which makes sense--direct field vs hash lookup)
• Worse for memory, too; the hashCode field is free in the VM today, the Expando uses additional memory

Neither is particularly significant if you're using large lists, but if you have lots of small lists it might matter. Would need to look at real world benchmarks to go further.

Two directions left to explore:

• Performance implications of additional implementation class
• Whether toBuilder could/should expand its generic type so we can use BuiltList<Null> always for the empty list

[davidmorgan]

Hmmmm if we move rebuild and toBuilder to an extension method, the builder created will always have the statically expected type, instead of the (maybe too narrow) actual type...

[davidmorgan]

The main downside with that seems to be that it will require explicit imports of built_collection.dart even when the type is only used implicitly :/

[ahmednfwela]

@davidmorgan We started facing this recently, is there a way I can contribute to this ?

[davidmorgan]

It would be a breaking change, so I'm afraid it won't happen any time soon: built_collection is widely used so breaking changes are very expensive.

I have just started on breaking changes for a new major release of built_collection--I plan to slowly accumulate fixes and breaking changes before making one single breaking change. So, this is a possible change that could happen, but I can't say for sure either way yet, sorry!

[ahmednfwela]

@davidmorgan I have added a non-breaking const implementation of BuiltList here #282

[davidmorgan]

Thanks for the PR, it's good to see an example of how this could work.

Unfortunately I don't see any fast path to landing the change.

There is a possible drawback to adding a const implementation: going from 1 to 2 possible implementations of an interface means that some optimizations are no longer possible, the compiler (VM, dart2js, etc) can no longer assume BuiltList is always backed by the same implementation.

So we'd need to check that carefully and compare to the other options, I think the main other one is moving the hash to an expando instead of a field.

[ahmednfwela]

while Expando will be enough for BuiltList and BuiltSet, it won't work with Map, since you will need to cache keys and values as well.

as for the performance implications we can write some benchmarks for it, but I have no idea which would be the best.

I am ready to explore multiple options and optimizations to make this land as quickly as possible.

[davidmorgan]

I don't think there is any way to land this quickly, it will need to be part of a major release that includes all the other breaking changes we can think of. Sorry about that.

I landed one breaking changes in the depot already but I think there are a lot more small breaking changes that will need to land before the next release.

[ahmednfwela]

Is adding a new factory considered a breaking change ?

[ahmednfwela]

I have created #283 as well to explore the expando option

[davidmorgan]

Thanks.

I think the expando approach is interesting as it means there is nothing wrapped but the SDK collection, opening the possibility that we use an inline class in future.

But, this is also not possible to land quickly. I expect it will be six months to a year before I'm ready to make a new major version release of built_collection.

Sorry about that--I know it's frustrating to not be able to change collection classes easily since they are so pervasive in a codebase. Unfortunately that also means they are very expensive to change and so inevitably progress is slow. You might want to fork your own collection types in the meantime--there is no particularly high cost to doing that since you can always use the SDK collection types to interoperate.

[ahmednfwela]

The main reason I wanted this change is because we are reworking the dart-dio openapi generator, which uses built_value for serialization, one main problem we had is that we couldn't provide default values to users at all.
But I guess it will have to wait for some time

[davidmorgan]

If you don't mind giving a bit more detail about your use case, please, I'd love to hear about it--and it will help when evaluating next steps. Thanks!

[plammens]

Is it really necessary to cache the hash code?

It seems to me we are measuring the possible performance implications of adding const constructors while keeping this feature by using an expando etc.; but instead we might need to measure whether the performance benefit of caching the hash code is really worth all this hassle.

[davidmorgan]

It's true that dropping the caching would not change the API, but it's still an important deviation from existing behavior, so I'd want to do that as a breaking change ... which means it won't happen any time soon.