Gentle start with removing explicit interfaces

Gentle start with removing explicit interfaces
Committed: https://code.google.com/p/dart/source/detail?r=11248

[hausner, 2012-08-21 18:08:58]

Added renaming of internal functions in object_store and class Type as discussed
offline with Ivan.

Regis, Ivan, please take a look whether the renaming makes sense from a
VM-internal viewpoint.

If ok, I will change the rest of the *Interface names as I change the dart
corelib code from interfaces to (abstract) classes.

[hausner, 2012-08-21 18:27:14]

Adding Mads and Kasper as FYI.

[sra1, 2012-08-21 18:31:39]

http://codereview.chromium.org/10832401/diff/5/corelib/src/function.dart
File corelib/src/function.dart (right):

http://codereview.chromium.org/10832401/diff/5/corelib/src/function.dart#newc...
corelib/src/function.dart:7: class Function {
This should be abstract, otherwise it enables this nonsense:

  new Function();

http://codereview.chromium.org/10832401/diff/5/corelib/src/hashable.dart
File corelib/src/hashable.dart (right):

http://codereview.chromium.org/10832401/diff/5/corelib/src/hashable.dart#newc...
corelib/src/hashable.dart:8: abstract int hashCode();
I believe this 'abstract' is optional.
It would certainly look less cluttered without it.

[regis, 2012-08-21 19:13:22]

LGTM with some concerns about readability.

http://codereview.chromium.org/10832401/diff/5/corelib/src/function.dart
File corelib/src/function.dart (right):

http://codereview.chromium.org/10832401/diff/5/corelib/src/function.dart#newc...
corelib/src/function.dart:7: class Function {
I agree with Stephen.

http://codereview.chromium.org/10832401/diff/5/runtime/vm/class_finalizer.cc
File runtime/vm/class_finalizer.cc (right):

http://codereview.chromium.org/10832401/diff/5/runtime/vm/class_finalizer.cc#...
runtime/vm/class_finalizer.cc:1347: interface.IsNumber() ||
Besides type testers, we also have class testers and handle tester:
IsObjectType(), IsObjectClass(), and IsObject(); IsDynamicType() and
IsDynamicClass() (no handle tester for Dynamic).

With your rename, I guess all flavors would have the same name. The code would
be harder to read, since you would need  to known if the receiver is a class, a
type, or a handle:
handle.IsString()  VM class String or one of its subclass?
cls.IsString()  cls == String?   
type.IsString()  dart String type?

[regis, 2012-08-21 19:36:19]

On 2012/08/21 19:13:22, regis wrote:
> LGTM with some concerns about readability.
> 
> http://codereview.chromium.org/10832401/diff/5/corelib/src/function.dart
> File corelib/src/function.dart (right):
> 
>
http://codereview.chromium.org/10832401/diff/5/corelib/src/function.dart#newc...
> corelib/src/function.dart:7: class Function {
> I agree with Stephen.
> 
> http://codereview.chromium.org/10832401/diff/5/runtime/vm/class_finalizer.cc
> File runtime/vm/class_finalizer.cc (right):
> 
>
http://codereview.chromium.org/10832401/diff/5/runtime/vm/class_finalizer.cc#...
> runtime/vm/class_finalizer.cc:1347: interface.IsNumber() ||
> Besides type testers, we also have class testers and handle tester:
> IsObjectType(), IsObjectClass(), and IsObject(); IsDynamicType() and
> IsDynamicClass() (no handle tester for Dynamic).
> 
> With your rename, I guess all flavors would have the same name. The code would
> be harder to read, since you would need  to known if the receiver is a class,
a
> type, or a handle:
> handle.IsString()  VM class String or one of its subclass?
> cls.IsString()  cls == String?   
> type.IsString()  dart String type?

Me again.
Just to clarify, I would prefer the more verbose rename:

type.IsXxxxInterface() -> type.IsXxxxType() instead of type.IsXxxx()
type.IsXxxxType() -> unchanged instead of type.IsXxxx()
handle.IsXxxx() -> unchanged.
class.IsXxxxClass() -> unchanged.

[siva, 2012-08-21 21:22:06]

http://codereview.chromium.org/10832401/diff/5/runtime/vm/class_finalizer.cc
File runtime/vm/class_finalizer.cc (right):

http://codereview.chromium.org/10832401/diff/5/runtime/vm/class_finalizer.cc#...
runtime/vm/class_finalizer.cc:1131: if (cls.is_interface()) {
Are we going to remove the notion of interface from the VM, since all classes
are interfaces now does it make sense to still have this distinction?

http://codereview.chromium.org/10832401/diff/5/runtime/vm/object.cc
File runtime/vm/object.cc (right):

http://codereview.chromium.org/10832401/diff/5/runtime/vm/object.cc#newcode762
runtime/vm/object.cc:762: name = Symbols::New("num");
why not make "num" also a predefined symbol in the vm isolate.

This is going to generate a new class id for num, don't you want to use
kNumberCid for this?

[hausner, 2012-08-23 00:25:06]

PTAL

- Renamed Type::IsXxxx to Type::IsXxxxType as Regis suggested.
- Made interface classes uninstantiable
- Fixed class finalizer so it recognizes illegal extends/implements of
  abstract classes from corelib.

http://codereview.chromium.org/10832401/diff/5/corelib/src/function.dart
File corelib/src/function.dart (right):

http://codereview.chromium.org/10832401/diff/5/corelib/src/function.dart#newc...
corelib/src/function.dart:7: class Function {
On 2012/08/21 18:31:39, sra1 wrote:
> This should be abstract, otherwise it enables this nonsense:
> 
>   new Function();
> 

abstract does not prevent allocation of an instance. For that, we need to add a
private constructor so a default constructor does not get added.

Added abstract and private constructor.

http://codereview.chromium.org/10832401/diff/5/corelib/src/hashable.dart
File corelib/src/hashable.dart (right):

http://codereview.chromium.org/10832401/diff/5/corelib/src/hashable.dart#newc...
corelib/src/hashable.dart:8: abstract int hashCode();
On 2012/08/21 18:31:39, sra1 wrote:
> I believe this 'abstract' is optional.
> It would certainly look less cluttered without it.

It is, but 

1) The VM does not implement this yet.
2) It's nicer to explicitly mark abstract functions.

http://codereview.chromium.org/10832401/diff/5/runtime/vm/class_finalizer.cc
File runtime/vm/class_finalizer.cc (right):

http://codereview.chromium.org/10832401/diff/5/runtime/vm/class_finalizer.cc#...
runtime/vm/class_finalizer.cc:1131: if (cls.is_interface()) {
On 2012/08/21 21:22:06, asiva wrote:
> Are we going to remove the notion of interface from the VM, since all classes
> are interfaces now does it make sense to still have this distinction?

Yes, this will go away, I would expect.

http://codereview.chromium.org/10832401/diff/5/runtime/vm/class_finalizer.cc#...
runtime/vm/class_finalizer.cc:1347: interface.IsNumber() ||
Agree. Changing to IsFunctionType() and IsNumberType() as you suggest.

On 2012/08/21 19:13:22, regis wrote:
> Besides type testers, we also have class testers and handle tester:
> IsObjectType(), IsObjectClass(), and IsObject(); IsDynamicType() and
> IsDynamicClass() (no handle tester for Dynamic).
> 
> With your rename, I guess all flavors would have the same name. The code would
> be harder to read, since you would need  to known if the receiver is a class,
a
> type, or a handle:
> handle.IsString()  VM class String or one of its subclass?
> cls.IsString()  cls == String?   
> type.IsString()  dart String type?

http://codereview.chromium.org/10832401/diff/5/runtime/vm/object.cc
File runtime/vm/object.cc (right):

http://codereview.chromium.org/10832401/diff/5/runtime/vm/object.cc#newcode762
runtime/vm/object.cc:762: name = Symbols::New("num");
On 2012/08/21 21:22:06, asiva wrote:
> why not make "num" also a predefined symbol in the vm isolate.
> 
> This is going to generate a new class id for num, don't you want to use
> kNumberCid for this?

Done.

[regis, 2012-08-23 03:53:10]

LGTM!

http://codereview.chromium.org/10832401/diff/14022/corelib/src/comparable.dart
File corelib/src/comparable.dart (right):

http://codereview.chromium.org/10832401/diff/14022/corelib/src/comparable.dar...
corelib/src/comparable.dart:8: factory Comparable._unistantiable() => null;
s/_unistantiable/_uninstantiable/

http://codereview.chromium.org/10832401/diff/14022/runtime/vm/object.cc
File runtime/vm/object.cc (right):

http://codereview.chromium.org/10832401/diff/14022/runtime/vm/object.cc#newco...
runtime/vm/object.cc:10872: return "Function";
This is only used for debugging, so I would be more specific: "DartFunction" or
"Function type class"

http://codereview.chromium.org/10832401/diff/14022/runtime/vm/object.h
File runtime/vm/object.h (right):

http://codereview.chromium.org/10832401/diff/14022/runtime/vm/object.h#newcod...
runtime/vm/object.h:877: // Check if this type represents the 'num' interface.
s/interface/type/

http://codereview.chromium.org/10832401/diff/14022/runtime/vm/object_store.h
File runtime/vm/object_store.h (right):

http://codereview.chromium.org/10832401/diff/14022/runtime/vm/object_store.h#...
runtime/vm/object_store.h:54: return function_type_;
fits on one line

http://codereview.chromium.org/10832401/diff/14022/runtime/vm/raw_object_snap...
File runtime/vm/raw_object_snapshot.cc (left):

http://codereview.chromium.org/10832401/diff/14022/runtime/vm/raw_object_snap...
runtime/vm/raw_object_snapshot.cc:56: 
Blank line removed by accident? Other functions have it.

http://codereview.chromium.org/10832401/diff/14022/runtime/vm/raw_object_snap...
runtime/vm/raw_object_snapshot.cc:188: 
ditto

http://codereview.chromium.org/10832401/diff/14022/runtime/vm/raw_object_snap...
File runtime/vm/raw_object_snapshot.cc (right):

http://codereview.chromium.org/10832401/diff/14022/runtime/vm/raw_object_snap...
runtime/vm/raw_object_snapshot.cc:1973: UNIMPLEMENTED();
Better: UNREACHABLE();  // DartFunction is an abstract class.

http://codereview.chromium.org/10832401/diff/14022/runtime/vm/raw_object_snap...
runtime/vm/raw_object_snapshot.cc:1981: UNIMPLEMENTED();
ditto

[Ivan Posva, 2012-08-23 06:13:44]

LGTM with comments.

-Ivan

http://codereview.chromium.org/10832401/diff/14022/corelib/src/comparable.dart
File corelib/src/comparable.dart (right):

http://codereview.chromium.org/10832401/diff/14022/corelib/src/comparable.dar...
corelib/src/comparable.dart:8: factory Comparable._unistantiable() => null;
Can you make these throw exceptions instead to ensure that not even library code
that has access to these constructors instantiates any objects?

http://codereview.chromium.org/10832401/diff/14022/corelib/src/hashable.dart
File corelib/src/hashable.dart (right):

http://codereview.chromium.org/10832401/diff/14022/corelib/src/hashable.dart#...
corelib/src/hashable.dart:8: factory Hashable._uninstantiable() => null;
ditto. Like in Function.

http://codereview.chromium.org/10832401/diff/14022/corelib/src/num.dart
File corelib/src/num.dart (right):

http://codereview.chromium.org/10832401/diff/14022/corelib/src/num.dart#newcode7
corelib/src/num.dart:7: abstract class num implements Comparable, Hashable {
This should be extends as far as I am concerned.

[sra1, 2012-08-23 18:18:18]

http://codereview.chromium.org/10832401/diff/5/corelib/src/function.dart
File corelib/src/function.dart (right):

http://codereview.chromium.org/10832401/diff/5/corelib/src/function.dart#newc...
corelib/src/function.dart:7: class Function {
On 2012/08/23 00:25:06, hausner wrote:
> On 2012/08/21 18:31:39, sra1 wrote:
> > This should be abstract, otherwise it enables this nonsense:
> > 
> >   new Function();
> > 
> 
> abstract does not prevent allocation of an instance. For that, we need to add
a
> private constructor so a default constructor does not get added.
> 
> Added abstract and private constructor.

Having to write constructors like this is ridiculous.
It is a lot of work.
And it adds very little value.

Making the class abstract does have one of the desired effects: dissuading

  new Function();

The spec says about abstract class: """
... If A is intended to be abstract, we want the static checker to warn
about any attempt to instantiate A, and we do not want the checker to complain
about unimplemented methods in A.
"""

And under "new":

"""It is a static warning if q is a constructor of an abstract class and q is
not a factory constructor."""



I had overlooked that we *may* also want to prevent 'class A extends Function',
and permit only
'class A implements Function'.

The library private constructor is ill-matched to this.  The constructor has the
wrong scope - being library private it enforces nothing within the same library.
 It also smacks of C++ - using some arcane implication of a constructor instead
of declaring intent.

There is almost no difference between

class B extends A {}
class B implements A {}

when A is 100% abstract (no implementations) and has no generative constructor.

So I would recommend removing all the private constructors at this point.
Let people write  'class Widget extends Hashable'.
The editor could have an optional warning that the 'extends' could be replaced
by 'implements' since Hashable has no non-abstract members.

http://codereview.chromium.org/10832401/diff/14022/corelib/src/num.dart
File corelib/src/num.dart (right):

http://codereview.chromium.org/10832401/diff/14022/corelib/src/num.dart#newcode7
corelib/src/num.dart:7: abstract class num implements Comparable, Hashable {
On 2012/08/23 06:13:44, Ivan Posva wrote:
> This should be extends as far as I am concerned.

You can only extend one class, but you can implement the implicit interface of
many classes.
There is no reason to choose Comparable over Hashable or vice-versa since
neither has any methods implementations to inherit.
So always 'implement' unless you really mean 'extend'.

[Ivan Posva, 2012-08-23 18:29:13]

http://codereview.chromium.org/10832401/diff/5/corelib/src/function.dart
File corelib/src/function.dart (right):

http://codereview.chromium.org/10832401/diff/5/corelib/src/function.dart#newc...
corelib/src/function.dart:7: class Function {
It seems like this is mostly a discussion you should have with Gilad. For some
of the very basic pure interface like classes we need to ensure that they cannot
be instantiated, so we are working with the tools that have proven useful.

On 2012/08/23 18:18:18, sra1 wrote:
> On 2012/08/23 00:25:06, hausner wrote:
> > On 2012/08/21 18:31:39, sra1 wrote:
> > > This should be abstract, otherwise it enables this nonsense:
> > > 
> > >   new Function();
> > > 
> > 
> > abstract does not prevent allocation of an instance. For that, we need to
add
> a
> > private constructor so a default constructor does not get added.
> > 
> > Added abstract and private constructor.
> 
> Having to write constructors like this is ridiculous.
> It is a lot of work.
> And it adds very little value.
> 
> Making the class abstract does have one of the desired effects: dissuading
> 
>   new Function();
> 
> The spec says about abstract class: """
> ... If A is intended to be abstract, we want the static checker to warn
> about any attempt to instantiate A, and we do not want the checker to complain
> about unimplemented methods in A.
> """
> 
> And under "new":
> 
> """It is a static warning if q is a constructor of an abstract class and q is
> not a factory constructor."""
> 
> 
> 
> I had overlooked that we *may* also want to prevent 'class A extends
Function',
> and permit only
> 'class A implements Function'.
> 
> The library private constructor is ill-matched to this.  The constructor has
the
> wrong scope - being library private it enforces nothing within the same
library.
>  It also smacks of C++ - using some arcane implication of a constructor
instead
> of declaring intent.
> 
> There is almost no difference between
> 
> class B extends A {}
> class B implements A {}
> 
> when A is 100% abstract (no implementations) and has no generative
constructor.
> 
> So I would recommend removing all the private constructors at this point.
> Let people write  'class Widget extends Hashable'.
> The editor could have an optional warning that the 'extends' could be replaced
> by 'implements' since Hashable has no non-abstract members.
> 
> 
>

[hausner, 2012-08-23 18:34:10]

I agree with Ivan that we should have the language discussion outside of this
change list. What I checked in reflects the language design as it stands now.

Thanks for the reviews.

http://codereview.chromium.org/10832401/diff/14022/corelib/src/comparable.dart
File corelib/src/comparable.dart (right):

http://codereview.chromium.org/10832401/diff/14022/corelib/src/comparable.dar...
corelib/src/comparable.dart:8: factory Comparable._unistantiable() => null;
On 2012/08/23 03:53:10, regis wrote:
> s/_unistantiable/_uninstantiable/

Done.

http://codereview.chromium.org/10832401/diff/14022/corelib/src/comparable.dar...
corelib/src/comparable.dart:8: factory Comparable._unistantiable() => null;
On 2012/08/23 06:13:44, Ivan Posva wrote:
> Can you make these throw exceptions instead to ensure that not even library
code
> that has access to these constructors instantiates any objects?

Done.

http://codereview.chromium.org/10832401/diff/14022/corelib/src/function.dart
File corelib/src/function.dart (right):

http://codereview.chromium.org/10832401/diff/14022/corelib/src/function.dart#...
corelib/src/function.dart:9: throw const UnsupportedOperationException(
returning null as I do in other cases would suffice too. I guess it's a matter
of style.

http://codereview.chromium.org/10832401/diff/14022/corelib/src/hashable.dart
File corelib/src/hashable.dart (right):

http://codereview.chromium.org/10832401/diff/14022/corelib/src/hashable.dart#...
corelib/src/hashable.dart:8: factory Hashable._uninstantiable() => null;
On 2012/08/23 06:13:44, Ivan Posva wrote:
> ditto. Like in Function.

Done.

http://codereview.chromium.org/10832401/diff/14022/corelib/src/num.dart
File corelib/src/num.dart (right):

http://codereview.chromium.org/10832401/diff/14022/corelib/src/num.dart#newcode7
corelib/src/num.dart:7: abstract class num implements Comparable, Hashable {
On 2012/08/23 06:13:44, Ivan Posva wrote:
> This should be extends as far as I am concerned.

It cannot be extends, because we can't extend more than one class. We lost the
ability to inherit methods from more than one type with the elimination of
explicit interfaces.

http://codereview.chromium.org/10832401/diff/14022/runtime/vm/object.cc
File runtime/vm/object.cc (right):

http://codereview.chromium.org/10832401/diff/14022/runtime/vm/object.cc#newco...
runtime/vm/object.cc:10872: return "Function";
On 2012/08/23 03:53:10, regis wrote:
> This is only used for debugging, so I would be more specific: "DartFunction"
or
> "Function type class" 

Done.

http://codereview.chromium.org/10832401/diff/14022/runtime/vm/object.h
File runtime/vm/object.h (right):

http://codereview.chromium.org/10832401/diff/14022/runtime/vm/object.h#newcod...
runtime/vm/object.h:877: // Check if this type represents the 'num' interface.
On 2012/08/23 03:53:10, regis wrote:
> s/interface/type/

Done.

http://codereview.chromium.org/10832401/diff/14022/runtime/vm/object_store.h
File runtime/vm/object_store.h (right):

http://codereview.chromium.org/10832401/diff/14022/runtime/vm/object_store.h#...
runtime/vm/object_store.h:54: return function_type_;
On 2012/08/23 03:53:10, regis wrote:
> fits on one line

Done.

http://codereview.chromium.org/10832401/diff/14022/runtime/vm/raw_object_snap...
File runtime/vm/raw_object_snapshot.cc (left):

http://codereview.chromium.org/10832401/diff/14022/runtime/vm/raw_object_snap...
runtime/vm/raw_object_snapshot.cc:56: 
On 2012/08/23 03:53:10, regis wrote:
> Blank line removed by accident? Other functions have it.
Blankness restored to former glory.

http://codereview.chromium.org/10832401/diff/14022/runtime/vm/raw_object_snap...
runtime/vm/raw_object_snapshot.cc:188: 
On 2012/08/23 03:53:10, regis wrote:
> ditto

Done.

http://codereview.chromium.org/10832401/diff/14022/runtime/vm/raw_object_snap...
File runtime/vm/raw_object_snapshot.cc (right):

http://codereview.chromium.org/10832401/diff/14022/runtime/vm/raw_object_snap...
runtime/vm/raw_object_snapshot.cc:1973: UNIMPLEMENTED();
On 2012/08/23 03:53:10, regis wrote:
> Better: UNREACHABLE();  // DartFunction is an abstract class.

Done.

http://codereview.chromium.org/10832401/diff/14022/runtime/vm/raw_object_snap...
runtime/vm/raw_object_snapshot.cc:1981: UNIMPLEMENTED();
On 2012/08/23 03:53:10, regis wrote:
> ditto

Done.

[bakster, 2012-08-24 06:32:50]

http://codereview.chromium.org/10832401/diff/21005/corelib/src/comparable.dart
File corelib/src/comparable.dart (right):

http://codereview.chromium.org/10832401/diff/21005/corelib/src/comparable.dar...
corelib/src/comparable.dart:11: }
I don't understand why this is needed. If the class is abstract a new operation
will throw an error.
That should be sufficient.

[hausner, 2012-08-24 15:25:45]

http://codereview.chromium.org/10832401/diff/21005/corelib/src/comparable.dart
File corelib/src/comparable.dart (right):

http://codereview.chromium.org/10832401/diff/21005/corelib/src/comparable.dar...
corelib/src/comparable.dart:11: }
I checked this with Gilad of course. He says that an abstract class can be
instantiated.

It seems there is some confusion about the semantics of abstract classes. It
might be worthwhile for you, Kasper and Gilad to confirm that you all have the
same idea.




On 2012/08/24 06:32:50, bak1 wrote:
> I don't understand why this is needed. If the class is abstract a new
operation
> will throw an error.
> That should be sufficient.