diff --git a/CHANGELOG.md b/CHANGELOG.md index 35e84634635d..d69cd6ad52a7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,7 @@ Compatibility: * Add `String#bytesplice` (#3039, @itarato). * Add `String#byteindex` and `String#byterindex` (#3039, @itarato). * Add implementations of `rb_proc_call_with_block`, `rb_proc_call_kw`, `rb_proc_call_with_block_kw` and `rb_funcall_with_block_kw` (#3068, @andrykonchin). +* Make `autoload` thread-safe, that is only publish the autoloaded constant once the file is fully loaded (#2431, #3040, @eregon). Performance: diff --git a/spec/mspec/lib/mspec/matchers/have_class_variable.rb b/spec/mspec/lib/mspec/matchers/have_class_variable.rb index dd43ced621bd..8984e728ea1e 100644 --- a/spec/mspec/lib/mspec/matchers/have_class_variable.rb +++ b/spec/mspec/lib/mspec/matchers/have_class_variable.rb @@ -1,8 +1,11 @@ require 'mspec/matchers/variable' class HaveClassVariableMatcher < VariableMatcher - self.variables_method = :class_variables - self.description = 'class variable' + self.description = 'class variable' + + private def check(object, variable) + object.class_variable_defined?(variable) + end end module MSpecMatchers diff --git a/spec/mspec/lib/mspec/matchers/have_constant.rb b/spec/mspec/lib/mspec/matchers/have_constant.rb index 6ec7c75b8581..f45111581649 100644 --- a/spec/mspec/lib/mspec/matchers/have_constant.rb +++ b/spec/mspec/lib/mspec/matchers/have_constant.rb @@ -1,8 +1,12 @@ require 'mspec/matchers/variable' class HaveConstantMatcher < VariableMatcher - self.variables_method = :constants - self.description = 'constant' + self.description = 'constant' + + private def check(object, variable) + # Differs from object.const_defined?(variable, false) for undefined constants + object.constants(false).include?(variable) + end end module MSpecMatchers diff --git a/spec/mspec/lib/mspec/matchers/have_instance_variable.rb b/spec/mspec/lib/mspec/matchers/have_instance_variable.rb index de51b3209d7f..2214527f7887 100644 --- a/spec/mspec/lib/mspec/matchers/have_instance_variable.rb +++ b/spec/mspec/lib/mspec/matchers/have_instance_variable.rb @@ -1,8 +1,11 @@ require 'mspec/matchers/variable' class HaveInstanceVariableMatcher < VariableMatcher - self.variables_method = :instance_variables - self.description = 'instance variable' + self.description = 'instance variable' + + private def check(object, variable) + object.instance_variable_defined?(variable) + end end module MSpecMatchers diff --git a/spec/mspec/lib/mspec/matchers/variable.rb b/spec/mspec/lib/mspec/matchers/variable.rb index 4d801ea33778..6895b6b261d2 100644 --- a/spec/mspec/lib/mspec/matchers/variable.rb +++ b/spec/mspec/lib/mspec/matchers/variable.rb @@ -1,6 +1,6 @@ class VariableMatcher class << self - attr_accessor :variables_method, :description + attr_accessor :description end def initialize(variable) @@ -9,7 +9,7 @@ def initialize(variable) def matches?(object) @object = object - @object.send(self.class.variables_method).include? @variable + check(@object, @variable) end def failure_message diff --git a/spec/ruby/core/kernel/autoload_spec.rb b/spec/ruby/core/kernel/autoload_spec.rb index 0404caec6d8d..276ed7afb304 100644 --- a/spec/ruby/core/kernel/autoload_spec.rb +++ b/spec/ruby/core/kernel/autoload_spec.rb @@ -111,7 +111,7 @@ def go end end -Kernel.autoload :KSAutoloadBB, "no_autoload.rb" +Kernel.autoload :KSAutoloadBB, "autoload_never_used.rb" describe "Kernel.autoload" do before :all do @@ -133,8 +133,17 @@ def go Kernel.autoload?(:KSAutoloadAA).should == @non_existent end - it "sets the autoload constant in Object's constant table" do + it "sets the autoload constant in the caller class's constant table" do Object.should have_constant(:KSAutoloadBB) + Kernel.should_not have_constant(:KSAutoloadBB) + + module KernelSpecs + class AutoloadInCallerClass + Kernel.autoload :KSAutoloadCC, "autoload_never_used.rb" + AutoloadInCallerClass.should have_constant(:KSAutoloadCC) + Kernel.should_not have_constant(:KSAutoloadBB) + end + end end it "calls #to_path on non-String filenames" do @@ -167,6 +176,9 @@ def go it "returns the name of the file that will be autoloaded" do Kernel.autoload :KSAutoload, "autoload.rb" Kernel.autoload?(:KSAutoload).should == "autoload.rb" + Kernel.should_not have_constant(:KSAutoload) + Object.should_not have_constant(:KSAutoload) + self.class.should have_constant(:KSAutoload) end it "returns nil if no file has been registered for a constant" do diff --git a/spec/ruby/core/module/autoload_spec.rb b/spec/ruby/core/module/autoload_spec.rb index af04ab26c874..ed4dd8b683a4 100644 --- a/spec/ruby/core/module/autoload_spec.rb +++ b/spec/ruby/core/module/autoload_spec.rb @@ -299,25 +299,28 @@ module ModuleSpecs::Autoload autoload :RequiredDirectlyNoConstant, fixture(__FILE__, "autoload_required_directly_no_constant.rb") end @path = fixture(__FILE__, "autoload_required_directly_no_constant.rb") - @remove << :RequiredDirectlyNoConstant @check = -> { [ defined?(ModuleSpecs::Autoload::RequiredDirectlyNoConstant), - ModuleSpecs::Autoload.constants(false).include?(:RequiredDirectlyNoConstant), ModuleSpecs::Autoload.const_defined?(:RequiredDirectlyNoConstant), ModuleSpecs::Autoload.autoload?(:RequiredDirectlyNoConstant) ] } ScratchPad.record @check - @check.call.should == ["constant", true, true, @path] + @check.call.should == ["constant", true, @path] $:.push File.dirname(@path) begin require "autoload_required_directly_no_constant.rb" ensure $:.pop end - ScratchPad.recorded.should == [nil, true, false, nil] - @check.call.should == [nil, true, false, nil] + ScratchPad.recorded.should == [nil, false, nil] + @check.call.should == [nil, false, nil] + + # undefined constant, CRuby 3.1 & 3.2 still do it here, but it is inconsistent, the constant should be removed + if ModuleSpecs::Autoload.constants(false).include?(:RequiredDirectlyNoConstant) + @remove << :RequiredDirectlyNoConstant + end end end @@ -568,10 +571,17 @@ class LexicalScope LexicalScope::DeclaredInParentDefinedInCurrent.should == :declared_in_parent_defined_in_current end - # Basically, the parent autoload constant remains in a "undefined" state self.autoload?(:DeclaredInParentDefinedInCurrent).should == nil const_defined?(:DeclaredInParentDefinedInCurrent).should == false -> { DeclaredInParentDefinedInCurrent }.should raise_error(NameError) + ruby_version_is ""..."3.1" do + # Basically, the parent autoload constant remains in a "undefined" state + self.constants(false).should.include?(:DeclaredInParentDefinedInCurrent) + end + ruby_version_is "3.1" do + # The autoload constant has been removed + self.constants(false).should_not.include?(:DeclaredInParentDefinedInCurrent) + end ModuleSpecs::Autoload::LexicalScope.send(:remove_const, :DeclaredInParentDefinedInCurrent) end @@ -592,13 +602,11 @@ module Autoload -> { DeclaredInCurrentDefinedInParent }.should complain( - /Expected .*autoload_callback.rb to define ModuleSpecs::Autoload::DeclaredInCurrentDefinedInParent but it didn't/, - verbose: true, - ) + /Expected .*autoload_callback.rb to define ModuleSpecs::Autoload::DeclaredInCurrentDefinedInParent but it didn't/, verbose: true) -> { DeclaredInCurrentDefinedInParent - }.should_not complain(/.*/, verbose: true) + }.should_not complain(verbose: true) self.autoload?(:DeclaredInCurrentDefinedInParent).should == nil const_defined?(:DeclaredInCurrentDefinedInParent).should == false ModuleSpecs.const_defined?(:DeclaredInCurrentDefinedInParent).should == true @@ -607,8 +615,9 @@ module Autoload end end + # like net/https used to do `module Net; autoload :OpenSSL, 'openssl'; end` before https://github.com/ruby/net-http/commit/369c3fd708 ruby_version_is "3.1" do - it "looks up in parent scope after failed autoload" do + it "looks up in parent scope when declared in current and defined in parent" do @remove << :DeclaredInCurrentDefinedInParent module ModuleSpecs::Autoload ScratchPad.record -> { @@ -617,10 +626,11 @@ module ModuleSpecs::Autoload class LexicalScope autoload :DeclaredInCurrentDefinedInParent, fixture(__FILE__, "autoload_callback.rb") - -> { DeclaredInCurrentDefinedInParent }.should_not raise_error(NameError) - # Basically, the autoload constant remains in a "undefined" state + DeclaredInCurrentDefinedInParent.should == :declared_in_current_defined_in_parent + # The autoload constant has been removed self.autoload?(:DeclaredInCurrentDefinedInParent).should == nil const_defined?(:DeclaredInCurrentDefinedInParent).should == false + self.constants(false).should_not.include?(:DeclaredInCurrentDefinedInParent) -> { const_get(:DeclaredInCurrentDefinedInParent) }.should raise_error(NameError) end @@ -630,7 +640,7 @@ class LexicalScope 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 + it "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 -> { @@ -643,7 +653,7 @@ class LexicalScope # Basically, the autoload constant remains in a "undefined" state self.autoload?(:DeclaredInCurrentDefinedInParent).should == nil const_defined?(:DeclaredInCurrentDefinedInParent).should == false - self.should have_constant(:DeclaredInCurrentDefinedInParent) + self.constants(false).should.include?(:DeclaredInCurrentDefinedInParent) -> { const_get(:DeclaredInCurrentDefinedInParent) }.should raise_error(NameError) end @@ -792,14 +802,12 @@ module ModuleSpecs::Autoload -> { ModuleSpecs.autoload "a name", @non_existent }.should raise_error(NameError) end - it "shares the autoload request across dup'ed copies of modules" do - require fixture(__FILE__, "autoload_s.rb") - @remove << :S + it "does not share the autoload request internals across dup'ed copies of modules" do filename = fixture(__FILE__, "autoload_t.rb") mod1 = Module.new { autoload :T, filename } - -> { - ModuleSpecs::Autoload::S = mod1 - }.should complain(/already initialized constant/) + ModuleSpecs::Autoload::S = mod1 + @remove << :S + mod2 = mod1.dup mod1.autoload?(:T).should == filename @@ -905,6 +913,20 @@ class ModuleSpecs::Autoload::Z < ModuleSpecs::Autoload::ZZ t2_exc.should be_nil end + it "blocks other threads until the file is done loading so no partial modules are seen" do + ModuleSpecs::Autoload.autoload :ThreadSafe, fixture(__FILE__, "autoload_thread_safe.rb") + barrier = ModuleSpecs::CyclicBarrier.new 2 + ScratchPad.record(barrier) + + thread = Thread.new { # starts the autoload + ModuleSpecs::Autoload::ThreadSafe.foo.should == 42 + } + barrier.await + ModuleSpecs::Autoload::ThreadSafe.foo.should == 42 # ThreadSafe should only be published once the whole file is loaded + + thread.join + end + # https://bugs.ruby-lang.org/issues/10892 it "blocks others threads while doing an autoload" do file_path = fixture(__FILE__, "repeated_concurrent_autoload.rb") diff --git a/spec/ruby/core/module/fixtures/autoload_thread_safe.rb b/spec/ruby/core/module/fixtures/autoload_thread_safe.rb new file mode 100644 index 000000000000..c36eb7cf5dd7 --- /dev/null +++ b/spec/ruby/core/module/fixtures/autoload_thread_safe.rb @@ -0,0 +1,20 @@ +module ModuleSpecs::Autoload + class ThreadSafe + def self.bar # to illustrate partial loading + end + + barrier = ScratchPad.recorded + barrier.await + + # the main thread should be waiting for this file to be fully loaded + Thread.pass until Thread.main.stop? + 10.times do + Thread.pass + Thread.main.should.stop? + end + + def self.foo + 42 + end + end +end diff --git a/spec/ruby/core/module/fixtures/classes.rb b/spec/ruby/core/module/fixtures/classes.rb index a434e7b0b8fc..c02c8a723a23 100644 --- a/spec/ruby/core/module/fixtures/classes.rb +++ b/spec/ruby/core/module/fixtures/classes.rb @@ -475,7 +475,7 @@ def extend_object(obj) end class CyclicBarrier - def initialize(count = 1) + def initialize(count) @count = count @state = 0 @mutex = Mutex.new diff --git a/spec/ruby/library/syslog/constants_spec.rb b/spec/ruby/library/syslog/constants_spec.rb index 2b9524c53d84..e79b0bb699d7 100644 --- a/spec/ruby/library/syslog/constants_spec.rb +++ b/spec/ruby/library/syslog/constants_spec.rb @@ -17,7 +17,7 @@ it "includes the Syslog constants" do @constants.each do |c| - Syslog::Constants.should have_constant(c) + Syslog::Constants.should.const_defined?(c, true) end end end diff --git a/spec/tags/core/kernel/require_tags.txt b/spec/tags/core/kernel/require_tags.txt index b2a99682a533..fcc410e96e8e 100644 --- a/spec/tags/core/kernel/require_tags.txt +++ b/spec/tags/core/kernel/require_tags.txt @@ -2,8 +2,6 @@ slow:Kernel#require (concurrently) blocks based on the path slow:Kernel.require (concurrently) blocks based on the path slow:Kernel#require ($LOADED_FEATURES) unicode_normalize is part of core and not $LOADED_FEATURES slow:Kernel.require ($LOADED_FEATURES) unicode_normalize is part of core and not $LOADED_FEATURES -fails:Kernel#require (non-extensioned path) loads a .rb extensioned file when a C-extension file exists on an earlier load path -fails:Kernel#require (non-extensioned path) does not load a feature twice when $LOAD_PATH has been modified slow:Kernel#require ($LOADED_FEATURES) complex, enumerator, rational, thread, ruby2_keywords are already required slow:Kernel.require ($LOADED_FEATURES) complex, enumerator, rational, thread, ruby2_keywords are already required slow:Kernel#require complex, enumerator, rational, thread, ruby2_keywords, fiber are already required diff --git a/src/main/java/org/truffleruby/core/kernel/KernelNodes.java b/src/main/java/org/truffleruby/core/kernel/KernelNodes.java index 368e17f342c8..6791085f6f3c 100644 --- a/src/main/java/org/truffleruby/core/kernel/KernelNodes.java +++ b/src/main/java/org/truffleruby/core/kernel/KernelNodes.java @@ -572,7 +572,7 @@ protected static RubyDynamicObject copyable(Object object, Object freeze, final RubyClass selfMetaClass = metaClassNode.execute(node, object); if (isSingletonProfile.profile(node, selfMetaClass.isSingleton)) { final RubyClass newObjectMetaClass = lazySingletonClassNode.get(node).execute(newObject); - newObjectMetaClass.fields.initCopy(selfMetaClass); + newObjectMetaClass.fields.initCopy(getContext(node), selfMetaClass, node); } final boolean copyFrozen = freeze instanceof Nil; diff --git a/src/main/java/org/truffleruby/core/klass/ClassNodes.java b/src/main/java/org/truffleruby/core/klass/ClassNodes.java index 467c5279557c..c75f7661bf3e 100644 --- a/src/main/java/org/truffleruby/core/klass/ClassNodes.java +++ b/src/main/java/org/truffleruby/core/klass/ClassNodes.java @@ -335,6 +335,14 @@ protected Object getSuperClass(RubyClass rubyClass) { } } + @Primitive(name = "class_non_singleton_class") + public abstract static class NonSingletonClassNode extends PrimitiveArrayArgumentsNode { + @Specialization + protected RubyClass nonSingletonClass(RubyClass rubyClass) { + return rubyClass.nonSingletonClass; + } + } + @CoreMethod(names = { "__allocate__", "__layout_allocate__" }, constructor = true, visibility = Visibility.PRIVATE) public abstract static class AllocateNode extends CoreMethodArrayArgumentsNode { @Specialization diff --git a/src/main/java/org/truffleruby/core/module/ConstantLookupResult.java b/src/main/java/org/truffleruby/core/module/ConstantLookupResult.java index 408059dc5e61..e2de856a7acd 100644 --- a/src/main/java/org/truffleruby/core/module/ConstantLookupResult.java +++ b/src/main/java/org/truffleruby/core/module/ConstantLookupResult.java @@ -22,13 +22,14 @@ public class ConstantLookupResult { @CompilationFinal(dimensions = 1) private final Assumption[] assumptions; public ConstantLookupResult(RubyConstant constant, Assumption... assumptions) { - assert constant == null || !(constant.isAutoload() && constant.getAutoloadConstant().isAutoloadingThread()); + assert constant == null || + !(constant.isAutoload() && constant.getAutoloadConstant().isAutoloadingThreadAndUnset()); this.constant = constant; this.assumptions = assumptions; } public boolean isFound() { - return constant != null && !constant.isUndefined(); + return constant != null; } public boolean isDeprecated() { diff --git a/src/main/java/org/truffleruby/core/module/ModuleFields.java b/src/main/java/org/truffleruby/core/module/ModuleFields.java index e2a59c474319..97b0771a4b31 100644 --- a/src/main/java/org/truffleruby/core/module/ModuleFields.java +++ b/src/main/java/org/truffleruby/core/module/ModuleFields.java @@ -39,6 +39,7 @@ import org.truffleruby.core.string.ImmutableRubyString; import org.truffleruby.core.string.StringUtils; import org.truffleruby.core.symbol.RubySymbol; +import org.truffleruby.language.AutoloadConstant; import org.truffleruby.language.Nil; import org.truffleruby.language.RubyConstant; import org.truffleruby.language.RubyDynamicObject; @@ -162,19 +163,24 @@ public void afterConstructed() { getName(); } - public RubyConstant getAdoptedByLexicalParent( - RubyContext context, - RubyModule lexicalParent, - String name, + public RubyConstant getAdoptedByLexicalParent(RubyContext context, RubyModule lexicalParent, String name, Node currentNode) { + return getAdoptedByLexicalParent(context, lexicalParent, name, currentNode, true); + } + + private RubyConstant getAdoptedByLexicalParent(RubyContext context, RubyModule lexicalParent, String name, + Node currentNode, boolean setConstant) { assert name != null; - RubyConstant previous = lexicalParent.fields.setConstantInternal( - context, - currentNode, - name, - rubyModule, - false); + RubyConstant previous = null; + if (setConstant) { + previous = lexicalParent.fields.setConstantInternal( + context, + currentNode, + name, + rubyModule, + null); + } if (!hasFullName()) { // Tricky, we need to compare with the Object class, but we only have a Class at hand. @@ -207,7 +213,8 @@ public void updateAnonymousChildrenModules(RubyContext context) { context, rubyModule, entry.getKey(), - null); + null, + false); } } } @@ -222,7 +229,7 @@ public ModuleChain getFirstModuleChain() { } @TruffleBoundary - public void initCopy(RubyModule from) { + public void initCopy(RubyContext context, RubyModule from, Node node) { // Do not copy name, the copy is an anonymous module final ModuleFields fromFields = from.fields; @@ -239,7 +246,12 @@ public void initCopy(RubyModule from) { for (Entry entry : fromFields.constants.entrySet()) { final RubyConstant constant = entry.getValue().getConstant(); if (constant != null) { - this.constants.put(entry.getKey(), new ConstantEntry(constant)); + if (constant.isAutoload()) { + var autoloadConstant = constant.getAutoloadConstant(); + setAutoloadConstant(context, node, constant.getName(), autoloadConstant.getFeature()); + } else { + this.constants.put(entry.getKey(), new ConstantEntry(constant.copy(rubyModule))); + } } } @@ -427,15 +439,14 @@ public RubyConstant setConstant(RubyContext context, Node currentNode, String na name, currentNode); } else { - return setConstantInternal(context, currentNode, name, value, false); + return setConstantInternal(context, currentNode, name, value, null); } } @TruffleBoundary - public void setAutoloadConstant(RubyContext context, Node currentNode, String name, Object filename, - String javaFilename) { - RubyConstant autoloadConstant = setConstantInternal(context, currentNode, name, filename, true); - if (autoloadConstant == null) { + public void setAutoloadConstant(RubyContext context, Node currentNode, String name, Object filename) { + RubyConstant constant = setConstantInternal(context, currentNode, name, null, new AutoloadConstant(filename)); + if (constant == null) { return; } @@ -443,47 +454,57 @@ public void setAutoloadConstant(RubyContext context, Node currentNode, String na RubyLanguage.LOGGER.info(() -> String.format( "%s: setting up autoload %s with %s", context.fileLine(context.getCallStack().getTopMostUserSourceSection()), - autoloadConstant, + constant, filename)); } final ReentrantLockFreeingMap fileLocks = context.getFeatureLoader().getFileLocks(); - final ReentrantLock lock = fileLocks.get(javaFilename); + final ReentrantLock lock = fileLocks.get(constant.getAutoloadConstant().getAutoloadPath()); if (lock.isLocked()) { // We need to handle the new autoload constant immediately // if Object.autoload(name, filename) is executed from filename.rb - GetConstantNode.autoloadConstantStart(context, autoloadConstant, currentNode); + GetConstantNode.autoloadConstantStart(context, constant, currentNode); } - context.getFeatureLoader().addAutoload(autoloadConstant); + context.getFeatureLoader().addAutoload(constant); } private RubyConstant setConstantInternal(RubyContext context, Node currentNode, String name, Object value, - boolean autoload) { + AutoloadConstant autoloadConstant) { checkFrozen(context, currentNode); SharedObjects.propagate(context.getLanguageSlow(), rubyModule, value); - final String autoloadPath = autoload - ? RubyGuards.getJavaString(value) - : null; + final boolean autoload = autoloadConstant != null; + final String autoloadPath = autoload ? autoloadConstant.getAutoloadPath() : null; + RubyConstant previous; RubyConstant newConstant; ConstantEntry previousEntry; do { previousEntry = constants.get(name); previous = previousEntry != null ? previousEntry.getConstant() : null; - if (autoload && previous != null) { - if (previous.hasValue()) { - // abort, do not set an autoload constant, the constant already has a value - return null; - } else if (previous.isAutoload() && - previous.getAutoloadConstant().getAutoloadPath().equals(autoloadPath)) { - // already an autoload constant with the same path, - // do nothing so we don't replace the AutoloadConstant#autoloadLock which might be already acquired - return null; + + if (previous != null) { + if (autoload) { + if (previous.hasValue()) { + // abort, do not set an autoload constant, the constant already has a value + return null; + } else if (previous.isAutoload() && + previous.getAutoloadConstant().getAutoloadPath().equals(autoloadPath)) { + // already an autoload constant with the same path, + // do nothing so we don't replace the AutoloadConstant#autoloadLock which might be already acquired + return null; + } + } else { + if (previous.isAutoload() && previous.getAutoloadConstant().isAutoloadingThread() && + !previous.getAutoloadConstant().isPublished()) { + previous.getAutoloadConstant().setUnpublishedValue(value, currentNode); + return previous; + } } } - newConstant = newConstant(currentNode, name, value, autoload, previous); + + newConstant = newConstant(currentNode, name, value, autoloadConstant, previous); } while (!ConcurrentOperations.replace(constants, name, previousEntry, new ConstantEntry(newConstant))); if (previousEntry != null) { @@ -497,12 +518,12 @@ private RubyConstant setConstantInternal(RubyContext context, Node currentNode, return autoload ? newConstant : previous; } - private RubyConstant newConstant(Node currentNode, String name, Object value, boolean autoload, + private RubyConstant newConstant(Node currentNode, String name, Object value, AutoloadConstant autoloadConstant, RubyConstant previous) { final boolean isPrivate = previous != null && previous.isPrivate(); final boolean isDeprecated = previous != null && previous.isDeprecated(); final SourceSection sourceSection = currentNode != null ? currentNode.getSourceSection() : null; - return new RubyConstant(rubyModule, name, value, isPrivate, autoload, isDeprecated, sourceSection); + return new RubyConstant(rubyModule, name, value, isPrivate, autoloadConstant, isDeprecated, sourceSection); } @TruffleBoundary @@ -695,15 +716,11 @@ public void deprecateConstant(RubyContext context, Node currentNode, String name } @TruffleBoundary - public boolean undefineConstantIfStillAutoload(RubyConstant autoloadConstant) { + public boolean removeConstantIfStillAutoload(RubyConstant autoloadConstant) { final ConstantEntry constantEntry = constants.get(autoloadConstant.getName()); final boolean replace = constantEntry != null && constantEntry.getConstant() == autoloadConstant; - if (replace && - constants.replace( - autoloadConstant.getName(), - constantEntry, - new ConstantEntry(autoloadConstant.undefined()))) { - constantEntry.invalidate("undefine if still autoload", rubyModule, autoloadConstant.getName()); + if (replace && constants.remove(autoloadConstant.getName(), constantEntry)) { + constantEntry.invalidate("remove if still autoload", rubyModule, autoloadConstant.getName()); return true; } else { return false; diff --git a/src/main/java/org/truffleruby/core/module/ModuleNodes.java b/src/main/java/org/truffleruby/core/module/ModuleNodes.java index ccad0292813b..cb926a4cb27e 100644 --- a/src/main/java/org/truffleruby/core/module/ModuleNodes.java +++ b/src/main/java/org/truffleruby/core/module/ModuleNodes.java @@ -653,8 +653,7 @@ protected Object autoload(RubyModule module, String name, Object filename, throw new RaiseException(getContext(), coreExceptions().argumentError("empty file name", this)); } - final String javaStringFilename = RubyGuards.getJavaString(filename); - module.fields.setAutoloadConstant(getContext(), this, name, filename, javaStringFilename); + module.fields.setAutoloadConstant(getContext(), this, name, filename); return nil; } } @@ -1589,11 +1588,11 @@ public abstract static class InitializeCopyNode extends CoreMethodArrayArguments @Specialization(guards = { "!isRubyClass(self)", "!isRubyClass(from)" }) protected Object initializeCopyModule(RubyModule self, RubyModule from) { - self.fields.initCopy(from); + self.fields.initCopy(getContext(), from, this); final RubyClass selfMetaClass = getSingletonClass(self); final RubyClass fromMetaClass = getSingletonClass(from); - selfMetaClass.fields.initCopy(fromMetaClass); + selfMetaClass.fields.initCopy(getContext(), fromMetaClass, this); return nil; } @@ -1609,7 +1608,7 @@ protected Object initializeCopyClass(RubyClass self, RubyClass from, throw new RaiseException(getContext(), coreExceptions().typeError("can't copy singleton class", this)); } - self.fields.initCopy(from); + self.fields.initCopy(getContext(), from, this); final RubyClass selfMetaClass = getSingletonClass(self); final RubyClass fromMetaClass = from.getMetaClass(); @@ -1617,7 +1616,7 @@ protected Object initializeCopyClass(RubyClass self, RubyClass from, assert fromMetaClass.isSingleton; assert self.getMetaClass().isSingleton; - selfMetaClass.fields.initCopy(fromMetaClass); // copy class methods + selfMetaClass.fields.initCopy(getContext(), fromMetaClass, this); // copy class methods return nil; } @@ -1745,6 +1744,20 @@ protected Object name(RubyModule module) { } } + @Primitive(name = "caller_declaration_context") + public abstract static class CallerDeclarationContextNode extends PrimitiveNode { + @TruffleBoundary + @Specialization + protected RubyModule callerDeclarationContext() { + var frame = getContext().getCallStack().getCallerFrame(FrameAccess.READ_ONLY); + if (frame == null) { + return coreLibrary().objectClass; + } + DeclarationContext declarationContext = RubyArguments.getDeclarationContext(frame.getArguments()); + return declarationContext.getModuleToDefineMethods(); + } + } + @Primitive(name = "caller_nesting") public abstract static class CallerNestingNode extends PrimitiveArrayArgumentsNode { @Specialization @@ -2197,7 +2210,7 @@ protected Object removeConstant(RubyModule module, String name) { getContext(), coreExceptions().nameErrorConstantNotDefined(module, name, this)); } else { - if (oldConstant.isAutoload() || oldConstant.isUndefined()) { + if (oldConstant.isAutoload()) { return nil; } else { return oldConstant.getValue(); diff --git a/src/main/java/org/truffleruby/core/module/ModuleOperations.java b/src/main/java/org/truffleruby/core/module/ModuleOperations.java index 9e87b02666d1..4a5ed47b49bd 100644 --- a/src/main/java/org/truffleruby/core/module/ModuleOperations.java +++ b/src/main/java/org/truffleruby/core/module/ModuleOperations.java @@ -114,10 +114,9 @@ public static Iterable> getAllConstants(RubyModule return constants.entrySet(); } - /** NOTE: This method returns false for an undefined RubyConstant */ public static boolean isConstantDefined(RubyConstant constant) { - return constant != null && !constant.isUndefined() && - !(constant.isAutoload() && constant.getAutoloadConstant().isAutoloadingThread()); + return constant != null && + !(constant.isAutoload() && constant.getAutoloadConstant().isAutoloadingThreadAndUnset()); } /** NOTE: This method might return an undefined RubyConstant */ @@ -128,7 +127,8 @@ private static boolean constantExists(RubyConstant constant, ArrayList assumptions) { return constantExists(constantEntry.getConstant(), assumptions); } diff --git a/src/main/java/org/truffleruby/language/AutoloadConstant.java b/src/main/java/org/truffleruby/language/AutoloadConstant.java index 86bd3c5a0def..35b1dd1ea4bc 100644 --- a/src/main/java/org/truffleruby/language/AutoloadConstant.java +++ b/src/main/java/org/truffleruby/language/AutoloadConstant.java @@ -17,13 +17,16 @@ import org.truffleruby.core.mutex.MutexOperations; import org.truffleruby.language.library.RubyStringLibrary; -public class AutoloadConstant { +public final class AutoloadConstant { private final Object feature; private final String autoloadPath; private volatile ReentrantLock autoloadLock; + private Object unpublishedValue = null; + private Node unpublishedValueNode = null; + private boolean published = false; - AutoloadConstant(Object feature) { + public AutoloadConstant(Object feature) { assert RubyStringLibrary.getUncached().isRubyString(feature); this.feature = feature; this.autoloadPath = RubyGuards.getJavaString(this.feature); @@ -64,4 +67,41 @@ public boolean isAutoloadingThread() { return autoloadLock != null && autoloadLock.isHeldByCurrentThread(); } + public boolean isAutoloadingThreadAndUnset() { + return isAutoloadingThread() && !hasUnpublishedValue(); + } + + public boolean shouldPublish() { + return hasUnpublishedValue() && !isPublished(); + } + + private boolean hasUnpublishedValue() { + assert isAutoloadingThread(); + return unpublishedValue != null; + } + + public Object getUnpublishedValue() { + assert isAutoloadingThread(); + return unpublishedValue; + } + + public void setUnpublishedValue(Object unpublishedValue, Node currentNode) { + assert isAutoloadingThread(); + assert RubyGuards.assertIsValidRubyValue(unpublishedValue); + this.unpublishedValue = unpublishedValue; + this.unpublishedValueNode = currentNode; + } + + public boolean isPublished() { + assert isAutoloadingThread(); + return published; + } + + public void publish(RubyContext context, RubyConstant constant) { + assert isAutoloadingThread(); + assert hasUnpublishedValue(); + this.published = true; + constant.getDeclaringModule().fields.setConstant(context, unpublishedValueNode, constant.getName(), + unpublishedValue); + } } diff --git a/src/main/java/org/truffleruby/language/RubyConstant.java b/src/main/java/org/truffleruby/language/RubyConstant.java index fe6718629571..fd59979186da 100644 --- a/src/main/java/org/truffleruby/language/RubyConstant.java +++ b/src/main/java/org/truffleruby/language/RubyConstant.java @@ -31,52 +31,31 @@ public class RubyConstant implements ObjectGraphNode { private final boolean isDeprecated; private final AutoloadConstant autoloadConstant; - /** A autoload constant can become "undefined" after the autoload loads the file but the constant is not defined by - * the file */ - private final boolean undefined; private final SourceSection sourceSection; public RubyConstant( - RubyModule declaringModule, - String name, - Object value, - boolean isPrivate, - boolean autoload, - boolean isDeprecated, - SourceSection sourceSection) { - this( - declaringModule, - name, - value, - isPrivate, - autoload ? new AutoloadConstant(value) : null, - false, - isDeprecated, - sourceSection); - } - - private RubyConstant( RubyModule declaringModule, String name, Object value, boolean isPrivate, AutoloadConstant autoloadConstant, - boolean undefined, boolean isDeprecated, SourceSection sourceSection) { - assert !undefined || autoloadConstant == null : "undefined and autoload are exclusive"; - this.declaringModule = declaringModule; this.name = name; this.value = value; this.isPrivate = isPrivate; this.isDeprecated = isDeprecated; this.autoloadConstant = autoloadConstant; - this.undefined = undefined; this.sourceSection = sourceSection; } + public RubyConstant copy(RubyModule declaringModule) { + assert !isAutoload(); + return new RubyConstant(declaringModule, name, value, isPrivate, null, isDeprecated, sourceSection); + } + public RubyModule getDeclaringModule() { return declaringModule; } @@ -86,7 +65,7 @@ public String getName() { } public boolean hasValue() { - return !isAutoload() && !undefined; + return !isAutoload(); } public Object getValue() { @@ -116,7 +95,6 @@ public RubyConstant withPrivate(boolean isPrivate) { value, isPrivate, autoloadConstant, - undefined, isDeprecated, sourceSection); } @@ -132,17 +110,11 @@ public RubyConstant withDeprecated() { value, isPrivate, autoloadConstant, - undefined, true, sourceSection); } } - public RubyConstant undefined() { - assert isAutoload(); - return new RubyConstant(declaringModule, name, null, isPrivate, null, true, isDeprecated, sourceSection); - } - @TruffleBoundary public boolean isVisibleTo(RubyContext context, LexicalScope lexicalScope, RubyModule module) { assert lexicalScope == null || lexicalScope.getLiveModule() == module; @@ -178,10 +150,6 @@ public boolean isVisibleTo(RubyContext context, LexicalScope lexicalScope, RubyM return false; } - public boolean isUndefined() { - return undefined; - } - public boolean isAutoload() { return autoloadConstant != null; } diff --git a/src/main/java/org/truffleruby/language/constants/GetConstantNode.java b/src/main/java/org/truffleruby/language/constants/GetConstantNode.java index e992fbc01d63..40ddce86e00c 100644 --- a/src/main/java/org/truffleruby/language/constants/GetConstantNode.java +++ b/src/main/java/org/truffleruby/language/constants/GetConstantNode.java @@ -21,6 +21,7 @@ import org.truffleruby.core.module.ModuleOperations; import org.truffleruby.core.module.RubyModule; import org.truffleruby.core.symbol.RubySymbol; +import org.truffleruby.language.AutoloadConstant; import org.truffleruby.language.LexicalScope; import org.truffleruby.language.RubyBaseNode; import org.truffleruby.language.RubyConstant; @@ -71,27 +72,33 @@ protected Object getConstant( } @TruffleBoundary - @Specialization(guards = { "autoloadConstant != null", "autoloadConstant.isAutoload()" }) + @Specialization(guards = { "constant != null", "constant.isAutoload()" }) protected Object autoloadConstant( LexicalScope lexicalScope, RubyModule module, String name, - RubyConstant autoloadConstant, + RubyConstant constant, LookupConstantInterface lookupConstantNode, boolean callConstMissing, @Cached @Shared LazyDispatchNode constMissingNode, @Cached DispatchNode callRequireNode) { - final Object feature = autoloadConstant.getAutoloadConstant().getFeature(); + final AutoloadConstant autoloadConstant = constant.getAutoloadConstant(); + final Object feature = autoloadConstant.getFeature(); - if (autoloadConstant.getAutoloadConstant().isAutoloadingThread()) { - // Pretend the constant does not exist while it is autoloading - return doMissingConstant(module, name, getSymbol(name), callConstMissing, constMissingNode.get(this)); + if (autoloadConstant.isAutoloadingThread()) { + var unpublishedValue = autoloadConstant.getUnpublishedValue(); + if (unpublishedValue != null) { + return unpublishedValue; + } else { + // Pretend the constant does not exist while it is autoloading + return doMissingConstant(module, name, getSymbol(name), callConstMissing, constMissingNode.get(this)); + } } final FeatureLoader featureLoader = getContext().getFeatureLoader(); final String expandedPath = featureLoader - .findFeature(autoloadConstant.getAutoloadConstant().getAutoloadPath()); + .findFeature(autoloadConstant.getAutoloadPath()); if (expandedPath != null && featureLoader.getFileLocks().isCurrentThreadHoldingLock(expandedPath)) { // We found an autoload constant while we are already require-ing the autoload file, // consider it missing to avoid circular require warnings and calling #require twice. @@ -112,20 +119,27 @@ protected Object autoloadConstant( RubyLanguage.LOGGER.info(() -> String.format( "%s: autoloading %s with %s", getContext().fileLine(getContext().getCallStack().getTopMostUserSourceSection()), - autoloadConstant, - autoloadConstant.getAutoloadConstant().getAutoloadPath())); + constant, + autoloadConstant.getAutoloadPath())); } // Mark the autoload constant as loading already here and not in RequireNode so that recursive lookups act as "being loaded" - autoloadConstantStart(getContext(), autoloadConstant, this); + autoloadConstantStart(getContext(), constant, this); try { - callRequireNode.call(coreLibrary().mainObject, "require", feature); + if (!constant.getAutoloadConstant().isPublished()) { + try { + callRequireNode.call(coreLibrary().mainObject, "require", feature); + } finally { + if (autoloadConstant.shouldPublish()) { + autoloadConstant.publish(getContext(), constant); + } + } + } // This needs to run while the autoload is marked as isAutoloading(), to avoid infinite recursion - return autoloadResolveConstant(lexicalScope, module, name, autoloadConstant, lookupConstantNode, - callConstMissing); + return autoloadResolveConstant(lexicalScope, module, name, constant, lookupConstantNode, callConstMissing); } finally { - autoloadConstantStop(autoloadConstant); + autoloadConstantStop(constant); } } @@ -133,10 +147,12 @@ protected Object autoloadConstant( public static void autoloadConstantStart(RubyContext context, RubyConstant autoloadConstant, Node currentNode) { autoloadConstant.getAutoloadConstant().startAutoLoad(context, currentNode); - // We need to notify cached lookup that we are autoloading the constant, as constant - // lookup changes based on whether an autoload constant is loading or not (constant - // lookup ignores being-autoloaded constants). - autoloadConstant.getDeclaringModule().fields.newConstantVersion(autoloadConstant.getName()); + if (!autoloadConstant.getAutoloadConstant().isPublished()) { + // We need to notify cached lookup that we are autoloading the constant, as constant + // lookup changes based on whether an autoload constant is loading or not (constant + // lookup ignores being-autoloaded constants). + autoloadConstant.getDeclaringModule().fields.newConstantVersion(autoloadConstant.getName()); + } } @TruffleBoundary @@ -146,20 +162,18 @@ public static void autoloadConstantStop(RubyConstant autoloadConstant) { /** Subset of {@link #autoloadResolveConstant} which does not try to resolve the constant. */ @TruffleBoundary - public static boolean autoloadUndefineConstantIfStillAutoload(RubyConstant autoloadConstant) { + public static boolean autoloadRemoveConstantIfStillAutoload(RubyConstant autoloadConstant) { final RubyModule autoloadConstantModule = autoloadConstant.getDeclaringModule(); final ModuleFields fields = autoloadConstantModule.fields; - return fields.undefineConstantIfStillAutoload(autoloadConstant); + return fields.removeConstantIfStillAutoload(autoloadConstant); } @TruffleBoundary - public static void logAutoloadResult(RubyContext context, RubyConstant constant, boolean undefined) { + public static void logAutoloadResult(RubyContext context, RubyConstant constant, boolean removed) { if (context.getOptions().LOG_AUTOLOAD) { final SourceSection section = context.getCallStack().getTopMostUserSourceSection(); final String message = context.fileLine(section) + ": " + constant + " " + - (undefined - ? "was marked as undefined as it was not assigned in " - : "was successfully autoloaded from ") + + (removed ? "was removed as it was not assigned in " : "was successfully autoloaded from ") + constant.getAutoloadConstant().getAutoloadPath(); RubyLanguage.LOGGER.info(message); } @@ -180,11 +194,11 @@ public Object autoloadResolveConstant(LexicalScope lexicalScope, RubyModule modu // all is good, just return that constant logAutoloadResult(getContext(), autoloadConstant, false); } else { - // If the autoload constant was not set in the ancestors, undefine the constant - boolean undefined = fields.undefineConstantIfStillAutoload(autoloadConstant); - logAutoloadResult(getContext(), autoloadConstant, undefined); + // If the autoload constant was not set in the ancestors, remove the constant + boolean removed = fields.removeConstantIfStillAutoload(autoloadConstant); + logAutoloadResult(getContext(), autoloadConstant, removed); - // redo lookup, to consider the undefined constant + // redo lookup, to consider the removed constant resolvedConstant = lookupConstantNode.lookupConstant(lexicalScope, module, name, true); } @@ -192,7 +206,7 @@ public Object autoloadResolveConstant(LexicalScope lexicalScope, RubyModule modu } @Specialization( - guards = { "isNullOrUndefined(constant)", "guardName(node, name, cachedName, sameNameProfile)" }, + guards = { "constant == null", "guardName(node, name, cachedName, sameNameProfile)" }, limit = "getCacheLimit()") protected static Object missingConstantCached( LexicalScope lexicalScope, @@ -209,7 +223,7 @@ protected static Object missingConstantCached( return doMissingConstant(module, name, symbolName, callConstMissing, constMissingNode.get(node)); } - @Specialization(guards = "isNullOrUndefined(constant)") + @Specialization(guards = "constant == null") protected Object missingConstantUncached( LexicalScope lexicalScope, RubyModule module, @@ -231,10 +245,6 @@ private static Object doMissingConstant(RubyModule module, String name, RubySymb } } - protected boolean isNullOrUndefined(Object constant) { - return constant == null || ((RubyConstant) constant).isUndefined(); - } - @SuppressFBWarnings("ES") protected boolean guardName(Node node, String name, String cachedName, InlinedConditionProfile sameNameProfile) { // This is likely as for literal constant lookup the name does not change and Symbols diff --git a/src/main/java/org/truffleruby/language/loader/RequireNode.java b/src/main/java/org/truffleruby/language/loader/RequireNode.java index 32f185f3258d..cf92dda5290c 100644 --- a/src/main/java/org/truffleruby/language/loader/RequireNode.java +++ b/src/main/java/org/truffleruby/language/loader/RequireNode.java @@ -104,7 +104,6 @@ private boolean requireConsideringAutoload(String feature, String expandedPath, } } - if (getContext().getOptions().LOG_AUTOLOAD && !toAutoload.isEmpty()) { String info = toAutoload .stream() @@ -130,9 +129,12 @@ private boolean requireConsideringAutoload(String feature, String expandedPath, final List releasedConstants = featureLoader.getAutoloadConstants(expandedPath); for (RubyConstant constant : releasedConstants) { if (constant.getAutoloadConstant().isAutoloadingThread() && !alreadyAutoloading.contains(constant)) { - final boolean undefined = GetConstantNode - .autoloadUndefineConstantIfStillAutoload(constant); - GetConstantNode.logAutoloadResult(getContext(), constant, undefined); + if (constant.getAutoloadConstant().shouldPublish()) { + constant.getAutoloadConstant().publish(getContext(), constant); + } + + final boolean removed = GetConstantNode.autoloadRemoveConstantIfStillAutoload(constant); + GetConstantNode.logAutoloadResult(getContext(), constant, removed); GetConstantNode.autoloadConstantStop(constant); featureLoader.removeAutoload(constant); } diff --git a/src/main/ruby/truffleruby/core/kernel.rb b/src/main/ruby/truffleruby/core/kernel.rb index c391a591c08b..a7af3b023440 100644 --- a/src/main/ruby/truffleruby/core/kernel.rb +++ b/src/main/ruby/truffleruby/core/kernel.rb @@ -202,8 +202,8 @@ def abort(msg = nil) module_function :abort def autoload(name, file) - nesting = Primitive.caller_nesting - mod = nesting.first || (Primitive.equal?(Kernel, self) ? Kernel : Object) + mod = Primitive.caller_declaration_context + mod = Primitive.class_non_singleton_class(mod) if Primitive.is_a?(mod, Class) if Primitive.equal?(mod, self) super(name, file) # Avoid recursion else @@ -213,10 +213,11 @@ def autoload(name, file) module_function :autoload def autoload?(name) - if Primitive.equal?(Kernel, self) + mod = Primitive.caller_declaration_context + if Primitive.equal?(mod, self) super(name) # Avoid recursion else - Object.autoload?(name) + mod.autoload?(name) end end module_function :autoload?