ruby-changes:60729
From: Jeremy <ko1@a...>
Date: Fri, 10 Apr 2020 16:29:21 +0900 (JST)
Subject: [ruby-changes:60729] 900e83b501 (master): Turn class variable warnings into exceptions
https://git.ruby-lang.org/ruby.git/commit/?id=900e83b501 From 900e83b50115afda3f79712310e4cb95e4508972 Mon Sep 17 00:00:00 2001 From: Jeremy Evans <code@j...> Date: Fri, 27 Mar 2020 15:08:52 -0700 Subject: Turn class variable warnings into exceptions This changes the following warnings: * warning: class variable access from toplevel * warning: class variable @foo of D is overtaken by C into RuntimeErrors. Handle defined?(@@foo) at toplevel by returning nil instead of raising an exception (the previous behavior warned before returning nil when defined? was used). Refactor the specs to avoid the warnings even in older versions. The specs were checking for the warnings, but the purpose of the related specs as evidenced from their description is to test for behavior, not for warnings. Fixes [Bug #14541] diff --git a/bootstraptest/test_eval.rb b/bootstraptest/test_eval.rb index fa04323..5d2593c 100644 --- a/bootstraptest/test_eval.rb +++ b/bootstraptest/test_eval.rb @@ -250,7 +250,9 @@ assert_equal 'ok', %q{ https://github.com/ruby/ruby/blob/trunk/bootstraptest/test_eval.rb#L250 assert_equal 'ok', %q{ begin - 12.instance_eval { @@a } + class A + 12.instance_eval { @@a } + end rescue NameError :ok end @@ -258,7 +260,9 @@ assert_equal 'ok', %q{ https://github.com/ruby/ruby/blob/trunk/bootstraptest/test_eval.rb#L260 assert_equal 'ok', %q{ begin - 12.instance_exec { @@a } + class A + 12.instance_exec { @@a } + end rescue NameError :ok end diff --git a/bootstraptest/test_insns.rb b/bootstraptest/test_insns.rb index 1269d7d..cb54d0d 100644 --- a/bootstraptest/test_insns.rb +++ b/bootstraptest/test_insns.rb @@ -64,8 +64,8 @@ tests = [ https://github.com/ruby/ruby/blob/trunk/bootstraptest/test_insns.rb#L64 [ 'setinstancevariable', %q{ @x = true }, ], [ 'getinstancevariable', %q{ @x = true; @x }, ], - [ 'setclassvariable', %q{ @@x = true }, ], - [ 'getclassvariable', %q{ @@x = true; @@x }, ], + [ 'setclassvariable', %q{ class A; @@x = true; end }, ], + [ 'getclassvariable', %q{ class A; @@x = true; @@x end }, ], [ 'setconstant', %q{ X = true }, ], [ 'setconstant', %q{ Object::X = true }, ], diff --git a/bootstraptest/test_syntax.rb b/bootstraptest/test_syntax.rb index a111990..fa27bf2 100644 --- a/bootstraptest/test_syntax.rb +++ b/bootstraptest/test_syntax.rb @@ -268,8 +268,10 @@ assert_equal %q{}, %q{ https://github.com/ruby/ruby/blob/trunk/bootstraptest/test_syntax.rb#L268 defined?(@@a) } assert_equal %q{class variable}, %q{ - @@a = 1 - defined?(@@a) + class A + @@a = 1 + defined?(@@a) + end } assert_equal %q{}, %q{ defined?($a) diff --git a/insns.def b/insns.def index aab5cca..58349b0 100644 --- a/insns.def +++ b/insns.def @@ -236,7 +236,7 @@ getclassvariable https://github.com/ruby/ruby/blob/trunk/insns.def#L236 /* "class variable access from toplevel" warning can be hooked. */ // attr bool leaf = false; /* has rb_warning() */ { - val = rb_cvar_get(vm_get_cvar_base(vm_get_cref(GET_EP()), GET_CFP()), id); + val = rb_cvar_get(vm_get_cvar_base(vm_get_cref(GET_EP()), GET_CFP(), 1), id); } /* Set value of class variable id of klass as val. */ @@ -249,7 +249,7 @@ setclassvariable https://github.com/ruby/ruby/blob/trunk/insns.def#L249 // attr bool leaf = false; /* has rb_warning() */ { vm_ensure_not_refinement_module(GET_SELF()); - rb_cvar_set(vm_get_cvar_base(vm_get_cref(GET_EP()), GET_CFP()), id, val); + rb_cvar_set(vm_get_cvar_base(vm_get_cref(GET_EP()), GET_CFP(), 1), id, val); } /* Get constant variable id. If klass is Qnil and allow_nil is Qtrue, constants diff --git a/spec/ruby/core/exception/name_spec.rb b/spec/ruby/core/exception/name_spec.rb index d1def51..c8a49b4 100644 --- a/spec/ruby/core/exception/name_spec.rb +++ b/spec/ruby/core/exception/name_spec.rb @@ -21,9 +21,7 @@ describe "NameError#name" do https://github.com/ruby/ruby/blob/trunk/spec/ruby/core/exception/name_spec.rb#L21 it "returns a class variable name as a symbol" do -> { - -> { - @@doesnt_exist - }.should complain(/class variable access from toplevel/) + eval("class singleton_class::A; @@doesnt_exist end", binding, __FILE__, __LINE__) }.should raise_error(NameError) { |e| e.name.should == :@@doesnt_exist } end diff --git a/spec/ruby/core/exception/receiver_spec.rb b/spec/ruby/core/exception/receiver_spec.rb index 7c57d63..d1c23b6 100644 --- a/spec/ruby/core/exception/receiver_spec.rb +++ b/spec/ruby/core/exception/receiver_spec.rb @@ -28,10 +28,8 @@ describe "NameError#receiver" do https://github.com/ruby/ruby/blob/trunk/spec/ruby/core/exception/receiver_spec.rb#L28 it "returns the Object class when an undefined class variable is called" do -> { - -> { - @@doesnt_exist - }.should complain(/class variable access from toplevel/) - }.should raise_error(NameError) {|e| e.receiver.should equal(Object) } + eval("class singleton_class::A; @@doesnt_exist end", binding, __FILE__, __LINE__) + }.should raise_error(NameError) {|e| e.receiver.should equal(singleton_class::A) } end it "returns a class when an undefined class variable is called in a subclass' namespace" do diff --git a/spec/ruby/language/defined_spec.rb b/spec/ruby/language/defined_spec.rb index ed61ed3..f2da8a9 100644 --- a/spec/ruby/language/defined_spec.rb +++ b/spec/ruby/language/defined_spec.rb @@ -275,9 +275,7 @@ describe "The defined? keyword for an expression" do https://github.com/ruby/ruby/blob/trunk/spec/ruby/language/defined_spec.rb#L275 end it "returns nil for an expression with '!' and an unset class variable" do - -> { - @result = defined?(!@@defined_specs_undefined_class_variable) - }.should complain(/class variable access from toplevel/) + @result = eval("class singleton_class::A; defined?(!@@doesnt_exist) end", binding, __FILE__, __LINE__) @result.should be_nil end @@ -286,9 +284,7 @@ describe "The defined? keyword for an expression" do https://github.com/ruby/ruby/blob/trunk/spec/ruby/language/defined_spec.rb#L284 end it "returns nil for an expression with 'not' and an unset class variable" do - -> { - @result = defined?(not @@defined_specs_undefined_class_variable) - }.should complain(/class variable access from toplevel/) + @result = eval("class singleton_class::A; defined?(not @@doesnt_exist) end", binding, __FILE__, __LINE__) @result.should be_nil end @@ -897,17 +893,21 @@ describe "The defined? keyword for a variable scoped constant" do https://github.com/ruby/ruby/blob/trunk/spec/ruby/language/defined_spec.rb#L893 end it "returns nil if the class scoped constant is not defined" do - -> { - @@defined_specs_obj = DefinedSpecs::Basic - defined?(@@defined_specs_obj::Undefined).should be_nil - }.should complain(/class variable access from toplevel/) + eval(<<-END, binding, __FILE__, __LINE__) + class singleton_class::A + @@defined_specs_obj = DefinedSpecs::Basic + defined?(@@defined_specs_obj::Undefined).should be_nil + end + END end it "returns 'constant' if the constant is defined in the scope of the class variable" do - -> { - @@defined_specs_obj = DefinedSpecs::Basic - defined?(@@defined_specs_obj::A).should == "constant" - }.should complain(/class variable access from toplevel/) + eval(<<-END, binding, __FILE__, __LINE__) + class singleton_class::A + @@defined_specs_obj = DefinedSpecs::Basic + defined?(@@defined_specs_obj::A).should == "constant" + end + END end it "returns nil if the local scoped constant is not defined" do diff --git a/variable.c b/variable.c index 9ff0326..3bcee43 100644 --- a/variable.c +++ b/variable.c @@ -3064,7 +3064,8 @@ cvar_overtaken(VALUE front, VALUE target, ID id) https://github.com/ruby/ruby/blob/trunk/variable.c#L3064 st_data_t did = (st_data_t)id; if (RTEST(ruby_verbose) && original_module(front) != original_module(target)) { - rb_warning("class variable % "PRIsVALUE" of %"PRIsVALUE" is overtaken by %"PRIsVALUE"", + rb_raise(rb_eRuntimeError, + "class variable % "PRIsVALUE" of %"PRIsVALUE" is overtaken by %"PRIsVALUE"", ID2SYM(id), rb_class_name(original_module(front)), rb_class_name(original_module(target))); } diff --git a/vm_insnhelper.c b/vm_insnhelper.c index a253a22..2fca2bd 100644 --- a/vm_insnhelper.c +++ b/vm_insnhelper.c @@ -989,7 +989,7 @@ vm_get_ev_const(rb_execution_context_t *ec, VALUE orig_klass, ID id, bool allow_ https://github.com/ruby/ruby/blob/trunk/vm_insnhelper.c#L989 } static inline VALUE -vm_get_cvar_base(const rb_cref_t *cref, rb_control_frame_t *cfp) +vm_get_cvar_base(const rb_cref_t *cref, rb_control_frame_t *cfp, int top_level_raise) { VALUE klass; @@ -1002,8 +1002,8 @@ vm_get_cvar_base(const rb_cref_t *cref, rb_control_frame_t *cfp) https://github.com/ruby/ruby/blob/trunk/vm_insnhelper.c#L1002 CREF_PUSHED_BY_EVAL(cref))) { cref = CREF_NEXT(cref); } - if (!CREF_NEXT(cref)) { - rb_warn("class variable access from toplevel"); + if (top_level_raise && !CREF_NEXT(cref)) { + rb_raise(rb_eRuntimeError, "class variable access from toplevel"); } klass = vm_get_iclass(cfp, CREF_CLASS(cref)); @@ -3567,7 +3567,7 @@ vm_defined(rb_execution_context_t *ec, rb_control_frame_t *reg_cfp, rb_num_t op_ https://github.com/ruby/ruby/blob/trunk/vm_insnhelper.c#L3567 break; case DEFINED_CVAR: { const rb_cref_t *cref = vm_get_cref(GET_EP()); - klass = vm_get_cvar_base(cref, GET_CFP()); + klass = vm_get_cvar_base(cref, GET_CFP(), 0); if (rb_cvar_defined(klass, SYM2ID(obj))) { expr_type = DEFINED_CVAR; } -- cgit v0.10.2 -- ML: ruby-changes@q... Info: http://www.atdot.net/~ko1/quickml/