Skip to content

Commit

Permalink
[Truffle] Fixed class variable lookup from singleton classes by attac…
Browse files Browse the repository at this point in the history 
…hing their companion class to them.
@nirvdrum
nirvdrum committed Mar 7, 2015
1 parent d9e8379 commit 0e29b6b
Showing 3 changed files with 31 additions and 5 deletions.
8 changes: 8 additions & 0 deletions truffle/src/main/java/org/jruby/truffle/runtime/ModuleOperations.java
View file
Original file line number Diff line number Diff line change
@@ -243,6 +243,14 @@ public static Object lookupClassVariable(RubyModule module, String name) {
}
}

if (module instanceof RubyClass) {
RubyClass klass = (RubyClass) module;

if (klass.isSingleton()) {
return lookupClassVariable(((RubyClass) module).getAttached(), name);
}

This comment has been minimized.

Sign in to view

Copy link
@eregon

eregon Mar 7, 2015

Member

Recursion should be used as last resort in general.
In particular, here MRI does not look in the hierarchy of the sclass, just in the sclass itself.

This comment has been minimized.

Sign in to view

Copy link
@eregon

eregon Mar 7, 2015

Member

Just after // Look in ancestors, module should be re-assigned to the non-singleton class if module is a sclass to behave like MRI. It could also be a new variable, but re-assigning prevent using the sclass we probably don't want after.

This comment has been minimized.

Sign in to view

Copy link
@nirvdrum

nirvdrum Mar 7, 2015

Author Contributor

Okay. I wasn't sure how far the chain went. That's easy enough to change. I also made it the last step so it would be skipped in the most common cases.

This comment has been minimized.

Sign in to view

Copy link
@eregon

eregon Mar 7, 2015

Member

We need and additional test since the sclass could just be the sclass of a normal object, in which case we do not do any special treatment and just look up in the ancestors of the sclass itself:

if (module instanceof RubyClass && ((RubyClass) module).isSingleton() && ((RubyClass) module).getAttached() instanceof RubyModule) {
    module = (RubyModule) ((RubyClass) module).getAttached();
}

// Nothing found

return null;
9 changes: 7 additions & 2 deletions truffle/src/main/java/org/jruby/truffle/runtime/core/RubyBasicObject.java
View file
Original file line number Diff line number Diff line change
@@ -104,8 +104,13 @@ public RubyClass getSingletonClass(Node currentNode) {

final RubyClass logicalClass = metaClass;

metaClass = RubyClass.createSingletonClassOfObject(getContext(), logicalClass,
String.format("#<Class:#<%s:0x%x>>", logicalClass.getName(), getObjectID()));
if (this instanceof RubyModule) {
metaClass = RubyClass.createSingletonClassOfObject(getContext(), logicalClass, (RubyModule) this,
String.format("#<Class:#<%s:0x%x>>", logicalClass.getName(), getObjectID()));
} else {
metaClass = RubyClass.createSingletonClassOfObject(getContext(), logicalClass, logicalClass,

This comment has been minimized.

Sign in to view

Copy link
@eregon

eregon Mar 7, 2015

Member

So this sets attached to the logicalClass? I would prefer null if we do not want to handle other cases than modules.

This comment has been minimized.

Sign in to view

Copy link
@nirvdrum

nirvdrum Mar 7, 2015

Author Contributor

I thought I stuck with the semantics of your gif. It looked like every sclass was attached to something. Did I get this wrong? I liked not having to do null checks if avoidable.

This comment has been minimized.

Sign in to view

Copy link
@eregon

eregon Mar 7, 2015

Member

attached is the sclass single and only instance. If we want it only for modules (seems we only need it for that now), then it should be null for normal objects. An object's logical class is not the only instance of its singleton class, that's the object itself. No need for null checks since no one is supposed to use attached on a sclass of sth else than a module (but an instanceof check as above). Otherwise we will break the code using that logical class as default when we introduce attached for normal objects.
String.format("#<Class:#<%s:0x%x>>", logicalClass.getName(), getObjectID()));
}

if (DebugOperations.verySlowIsFrozen(this)) {
DebugOperations.verySlowFreeze(metaClass);
19 changes: 16 additions & 3 deletions truffle/src/main/java/org/jruby/truffle/runtime/core/RubyClass.java
View file
Original file line number Diff line number Diff line change
@@ -33,6 +33,7 @@ public class RubyClass extends RubyModule {

private final boolean isSingleton;
private final Set<RubyClass> subClasses = Collections.newSetFromMap(new WeakHashMap<RubyClass, Boolean>());
@CompilationFinal private RubyModule attached;

This comment has been minimized.

Sign in to view

Copy link
@eregon

eregon Mar 7, 2015

Member

Why this cannot be final? (it is very useful to have true final fields for concurrency)

This comment has been minimized.

Sign in to view

Copy link
@nirvdrum

nirvdrum Mar 7, 2015

Author Contributor

It can. I originally didn't have this in the constructor and forgot to clean up afterwards.

/**
* This constructor supports initialization and solves boot-order problems and should not
@@ -48,10 +49,10 @@ public RubyClass(RubyContext context, RubyModule lexicalParent, RubyClass superc
ensureSingletonConsistency();
}

protected static RubyClass createSingletonClassOfObject(RubyContext context, RubyClass superclass, String name) {
protected static RubyClass createSingletonClassOfObject(RubyContext context, RubyClass superclass, RubyModule attached, String name) {
// We also need to create the singleton class of a singleton class for proper lookup and consistency.
// See rb_singleton_class() documentation in MRI.
return new RubyClass(context, superclass.getLogicalClass(), null, superclass, name, true).ensureSingletonConsistency();
return new RubyClass(context, superclass.getLogicalClass(), null, superclass, name, true, attached).ensureSingletonConsistency();
}

protected RubyClass(RubyContext context, RubyClass classClass, RubyModule lexicalParent, RubyClass superclass, String name, boolean isSingleton) {
@@ -64,6 +65,14 @@ protected RubyClass(RubyContext context, RubyClass classClass, RubyModule lexica
}
}

protected RubyClass(RubyContext context, RubyClass classClass, RubyModule lexicalParent, RubyClass superclass, String name, boolean isSingleton, RubyModule attached) {
this(context, classClass, lexicalParent, superclass, name, isSingleton);

assert isSingleton == true;

this.attached = attached;
}

public void initialize(RubyClass superclass) {
unsafeSetSuperclass(superclass);
ensureSingletonConsistency();
@@ -124,7 +133,7 @@ private RubyClass createOneSingletonClass() {
}

metaClass = new RubyClass(getContext(),
getLogicalClass(), null, singletonSuperclass, String.format("#<Class:%s>", getName()), true);
getLogicalClass(), null, singletonSuperclass, String.format("#<Class:%s>", getName()), true, this);

return metaClass;
}
@@ -137,6 +146,10 @@ public boolean isSingleton() {
return isSingleton;
}

public RubyModule getAttached() {
return attached;
}

public RubyClass getSuperClass() {
CompilerAsserts.neverPartOfCompilation();

8 comments on commit 0e29b6b

@nirvdrum
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eregon Please review this when you get some time. I'm not happy with all the instanceof checks. I think some of them can be converted to specialization guards, particularly in SingletonClassNode, but doing it this way allowed me to hit every case at once. If this is the wrong approach, please feel free to revert. But it appears to be working for class << self in both classes and modules.

@eregon
Copy link
Member

@eregon eregon commented on 0e29b6b Mar 7, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To avoid the instanceof checks we could just implement it for all objects like MRI does. So make attached an Object and always assign it when creating the singleton class.

About constructors, I am very much in favor of not creating new ones, but rather having a single one that does most of the job. It is much harder to follow the flow with many constructors. What we really need is constructors with a "name", which can achieved with factory methods on the class, and enforced by making the constructor private. Adding the extra parameter to all callers is also almost trivial with refactoring so keeping the old constructor has no real benefit. Keeping the assert would be good though in the form of assert !isSingleton || attached != null.

@eregon
Copy link
Member

@eregon eregon commented on 0e29b6b Mar 7, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems there is not yet a compelling use case to have attached for non-Module, so let's keep the instanceof checks (MRI uses it for detailed errors).

@nirvdrum
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can change the constructor stuff easily enough. I intentionally kept the commit a bit "incomplete" to illustrate the general idea. I didn't want to change a bunch of files just to find out this was the wrong approach. I probably should have just done this on a branch though... I'll go through and clean up later today.

@nirvdrum
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, thanks for the thorough review!

@eregon
Copy link
Member

@eregon eregon commented on 0e29b6b Mar 7, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, doing it in a branch or PR would be quite nice to be able to edit the commit :)

@nirvdrum
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eregon Yeah, it was late and I wasn't thinking. Want me to revert the commit and we can clean this up on a different branch?

@eregon
Copy link
Member

@eregon eregon commented on 0e29b6b Mar 7, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As you prefer, I can look up the resulting files as well. But I do find value in having commits doing just the right thing too, so feel free to do as you see fit.

Please sign in to comment.