ceylon / ceylon-spec

DEPRECATED
Apache License 2.0
108 stars 34 forks source link

disallow qualifying constructors with *instances* #1366

Closed jvasileff closed 9 years ago

jvasileff commented 9 years ago

This is currently allowed:

myPoint.withCartesian(1.0, 2.0);

where withCartesian is a constructor. I suggest that this be disallowed, requiring:

Point.withCartesian(1.0, 2.0);

for two reasons:

  1. Constructors are not polymorphic
  2. The first can be confusing, since it implies that the instance matters, as it does with immutable builder methods that look similar.
lucaswerkmeister commented 9 years ago

:+1:

sgalles commented 9 years ago

Yes indeed, and BTW, it also makes the current behavior regarding name collision somewhat suprising. A constructor can hide a member of its class This compiles.


interface Bar{
    shared Integer foo() => nothing;
}

class Foo satisfies Bar{
    shared new (){}
    shared new foo(){}
}

shared void run(){
    Foo a = Foo().foo();
}

and also this doesn't compile, but it probably should

class Foo{
    shared Integer foo() => nothing;
    shared new foo(){}
}
sgalles commented 9 years ago

Ah, an interesting variation of the same problem. This should not compile but curently is accepted.

class Foo {
    shared new (){}
    shared new foo{}
}

class Bar() extends Foo(){
}

shared void run(){
    print(Bar.foo.hash); // should be rejected   by the typechecker
}
quintesse commented 9 years ago

The fix makes the following test fail: ceylon/ceylon-compiler#2234

Specifically this line:

bug2234_check(oi.foo.name=="foo", "spec#1129.3");

But I guess this isn't allowed anymore then?

quintesse commented 9 years ago

I commented out some tests in the language module related to this so I can continue testing, please re-enable or change them when it's apparent what has to be done: ceylon/ceylon.language@99801f65f9f2512b2d9adbbd3a4d7fa74cd39120

gavinking commented 9 years ago

@quintesse that change looks right to me.

quintesse commented 9 years ago

@gavinking I'm afraid I don't know what it is you are referring to. What change looks right?

gavinking commented 9 years ago

@quintesse your commit to the language module tests

quintesse commented 9 years ago

Well I'm just commenting out some tests that don't work anymore, that can't be right can it? :) Or do you mean they should just be removed? But I guess the person that added them wanted to test something

gavinking commented 9 years ago

Well the whole point of this issue is to disallow something we used to allow!

gavinking commented 9 years ago

I'm closing this issue and opening #1431 for the separate problem noticed by @sgalles.