sdk
sdk copied to clipboard
dart-lang
[breaking change] Mark `IOOverrides` as `base`
Change Intent
IOOverrides should be marked as base to prevent implementation of the class.
Justification
This class is used to override behavior of certain APIs in dart:io. There's no reason for developers to implement this class and marking it as base will allow for additional overrides to be added to the class without requiring a breaking change (e.g., [ dart:io ] Added override for exit(...) in IOOverrides).
Impact
Code implementing IOOverrides will no longer compile.
Mitigation
PRs should be sent to remove cases where IOOverrides is implemented, where possible. There's only a handful of cases found on GitHub.
Change Timeline
ASAP.
Associated CLs
https://dart-review.googlesource.com/c/sdk/+/389520
Summary: The IOOverrides class is being marked as base to prevent developers from implementing it. This change will break code that currently implements IOOverrides, but there are only a few such cases found on GitHub.
What are the use cases for extending the class? Is it feasible to mark it as final instead?
There's no reason for developers to implement this class and marking it as
basewill allow for additional overrides to be added to the class without requiring a breaking change
I disagree with "no reason". For example, consider something like https://github.com/google/file.dart/pull/215. I intentionally used implements io.IOOverrides because I want to be broken if the interface changes. Without breakage, a method could be added to IOOverrides and then a class such as MemoryFileSystemIOOverrides would silently continue "working" even though it would be allowing reads/writes to the physical file system instead of to the MemoryFileSystem as intended.
PRs should be sent to remove cases where
IOOverridesis implemented, where possible. There's only a handful of cases found on GitHub.
Every single one of those cases is:
class FakeIOOveride extends Fake implements IOOverrides {}
- What should those sites use instead? Isn't
class FakeFoo extends Fake implements Foothe expected pattern? And a class can'textendmore than one class. Or is aFakeIOOverrideclass not something ever worth having? - Those implementations wouldn't even be broken if
IOOverridesis modified.
IMO changing IOOverrides to disallow implements is unnecessarily restrictive. It feels like a slightly weird tradeoff to definitely break code now to avoid potentially breaking code in the future. I realize that in general it's better to break things earlier so that there would be less stuff broken later, but IOOverrides doesn't really strike me as a class that is commonly derived from anyway.
I also feel like anyone using implements should be aware (or be made aware) that their code will break if the base class interface changes. Could the IOOverrides documentation instead just recommend that derived classes use extends instead of implements? Or document that the Dart Authors reserve the right to modify IOOverrides at will, and that no warranty, expressed or implied, is given to the stability of the IOOverrides interface, and anyone that implements IOOverrides hereby waives any right to complain if their code is broken?
What should those sites use instead?
An instance of IOOverrides.
The IOOverrides class is a value-class. Its behavior is entirely determined by its state, and its entire state can be set through the constructor.
If you need an IOOverrides object with a specific behavior, you can create one with that behavior. You never need a fake for a value object that is completely configurable.
This is the same reasoning I usually use to make classes final. That, or "has deep integration with something that you can't possibly mock anyway" like Zone.
Or is a
FakeIOOverrideclass not something ever worth having?
That.
A mock is not needed, so not being possible is not a problem. It's just a migration cost up-front, with the benefit of preventing someone doing an implements IOOverrides in production, rather than just for testing where we don't care as much when things break.
What should those sites use instead?
An instance of
IOOverrides. TheIOOverridesclass is a value-class. Its behavior is entirely determined by its state, and its entire state can be set through the constructor. If you need anIOOverridesobject with a specific behavior, you can create one with that behavior. You never need a fake for a value object that is completely configurable.
Isn't the intent class FakeIOOveride extends Fake implements IOOverrides {} to throw if something does any IO operations? If people manually configured an IOOverrides object, wouldn't it have the same problem I mentioned in https://github.com/dart-lang/sdk/issues/56468#issuecomment-2320201827 where things could get added to IOOverrides, those instances would not break, and IO operations could silently leak through?
I can't say what the intent of the FakeIOOveride (sic) is. I can say what it does today, and that that thing won't be broken by adding more overrides. (Actually, it seems the class is never used, so the fix is to remove it.)
If a new override is added to IOOverrides, then it's for something that you cannot override today.
That means that not throwing is preserving existing tested behavior. That's what we want.
If a new way to do IO is added, without an override, then code can start using that, and you can't stop them. If an override for the feature is later added, nothing changes other than you now being able to do something.
At that point, you will need to decide whether that new override-capable feature should be prevented too. Maybe it shouldn't, maybe it's for accessing the process environment, not "real IO", and using it is fine. Just starting to throw the moment an override becomes possible is a breaking change.
So yes, absolutely! Adding a new override option will not cause new throws, and that's exactly what is intended.
I also feel like anyone using
implementsshould be aware (or be made aware) that their code will break if the base class interface changes.
That's not how semantic versioning and breaking changes work. Anyone using implements assumes that they can keep doing so until the next major version increment, because breaking them is a breaking change that requires a major version increment.
What is the final conclusion on this breaking change ? Are we accepting it ?
@leonsenft @vsmenon @Hixie please take a look at this breaking change request.
Is @jamesderlin's concern not valid? I'm not sure I understand how we are addressing his comment above.
FWIW, in general I find having relying on zones to override APIs in a global manner in this way is bug prone and hard to debug. I would much rather dart:io use a mechanism like package:file's FileSystem, removing the global File constructor entirely. But I imagine that ship has long sailed.
Two things:
- There are a couple of instances in the Flutter tree (in the Flutter CLI) where IOOverrides is extended. Is there a migration guide explaining how those cases would be handled?
- Why do this only for IOOverrides? Why not also do it for HttpOverrides?
Two things:
- There are a couple of instances in the Flutter tree (in the Flutter CLI) where IOOverrides is extended. Is there a migration guide explaining how those cases would be handled?
This shouldn't impact anything. Marking it as base allows for it to be extended, and adding new members won't change behavior for projects that extend IOOverrides as they'll retain the default behavior they were already using.
- Why do this only for IOOverrides? Why not also do it for HttpOverrides?
I'd be happy to do that too :) I forgot that existed.
Reading the thread more carefully, I'm not sure that @jamesderlin's questions about their migration have been addressed by @lrhn's comments. In particular, the constructor of IOOverrides doesn't take any arguments, so I'm having trouble making sense of this comment: "The IOOverrides class is a value-class. Its behavior is entirely determined by its state, and its entire state can be set through the constructor."
I'm having trouble making sense of this comment: "The IOOverrides class is a value-class. Its behavior is entirely determined by its state, and its entire state can be set through the constructor."
You're right to be confused, because IOOverrides is an abstract class and isn't instantiable.
@lrhn's argument is that there really isn't a reason to be implementing IOOverrides vs extending it. The only benefit of implementing it would come in the form of being notified that we've added a new override when a compile time error shows up after an SDK upgrade. In the case of a mock implementing IOOverrides, this will result in code crashing if it's invoking a previously non-overridable function that's been added to IOOverrides.
Marking this class as base ensures that dart:io behavior remains unchanged between SDK releases where new overrides are added. It's a more stable configuration for users and removes the need for us to make a breaking change request for every new override we introduce.