-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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 can claim to implement an interface even if the other has privates #25462
Comments
Yes, that's a known feature. So, general rule: Don't expose a public interface with private members, and assume that the instances of that type will have those members (well, not if you get those instances from someone outside the library, if they are all created inside the library, then you can assume it - and "this" will have them). I guess the analyzer could hint about such a problem if we can detect it safely enough to not cause too many false warnings. It's perfectly OK to have private members. It's just not OK to get an instance as a parameter to a public method and assume the instance knows about the private members. |
The problem is that this affects all classes ever. You're basically saying "don't use privates in publicly visible classes", which is untenable IMHO. |
(It would be different if you could distinguish "interfaces" from abstract base classes.) |
You can use private members in publicly visible classes, and we do it all the time. When a class is both used as an interface and a default implementation, that only works because the private members used by the implementation doesn't leak into the exported interface. If we removed the private members from the interface inside the declaring library, then a lot of existing code would start warning. Something like You can actually see that the interface contains the inaccessible members: library a;
import "b.dart";
class A {
int _foo = 42;
}
abstract class C extends B { // Abstract because otherwise it would need to implement _foo.
foo() { print(_foo); } // No warning because _foo is inherited from B;
} library b;
import "a.dart";
class B implements A {} // No warning, doesn't implement "_foo", but it's not accessible. |
Flutter has a number of tree structures that are designed to have an internally-defined abstract base class that you (as an author) subclass, and then the subclasses are instantiated and connected into a tree. The base class has private and public members; the public API uses these privates for all the logic the class hierarchy exposes. Obviously, given such a design, if you say your class "implements" the Flutter-defined abstract base class but doesn't actually "extends" it, then everything will break. Pretty much as soon as you hand in the class to the aforementioned public methods, they'll crash, much as if you'd handed in a "Timer" or "FileSystemEntity" or a "Vector3" instead of the actual expected type. The difference is that the analyzer will not report any problems, like it would if you did try to pass in a "Timer" or "Vector3". It will happily say that all the contracts are met. If the class only uses the privates for some rare cases, then you might not know that anything is wrong for quite some time. |
You could implement an private interface |
That would be a runtime check. What I would much rather have is an analyzer-level check. |
We've talked about supporting something like a NoImplicitInterface annotation on classes for DDC/strong mode that prevents other classes from "implementing" them (only extension allowed). You'd have to mark the class explicitly yourself though. cc @vsmenon |
We've talked about something similar for the analyzer code base, but actually want something more general. I want to be able to mark each public class to indicate which uses of that class we are committing to support (implement, extend, mix in, some combination, or none of the above) so that clients know what is and isn't safe to count on in terms of backward compatibility. |
Fixes flutter#120345. The main change here is that when a single-axis DragGestureRecognizer (say, a VerticalDragGestureRecognizer) sees the end of a drag, it now projects the drag's velocity onto its own single axis, and then clamps that. Previously it would clamp the velocity's magnitude as a 2D vector, and then consumers would project *that*, which resulted in clamping too hard when the motion wasn't precisely on-axis. For PanGestureRecognizer, the behavior is unchanged: we continue clamping the velocity as a 2D vector, which is the geometrically nice behavior when we're not trying to focus on a single axis. When I'd first looked at this code, I was concerned that fixing this issue might require a breaking change. But happily I believe it doesn't, for two reasons: * This base class DragGestureRecognizer is effectively sealed (even though Dart doesn't quite yet have that feature), because it has private abstract methods. Dart won't actually stop you extending it, but that seems like a bug in Dart (*); when the class's own method implementations go try to call those methods, they'll produce runtime errors. So it's not possible to subclass it in any useful way from outside the library, which means there are likely no such subclasses anywhere. That means we can freely alter the API between the base class and its subclasses. (*) In fact, it's in the Dart tracker: dart-lang/sdk#25462 * We're also changing the behavior of the `onEnd` callback: previously it would return a DragEndDetails with a two-dimensional velocity, even for a single-axis recognizer, and now it returns a velocity that's always zero in the cross axis. In principle that could be a breaking change; it's a bit odd for a VerticalDragGestureRecognizer to be telling you there was horizontal as well as vertical motion, but someone could nevertheless be depending on that. Fortunately, it turns out that the `onUpdate` callback's behavior already agrees with the view that that is an odd thing that shouldn't happen: it already returns an axis-locked velocity from single-axis recognizers. So the existing `onEnd` behavior is inconsistent with that; and whatever someone might try to do with cross-axis velocity from `onEnd`, it seems like it'd be hard for them to be doing it successfully already when `onUpdate` isn't going along. That makes me hopeful that nobody is depending on that behavior and we can freely clean it up. Also add tests, not only for the changes but for the existing behavior: it turns out that the fancy 2-D clamping, which we keep for PanGestureRecognizer because it's helpful there, wasn't tested at all.
File A:
File B:
This code has no analyzer warnings, despite being guaranteed to crash.
The text was updated successfully, but these errors were encountered: