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.

Sign up for GitHub

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

Jump to bottom

[breaking change] Make dart:io FileSystemEvent sealed in Dart 3.1 #52027

Closed
brianquinlan opened this issue Apr 13, 2023 · 13 comments
Closed

[breaking change] Make dart:io FileSystemEvent sealed in Dart 3.1 #52027

brianquinlan opened this issue Apr 13, 2023 · 13 comments
Assignees
@brianquinlan
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. breaking-change-approved breaking-change-request This tracks requests for feedback on breaking changes library-io

Comments

@brianquinlan
Copy link
Contributor

brianquinlan commented Apr 13, 2023 •
edited by mit-mit

Change Intent

Make FileSystemEvent sealed starting in Dart 3.1.

Justification

It would be nice to be able to exhaustively pattern match on the events produced by FileSystemEntity.watch so FileSystemEvent should be sealed.

Impact

Code targeting Dart 2 will not be able to implements or extends FileSystemEvent (FileSystemEvent is final so Dart 3 code already cannot implement or extend it).

The only known package that implements a FileSystemEvent is package:watcher, which I will fix.

In Dart 3.0.0, the constructors for FileSystemEvent subclasses are public so the only obvious reason for implementing a FileSystemEvent is gone.

Mitigation

Code that implements or extends FileSystemEvent will need to be modified but the modification is straightforward i.e. use the new public constructors for those classes instead of creating your own subclasses.
@brianquinlan brianquinlan added the breaking-change-request This tracks requests for feedback on breaking changes label Apr 13, 2023
@kevmoo
Copy link
Member

kevmoo commented Apr 13, 2023

Is this really breaking? I thought sealed was only available in Dart 3?

@brianquinlan
Copy link
Contributor Author

brianquinlan commented Apr 13, 2023 •
edited

My reading of the version section of the class modifiers design is that sealed is not ignored even for pre-feature libraries.

@kevmoo
Copy link
Member

kevmoo commented Apr 13, 2023

Oh! This is also a cherry-pick, right?

I lean towards LGTM, but I'd like thoughts from @mit-mit

@brianquinlan brianquinlan changed the title [breaking change] Make dart:io FileSystemEvent sealed [breaking change] Make dart:io FileSystemEvent sealed in Dart 3.1 Apr 13, 2023
@brianquinlan
Copy link
Contributor Author

This isn't a cherry-pick...I'd like the change to be made in Dart 3.1

I was planning on making FileSystemEvent in 3 stages as described in #51912

@lrhn lrhn added the area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. label Apr 13, 2023
@itsjustkevin
Copy link
Contributor

@Hixie @grouma and @vsmenon to review this breaking change.

@Hixie
Copy link
Contributor

Hixie commented Apr 24, 2023

Fine by me (in particular because this doesn't seem to hurt anyone doing testing).

@brianquinlan
Copy link
Contributor Author

Ping for @grouma and @vsmenon

@vsmenon
Copy link
Member

vsmenon commented May 3, 2023

lgtm

@grouma
Copy link
Member

grouma commented May 4, 2023

LGTM

@modulovalue modulovalue mentioned this issue May 5, 2023
Closed
copybara-service bot pushed a commit that referenced this issue May 22, 2023
@brianquinlan
[io] Make FileSystemEvent sealed
c3e815b 
Bug: #52027, #51912
Change-Id: I154ffe5901e4248f48400e6c84568c9fe6dbcd83
CoreLibraryReviewExempt: aske on holiday
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/302452
Reviewed-by: Sigmund Cherem <sigmund@google.com>
Commit-Queue: Brian Quinlan <bquinlan@google.com>
Reviewed-by: Lasse Nielsen <lrn@google.com>
Reviewed-by: Siva Annamalai <asiva@google.com>
@elliette
Copy link
Contributor

elliette commented May 23, 2023 •
edited

@brianquinlan What is the status of the fix for package:watcher mentioned above? webdev serve is currently broken on the main branch of the Dart SDK with the following stack trace (see below).

I think we will need to update the version of package:watcher in build_runner to fix. FYI @jakemac53

CC @annagrin @nshahan

[INFO] Connecting to the build daemon...
[SEVERE] Failed to build build_runner:build_runner:
[SEVERE] ../../../.pub-cache/hosted/pub.dev/watcher-1.0.2/lib/src/constructable_file_system_event.dart:7:57: Error: The class 'FileSystemEvent' can't be extended, implemented, or mixed in outside of its library because it's a sealed class.
[SEVERE] abstract class _ConstructableFileSystemEvent implements FileSystemEvent {
[SEVERE]                                                         ^
[SEVERE] Failed to build build_runner:build_runner:
[SEVERE] ../../../.pub-cache/hosted/pub.dev/watcher-1.0.2/lib/src/constructable_file_system_event.dart:7:57: Error: The class 'FileSystemEvent' can't be extended, implemented, or mixed in outside of its library because it's a sealed class.
[SEVERE] abstract class _ConstructableFileSystemEvent implements FileSystemEvent {
[SEVERE]                                                         ^
Unhandled exception:
Bad state: Unable to start build daemon.
#0      _handleDaemonStartup.<anonymous closure> (package:build_daemon/client.dart:79:21)
#1      _runUserCode (dart:async/stream_pipe.dart:11:23)
#2      Stream.firstWhere.<anonymous closure> (dart:async/stream.dart:1698:9)
#3      _RootZone.runGuarded (dart:async/zone.dart:1582:10)
#4      _BufferingStreamSubscription._sendDone.sendDone (dart:async/stream_impl.dart:392:13)
#5      _BufferingStreamSubscription._sendDone (dart:async/stream_impl.dart:402:7)
#6      _BufferingStreamSubscription._close (dart:async/stream_impl.dart:291:7)
#7      _SyncBroadcastStreamController._sendDone.<anonymous closure> (dart:async/broadcast_stream_controller.dart:399:22)
#8      _BroadcastStreamController._forEachListener (dart:async/broadcast_stream_controller.dart:322:15)
#9      _SyncBroadcastStreamController._sendDone (dart:async/broadcast_stream_controller.dart:398:7)
#10     _BroadcastStreamController.close (dart:async/broadcast_stream_controller.dart:268:5)
#11     _AsBroadcastStreamController.close (dart:async/broadcast_stream_controller.dart:505:27)
#12     _RootZone.runGuarded (dart:async/zone.dart:1582:10)
#13     _BufferingStreamSubscription._sendDone.sendDone (dart:async/stream_impl.dart:392:13)
#14     _BufferingStreamSubscription._sendDone (dart:async/stream_impl.dart:402:7)
#15     _BufferingStreamSubscription._close (dart:async/stream_impl.dart:291:7)
#16     _SinkTransformerStreamSubscription._close (dart:async/stream_transformers.dart:87:11)
#17     _EventSinkWrapper.close (dart:async/stream_transformers.dart:21:11)
#18     _StringAdapterSink.close (dart:convert/string_conversion.dart:241:11)
#19     _LineSplitterSink.close (dart:convert/line_splitter.dart:141:11)
#20     _SinkTransformerStreamSubscription._handleDone (dart:async/stream_transformers.dart:132:24)
#21     _RootZone.runGuarded (dart:async/zone.dart:1582:10)
#22     _BufferingStreamSubscription._sendDone.sendDone (dart:async/stream_impl.dart:392:13)
#23     _BufferingStreamSubscription._sendDone (dart:async/stream_impl.dart:402:7)
#24     _BufferingStreamSubscription._close (dart:async/stream_impl.dart:291:7)
#25     _SinkTransformerStreamSubscription._close (dart:async/stream_transformers.dart:87:11)
#26     _EventSinkWrapper.close (dart:async/stream_transformers.dart:21:11)
#27     _StringAdapterSink.close (dart:convert/string_conversion.dart:241:11)
#28     _Utf8ConversionSink.close (dart:convert/string_conversion.dart:295:20)
#29     _ConverterStreamEventSink.close (dart:convert/chunked_conversion.dart:78:18)
#30     _SinkTransformerStreamSubscription._handleDone (dart:async/stream_transformers.dart:132:24)
#31     _RootZone.runGuarded (dart:async/zone.dart:1582:10)
#32     _BufferingStreamSubscription._sendDone.sendDone (dart:async/stream_impl.dart:392:13)
#33     _BufferingStreamSubscription._sendDone (dart:async/stream_impl.dart:402:7)
#34     _BufferingStreamSubscription._close (dart:async/stream_impl.dart:291:7)
#35     _SyncStreamControllerDispatch._sendDone (dart:async/stream_controller.dart:792:19)
#36     _StreamController._closeUnchecked (dart:async/stream_controller.dart:647:7)
#37     _StreamController.close (dart:async/stream_controller.dart:640:5)
#38     _Socket._onData (dart:io-patch/socket_patch.dart:2388:21)
#39     _RootZone.runUnaryGuarded (dart:async/zone.dart:1594:10)
#40     _BufferingStreamSubscription._sendData (dart:async/stream_impl.dart:339:11)
#41     _BufferingStreamSubscription._add (dart:async/stream_impl.dart:271:7)
#42     _SyncStreamControllerDispatch._sendData (dart:async/stream_controller.dart:784:19)
#43     _StreamController._add (dart:async/stream_controller.dart:658:7)
#44     _StreamController.add (dart:async/stream_controller.dart:606:5)
#45     new _RawSocket.<anonymous closure> (dart:io-patch/socket_patch.dart:1906:35)
#46     _NativeSocket.issueReadEvent.issue (dart:io-patch/socket_patch.dart:1349:18)
#47     _microtaskLoop (dart:async/schedule_microtask.dart:40:21)
#48     _startMicrotaskLoop (dart:async/schedule_microtask.dart:49:5)
#49     _runPendingImmediateCallback (dart:isolate-patch/isolate_patch.dart:123:13)
#50     _RawReceivePort._handleMessage (dart:isolate-patch/isolate_patch.dart:190:5)

@jakemac53
Copy link
Contributor

@elliette can you run a dart pub upgrade? Current constraints in build_runner do allow the latest watcher as far as I can tell.

@jakemac53
Copy link
Contributor

(this would still be breaking for anybody on an older version of watcher though)

@nshahan
Copy link
Contributor

nshahan commented May 24, 2023

It appears that there were two breaks that appeared at roughly the same time and we misdiagnosed this change as the cause for the other issue. Jake is right, running a pub upgrade appears to fix the issue with package:watcher and package:build_runner_core has been updated this morning so that problem will be fixed as well. Our tests in the SDK are now passing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@brianquinlan brianquinlan

Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. breaking-change-approved breaking-change-request This tracks requests for feedback on breaking changes library-io
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
10 participants
@kevmoo @Hixie @jakemac53 @grouma @vsmenon @nshahan @lrhn @brianquinlan @itsjustkevin @elliette