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 can claim to implement an interface even if the other has privates #25462

Open
Hixie opened this issue Jan 14, 2016 · 9 comments
Open

Class can claim to implement an interface even if the other has privates #25462

Hixie opened this issue Jan 14, 2016 · 9 comments
Labels
area-language Dart language related items (some items might be better tracked at github.com/dart-lang/language). type-enhancement A request for a change that isn't a bug

Comments

@Hixie
Copy link
Contributor

Hixie commented Jan 14, 2016

File A:

class A {
  void public() { }
  void _private() { }
}
void test (A x) { x._private(); }

File B:

import 'A';
class B implements A {
  void public() { }
}
void main() {
  test(new B());
}

This code has no analyzer warnings, despite being guaranteed to crash.

@lrhn lrhn added type-enhancement area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. labels Jan 14, 2016
@lrhn
Copy link
Member

lrhn commented Jan 14, 2016

Yes, that's a known feature.
Inside a library, the interface of a class declaration also contains the private members, outside of the library it only contains the visible members. The uses the wording "accessible" when talking about these members, and it's only a warning if a non-abstract class' interface contains an accessible member which isn't implemented by the class. If the member is not accessible, there is no warning.

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.

@Hixie
Copy link
Contributor Author

Hixie commented Jan 14, 2016

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.

@Hixie
Copy link
Contributor Author

Hixie commented Jan 14, 2016

(It would be different if you could distinguish "interfaces" from abstract base classes.)

@lrhn
Copy link
Member

lrhn commented Jan 14, 2016

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.
It does mean that you have to be careful what you assume. You can use the private members on this and on any instance you have created yourself, but if you get an instance from the outside, you should not assume that the private members are there - but the type system can't enforce that.

If we removed the private members from the interface inside the declaring library, then a lot of existing code would start warning. Something like Foo foo = new Foo(42); foo._initialized = true; would warn even if I am completely sure that it's the right kind of Foo.
So, it's a trade-off. If we had a way to express both the external interface and the internal interface, then we could annotate appropriately, but there is only one interface introduced per class - which, technically, includes all the private members, it just doesn't warn if you don't implement something you can't see.

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.

@Hixie
Copy link
Contributor Author

Hixie commented Jan 14, 2016

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.

@zoechi
Copy link
Contributor

zoechi commented Jan 14, 2016

You could implement an private interface abstract class _MyTreeItem{} and check for is _MyTreeItem instead, just as a workaround.

@Hixie
Copy link
Contributor Author

Hixie commented Jan 14, 2016

That would be a runtime check. What I would much rather have is an analyzer-level check.

@leafpetersen
Copy link
Member

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

@bwilkerson
Copy link
Member

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.

@kevmoo kevmoo added P2 A bug or feature request we're likely to work on type-enhancement A request for a change that isn't a bug and removed Priority-Medium labels Mar 1, 2016
@Hixie Hixie changed the title Class can claim to be implement an interface even if the other has privates Class can claim to implement an interface even if the other has privates Mar 12, 2016
@bwilkerson bwilkerson added area-language Dart language related items (some items might be better tracked at github.com/dart-lang/language). and removed P2 A bug or feature request we're likely to work on type-enhancement A request for a change that isn't a bug area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. labels Feb 14, 2018
@lrhn lrhn added the type-enhancement A request for a change that isn't a bug label Jun 25, 2018
gnprice added a commit to gnprice/flutter that referenced this issue Mar 9, 2023
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-language Dart language related items (some items might be better tracked at github.com/dart-lang/language). type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests

6 participants