Skip to content

Commit 0c3838a

Browse files
committed
Improve documentation for why we need to create a singleton class of a class eagerly
* And do some renames for clarity.
1 parent 18535ca commit 0c3838a

File tree

2 files changed

+22
-9
lines changed

2 files changed

+22
-9
lines changed

src/main/java/org/truffleruby/core/klass/ClassNodes.java

Lines changed: 18 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -67,9 +67,6 @@ public static RubyClass createBootClass(RubyLanguage language, RubyClass classCl
6767
@TruffleBoundary
6868
public static RubyClass createSingletonClassOfObject(RubyContext context, SourceSection sourceSection,
6969
RubyClass superclass, RubyDynamicObject attached) {
70-
// We also need to create the singleton class of a singleton class for proper lookup and consistency.
71-
// See rb_singleton_class() documentation in MRI.
72-
// Allocator is null here, we cannot create instances of singleton classes.
7370
assert attached != null;
7471
final RubyClass rubyClass = createRubyClass(
7572
context,
@@ -136,19 +133,35 @@ public static void initialize(RubyContext context, RubyClass rubyClass) {
136133
ensureItHasSingletonClassCreated(context, rubyClass);
137134
}
138135

136+
/** We also need to create the singleton class of any class exposed to the user for proper lookup and consistency.
137+
* This is not so intuitive, but basically it follows the rule of: "every Class object exposed to the user must have
138+
* a singleton class", i.e., only (singleton) classes not exposed to the user can have their own singleton class
139+
* created lazily. An example is `class K; def self.foo; end; end; sc = k.new.singleton_class`. `sc` there must have
140+
* its singleton class created, otherwise `Primitive.class_of(sc).ancestors` would be `[Class, Module, Object,
141+
* Kernel, BasicObject]` and `sc.foo` would not find method foo (which is defined on `#<Class:K>`). With `sc` having
142+
* the singleton class as soon as `sc` is exposed to the user, then it's fine, and
143+
* `Primitive.class_of(sc).ancestors` is `[#<Class:#<Class:#<K:0xc8>>>, #<Class:K>, #<Class:Object>,
144+
* #<Class:BasicObject>, Class, Module, Object, Kernel, BasicObject]`. See rb_singleton_class() documentation in
145+
* MRI. In theory, it might be possible to do this lazily in `MetaClassNode` but it's unlikely not a good
146+
* performance trade-off (add a check at every usage vs do it eagerly and no check). As an anecdote a single
147+
* ruby/spec fails when not calling this in {@link #createSingletonClassOfObject}, maybe there is an opportunity?
148+
* TODO (eregon, 23 March 2023): CRuby (3.1) seems to deal with this better, `RBASIC_CLASS(sc).ancestors` is
149+
* `[#<Class:K>, #<Class:Object>, #<Class:BasicObject>, Class, Module, Object, Kernel, BasicObject]` so it points to
150+
* the superclass (which is a singleton class) and then somehow knows this is not this class's singleton class
151+
* (maybe by comparing `attached`), so on `sc.singleton_class` it actually creates it. */
139152
private static RubyClass ensureItHasSingletonClassCreated(RubyContext context, RubyClass rubyClass) {
140153
getLazyCreatedSingletonClass(context, rubyClass);
141154
return rubyClass;
142155
}
143156

144157
@TruffleBoundary
145-
public static RubyClass getSingletonClass(RubyContext context, RubyClass rubyClass) {
158+
public static RubyClass getSingletonClassOfClass(RubyContext context, RubyClass rubyClass) {
146159
// We also need to create the singleton class of a singleton class for proper lookup and consistency.
147160
// See rb_singleton_class() documentation in MRI.
148161
return ensureItHasSingletonClassCreated(context, getLazyCreatedSingletonClass(context, rubyClass));
149162
}
150163

151-
public static RubyClass getSingletonClassOrNull(RubyContext context, RubyClass rubyClass) {
164+
public static RubyClass getSingletonClassOfClassOrNull(RubyContext context, RubyClass rubyClass) {
152165
RubyClass metaClass = rubyClass.getMetaClass();
153166
if (metaClass.isSingleton) {
154167
return ensureItHasSingletonClassCreated(context, metaClass);

src/main/java/org/truffleruby/language/objects/SingletonClassNode.java

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -82,13 +82,13 @@ protected RubyClass singletonClassImmutableObject(ImmutableRubyObject value) {
8282
limit = "getIdentityCacheContextLimit()")
8383
protected RubyClass singletonClassClassCached(RubyClass rubyClass,
8484
@Cached("rubyClass") RubyClass cachedClass,
85-
@Cached("getSingletonClassOrNull(getContext(), cachedClass)") RubyClass cachedSingletonClass) {
85+
@Cached("getSingletonClassOfClassOrNull(getContext(), cachedClass)") RubyClass cachedSingletonClass) {
8686
return cachedSingletonClass;
8787
}
8888

8989
@Specialization(replaces = "singletonClassClassCached")
9090
protected RubyClass singletonClassClassUncached(RubyClass rubyClass) {
91-
return ClassNodes.getSingletonClass(getContext(), rubyClass);
91+
return ClassNodes.getSingletonClassOfClass(getContext(), rubyClass);
9292
}
9393

9494
@Specialization(
@@ -110,8 +110,8 @@ private RubyClass noSingletonClass() {
110110
throw new RaiseException(getContext(), coreExceptions().typeErrorCantDefineSingleton(this));
111111
}
112112

113-
protected RubyClass getSingletonClassOrNull(RubyContext context, RubyClass rubyClass) {
114-
return ClassNodes.getSingletonClassOrNull(context, rubyClass);
113+
protected RubyClass getSingletonClassOfClassOrNull(RubyContext context, RubyClass rubyClass) {
114+
return ClassNodes.getSingletonClassOfClassOrNull(context, rubyClass);
115115
}
116116

117117
@TruffleBoundary

0 commit comments

Comments
 (0)