Skip to content

Commit dd3542a

Browse files
committed
Fix class variable cache not invalidated by class_variable_set
The previous invalidation walked subclasses but missed cvars from included modules, and skipped invalidation when creating a new cvar on modules entirely. Always invalidate when a new class variable is created, since this should be relatively rare. We previously bumped the global state on any module inclusion, which should be far more common than this. [Bug #21978]
1 parent f0c7367 commit dd3542a

2 files changed

Lines changed: 32 additions & 22 deletions

File tree

test/ruby/test_variable.rb

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -120,6 +120,34 @@ class Parent
120120
TestVariable.send(:remove_const, :Parent) rescue nil
121121
end
122122

123+
def test_cvar_cache_invalidated_by_parent_class_variable_set
124+
m = Module.new { class_variable_set(:@@x, 1) }
125+
a = Class.new
126+
b = Class.new(a) do
127+
include m
128+
class_eval "def self.x; @@x; end"
129+
end
130+
assert_equal 1, b.x # warm cache
131+
a.class_variable_set(:@@x, 2)
132+
error = assert_raise(RuntimeError) { b.x }
133+
assert_match(/class variable @@x of .+ is overtaken by .+/, error.message)
134+
end
135+
136+
def test_cvar_cache_invalidated_by_module_class_variable_set
137+
m = Module.new
138+
n = Module.new
139+
b = Class.new do
140+
include m
141+
include n
142+
class_eval "def self.x; @@x; end"
143+
end
144+
m.class_variable_set(:@@x, 1)
145+
assert_equal 1, b.x # warm cache
146+
n.class_variable_set(:@@x, 2)
147+
error = assert_raise(RuntimeError) { b.x }
148+
assert_match(/class variable @@x of .+ is overtaken by .+/, error.message)
149+
end
150+
123151
def test_cvar_overtaken_by_module
124152
error = eval <<~EORB
125153
class ParentForModule

variable.c

Lines changed: 4 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -4246,20 +4246,6 @@ find_cvar(VALUE klass, VALUE * front, VALUE * target, ID id)
42464246
return v;
42474247
}
42484248

4249-
static void
4250-
check_for_cvar_table(VALUE subclass, VALUE key)
4251-
{
4252-
if (RB_TYPE_P(subclass, T_ICLASS)) return; // skip refinement ICLASSes
4253-
4254-
if (RTEST(rb_ivar_defined(subclass, key))) {
4255-
RB_DEBUG_COUNTER_INC(cvar_class_invalidate);
4256-
ruby_vm_global_cvar_state++;
4257-
return;
4258-
}
4259-
4260-
rb_class_foreach_subclass(subclass, check_for_cvar_table, key);
4261-
}
4262-
42634249
void
42644250
rb_cvar_set(VALUE klass, ID id, VALUE val)
42654251
{
@@ -4306,15 +4292,11 @@ rb_cvar_set(VALUE klass, ID id, VALUE val)
43064292
ent->global_cvar_state = GET_GLOBAL_CVAR_STATE();
43074293
}
43084294

4309-
// Break the cvar cache if this is a new class variable
4310-
// and target is a module or a subclass with the same
4311-
// cvar in this lookup.
4295+
// Break the cvar cache if this is a new class variable.
4296+
// Existing caches may have resolved this name to a different
4297+
// location in the hierarchy, so we must invalidate globally.
43124298
if (new_cvar) {
4313-
if (RB_TYPE_P(target, T_CLASS)) {
4314-
if (RCLASS_SUBCLASSES_FIRST(target)) {
4315-
rb_class_foreach_subclass(target, check_for_cvar_table, id);
4316-
}
4317-
}
4299+
ruby_vm_global_cvar_state++;
43184300
}
43194301
}
43204302

0 commit comments

Comments
 (0)