diff options
author | Jeremy Evans <code@jeremyevans.net> | 2021-10-08 12:54:26 -0900 |
---|---|---|
committer | GitHub <noreply@github.com> | 2021-10-08 14:54:26 -0700 |
commit | 08759edea8fb75d46c3e75217e6613465426a0d2 (patch) | |
tree | 7fb4c92e54b0e5fc3767ceeb6c0db645374ae54f | |
parent | ded5a66cb994c5731a17bc9a2420042248a2f1fe (diff) |
Remove autoload for constant if the autoload fails
Previously, if an autoload failed (the file was loaded, but the
constant was not defined by the autoloaded file). Ruby will try
to autoload again if you delete the autoloaded file from
$LOADED_FEATURES. With this change, the autoload and the
constant itself are removed as soon as it fails.
To handle cases where multiple threads are autoloading, when
deleting an autoload, handle the case where another thread
already deleted it.
Fixes [Bug #15790]
Notes
Notes:
Merged: https://github.com/ruby/ruby/pull/4715
Merged-By: jeremyevans <code@jeremyevans.net>
-rw-r--r-- | spec/ruby/core/module/autoload_spec.rb | 100 | ||||
-rw-r--r-- | spec/ruby/core/module/const_set_spec.rb | 3 | ||||
-rw-r--r-- | test/ruby/test_autoload.rb | 27 | ||||
-rw-r--r-- | variable.c | 38 |
4 files changed, 121 insertions, 47 deletions
diff --git a/spec/ruby/core/module/autoload_spec.rb b/spec/ruby/core/module/autoload_spec.rb index f17675846b..1d61646db5 100644 --- a/spec/ruby/core/module/autoload_spec.rb +++ b/spec/ruby/core/module/autoload_spec.rb @@ -441,21 +441,42 @@ describe "Module#autoload" do ScratchPad.recorded.should == [:raise, :raise] end - it "does not remove the constant from Module#constants if the loaded file does not define it, but leaves it as 'undefined'" do - path = fixture(__FILE__, "autoload_o.rb") - ScratchPad.record [] - ModuleSpecs::Autoload.autoload :O, path + ruby_version_is "3.1" do + it "removes the constant from Module#constants if the loaded file does not define it" do + path = fixture(__FILE__, "autoload_o.rb") + ScratchPad.record [] + ModuleSpecs::Autoload.autoload :O, path - ModuleSpecs::Autoload.const_defined?(:O).should == true - ModuleSpecs::Autoload.should have_constant(:O) - ModuleSpecs::Autoload.autoload?(:O).should == path + ModuleSpecs::Autoload.const_defined?(:O).should == true + ModuleSpecs::Autoload.should have_constant(:O) + ModuleSpecs::Autoload.autoload?(:O).should == path - -> { ModuleSpecs::Autoload::O }.should raise_error(NameError) + -> { ModuleSpecs::Autoload::O }.should raise_error(NameError) - ModuleSpecs::Autoload.should have_constant(:O) - ModuleSpecs::Autoload.const_defined?(:O).should == false - ModuleSpecs::Autoload.autoload?(:O).should == nil - -> { ModuleSpecs::Autoload.const_get(:O) }.should raise_error(NameError) + ModuleSpecs::Autoload.const_defined?(:O).should == false + ModuleSpecs::Autoload.should_not have_constant(:O) + ModuleSpecs::Autoload.autoload?(:O).should == nil + -> { ModuleSpecs::Autoload.const_get(:O) }.should raise_error(NameError) + end + end + + ruby_version_is ""..."3.1" do + it "does not remove the constant from Module#constants if the loaded file does not define it, but leaves it as 'undefined'" do + path = fixture(__FILE__, "autoload_o.rb") + ScratchPad.record [] + ModuleSpecs::Autoload.autoload :O, path + + ModuleSpecs::Autoload.const_defined?(:O).should == true + ModuleSpecs::Autoload.should have_constant(:O) + ModuleSpecs::Autoload.autoload?(:O).should == path + + -> { ModuleSpecs::Autoload::O }.should raise_error(NameError) + + ModuleSpecs::Autoload.const_defined?(:O).should == false + ModuleSpecs::Autoload.should have_constant(:O) + ModuleSpecs::Autoload.autoload?(:O).should == nil + -> { ModuleSpecs::Autoload.const_get(:O) }.should raise_error(NameError) + end end it "does not try to load the file again if the loaded file did not define the constant" do @@ -554,31 +575,54 @@ describe "Module#autoload" do # Basically, the parent autoload constant remains in a "undefined" state self.autoload?(:DeclaredInParentDefinedInCurrent).should == nil const_defined?(:DeclaredInParentDefinedInCurrent).should == false - self.should have_constant(:DeclaredInParentDefinedInCurrent) -> { DeclaredInParentDefinedInCurrent }.should raise_error(NameError) ModuleSpecs::Autoload::LexicalScope.send(:remove_const, :DeclaredInParentDefinedInCurrent) end end - it "and fails when finding the undefined autoload constant in the current scope when declared in current and defined in parent" do - @remove << :DeclaredInCurrentDefinedInParent - module ModuleSpecs::Autoload - ScratchPad.record -> { - DeclaredInCurrentDefinedInParent = :declared_in_current_defined_in_parent - } + ruby_version_is "3.1" do + it "looks up in parent scope after failed autoload" do + @remove << :DeclaredInCurrentDefinedInParent + module ModuleSpecs::Autoload + ScratchPad.record -> { + DeclaredInCurrentDefinedInParent = :declared_in_current_defined_in_parent + } - class LexicalScope - autoload :DeclaredInCurrentDefinedInParent, fixture(__FILE__, "autoload_callback.rb") - -> { DeclaredInCurrentDefinedInParent }.should raise_error(NameError) - # Basically, the autoload constant remains in a "undefined" state - self.autoload?(:DeclaredInCurrentDefinedInParent).should == nil - const_defined?(:DeclaredInCurrentDefinedInParent).should == false - self.should have_constant(:DeclaredInCurrentDefinedInParent) - -> { const_get(:DeclaredInCurrentDefinedInParent) }.should raise_error(NameError) + class LexicalScope + autoload :DeclaredInCurrentDefinedInParent, fixture(__FILE__, "autoload_callback.rb") + -> { DeclaredInCurrentDefinedInParent }.should_not raise_error(NameError) + # Basically, the autoload constant remains in a "undefined" state + self.autoload?(:DeclaredInCurrentDefinedInParent).should == nil + const_defined?(:DeclaredInCurrentDefinedInParent).should == false + -> { const_get(:DeclaredInCurrentDefinedInParent) }.should raise_error(NameError) + end + + DeclaredInCurrentDefinedInParent.should == :declared_in_current_defined_in_parent end + end + end + + ruby_version_is ""..."3.1" do + it "and fails when finding the undefined autoload constant in the current scope when declared in current and defined in parent" do + @remove << :DeclaredInCurrentDefinedInParent + module ModuleSpecs::Autoload + ScratchPad.record -> { + DeclaredInCurrentDefinedInParent = :declared_in_current_defined_in_parent + } + + class LexicalScope + autoload :DeclaredInCurrentDefinedInParent, fixture(__FILE__, "autoload_callback.rb") + -> { DeclaredInCurrentDefinedInParent }.should raise_error(NameError) + # Basically, the autoload constant remains in a "undefined" state + self.autoload?(:DeclaredInCurrentDefinedInParent).should == nil + const_defined?(:DeclaredInCurrentDefinedInParent).should == false + self.should have_constant(:DeclaredInCurrentDefinedInParent) + -> { const_get(:DeclaredInCurrentDefinedInParent) }.should raise_error(NameError) + end - DeclaredInCurrentDefinedInParent.should == :declared_in_current_defined_in_parent + DeclaredInCurrentDefinedInParent.should == :declared_in_current_defined_in_parent + end end end diff --git a/spec/ruby/core/module/const_set_spec.rb b/spec/ruby/core/module/const_set_spec.rb index b537d3f133..ba7810d17b 100644 --- a/spec/ruby/core/module/const_set_spec.rb +++ b/spec/ruby/core/module/const_set_spec.rb @@ -101,7 +101,7 @@ describe "Module#const_set" do mod.const_get(:Foo).should == 1 end - it "does not warn if the previous value was undefined" do + it "does not warn after a failed autoload" do path = fixture(__FILE__, "autoload_o.rb") ScratchPad.record [] mod = Module.new @@ -109,7 +109,6 @@ describe "Module#const_set" do mod.autoload :Foo, path -> { mod::Foo }.should raise_error(NameError) - mod.should have_constant(:Foo) mod.const_defined?(:Foo).should == false mod.autoload?(:Foo).should == nil diff --git a/test/ruby/test_autoload.rb b/test/ruby/test_autoload.rb index 98d513da87..7709760d19 100644 --- a/test/ruby/test_autoload.rb +++ b/test/ruby/test_autoload.rb @@ -456,6 +456,31 @@ p Foo::Bar end; end + def test_autoload_after_failed_and_removed_from_loaded_features + Dir.mktmpdir('autoload') do |tmpdir| + autoload_path = File.join(tmpdir, "test-bug-15790.rb") + File.write(autoload_path, '') + + assert_separately(%W[-I #{tmpdir}], <<-RUBY) + path = #{File.realpath(autoload_path).inspect} + autoload :X, path + assert_equal(path, Object.autoload?(:X)) + + assert_raise(NameError){X} + assert_nil(Object.autoload?(:X)) + assert_equal(false, Object.const_defined?(:X)) + + $LOADED_FEATURES.delete(path) + assert_equal(false, Object.const_defined?(:X)) + assert_nil(Object.autoload?(:X)) + + assert_raise(NameError){X} + assert_equal(false, Object.const_defined?(:X)) + assert_nil(Object.autoload?(:X)) + RUBY + end + end + def add_autoload(path) (@autoload_paths ||= []) << path ::Object.class_eval {autoload(:AutoloadTest, path)} @@ -463,7 +488,7 @@ p Foo::Bar def remove_autoload_constant $".replace($" - @autoload_paths) - ::Object.class_eval {remove_const(:AutoloadTest)} + ::Object.class_eval {remove_const(:AutoloadTest)} if defined? Object::AutoloadTest TestAutoload.class_eval {remove_const(:AutoloadTest)} if defined? TestAutoload::AutoloadTest end end diff --git a/variable.c b/variable.c index 8bdc733a4c..5712ccf0ba 100644 --- a/variable.c +++ b/variable.c @@ -2237,23 +2237,26 @@ autoload_delete(VALUE mod, ID id) struct autoload_const *ac; st_delete(tbl, &n, &load); - ele = get_autoload_data((VALUE)load, &ac); - VM_ASSERT(ele); - if (ele) { - VM_ASSERT(!list_empty(&ele->constants)); - } + /* Qfalse can indicate already deleted */ + if (load != Qfalse) { + ele = get_autoload_data((VALUE)load, &ac); + VM_ASSERT(ele); + if (ele) { + VM_ASSERT(!list_empty(&ele->constants)); + } - /* - * we must delete here to avoid "already initialized" warnings - * with parallel autoload. Using list_del_init here so list_del - * works in autoload_c_free - */ - list_del_init(&ac->cnode); + /* + * we must delete here to avoid "already initialized" warnings + * with parallel autoload. Using list_del_init here so list_del + * works in autoload_c_free + */ + list_del_init(&ac->cnode); - if (tbl->num_entries == 0) { - n = autoload; - st_delete(RCLASS_IV_TBL(mod), &n, &val); - } + if (tbl->num_entries == 0) { + n = autoload; + st_delete(RCLASS_IV_TBL(mod), &n, &val); + } + } } } @@ -2502,7 +2505,10 @@ rb_autoload_load(VALUE mod, ID id) result = rb_ensure(autoload_require, (VALUE)&state, autoload_reset, (VALUE)&state); - if (flag > 0 && (ce = rb_const_lookup(mod, id))) { + if (!(ce = rb_const_lookup(mod, id)) || ce->value == Qundef) { + rb_const_remove(mod, id); + } + else if (flag > 0) { ce->flag |= flag; } RB_GC_GUARD(load); |