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

[Class modifiers] Core library updates #50736

Closed
itsjustkevin opened this issue Dec 15, 2022 · 16 comments
Closed

[Class modifiers] Core library updates #50736

itsjustkevin opened this issue Dec 15, 2022 · 16 comments
Assignees
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. P2 A bug or feature request we're likely to work on
Milestone

Comments

@itsjustkevin
Copy link
Contributor

No description provided.

@leafpetersen
Copy link
Member

We should decide what (if any) changes we want to make in the core libraries to use these features. @lrhn do you want to drive that? I'm happy to be involved. cc @kallentu @munificent @natebosch @jakemac53 @stereotype441 @eernstg

@munificent munificent changed the title [NCM] Core library updates [Class modifiers] Core library updates Dec 17, 2022
@munificent
Copy link
Member

Yes, there's definitely some stuff we can do here. My hope is that the special case logic in the language spec around certain types being "sealed" can be replaced with uses of the new modifiers. Off the top of my head:

  • Mark num with sealed
  • Mark bool, double, int, Null, and String with final

@lrhn
Copy link
Member

lrhn commented Dec 20, 2022

The already unimplementable types are a given, and should be safe (and making num sealed will finally give us exhaustiveness of numbers, ... while also ensuring that we'll never make BigInt a num). Same with already restricted typed-data classes, which I'd make all final (don't care about exhausting typed-data list types).
We should ensure we don't rely only on the modifiers, if we plan to make SDK modifiers ignorable from legacy libraries.

I'd make Function final too. We left it implementable because we didn't want to make people remove their implements Function for Dart 2.0. Now we can language version it.
Same with Record, which should be unimplementable from the start, and not ignorable.

Could mark the FutureOr class, but it's really a place-holder, not the declaration that actually introdues the type, so it doesn't matter what it says. (If we had external typedef FutureOr<T>;, I'd use that to carry the documentation.)

I want to make MapEntry final, because I want to convert it to an inline class later. I think it can be done, it's already only implementable, and there are very few classes implementing it, and not for any good reason.
I can fix those. (Even if inline classes are released at the same time as class modifiers, I might have to go through final first, and only do the next step when we drop support for <3.0.0.)

I want to make StringConversionSinkMixin, ListMixin, MapMixin, and SetMixin deprecated aliases for StringConversionSinkBase, ListBase, MapBase, and SetBase, and make the latter four be mixin classes. (I want the same for IterableMixin and IteableBase, or just plain Iterable, but IterableBase has a const constructor that I can't combine with being a mixin.)

We should consider finalizing BigInt, since its implementations don't actually work with other implementations.
I don't think anyone implements it today.

Also consider our annotation classes Deprecated and pragma. I don't want tools to get confused about what a subclass of those mean. (But also, I'd love if subclassing them works, and tools treat them as the superclass that can just carry more information.)

A bunch of our implementation classes are effectively interface classes, because we have only private generative constructors.
Most of those are not intended as interfaces anyway, we just couldn't prevent it.
I'd go through those and see which we can reasonably make final. Maybe make the rest interface, but ... I don't like doing that, because it suggests an intent which wasn't here. Writing nothing and just having them be a class that you can't extend is less suggestive. Not great in the long run, though.

Another bunch of classes are interface classes. If we have blocked their default constructor, we can make them interface class. If we forgot, maybe this should be the time to make them interface class anyway, and rely on versioning to move people from extends to implements (if there is no implementation, it should be a safe migration).

Edit: We should consider making pure interfaces (which have no implementation) interfaces: Comparable, Sink, StringWriter. Maybe more.

Outside of the "core" platform libraries, I have less idea.
In dart:isolate, I think Capability could probably be either base or final. But SendPort implements Capability and can be implemented (I've done that, as a wrapper around another SendPort), so I'm not certain that locking down Capability will do anything.
Web is a foreign land.

@lrhn lrhn added the area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. label Dec 20, 2022
@lrhn
Copy link
Member

lrhn commented Dec 25, 2022

I also consider whether we should finalize RegExp and RegExpMatch.

The RegExpMatch class has been repeatedly modified by adding members when we support new RegExp features.
That, at least, should be made final.
But if we do that, it's basically impossible to implement RegExp, which returns RegExpMatches, and there is no public RegExpMatch constructor.

I'd be fine with making both final. It's an implementation class for the Pattern interface. If you want something else, implement Pattern, not RegExp.

@lrhn
Copy link
Member

lrhn commented Dec 27, 2022

The ChunkedConversionSink, StringConversionSink and ByteConversionSink classes are intended to be extended, and should be base classes to enforce that in the future.

It shouldn't be a problem to extend them instead of implementing, unless a class already has another superclass.
In that case, we can make them base mixin classes. The important part is that it allows us to add members to the class without breaking implements-subclasses.
(And combine StringConversionSinkBase and StringConversionSinkMixin into StringConversionSink.)

@lrhn
Copy link
Member

lrhn commented Jan 7, 2023

The LinkedListEntry class should be a base class (Edit: base mixin class, it's being used that way and it's reasonable). No other library can implement the private fields that the rest of the classes depend on. The LinkedList class might also be worth base-ing. It's an implementation class, not a type to mimick.
(Or we should deprecate the junk and get rid of it eventually. It's not a List anyway, and we have LinkedListQueue if you want a real linked list. The LinkedList class tries to be clever by letting users subclass the entry class, so they avoid having separate entry and value objects. That was not a good idea, and a horrible separation of concerns.)

Consider whether HashMap and LinkedHashMap should be base classes too. They're implementation classes, not intended as interfaces.

(We probably cannot make Object be a base class, even if nobody ever needs to implement Object.)

@mit-mit mit-mit added this to the Dart 3 beta 2 milestone Feb 8, 2023
@mit-mit
Copy link
Member

mit-mit commented Feb 23, 2023

The CL for this is https://dart-review.googlesource.com/c/sdk/+/284304

@vsmenon
Copy link
Member

vsmenon commented Feb 23, 2023

Does this need to be beta 2?

@vsmenon
Copy link
Member

vsmenon commented Feb 23, 2023

Tied to where we enforce mixin right now - @munificent

@vsmenon
Copy link
Member

vsmenon commented Apr 4, 2023

@lrhn @leafpetersen - What's the update here? Are we still trying get anything for the beta?

@leafpetersen
Copy link
Member

https://dart-review.git.corp.google.com/c/sdk/+/287600 just landed, hopefully it sticks. @lrhn @natebosch are there any other CLs we're still trying to land?

@lrhn
Copy link
Member

lrhn commented Apr 4, 2023

I believe that last CL for the "core" (backend-agnostic) platform libraries: core, async, collection, convert, math, typed_data and _internal, has landed.
Iff it sticks, those are good.

For the VM:

@brianquinlan is doing dart:io and dart:_http. (I saw an error that could be related, but is probably close or done).
@dcharkes Is doing dart:ffi. (Done?)
@bkonyi Did dart:developer.
@mkustermann Did dart:isolate.
Most likely, nobody thought about dart:mirrors.

I have no idea about the web libraries.

@natebosch
Copy link
Member

Most likely, nobody thought about dart:mirrors.

I would expect we could make every class final there - I don't think we expect any implementation or subclassing.

@dcharkes
Copy link
Contributor

dcharkes commented Apr 5, 2023

@dcharkes Is doing dart:ffi. (Done?)

Yep

Not. Blocked by Flutter not having been migrated yet.

@lrhn
Copy link
Member

lrhn commented Apr 5, 2023

I actually have one more CL. "Just one more thing."

@vsmenon vsmenon added the P2 A bug or feature request we're likely to work on label Apr 5, 2023
@leafpetersen
Copy link
Member

I'm going to close this out, anything further will be a cherry pick.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. P2 A bug or feature request we're likely to work on
Projects
None yet
Development

No branches or pull requests

8 participants