-
Notifications
You must be signed in to change notification settings - Fork 53
Updates, tweaks, and fixes for copy-on-write behavior. #302
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
base: master
Are you sure you want to change the base?
Conversation
- Depends on SDK 3.0.0. - Removes null-soundness checks. - Changes pedantic lints to official lints. Fixes new lint warnings. - `BuiltList` and `BuiltSet` have "efficient" `.toList()` and `.toSet()` which reuses the backing list/set in a `CopyOnWrite{List,Set}`, and only makes a copy if that list is written to. The lazy operations returning a view on the type should not be based on the shared view. Similar issue exists for maps, but is harder to fix.
if (_wrapper._copyBeforeWrite) { | ||
return _source.moveNext(); | ||
} | ||
throw ConcurrentModificationError(_wrapper); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is probably too drastic. It considers it a concurrent modification even if the elements never changed (like doing remove(notInSet)
).
That's avoidable by making the CopyOnWriteSet
recognize which operations are no-ops first, and not copy until a real change is performed.
(For remove, do if (!_set.contains(element)) return; _copy(); _set.remove(element);
, similarly or more complicated for all the other mutating operations.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Mostly looks good, one question.
built_collection
is in a bit of a weird place, there are places in the API which can clearly be better / more strongly typed, so I started making those changes
https://github.com/google/built_collection.dart/blob/master/CHANGELOG.md
but they take some work to land in google3, and it doesn't make sense to make a series of breaking releases for the open source world. So there's a pile of work to get through before there can be another release.
I still want to do it but I'm not sure when it can be high enough priority to get to :) ... for right now I need to get build_runner
under control, but maybe March / April might be possible ... if you're interested in helping bring built_collection
into the Dart 3 world, that'd be great :) ... it was written for Dart 1, so we're skipping a version ;)
Since there is already a breaking releases on the table it's interesting to think of other things we could do, I wrote a doc go/built-collection-6 ... back in 2022, apparently.
|
||
bool hasExactElementType(Type type) => E == type; | ||
bool hasEquivalentElementType<T>() => | ||
this is _BuiltList<T> && TypeHelper<T>() is TypeHelper<E>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Am I correct in thinking this loosens the "exact" requirement?
That was intentionally strict.
It seems like immutable collections should not have to care about writes, but unfortunately toBuilder
propagates the runtime type and not the static type to the builder, so in fact the problem is the same.
If we don't check the exact type you could ask for a BuiltList<num>.of(other)
, get back a BuiltList<int>
, and then list.rebuild((b) => b.add(2.0))
throws.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should check that the type is an equivalent type, meaning a mutual subtype, not just a subtype in one direction. You can use BuiltList<Object?>
and BuiltList<dynamic>
interchangably, but you can still not use a BuiltList<int>
as the implementation of a BuiltList<num>
.
With the existing behavior
BuiltList os = BuiltList<Object?>.of(["a", "list"]);
var copy = BuiltList.of(os);
would not return os
again as the copy
because the static type of os
is BuiltList<dynamic>
, the runtime type is BuiltList<Object?>
, and the inference makes the copy
initializer a BuildList<dynamic>.of(os)
which doesn't choose the shortcut because List<Object?> != List<dynamic>
.
Using an equivalence check on the type instead, you can reuse a BuiltList<Object?>
as a BuiltList<dynamic>
and vice versa.
(Is it overkill? Possibly. I just got startled by the
/// `E` must not be `dynamic`.
comment which made no sense to me, until I found the Type
-object ==
comparison. Going by my standard rule of "Never ever use Type
objects for anything" - because they're so woefully dumb that they can't solve any real problems properly, I wanted to change that.)
BuiltList
andBuiltSet
have "efficient".toList()
and.toSet()
which reuses the backing list/set in a
CopyOnWrite{List,Set}
,and only makes a copy if that list is written to.
The lazy operations returning a view on the type should not
be based on the shared view.
Similar issue exists for maps, but is harder to fix.