Open HertzDevil opened 2 years ago
The Object.class
appears because of Crystal::MetaclassType#replace_type_parameters
:
class Crystal::MetaclassType
def replace_type_parameters(instance)
instance_type.replace_type_parameters(instance).metaclass
end
end
class Crystal::GenericModuleInstanceMetaclassType
def parents
instance_type.generic_type.metaclass.parents.try &.map do |parent|
parent.replace_type_parameters(instance_type)
end
end
end
The direct parents of B(Int32).class
are the results of substituting T = Int32
into B.class
's direct parents, which are just Class
. But Class
's instance type is currently Object
, so performing any type parameter substitution on Class
will produce Object.class
.
Metaclasses of generic class instances are immune because they must have a superclass (Reference
or Value
, neither of which is generic).
The assessment sounds good and the proposed solution seems convincing.
However, I fear this might be too big a breaking change in regards to how class methods on Object
are currently utilized.
As I understand it, Object.from_x
methods (which are used quite a lot) would not be directly affected. But use cases like Enum::ValueConverter(T).from_json
which expect to have Object.from_json
in the same scope would break. I'm not sure how much that would be used, but it feels too much of a breaking change.
I don't think we can do that before 2.0. If we can avoid it altogether, that would even be better.
I personally don't like the idea of making such methods instance methods of Class
. I guess it makes sense, but still, it doesn't look right to me.
Could it be an option to implicitly define class methods of Object
as instance methods of Class
? Looking from a user's perspective, I can't really see why we would need to differentiate the namespaces Object.class
and Class
. Not the types, just the namespaces where people define methods like Object.from_json
.
This is obviously more complex. But if we can avoid a breaking change with regards to the defintion of methods that are supposed to be available on all types, I think that should be worth it.
The method in question is actually also related to #5697, so if we succeed in debloating Object
's interface then there would be less friction here, and I think that is the way to go.
Another problem I just discovered is that generic module instances implicitly inherit Object.allocate
because of the current hierarchy:
module Foo(T)
end
puts Foo(Int32).allocate # can't execute `obj.to_s(self)` at /usr/lib/crystal/io.cr:174:5: `obj.to_s(self)` has no type
module Foo(T)
end
class Bar
include Foo(Int32)
@x = 1
end
puts Foo(Int32).allocate # => #<Bar:0x7f308c5a1ea0>
module Foo(T)
end
class Bar
include Foo(Int32)
end
class Baz
include Foo(Int32)
end
Foo(Int32).allocate # BUG: called llvm_struct_type for (Bar | Baz)
.allocate
is properly undefined for uninstantiated generic module metaclasses:
module Foo(T)
end
Foo.allocate # Error: undefined method 'allocate' for Foo(T):Module (modules cannot be instantiated)
Modules that don't include any other types have no ancestors at all, not even
Object
:It follows that
Class
should be the sole direct parent of those modules' metaclasses. Instead, the metaclasses of generic module instances containObject.class
in their hierarchy:This is unexpected, because
B(Int32)
still isn't a class. This hierarchy directly affects class method lookup. The standard library seems to rely on this in at least one place:https://github.com/crystal-lang/crystal/blob/b35c8c9ceac1d55033ffdf59a350793012172b5f/spec/std/json/serialization_spec.cr#L280-L283
Where
from_json
is defined as:A
String
is not aJSON::PullParser
, so the overload inEnum::ValueConverter.class
fails to match and the one inObject.class
is chosen. Now suppose that we specializeEnum::ValueConverter(JSONSpecEnum)
ourself. The following no longer compiles (as it shouldn't):We should remove
Object.class
from the ancestors of those generic module instance metaclasses. Even in that case, the same behaviour can be achieved with either of the following: