Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Class variable in subclass is not overtaken by class variable in superclass #1554

Closed
DavidEGrayson opened this Issue Mar 9, 2014 · 9 comments

Comments

Projects
None yet
4 participants
@DavidEGrayson
Copy link
Contributor

DavidEGrayson commented Mar 9, 2014

I was experimenting with class variables and I noticed that JRuby's class variables behave differently than MRI's. If a class variable is defined in a superclass after one of its subclasses already has a class variable of the same name, MRI detects the situation and merges the two variables into one. JRuby does not detect it, and it lets the class variable have a different value in the subclass and parent class.

Here is a simple script to demonstrate the difference:

class A
end

class B < A
  @@cv = :cvb  
  def cvb
    @@cv
  end
end

class A
  @@cv = :cva
end

p B.new.cvb

Here is shell output showing how JRuby prints :cvb:

$ jruby -v test.rb
jruby 1.7.11 (1.9.3p392) 2014-02-24 86339bb on Java HotSpot(TM) 64-Bit Server VM 1.7.0_45-b18 [Windows 8-amd64]
:cvb

Here is shell output showing how MRI prints a warning and then prints :cva:

$ ruby -v test.rb
ruby 2.0.0p0 (2013-02-24) [x64-mingw32]
test.rb:7: warning: class variable @@cv of B is overtaken by A
:cva

(Sorry if this is a duplicate; I searched through the names of all open issues but not closed ones.)

@headius

This comment has been minimized.

Copy link
Member

headius commented Mar 9, 2014

Wow...this is completely new to me. We should try to track down the thread where this became standard because I have never seen this before.

In any case...bug in JRuby.

@DavidEGrayson

This comment has been minimized.

Copy link
Contributor Author

DavidEGrayson commented Jan 22, 2015

I tested this again today: MRI 2.2.0p0 prints :cva while JRuby 9.0.0.0.pre1 prints :cvb, so the bug is still present and nothing has changed.

@DavidEGrayson

This comment has been minimized.

Copy link
Contributor Author

DavidEGrayson commented Jul 24, 2015

I just tested it again: this bug is still present in JRuby 9.0.0.0.

@kares

This comment has been minimized.

Copy link
Member

kares commented Jan 17, 2017

still an issue on latest release (9.1.7.0)

@kares kares added this to the JRuby 9.1.8.0 milestone Jan 17, 2017

@headius

This comment has been minimized.

Copy link
Member

headius commented Mar 3, 2017

Class variables are so weird. Not going to make 9.1.8.0 but should be fixed for 9.2.

@headius headius modified the milestones: JRuby 9.2.0.0, JRuby 9.1.8.0 Mar 3, 2017

@headius

This comment has been minimized.

Copy link
Member

headius commented May 16, 2018

Ok...this is really unintuitive. I'm not sure how MRI represents class variables that would cause a later set in A to show up for instances of B.

@headius

This comment has been minimized.

Copy link
Member

headius commented May 16, 2018

After looking at the MRI logic, I do not believe this is a good behavior. However I'm on the fence about fixing it.

The logic in MRI appears to work as follows:

  • Search the target class for the class variable.
  • Search the target class's ancestors for the class variable, apparently every time unconditionally.
  • If the class variable is found in both the target class and one of its ancestors, warn (verbose) of the overtaking and delete the variable from the target class. Use the one from the parent class.

As a result, reading a class variable from a class could cause that class's value for that variable to be deleted if some other class later wrote the class variable at a higher level. The logic for this basically is backwards: the higher class always takes priority over the lower class. MRI deletes on get because it has no ability to check child classes for the same class variable.

In order to support this, we'd need to be doing the hierarchy search on every get and set (gross). We might be able to improve this if we walk down-hierarchy on set to clear out values in descendants, since then all sets would go to the highest already-set level, and gets would just walk up and find that level.

@headius

This comment has been minimized.

Copy link
Member

headius commented May 16, 2018

Additional wrinkles:

  • If a class variable is later set in a module, any class hierarchies below that module with the same class var should reflect the new value from the module. I'm not sure what happens if a module gets included and has a variable that appears higher up, but I assume it gets preempted by the higher-up. I'm also not sure whether the class var goes on the module or on the included module wrapper.
  • I'm not sure how prepend affects this.

Basically the logic of having class variables override each other from above requires some serious gymnastics in the type hierarchy, while simultaneously making class variables significantly slower (since you have to search hierarchy every time no matter what).

What a stupid feature.

headius added a commit to headius/jruby that referenced this issue May 16, 2018

Always search full hierarchy for highest definition of a cvar.
This makes the feature basically useless for anything requiring
performance, since it always has to search the entire hierarchy
for a late-assigned value further up.

Additional work using our weak subclass hierarchy might be able
to make reads slightly cheaper (giving up at first successfuly
read) by walking down the hierarchy on every write and killing
other vars, but the concurrency aspects of that make my head spin.

Fixes jruby#1554.
@headius

This comment has been minimized.

Copy link
Member

headius commented May 16, 2018

Well I pushed a PR that fixes it by always searching everything, like it appears MRI does. We can debate whether it should go into 9.2.

@enebo enebo modified the milestones: JRuby 9.2.0.0, JRuby 9.2.1.0 May 24, 2018

@enebo enebo closed this in #5175 Sep 19, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.